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

repository.save() does not update the returned entity when fields are marked updatable=false [DATAJPA-1421] #1735

Closed
spring-projects-issues opened this issue Sep 11, 2018 · 7 comments
Assignees
Labels
status: declined A suggestion or change that we don't feel we should currently apply type: bug A general bug

Comments

@spring-projects-issues
Copy link

Jason Bodnar opened DATAJPA-1421 and commented

I'm using Spring Data's annotations to add auditing data to my entities when they are saved or updated. When I create the entity the createdBy, createdDate, lastModifiedBy and lastModifiedDate get set on the object returned by repository.save(). Unfortunately, when I call repository.save() to update an existing entity the object returned does not have the createdBy and createdDate set. All the fields are set correctly in the database though


Affects: 2.0.10 (Kay SR10)

Reference URL: https://github.com/jcbodnar/auditing-example

@spring-projects-issues
Copy link
Author

Jens Schauder commented

This behavior is not caused by the auditing feature setting values or not. You are essentially resetting the fields to null.

Here is what is happening:

  1. You save the first instances i1. Fields get set and since your transaction only spans the call to the repository changes get flushed as expected.

  2. You create a fresh instance i2. That instance doesn't have any of the auditing fields set.

  3. You invoke save.

    a) Which does a JPA merge, which loads the data from the original instance, creates a new one i3. Then it copies the data from i2 over into i3. The auditing fields are now null since that are the values from i2.

    b) The auditing listener kicks in and updates the lastModified and lastModifiedBy fields. It does not change the created... fields since it's not a new instance, but an update, so those stay null.

    c) The changes get flushed to the database, but not the created... fields since those are annotated with updatable=false so they never get updated.

  4. you compare the created... fields of i3 (null) with those of i1 (not null) and the test fails

@spring-projects-issues
Copy link
Author

Jason Bodnar commented

Then the docs need to be updated:

https://docs.spring.io/spring-data/commons/docs/current/api/org/springframework/data/repository/CrudRepository.html#save-S-

Use the returned instance for further operations as the save operation might have changed the entity instance completely.
 
If I used the instance returned by the save operation then I would be using an instance that does not have the created fields that exist on the resource in the repository

@spring-projects-issues
Copy link
Author

Jens Schauder commented

If I used the instance returned by the save operation then I would be using an instance that does not have the created fields that exist on the resource in the repository.

Yes, because you set them to null and also made the updatable=false, so this is exactly what should happen.
See step 3a from above.

While I see that the combination of merging instances and non-updatable fields can be confusing I don't see what is to be improved about the documentation of the save method. Please clarify

@spring-projects-issues
Copy link
Author

Jens Schauder commented

Batch closing resolved issue without a fix version and a resolution indicating that there is nothing to release (Won't fix, Invalid ...)

@spring-projects-issues spring-projects-issues added type: bug A general bug status: declined A suggestion or change that we don't feel we should currently apply labels Dec 30, 2020
@kilaka
Copy link

kilaka commented Nov 19, 2023

My thought is that when a field is marked updatable=false, I would expect the original value to be used in the audit/merge process.
Another thought is for another annotation to always load the original values when a field is updatable=false, but this feels like it should be the default behavior.

Jens Schauder commented

This behavior is not caused by the auditing feature setting values or not. You are essentially resetting the fields to null.

Here is what is happening:

  1. You save the first instances i1. Fields get set and since your transaction only spans the call to the repository changes get flushed as expected.
  2. You create a fresh instance i2. That instance doesn't have any of the auditing fields set.
  3. You invoke save.
    a) Which does a JPA merge, which loads the data from the original instance, creates a new one i3. Then it copies the data from i2 over into i3. The auditing fields are now null since that are the values from i2.
    b) The auditing listener kicks in and updates the lastModified and lastModifiedBy fields. It does not change the created... fields since it's not a new instance, but an update, so those stay null.
    c) The changes get flushed to the database, but not the created... fields since those are annotated with updatable=false so they never get updated.
  4. you compare the created... fields of i3 (null) with those of i1 (not null) and the test fails

@agarcia-te
Copy link

agarcia-te commented Jun 18, 2024

@schauder

I believe it'd be helpful to focus on this from a different perspective.

Regardless of the reason why this issue occurs, the issue is still valid.

The purpose of this library is as follows:

Spring Data provides sophisticated support to transparently keep track of who created or changed an entity and when the change happened.

Extract from: https://docs.spring.io/spring-data/jpa/reference/auditing.html

Created fields should not be allowed to be updated otherwise they are no longer fulfilling its purpose.

I disagree with the following statement:

because you set them to null and also made the updatable=false, so this is exactly what should happen.

updatable=false means the field shouldn't be updated (included in update statements), it does not imply that it cannot be read.

That being said, even if we assume that to be true. If this is not the way to achieve this functionality, I believe an alternative should be provided.

If we have to either:

  • manually be loading and and setting the created fields
  • manually validating / rejecting changes to created fields

Then the library is no longer transparently keeping track of who created or changed an entity is it?

@sen-harshit03
Copy link

I agree with @agarcia-te. The primary purpose of auditing features is to ensure users see accurate timestamps when entities are created or updated.
In our API, when we create an entity and return the saved entity as a response, fields annotated with @LastModifiedDate and @LastModifiedBy are intended to remain empty in the database because we have configured them with insertable = false. However, after saving the entity, these fields are unexpectedly populated in the returned object from the save() operation.
As a result, the API response contains incorrect values for these fields. This discrepancy means that the frontend might display a value for last modified date and last modified by, but in the database, it remains null as expected. Such inconsistencies can lead to data integrity issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply type: bug A general bug
Projects
None yet
Development

No branches or pull requests

5 participants