... true, didn't have that on the radar... even simpler than portfolio clients that I had in mind... and I think with global configuration we should have a bit more freedom to experiment...
Cool, let's do that. I can get to that later today or tomorrow if that works for you... opening a first draft PR and then we see how that goes? Cheers On Tue, Oct 26, 2021 at 5:21 PM Petri Tuomola <[email protected]> wrote: > That makes sense - it should be safer and more conservative. > > We can start anywhere but my suggestion would be something that is as > simple as possible, but has integration tests built for it. That way we > could focus on getting the structure of the code with fineract-client / > Jackson right, rather than worrying about the intricacies of the business > logic. And once we’ve done one, then the rest should follow the same > pattern. > > How about something like the /configurations ie GlobalConfigurationApi? > Doesn’t get much simpler than that (i.e. no business logic at all) but > there are integration tests for this. > > Regards > Petri > > On 26 Oct 2021, at 18:18, Aleksandar Vidakovic <[email protected]> > wrote: > > ... 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 >>> >>> >>> >>> >> >
