... 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 >> >> >>
