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 <mailto: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 >> <mailto: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 >>>> <mailto: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 <mailto: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 >>>
