Hi Myrle, Thanks for your very helpful feedback.
On Mon, Apr 30, 2018 at 8:07 PM, Myrle Krantz <my...@apache.org> 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.ka...@mifos.org> > 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.ka...@mifos.org> > 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.ka...@mifos.org> > >> wrote: > >> > >>> Hi Myrle, > >>> > >>> Thanks for your elaborate feedback and corrections. > >>> > >>> On Wed, Apr 25, 2018 at 9:10 AM, Myrle Krantz <my...@apache.org> > 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.ka...@mifos.org> > >>>> 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. > >>>> > >>> > >>> > >> >
