MARLO hibernate entity use is fundamentally flawed - making bug fixes extremely time consuming

[Grant-Lay]

Currently at the moment MARLO is using hibernate entities the wrong way.

Generally most hibernate applications either do one of the following (sometimes a combination of both):

1. Map between Client exposed DTOs (Data transfer objects) and Hibernate entities. A data transfer object is a separate class to the entity and generally has no behavior (i.e. methods) besides possibly getters and setters. Using this architecture pattern and FreeMarker, the templates would bind not to the entity but to the DTO which would be accessible as a variable on the Struts Action.

2. Expose hibernate entities directly to the client and have FreeMarker bind directly to the entity. When the HTML form is submitted, the entity is then merged including its collections using the CASCASE.MERGE option.

From first appearances it would seem that MARLO uses the no2 pattern above, however on closer inspection it does something very very unusual and something that is definitely considered bad practice. For properties on the entity itself (e.g. Entity.myProperty) it allows the FreeMarker templates to bind to, but for the hibernate collections it creates a brand new collection on the entity (usually a list). For example in the entity FundingSource we have two collections:

1. List \<FundingSourceBudget> budgets;
2. Set \<FundingSourceBudget> fundingSourceBudgets;

From there we can see in the FundingSourceAction struts2 class, the prepare and save methods are busy copying from one collection to the other.

So what is wrong with the current pattern?

1. Because we now have a single hibernate transaction per HTTP request, if you make changes to a managed entity including fields that are not persisted to the database, the entity will trigger hibernate's dirty checking mechanism. So for example if your FreeMarker template add items to your budgets collection, then you will need to clear the list before the entity is flushed otherwise you will get a very hard to find exception as hibernate will complain that you have changed something.

2. The code which iterates through each collection and copies values to the other collection is very verbose and potentially error prone.

3. Because this pattern is so unusual, new developers have difficulty understanding what is going on, hence the calls for more documentation.

Therefore I suggest we move away from this architecture. We either choose option 1 above - create new DTOs and map to and from them, or we bind both the entities and their associated collections to the Freemarker templates (option 2).

I think it will be easier to do 1, as if we keep the variables the same as they are now (e.g. we create a collection on the DTO called budgets) we can avoid having to make changes to the Freemarker templates, but happy to hear other's feedback.

BTW if we do decide to use option 1, there are mapping frameworks we can use to avoid having to write our own mapper classes. One that I have recently used is MapStruct.

[htobon]

@Grant-Lay, @jurodca, is there any advance on this? I'm quite lost on how to tackle this issue. Could you both please meet and propose a workplan for this? Taking into account the current priorities.

Perhaps we could have a quick solution, and then scale it progressively. Does it make sense?

I'm happy to meet too.

Cheer, Hector

[Grant-Lay]

@htobon @jurodca I have implemented a DTO to entity mapping solution (using the mapStruct library) as part of the REST api changes on branch dev-add-marlo-web-services. We can use this as the pattern for creating new DTOs and mapping from entity to DTO and vice-versa.

I'm not sure how easy it will be to implement this in a gradual approach rather than all together and I think the mapping from DTO to entity will require us to start using cascade updates and deletes (rather than save and delete the child entities individually), so that will require a bit of thinking as well as some testing to make sure that we don't inadvertently break other pieces of functionality.

[jurodca]

@Grant-Lay I will look at your branch and will try understand the main concept behind during the day of today. Maybe on Monday 15th I will start asking you some questions directly in slack or by email (Hope this is ok).

I have also made an implementation with DTO using MapStruts some time ago in the branch "dev_agreements". So, I am going to compare both perspectives in order to get into a single point of view.

Grant, Do you think that the best way is to do it all at once? Or should we start this section by section? Impact Pathway, Projects, FS etc.

[jurodca]

Hi again @Grant-Lay. I looked at the implementation of MapStruct made in your branch and I don't have any questions about it. It's quite similar to what I did in my branch (dev_agreements).

I think we can use your implementation as a standard for new developments or for modifying the existing modules.

My idea then is to find a space at some time next week so we could talk about this, probably Wednesday 24th in my night. @Grant-Lay Can you please check the email I will send in a couple of minutes?

[Grant-Lay]

Hi @jurodca. I think it will be difficult (but not impossible) to do in sections. Perhaps the larger divisions (all entities in impact pathway, all entities in projects etc) you made in the comment above is what we should aim for rather than individual entities within the application.

However if we did it all at once, perhaps it would be a good chance to move from hibernate.xml files to JPA annotations?

[htobon]

Hi @Grant-Lay, 9pm as you suggested in the email would be ok.

[Grant-Lay]

After speaking to @jurodca - we have decided to try and implement a DTO pattern using Funding sources as a proof of concept. We should start work on this next week.

[htobon]

Thanks for the update. I'll move this issue to the next sprint.

[htobon]

@jurodca, can you please describe here what is going to be done in this activity as part of the current sprint? Milestone v4.2.4. Thank you in advance.

[jurodca]

Hi @htobon . After the conversation with @Grant-Lay , we decided to take the Funding Sources section as a pilot to develop an architecture based on DTO using MapSctruct.

The plan to follow is the following

1. Create new branch dev-dto-funding-sources
2. Create necessary DTOs based on the current Funding Sources logic
3. Pass the current logic of the actions used in Funding Sources at the level of managers
4. Reset the web part to recognize the created DTOs
5. Reset validations
6. Tests

The idea is that weekly dedicate approximately 5 hours of work and with @Grant-Lay review the work done and make the necessary adjustments.

[htobon]

Hi @jurodca, thanks for the update. Please go ahead with the proposed work-plan. Cheers.

[Grant-Lay]

@jurodca - should we merge the dev-add-marlo-web-services branch into dev-dto-funding-sources so that the mapstruct dependencies become available?

[jurodca]

Hi @Grant-Lay, can you check the branch dev-dto-funding-sources if it is neccesary? Or you think it's better to do the merge.

[Grant-Lay]

Hi @jurodca, I'm not sure how much work that you have done on this but perhaps you can create a new branch using dev-add-marlo-web-services as your starting point and then copy your changes into that branch?

[htobon]

Hi @jurodca, can you please let us know the status of this issue? I'll move this issue into "New" because I have not seen any progress on it. Thank you in advance.

[Grant-Lay]

Just a quick note that for read only screens (e.g. summary screens where the user can't edit an entity), you can refer to this commit as to how to implement a entity to DTO conversion.