8000 [DE-539] Feature #291 CRUD save returning server result by aburmeis · Pull Request #295 · arangodb/spring-data · GitHub
[go: up one dir, main page]
Skip to content

[DE-539] Feature #291 CRUD save returning server result #295

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Feb 6, 2024
Merged

[DE-539] Feature #291 CRUD save returning server result #295

merged 7 commits into from
Feb 6, 2024

Conversation

aburmeis
Copy link
Contributor
@aburmeis aburmeis commented Jan 9, 2024 •
edited by rashtao
Loading

SimpleArangoRepository.save() and saveAll() can now be configured to return the entity created from the server result instead of the original (#291). As the may lead to unexpected behaviour, the default is using the old behaviour.

The configuration can be done by a RepositoryFactoryCustomizer, which can be prepared in an upcoming spring-boot-starter version.

fixes #291

@cla-bot cla-bot bot added the cla-signed label Jan 9, 2024
@rashtao
Copy link
Collaborator
rashtao commented Feb 2, 2024

Hi @aburmeis ,

in the Spring Data Commons reference documentation you can find a chapter about Property population, see https://docs.spring.io/spring-data/data-commons/docs/current/reference/html/#mapping.property-population, stating the following:

  1. If the property is immutable but exposes a with… method (see below), we use the with… method to create a new entity instance with the new property value.
  2. If property access (i.e. access through getters and setters) is defined, we’re invoking the setter method.
  3. If the property is mutable we set the field directly.
  4. If the property is immutable we’re using the constructor to be used by persistence operations (see Object creation) to create a copy of the instance.
  5. By default, we set the field value directly.

This logic is implemented by org.springframework.data.mapping.PersistentPropertyAccessor, which should be used to set all the persistent properties (via setProperty()) and ultimately to create the object to return invoking getBean(). Depending on whether the properties that need to be updated are immutable or mutable, a new instance of the entity is created or the existing one is updated and returned.

The current implementation of Spring Data ArangoDB uses PersistentPropertyAccessor only to set the persistent properties and it never invokes getBean(). This means that the original properties are updated, if mutable. But then, ArangoOperations methods always return a new entity instance and never the same instance passed as parameter argument (e.g. in ArangoOperations#repsert(T)).

Changing this would be now a breaking change, which we could implement in the next major release. So for the moment I would suggest keeping the current behavior in ArangoOperations, thus always returning new entities and possibly modifying the original one.

On the other side, ArangoRepository.save() and ArangoRepository.saveAll() always return the original entities, potentially modified.

In addition to the problems you mentioned, this does not work with entities with immutable properties being updated from the server side (like auto-generated _key, _id, _rev and db computed values).

This also prevents the use of ArangoRepository.save() with Java records and Kotlin immutable data classes.

As temporary work-around, we can go with the solution you submitted in this PR, but note that this will likely change again in the next major release to reflect the behavior reported above.

Thanks for contributing!

@rashtao
Copy link
Collaborator
rashtao commented Feb 2, 2024

fixes #291

@rashtao rashtao changed the title Feature #291 CRUD save returning server result [DE-539] Feature #291 CRUD save returning server result Feb 2, 2024
@rashtao rashtao self-assigned this Feb 2, 2024
@rashtao rashtao self-requested a review February 2, 2024 11:48
@rashtao rashtao merged commit dfeb275 into arangodb:master Feb 6, 2024
@aburmeis aburmeis deleted the feature/crud-save branch February 26, 2024 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rashtao rashtao rashtao approved these changes

Assignees

@rashtao rashtao

Labels
cla-signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CRUD Repository save and saveAll should return fetched entity instead of original
2 participants
@aburmeis @rashtao
0