... the second approach sounds like a good strategy (as much as I'd like to overhaul things using the first approach)... taking things from the integration tests is a bit more conservative and makes sure to not drop anything... and in some cases we might be able to even extend the integration tests and cover more of the REST API (should be easier with the Java client lib vs. Rest Assured boilerplate code).
Just to say... I think I like approach 2... where do we start? With something small? Portfolio client? Let me know... Cheers, Aleks On Tue, Oct 26, 2021 at 6:52 AM Petri Tuomola <[email protected]> wrote: > Thanks Aleks > > I think this is one of the key things we should try to tackle. As Aleks > states, moving to type safe REST resource classes would simplify / reduce > the code by a huge amount and also mean that Swagger etc just magically > work. No need to code / maintain helper resource classes etc. > > Thinking through this, I think there are two ways of doing this: > > Approach 1 - one API at a time, do the following: > > Step #1: Move from raw JSON string to a DTO resource class > Step #2: Ensure that the community UI as well as the integration tests > still work after this. > Step #3: Rewrite the integration test to use the Swagger generated client. > > Approach 2 - one API at a time, do the following: > > Step #1: Rewrite the integration test to use the Swagger generated client > (including fixing any issues with the Swagger resource class) > Step #2: Use the Swagger resource class to create the DTO to be used for > parsing and remove separate Swagger class > Step #3: Ensure the community UI still works > > The second approach would mean we first need to fix the current Swagger > resource classes. This may be helpful (i.e allow us to get the DTO > definition right) or it may be throwaway work (if there’s a lot of rework > between the resource class vs the DTO definition). Another benefit may be > that at least step #1 could be done without breaking any existing clients. > > But I’m not sure which one would be most efficient. Perhaps we need to try > it out and see what makes sense. What do you think? > > Regards > Petri > > > > On 24 Oct 2021, at 00:21, Aleksandar Vidakovic <[email protected]> > wrote: > > Hi, > > ... just as a reminder: pretty much the whole REST resource classes in > Fineract abandoned type safety and keeps the request JSON body in a simple > string variable. That makes it pretty much impossible to extract any > information about the data structures. To get around that Sanyam added > manually in his GSoC project helper classes that have no other > functionality than re-introducing type information about the JSON data > structures; he did this by painstakingly introspecting the payloads. The > Swagger descriptor is generated based on those manually maintained helper > classes. I think this was a good choice given the constraints (minimal code > changes, 100% backward compatibility), but it is very maintenance heavy... > and if I think about it then I am not so sure if these helper classes were > on everyone's radar (e. g. when introducing new REST endpoints)... I didn't > check, but I would bet since Sanyam submitted these changes not a lot of > updates happened in that area. Also: there might have been some small parts > of the REST API that were not mapped at all. If I would bet again then I'd > say the Swagger file we have covers maybe 80% (again, not scientific, could > be more, could be less) of the entire API which is more or less accurate > (i. e. isn't missing any attributes, has all types correct etc.). > > Now that we have a Fineract Java client (code generated based on the > Swagger descriptor) as an official module we could use that client in the > integration tests. That would help with 2 things: reveal any wrong Swagger > mappings immediately and remove a ton of handcrafted boilerplate REST > Assured client code (that makes up probably half of the integration test > code); could help us make the integration tests a bit more appealing. I > don't think that the integration tests - as they are now - cover 100% of > the REST API (don't remember that we have too many pull requests in that > area either), but I think it would be a good start to use the official > Fineract Java client... this would make any missing pieces more visible. > > Mid/long term I think the best solution would be to fix the JSON mapping > in all REST resource classes. Instead of feeding Google GSON with raw JSON > strings and manually mapping it to Java classes we should use the Jackson > parser (pretty much standard in Spring Boot apps and really fast) and let > it do all the de-/serialization and mapping to Java. Again, that would make > a ton of code (10%? 15%?) disappear... code that we don't have to maintain > and test. For that to happen we would need to replace those JSON string > blobs with proper types (DTOs) in the REST resource classes. Once that is > in place then the whole Swagger stuff (or OpenAPI for that matter) is just > a case of simple annotations and we can ditch all these manually maintained > helper classes. When we add new REST endpoints then we just have to > remember to put proper annotations everywhere, easy. Side effect: makes API > requests/responses faster... > > Cheers, > > Aleks > > On Sat, Oct 23, 2021 at 5:27 PM Ed Cable <[email protected]> wrote: > >> Chinmay, Michael, and Manthan, >> >> We were running into some questions regarding the swagger definitions. >> Are they generated from source code and can be used to generate client >> libraries? If so, does it currently cover all of the APIs within Fineract >> 1.x? >> >> We have been reviewing the fineract-client email threads >> http://mail-archives.apache.org/mod_mbox/fineract-commits/202010.mbox/%[email protected]%3E >> and gist report >> >> https://gist.github.com/Grandolf49/f79537436a467dac0baa9458a38290c5 >> >> but still had some doubts. >> >> @[email protected] <[email protected]> Can you share what additional >> questions you have? >> >> Thanks, >> >> Ed >> >> >> >> >
