OSDir


[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Apache Fineract CN API Documentation


Hi Myrle,

Thanks for your very helpful feedback.

On Mon, Apr 30, 2018 at 8:07 PM, Myrle Krantz <myrle@xxxxxxxxxx> wrote:

> Hey Isaac,
>
> I think we're coming closer to our goal with this PR, but we still
> have some work to do.  I do like it better in the component-test
> module, though I don't think you'll want to add this to all of the
> component-tests.  Some of them really aren't suited to this approach.
>

Do we have to use a different approach ( possibly the documentation module
you earlier proposed ) for asynchronous calls (POST, PUT, DELETE ) from
synchronous ones (GET) ? This can be done later but I wanted to know your
thoughts on this.


> If I read your code correctly, you're getting a 404 Not Found, or a
> 403 out of every REST endpoint.  That doesn't surprise me because the
> endpoints are protected by a Spring Security filter which checks
> whether a JWT token was put into a header.  If the token isn't there,
> or isn't valid, then the filter throws a 403 before ever even checking
> if the endpoint exists.  The reason calling the http methods via the
> feign interface doesn't fail the same way, is because I've put a lot
> of work into setting up a valid security context via the feign
> interface for those calls.  If you look closely, you'll see there's an
> AutoUserContext which sets a thread-local variable containing the JWT
> token.  There's an anubis test artifact for generating a valid JWT
> token in the context of tests.  The feign client then has customized
> code to add an http header to the request using the content of that
> thread local.  It was coded to be as easy to use as possible, but as a
> result, it's almost invisible, and therefore not obvious for people
> trying to add another kind of client code.


> The JWT header will have to be added to the call to mockmvc, otherwise
> you'll never get a return value other than Not Found, (or invalid
> token).  The place this is done in the feign header is here:
> https://github.com/apache/fineract-cn-api/blob/develop/
> src/main/java/org/apache/fineract/cn/api/util/
> TokenedTargetInterceptor.java
>
> There's also code for adding a tenant header here:
> https://github.com/apache/fineract-cn-api/blob/develop/
> src/main/java/org/apache/fineract/cn/api/util/
> TenantedTargetInterceptor.java
>
> You'll need to create something equivalent for mockmvc, or else set
> these headers explicitly in every call.
>

Thanks for shedding more light on how this works. When I saw how "easy" it
was to do API calls, I asked myself how one could get to intercept
information such as the status of a response. Building a new test harness
that works with mockmvc can be a daunting task. I hope I can count on your
help when I run into frustrations.


> Also note:
> By adding REST calls to the end of each function, you are running the
> http calls for the test twice.  I'm not sure that's deliberate.  Maybe
> it would make more sense to create a separate test for generating
> documentation?
>

Okay, when we get a breakthrough with the right return status values, I'll
proceed with segregating tests aimed at generating documentation.


>
> As far as the documentation that is generated by the process, it's
> actually just the same information as in the test, but in a different
> form.  IMO it doesn't make sense to check that in.  It's enough to
> include the instructions for generating it in the README, or in the
> Fineract wikipedia.
>

So far, I created this document
<https://cwiki.apache.org/confluence/display/FINERACT/Apache+Fineract+CN+API+Documentation>
to
help developers generate the asciidoc files themselves from the unit tests.
Would you prefer that we put this in the repository's README file or leave
it on confluence ?

Beyond that, I feel bad that I haven't been responding to you as
> quickly as I would like.  I'd like to reduce my response time to 36
> hours on weekdays for you, so that you don't spend so much time
> blocked on this.  I'm setting it as a goal for myself.  Gentle
> encouragement to live up to this goal is appreciated. ; o)
>

Day by day, I appreciate how complex Fineract CN is. I'm very happy that
you've set aside time to mentor me on this project. I had been groping in
the dark for months now reason why I never even thought about other
relevant concerns like architecture, etc when I saw some light with the PoC
which started generating docs. I'll make sure to discuss updates on this
thread in a gentle manner as you requested. ;-)


> Best Regards,
> Myrle
>
>
> On Fri, Apr 27, 2018 at 3:54 PM, Isaac Kamga <isaac.kamga@xxxxxxxxx>
> wrote:
> > Hello everyone,
> >
> > I've made some changes to the approach for API documentation based on
> > Myrle's feedback.
> >
> > So I have reverted my recent changes to fineract-cn-customer and created
> a
> > pull request [1] for review. I am hoping that we'll get all necessary
> fixes
> > on the PR before I move on to documenting the other services.
> >
> > I have also documented the process [2] for API Documentation which
> > stipulates how a developer can generate the snippets for documentation (
> in
> > component-test's build/doc/ folder ) given that we don't want .adoc file
> > and such checked into the repository.
> >
> > I await your thoughts on this.
> >
> > [1] https://github.com/apache/fineract-cn-customer/pull/7
> > [2]
> > https://cwiki.apache.org/confluence/display/FINERACT/
> Apache+Fineract+CN+API+Documentation
> >
> >
> > On Wed, Apr 25, 2018 at 6:09 PM, Isaac Kamga <isaac.kamga@xxxxxxxxx>
> wrote:
> >
> >> Hello Myrle,
> >>
> >> I've done some tinkering and Spring REST Docs can indeed get the
> snippets
> >> generated for documentation in the component-test module. At least, that
> >> ensures that there's no need for an architectural change and the service
> >> module doesn't depend on the component-test module as Markus advised.
> >>
> >> I think there are still other concerns which you highlighted;
> >>
> >> 1. Checking in generated documentation : Should we leave them in
> >> component-test's build folder (which is ignored by git) and rather
> document
> >> the process of generating the snippets ?
> >>
> >> 2. Creating a documentation module : Do we still need to try this out ?
> >> This module will import the service and component-test modules.
> >>
> >> 3. MockMvc vs Spring Fox: Were you concerned about the security of
> MockMvc
> >> ?
> >>
> >> I'd like to have this discussed here so that we don't proceed with
> >> approaches to solving the problem which aren't worthwhile. I had earlier
> >> assumed that a mention on the Quarter 1 board report about my work on
> API
> >> documentation would have sufficed.
> >>
> >> If you're okay with the API documentation occurring in the
> component-test
> >> module, it may be okay for me to revert the commits I made to the
> >> repositories and submit a Pull request which you'll review and merge.
> >>
> >> At Your Service,
> >> Isaac Kamga.
> >>
> >> On Wed, Apr 25, 2018 at 11:53 AM, Isaac Kamga <isaac.kamga@xxxxxxxxx>
> >> wrote:
> >>
> >>> Hi Myrle,
> >>>
> >>> Thanks for your elaborate feedback and corrections.
> >>>
> >>> On Wed, Apr 25, 2018 at 9:10 AM, Myrle Krantz <myrle@xxxxxxxxxx>
> wrote:
> >>>
> >>>> Hey Isaac,
> >>>>
> >>>> I've looked at your changes in customer and I've found several
> >>>> problems with them:
> >>>>
> >>>> 1.) You've copied all the component tests out of the component test
> >>>> module into the service module.  This means that identical tests will
> >>>> have to be maintained in two places.  Because people aren't very good
> >>>> at keeping code in sync, you can be certain that these tests will
> >>>> diverge from each other a little more each time someone makes a
> >>>> change, and contributors (myself included) won't know which tests are
> >>>> authoritative.
> >>>
> >>>
> >>>> 2.) You've removed the Apache license header from at least one file.
> >>>> I added the Apache license header to the build files very recently, as
> >>>> one step towards making the code releasable.  We can't release the
> >>>> code without the ASF license headers.
> >>>>
> >>>
> >>> Ouch ! My bad. I would have left this commit as a PR for review first.
> >>>
> >>>
> >>>> 3.) I'm assuming the documentation is generated?  Do we really want to
> >>>> check in generated documentation?  And where is the process documented
> >>>> for generating the documentation?
> >>>>
> >>>
> >>> By default, the Spring REST Docs tool outputs documentation in the
> >>> service's build folder ( which is ignored by git ), so I placed them in
> >>> service/src/doc so they can be viewed.
> >>>
> >>>
> >>>>
> >>>> 4.) You added a compile dependency from service to test.  The service
> >>>> should not depend on test.  It should be possible to make changes and
> >>>> add environment variables and etc to the test repository without even
> >>>> thinking about whether a change might break production code or turn on
> >>>> test-configurations which weaken security in production code.
> >>>>
> >>>> In general dependencies between modules must be 1-way, or else
> >>>> compilation is difficult or impossible.  Component-tests should depend
> >>>> on service.  Service should not depend on component-test.  Markus
> >>>> mentioned this when you asked (2) You might ask, why are component
> >>>> tests a separate module? There are several reasons:
> >>>>
> >>>
> >>> I was concerned about duplicating the component-tests' tests in the
> >>> service module and asked about how to avoid the duplication for which
> I had
> >>> no feedback.(2)
> >>>
> >>>
> >>>> * We may wish to release the service and api modules and not release
> >>>> the component-test module.  This opens up some possibilities for the
> >>>> tests that would otherwise be closed off by ASF licensing
> >>>> requirements.
> >>>> * Keeping the code seperate helps contributors keep the concerns of
> >>>> testing and the concern of functionality separate in their thought
> >>>> processes.  Putting testing code in services in other projects has
> >>>> resulted in poor code or even major security vulnerabilities.  It's
> >>>> important here to think about solutions and their trade-offs carefully
> >>>> rather than just slapping things together until they "work" and
> >>>> calling it a day.
> >>>> * One of the things I hope to accomplish in the future is versioned
> >>>> component tests to protect against backwards incompatible changes
> >>>> which can't be detected via signature checking.  We can't implement
> >>>> that plan if the tests become entangled with the source code.
> >>>>
> >>>> As a result I think we need to completely rethink the approach to
> >>>> documentation that you've taken here.  Some alternatives we should
> >>>> consider:
> >>>> * Move mockMVC into component test and out of service.  Why did you
> >>>> put the documentation and component test code in service in the first
> >>>> place?
> >>>> * Create a separate documentation module which imports code from
> >>>> component-test and service.
> >>>> * Using Spring Fox instead of MockMVC.  When I originally evaluated
> >>>> REST documentation approaches, it was exactly this mixing of other
> >>>> concerns into testing that caused me to reject MockMVC in the first
> >>>> place.  Tutorials make good tests but there are many, many good tests
> >>>> which make terrible documentation.
> >>>>
> >>>
> >>> I'm okay with the approach to documentation being rethought.
> >>>
> >>>
> >>>>
> >>>> The use of MockMVC came up a few months ago when a potential GSoC
> >>>> intern suggested it.  I had thought, based on your response, that you
> >>>> understood why it doesn't work well with our architecture. (1) Which
> >>>> is why I didn't respond to that thread myself.  What changed?
> >>>>
> >>>> In general, when making architectural changes which change the pattern
> >>>> of all of the services it's not a good idea to just dump them into all
> >>>> the services and then ask for feedback.  You create a lot of
> >>>> unnecessary work for yourself and for your code reviewer.  A better
> >>>> approach is to make the change in the fineract-cn-template project
> >>>> first and then ask for feedback.  Better still would be to engage a
> >>>> discussion on the mailing list before doing a major architectural
> >>>> change like this.  If I had known you were working on this, I could
> >>>> have shared my thoughts before you started.
> >>>>
> >>>
> >>> I'm sorry I didn't open a discussion here about documenting the APIs.
> >>> I'm hoping we'll continue that discussion on this thread and fix the
> >>> mistakes I made.
> >>>
> >>>>
> >>>> Best Regards,
> >>>> Myrle
> >>>>
> >>>> 1.) https://lists.apache.org/thread.html/cc23a47229c262afbb6e474
> >>>> 9b7eec3117d084f84144877d59098713d@%3Cdev.fineract.apache.org%3E
> >>>> 2.) https://lists.apache.org/thread.html/d0fbdb5b21b916cc8dd3c2a
> >>>> 5061d3e90aa554bb74b63f8deabe69fe6@%3Cdev.fineract.apache.org%3E
> >>>
> >>>
> >>> At Your Service,
> >>> Isaac Kamga.
> >>>
> >>>>
> >>>>
> >>>> On Mon, Apr 23, 2018 at 8:42 PM, Isaac Kamga <isaac.kamga@xxxxxxxxx>
> >>>> wrote:
> >>>> > Hello everyone,
> >>>> >
> >>>> > Trust that you're doing great and congratulations to our recently
> >>>> selected
> >>>> > GSoC 2018 students.
> >>>> >
> >>>> > As you may have observed from Gitbox notifications, I have used the
> >>>> MockMvc
> >>>> > flavour of Spring Rest Docs <https://projects.spring.io/sp
> >>>> ring-restdocs/> to
> >>>> > document the APIs for most Fineract CN domain services viz identity,
> >>>> > office, customer, group, accounting, deposit, payroll and teller,
> >>>> created
> >>>> > and merged pull requests. There were failing tests in portfolio (
> as I
> >>>> > earlier highlighted last week on the list ) and so snippets weren't
> >>>> > generated to be used for documentation.
> >>>> >
> >>>> > After updating the affected repositories, you can view the
> >>>> documentation
> >>>> > for each of the services by opening the
> >>>> > service/src/doc/html5/html5/api-docs.html file.
> >>>> >
> >>>> > I happily await your feedback and enhancements.
> >>>> >
> >>>> > At Your Service,
> >>>> > Isaac Kamga.
> >>>>
> >>>
> >>>
> >>
>