git.net

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

Re: Apache Fineract CN API Documentation


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.

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.

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?

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.

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)

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.
>>>>
>>>
>>>
>>