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