Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/729 refactor settings releases #733

Merged
merged 17 commits into from
Feb 6, 2024

Conversation

StephGit
Copy link
Contributor

@StephGit StephGit commented Jan 24, 2024

Fixes #729

@StephGit StephGit requested a review from mburri January 24, 2024 12:49
@StephGit StephGit linked an issue Jan 24, 2024 that may be closed by this pull request
@StephGit StephGit marked this pull request as draft January 24, 2024 12:49
Copy link
Member

@mburri mburri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall - good work!
I have some minor comments and we have to discuss if it makes sense to further split up some of the angular components...

@StephGit StephGit force-pushed the feat/729-refactor-settings-releases branch from 084f9ce to b5b71bd Compare January 26, 2024 13:11
@StephGit StephGit marked this pull request as ready for review January 26, 2024 13:12
@StephGit StephGit requested a review from mburri January 26, 2024 13:14
@mburri mburri requested a review from yvespp January 30, 2024 12:17
@yvespp
Copy link
Member

yvespp commented Jan 31, 2024

Editing a release that has a deployment gives a Internal Server Error (500):

10:03:00,157 SEVERE [ch.mobi.itc.mobiliar.rest.exceptions.UncaughtExceptionMapper] (default task-15) Uncaught exception in rest call: javax.persistence.OptimisticLockException: Row was updated or deleted by another transaction (or unsaved-value mapping was incorrect) : [ch.puzzle.itc.mobiliar.business.releasing.entity.ReleaseEntity#505]
...
Caused by: org.hibernate.StaleObjectStateException: Row was updated or deleted by another transaction (or unsaved-value mapping was incorrect) : [ch.puzzle.itc.mobiliar.business.releasing.entity.ReleaseEntity#505]
...

How to reproduce with dev container:

  1. create a new release: test1
  2. create a deployment and select release test1
  3. click edit on the test1 release and without changing anything click save

=> Server error
All fields of a release should be changeable.

@yvespp
Copy link
Member

yvespp commented Jan 31, 2024

Editing a release that has a deployment gives a Internal Server Error (500):

Maybe this could be turned into an integration test? 😁

@mburri
Copy link
Member

mburri commented Feb 1, 2024

Editing a release that has a deployment gives a Internal Server Error (500):

10:03:00,157 SEVERE [ch.mobi.itc.mobiliar.rest.exceptions.UncaughtExceptionMapper] (default task-15) Uncaught exception in rest call: javax.persistence.OptimisticLockException: Row was updated or deleted by another transaction (or unsaved-value mapping was incorrect) : [ch.puzzle.itc.mobiliar.business.releasing.entity.ReleaseEntity#505]
...
Caused by: org.hibernate.StaleObjectStateException: Row was updated or deleted by another transaction (or unsaved-value mapping was incorrect) : [ch.puzzle.itc.mobiliar.business.releasing.entity.ReleaseEntity#505]
...

How to reproduce with dev container:

  1. create a new release: test1
  2. create a deployment and select release test1
  3. click edit on the test1 release and without changing anything click save

=> Server error All fields of a release should be changeable.

Reproducing the issue is much simpler:

  1. Create a release
  2. Edit the release (this will work the 1st time)
  3. Edit the same release again (this will never work again for this release, even after page loads and refreshes and server restarts)

Hibernate tracks concurrent modifacations of entities with a version property that is annotated with @Version

For the Release entity, this property is also annotated with @JsonIgnore, that is why it never reaches the client. Since the client does not know the property, it is not sent to the backend on update. When the json request is mapped back to the entity, the missing version property is assumed to have the value 0 which is correct for the first update. Hibernate then increases the value to 1. Subsequent updates will fail because hibernate compares the two values (they should be the same) and rejects the update, since it seems to be old data.

Removing the @JsonIgnore annotation an the version property should do the trick. But the client also needs to update its state... otherwise uses have to reload the page.

@yvespp
Copy link
Member

yvespp commented Feb 1, 2024

AFAIK we never exposed the Version in the rest entities. I believe this was handled by merging the entity back into the jpa context before persisting it.

@mburri
Copy link
Member

mburri commented Feb 2, 2024

AFAIK we never exposed the Version in the rest entities. I believe this was handled by merging the entity back into the jpa context before persisting it.

I've just checked all endpoints and this is true: the version property was never exposed. But, there are currently only a few endpoints that update an existing entity. I've checked them all. All of them accept a bunch of query parameters and/ or some form of a DTO. The backend then loads the entity, sets the new properties and then writes it back to the database. That's why there was never a problem with optimistic locking, and there are checks in place to prevent that different users to overwrite each others changes.

But in case of the new endpoint to edit releases, it's indeed the version property that prevents the entity being merged back into to the jpa context - that is when hibernate compares the property and throws the StaleObjectStateExcpetion in this case.

I strongly recommend to pass the version property to the frontend (and then send it back with the PUT request) in order to prevent users overwriting each others changes. Even if there are only a few users that use the application and they are infrequently creating or updating releases.

@StephGit
Copy link
Contributor Author

StephGit commented Feb 2, 2024

I quickly tested the current jsf implementation without exposing the version property. If two users edit the same entity the update fails for one user throwing javax.ejb.EJBTransactionRolledbackException: Row was updated or deleted by another transaction.

@yvespp
Copy link
Member

yvespp commented Feb 2, 2024

I quickly tested the current jsf implementation without exposing the version property. If two users edit the same entity the update fails for one user throwing javax.ejb.EJBTransactionRolledbackException: Row was updated or deleted by another transaction.

This is probably because JSF is stateful and keeps the entity with the version in the session?
I don't know what the best practises are with rest but something like the version or timestamp of the last modification seems to be needed.

@StephGit
Copy link
Contributor Author

StephGit commented Feb 2, 2024

I added the version in the frontend and wrapped the exception in a ConcurrentModificationException

@yvespp
Copy link
Member

yvespp commented Feb 2, 2024

Tested and works as expected, you can merge when ready

@yvespp yvespp added this to the Version 1.17.35 milestone Feb 2, 2024
@StephGit StephGit force-pushed the feat/729-refactor-settings-releases branch from 5159ef3 to b5df057 Compare February 6, 2024 07:44
@StephGit StephGit merged commit 6549fa5 into master Feb 6, 2024
1 check passed
@StephGit StephGit deleted the feat/729-refactor-settings-releases branch February 6, 2024 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace JSF GUI in Settings -> Releases page with Angular
3 participants