... agree... in the end the goal is to cleanup and improve the code quality... if we do this with Lombok or with IDE tools that automatically generate getters, setters, toString etc then that's also fine.
Probably the important tool to add would be ArchUnit to enforce a minimum compliance and actually break builds if someone gets too "creative" and ignores basic concepts like Java beans spec... On Fri, 26 Jul 2024, 20:29 Márta Jankovics, <marta.jankov...@dpc.hu> wrote: > Hi Alex, > > I agree with you, all these things you mention are definitely the benefit > of lombok. > But... > Lombok is just a good tool which can be used well and not-so-well. If we > understand what is behind the annotations then we can use it really well. > Please consider my arguments below and apply the advised limitations. > > Thank you, > Marta > > > On 26 Jul 2024, at 19:36, Aleksandar Vidakovic <chee...@monkeysintown.com> > wrote: > > ... note: another motivation to put Lombok in place was to avoid all these > "creative" naming patterns. > > On Fri, Jul 26, 2024 at 7:35 PM Aleksandar Vidakovic < > chee...@monkeysintown.com> wrote: > >> ... just wanted to mention one more: in some of the (domain) classes we >> have all sorts of typos and other "errors": some people came clearly from a >> PHP background and used snakecased naming instead of camelcased... if you >> put Lombok in those classes then it will strictly apply Java Bean standards >> etc. and things might look very weird... just to say that some of those >> classes need to be repaired anyway, Lombok or not. >> >> On Fri, Jul 26, 2024 at 7:05 PM Márta Jankovics <marta.jankov...@dpc.hu> >> wrote: >> >>> Hi Zeyad, >>> >>> Thank you for bringing up this topic. I did not participate in the >>> previous discussion that Alex mentioned, I just realised that entities >>> change one after other nowadays by adding lombok annotations all over >>> fineract and I must say that I do not agree with some of these changes. >>> Although I see the benefit of lombok of course in reducing the amount of >>> boilerplate code, improving readability and allowing developers to work >>> more efficiently, but I think we should consider some limitations. Lombok >>> is perfectly fine for DTOs but not for persistent entities. >>> Entities are business objects and yes they do some validations which can >>> not be done in validators - because the entity encapsulates the information >>> which is simply not visible for a validator - and also can not be done by >>> the database layer or could be but it is too late and the error message is >>> too ugly. >>> Let me bring some example: >>> - CommandSource is the representation of the audit entry which can not >>> be ever changed by anyone after it was persisted and it can not be >>> created without main audit information like entity- and action names, >>> request url, status etc. >>> I do not say it is readOnly because there is one case - the 4 eye >>> workflow - when the checker related fields can be updated later but nothing >>> else. >>> To add a public @Builder to this entity is not correct because then >>> someone might think that it can be created without these crucial data. >>> Adding public @Setter here is also not good, because only one or two >>> fields can be set after creation. >>> Even adding a public @NoArgsConstructor is not correct because of the >>> same reason. >>> - In general @AllArgsConstructor on persistent entities can only be used >>> if we do not have to also set the fields that only the framework can >>> set like auto generated id, Auditable fields (createdAt, createdOn, >>> updatedAt, updatedOn). >>> - Other entity types where we must be really careful with public >>> setters and constructors are the link entities. For example >>> ProductToGLAccountMapping.glAccount or productId can never be updated, >>> because these fields are like identifiers. The entity can be deleted and >>> recreated but never updated on these fields. And the entity can not be >>> created without these data. >>> Of course the data layer would catch this but as I mentioned above it >>> is too late. >>> >>> So what I suggest for most of the entities (there might be be some >>> exceptions as always): >>> - I would not add public Builder, NoArgsConstructor to persistent >>> entities - non-public should be fine and even a good choice. >>> - Choose the way of initialisation: either the public constructor (for >>> simpler entities) or the static initialiser (for entities more complex), >>> but do not publish both. >>> Having static initialisation and using builder (non public) framework I >>> think it is ok. >>> - Be very careful with public Setter and annotate the fields rather than >>> the entity itself (btw OTM list properties also can not be set). >>> >>> I think it is a good practice to use lombok with JPA entities but only >>> with these limitations. What do you think? >>> >>> Cheers, >>> Marta >>> >>> On 25 Jul 2024, at 19:20, Zeyad Nasef <zeyad.nasef....@gmail.com> wrote: >>> >>> Hi everyone, >>> >>> I hope this message finds you well. >>> >>> I would like to discuss the use of the @Builder annotation for entity >>> classes in Fineract. In a recent pull request #3984 >>> <https://github.com/apache/fineract/pull/3984/files>, I introduced the >>> @Builder pattern for entity creation. However, upon further discussion with >>> @Adam >>> Saghy <adamsa...@apache.org>, some concerns have emerged that warrant >>> community feedback: >>> >>> *Concerns with Using @Builder for Entities:* >>> >>> - Incompatibility with The ORM (interfere with lazy loading, entity >>> management ...etc.) >>> - No validation or transformation enforcement when instantiating the >>> entity. >>> - Builder pattern might be confusing and let others to set or >>> override things that should not be. >>> >>> *Some Proposed Solutions:* >>> >>> - Adding `@NoArgsConstructor` & `@AllArgsConstructor` for resolving >>> the JPA issues. >>> - Implementing methods like fullEntryFrom() >>> >>> <https://github.com/apache/fineract/blob/develop/fineract-core/src/main/java/org/apache/fineract/commands/domain/CommandSource.java#L136> >>> to encapsulate the creation logic, thereby enforcing validation and >>> transformations while utilizing the builder pattern to avoid large >>> constructors with many parameters. >>> - Limiting the builder’s access to private or package-private scope >>> and using @Builder.Default to provide specific methods for controlled >>> instantiation. >>> >>> >>> Your input on this matter would be greatly appreciated. >>> >>> Best regards, >>> Zeyad Nasef >>> >>> >>> >
