(Cc’ing the dev list to keep everyone in the loop) Aleksandar - thanks - that’s brilliant! This is exactly what I had in mind with my last paragraph re simplifying / removing boilerplate code. It’s great to hear you’ve already thought through the different aspects of this, and even tried it out so you know what the challenges are. Excellent news!
Do you happen to have any of the code lying around from your previous refactoring attempt? If yes, would be great to see if we can extract some PRs from that to address the low hanging fruit (e.g. Jersey upgrade and Spring Boot config)? If not, I can see if I can find some time to look into these in the next couple of weeks. Personally I think it would be great to start with some of the simpler things - Jersey, Spring Boot, perhaps the Postgres support - and then move onto clearing the manual JSON mapping. After these we would already be in a much better place. The domain / DTO work sounds more intrusive but perhaps that can then be tackled one package at a time, as you suggest. The only thing I was not quite sure about was the benefits of splitting this into multiple JARs. The code looks so tightly coupled that I’m wondering if it’s possible to separate modules that could actually evolve independently? If not, then having to handle multiple JARs is just going to add pain to the refactoring… Regards Petri > On 16 Oct 2021, at 18:00, Aleksandar Vidakovic <[email protected]> > wrote: > > Hi Petri, > > ... agree with what you wrote... some additional notes from some previous > attempts of mine to reduce the boilerplate: a while back (2 years?) I went > down the rabbithole and refactored Fineract 1.x "aggressively" just to see > how hard/easy it would be to do this and if it would be possible to keep > backward compatibility: > I used Lombok all over the place > introduce Mapstruct for more convenient (read: less manual code, more code > generation) mapping between api and domain layer > upgraded Jersey from 1.x to 2.3 > embrace Spring Boot starter packages (aka auto-configuration) as much as > possible and remove all old-school Spring XML config (and any home config - > e. g. JDBC configuration - that can be replaced with an auto-configuration > equivalent) > experimented with replacing the default Tomcat container with something more > performant like Undertow > started to replace all the manual JSON mapping with GSON with Jackson and let > it do it's magic; for that to happen all REST resource classes need to be > refactored for type safety (at the moment all JSON is handled in string blobs) > First I thought that cleaning up the api (DTOs) and domain classes would be > the easiest thing to achieve, in the end these should be only "dumb" classes > with simple attributes and a bunch of getters and setters... turns out that > these are the hardest to clean up, because over the years a lot of business > logic code leaked into these classes (see Loan.java, my favorite example). > Another seemingly simple thing makes cleanup efforts harder on that front: > pretty much all these classes were designed with immutability in mind; > all/most data is set via constructors; I see the utility of immutable data > structures, but having 25 constructor parameters makes this awfully hard to > debug and reason about; my preference would be some fluent builder api to set > these values (makes it very readable and clear to understand), but if we > would do this then a lot of existing code would break... changes in that area > pretty much change the whole code base. So, not sure if this is going to > happen soon. As a consequence this creates then a roadblock for using > Mapstruct, which works very well with "standard" bean classes (read: Lombok > @Data), but the existing DTOs and domain classes are just too inaccessible > for the Mapstruct code generator... which is a pity... just on that front we > could save a ton of lines of code. But: not all hope is lost here... will > come back to this a bit later here. > > Upgrading Jersey was quite straight forward with some - relatively - harmless > changes... the Spring configuration changed (found an easy way to add the > resource classes with just a handful lines of code); some JAX-RS annotations > needed to be fixed (the one I remember was somewhere around document > management... path was not mapped properly). This one could be a nice PR and > would immediately help us get rid of the old Jersey stuff. > > Getting rid of the XML configuration is something I would like to see go > rather sooner than later, because it creates all kind of weird issues if you > want to bring in some other Spring Boot components (I had a case where I > added Apache Camel to the mix and just couldn't get it work with Spring Java > config and had to revert back to the XML configuration). Refactoring this > part would introduce some kind of backward incompatibility, but only > concerning configuration... which I find acceptable given the improvements. > Overall, doable and wouldn't be that much of a disruptive PR. > > Once the Spring configuration issues are solved then replacing the servlet > engine becomes really easy... that change can be offered as an optional > Gradle build task. Default distribution would still be Tomcat (because of the > licenses etc.), but people could choose to build themselves a Fineract Jetty > or Undertow distribution themselves. Again, low hanging fruit once we change > to Spring Java config. > > Cleaning up the REST API layer would be a bit of a bigger undertaking in > terms of code changes, but overall less disruptive than touching the DTO and > domain classes. The problem in that area is that - pretty much - all JSON > HTTP request bodies are first stored in a string, that string is then used > with those manually created mapper classes to deserialize them into Java > classes (DTOs); serialization works pretty much the same way. I think the > reason for all this manual code was the somewhat irregular structure of the > JSON objects. Since all this mapping code was created a lot of improvements > went into Jackson... including a lot of convenience concerning even the most > challenging mapping requirements (e. g. try to map Java generic types). I > think it's a given that the manual mapping cannot beat Jackson in terms of > maintenance and performance... so this change would be actually something > that I would really like to see. A nice side effect would be that we could > get rid of all those Swagger related helper classes; those were introduced to > help with the lack of type safety in the REST resource classes (aka JSON > string blobs). If we could pull this one off we could get rid of all manual > JSON mapping and the Swagger helper classes... I bet this would reduce the > code size by at least 10%. > > I think before we attack these items (and anything else that we come up with) > it would be worth splitting up the code first "as is" into separate modules > (JARs) and publishing them on Maven central. Once that happens we can start > applying above refactorings on a not so critical module (e. g. portfolio > client), take the lessons learned and apply them one by one to all other > modules. > > My 2 cents... pretty sure there are more ideas to be added here. > > Cheers, > > Aleks > > > On Sat, Oct 16, 2021 at 7:15 AM Petri Tuomola <[email protected] > <mailto:[email protected]>> wrote: > James - thanks for sharing this > > I think there would be a lot of interesting “mini-projects” that could be > undertaken to refactor / modernise Fineract 1.x. The benefit of such an > approach would be backward compatibility, allowing us to bring improvements > to all Fineract 1.x users without a migration. > > As you state, performance is definitely one of the key areas that we should > look to address. I’m personally not certain that PostgresQL / EclipseLink > would give significantly better performance than MySQL / OpenJPA, but it > would be interesting to performance test this. However, I’m pretty sure we > can get significant performance improvements simply by tuning the existing > SQL queries / schema / business logic. > > But to undertake this, we’d first need a robust performance test suite in > place to establish the baseline and then to measure the improvements. Writing > the tests themselves may not be that much of a challenge - but we’d need the > infrastructure to run these in a consistent environment on a regular basis. > AWS / GCP would be one option, though it requires at least some funding… but > perhaps there are some free options out there? > > The migration to PostgresQL would be interesting due to the licensing issues > with MySQL JDBC connector though. I started working on this earlier but > didn’t get a chance to finish. Will see if I can pick this up when I have > some spare time again. > > Another possible improvement would be to see if we could rework the code to > reduce the amount of “boilerplate” code for serialisation / validation / data > access. In a typical pull request today, a large percentage of the code is > there just to add the new parameter(s) to the APIs / database / function > signatures. Perhaps with some of the modern libraries we could reduce the > amount of code that is required to simply pass data around, making the > Fineract code more focused on business functionality (and hence easier to > manage/refactor/maintain). > > Anyway - just some thoughts. I’m unfortunately at the moment completely > swamped with work, but will see if I can pick up some of these ideas when I > have some free time again. @Aleksandar Vidakovic - it would be great to see > your list of improvement / modernisation ideas? > > Thanks > > Regards > Petri > > >> On 7 Oct 2021, at 02:52, James Dailey <[email protected] >> <mailto:[email protected]>> wrote: >> >> Devs - >> >> During ApacheCON, a few of the Fineract participants gathered in the usual >> "Birds of a Feather" - where people of like interests flock together - and >> one topic we took up was some general roadmapping for Fineract1.x >> >> A key discussion was around componentization of Fineract1.x, and here is my >> attempt to bring this rich discussion back to the list. @Edward Cable >> <mailto:[email protected]> , @Victor Manuel Romero Rodriguez >> <mailto:[email protected]> , @Javier Borkenztain >> <mailto:[email protected]> >> >> Aleksandar had proposed, during his ApacheCON @home2021 talk that day, an >> effort to REVISE and RATIONALIZE the underlying components to be more >> "rational", "streamlined", "better at high volumes", which amounted to some >> fairly substantial restructuring of the existing components. >> This idea may help with the code debt that has crept into the code over the >> last 10 years. It would make the project more easily maintained. >> It may also allow fineract1.x to evolve into possibly a better behaving >> "microservice" (fineract1.x acting as a microservice itself) or (again >> maybe) a composition of microservices. >> In particular, the class structure around products and portfolio was >> highlighted as being highly complex to deal with. Currently, code changes >> in one area could have unanticipated impacts in other areas and test >> coverage is not sufficient to catch all of these issues. >> So, in summary, we admired the problem a bit, and agreed to start on some of >> the more manageable smaller changes, whilst still looking to a more >> intensive re-write. @Aleksandar Vidakovic >> <mailto:[email protected]> : are you able to put some high level >> Tickets in Jira for this? This will take some serious effort and needs to >> be scoped out. >> >> Secondly, this rewrite/revision of Fineract1.x was then compared to the >> FineractCN codebase strategy which began life as a rewrite of the Fineract >> structure from the ground up. Some devs are currently proposing PRs related >> to the core microservices (e.g. Identity) based on their experiences using >> the existing code base in production. The evolution of FineractCN into a >> full release will need some attention from the Community to make happen. >> The proposal for how to get to a release is still at ==> >> https://cwiki.apache.org/confluence/display/FINERACT/Apache+Fineract+CN+Community+Roadmap >> >> <https://cwiki.apache.org/confluence/display/FINERACT/Apache+Fineract+CN+Community+Roadmap> >> . Let's start a separate thread on that. >> >> Thirdly, going back to Fineract1.x, one of the key issues is around the >> scalability and performance of the platform. A number of participants >> mentioned that fineract1.x is starting to be used in increasingly >> large-scale scenarios. As discussed on list previously, there are potential >> strategies, including replacing OpenJPA with EclipseLink ==> >> https://issues.apache.org/jira/browse/FINERACT-849 >> <https://issues.apache.org/jira/browse/FINERACT-849>, (thank you @Yemdjih >> Kaze Nasser <mailto:[email protected]> ) and migrating to a Postgres >> option (whilst maintaining backwards >> compatibility?)...https://issues.apache.org/jira/browse/FINERACT-984 >> <https://issues.apache.org/jira/browse/FINERACT-984>. There was some >> interest in moving that forward. >> >> There were a lot of good discussions at Birds of a Feather and some fun >> moments. If we missed you this time, there's always next. >> >> Thank you. >> James >
