repository.save() returns duplicate child-elements (at least w/ postgresql) [DATAJDBC-456]

[spring-projects-issues]

Clemens Hahn opened DATAJDBC-456 and commented

I will explain this behavior based on the [example-project|https://github.com/spring-projects/spring-data-examples/blob/master/jdbc/basics/README.adoc].

I added a new child-entity to the LegoSet-Entity of type Color. LegoSet references this entity via Set<Color> colors.

• create a LegoSet
• add one Color
• call legoSetRepository.save(legoSet) and inspect the returned value
• the Set<Color> colors contains 2 Colors.

This does not happen using the embedded H2-Database! To reproduce it you can use a Postgresql Database!

To (hopefully quickly) reproduce this bug please apply the attached patch-file to the example-repo and execute the following (manual) steps:

• create a local postgresql-database: docker run --name spring-data-jdbc-kotlin-test -e POSTGRES_USER=integrationtest -e POSTGRES_PASSWORD=password -e POSTGRES_DB=spring-data-jdbc-kotlin -p 5438:5432 -d postgres:11
• create the schema in that database with src/main/resources/schema.sql (using a database-tool)
• start the test example.springdata.jdbc.basics.aggregate.AggregateTests#exerciseSomewhatComplexEntity
• the test will fails with:

java.lang.AssertionError: 
 Expected size:<1> but was:<2> in:
 <[Color(lego_set=1, r=255, g=0, b=0), Color(lego_set=1, r=255, g=0, b=0)]>

---

Attachments:

• DATAJDBC-456_patch_repo_save_duplicate_elements_set.txt (6.66 kB)

[spring-projects-issues]

Jens Schauder commented

The problem is in the construction of your class.

Since it is annotated with Lomboks @Data annotation its equals and hashcode implementations consider all fields.
But due to it's mapping of the lego_set column. That equality changes during saving.

In this case I recommend one of the following mappings:

Remove the lego_set/id column completely. You would only need an id if you reference further objects from the Color entity.
Also make Color immutable, because changing a color value after it got added to a Set will cause trouble.

Alternatively: rename the id column to e.g. id and give it some useful value during construction: Make it a String concatenating the color values, a name. Base equals and hashcode on that. This will result in an additional field in the database but is necessary if your entity will reference further entities.

[spring-projects-issues]

Clemens Hahn commented

Thank you for this investigation! Makes sense!

This results in a few further questions about that topic:

I need the ID-field on my sub-entities (will be exposed via a REST-API) - so removing this field is not an option for the moment.

I'm a fan of immutability. So I would like to make all fields of my entities immutable (btw: normally I write all entities as kotlin data classes). I am using UUIDs as IDs. This IDs are not auto-generated by my database. Therefore the application must handle this. Currently I use a ApplicationListener<BeforeSaveEvent> to set new IDs when detecting that the ID is null.

But because of Entity State Detection Strategies I have to work with a mutable ID-field (as far as I can survey spring-data-jdbc for now):

Problems with an immutable ID-field:

• of course I know when creating new entities in my application code. But if I set an ID in that moment spring-data would interpret this entity as not-new?
• working with a nullable ID-field: the org.springframework.data.relational.core.mapping.event.WithEntity has no setter; therefore I have to change the same instance of my entity to set an ID -> the ID-field must be mutable. If I could exchange the whole instance I could use a copy-method to set a random UUID.

I would appreciate if you could give me your thoughts about this situation and I hope this is not the wrong place for this discussion

[spring-projects-issues]

Jens Schauder commented

1. we are talking here about an entity inside an aggregate, not the aggregate root. I would expect a REST API to align quite nicely with aggregate boundaries. Therefore I'm surprised that a REST API requires an id in such an inner entity. But of course I don't know you full requirements.

2. What you say about the entity state detection strategy is correct but again only applies to aggregate roots. Entities referenced from the root in most cases don't need an id and if they have an id it does not get considered for entity state detection.

3. If we are talking about aggregate roots you have multiple options:
   a) Use an EntityCallBack. It is like an event but is allowed to create and return a new entity. BeforeSaveCallback is the one you are looking for.

b) use JdbcAggregateTemplate.insert it works like the save of a Repository but always tries to make an insert. You may use it either directly or implement a custom method in your repository based on it.

c) use a version attribute on the aggregate root. This is only available in 2.0.0-SNAPSHOT releases and there are some breaking changes coming up so you probably don't want to use it in production yet. But for longer term planning this might be an option. The version attribute will be used for state checking and it will be increased on your entity and in the database for you. See DATAJDBC-219.

Since we have some differentiation going on between aggregate roots and other entities I highly recommend reading this article about aggregates and their relevance for Spring Data JDBC if you haven't already

[spring-projects-issues]

Clemens Hahn commented

1. correct - I also talk about an entity inside an aggregation. Thinking in your example PurchaseOrder and OrderItem for me it makes sense to provide API-Entpoints like {{GET /purchases/

{purchaseOrderId}

}} and {{GET /purchases/{purchaseOrderId}/items/

{orderItemId}}} or even {{GET /orderitems/{orderItemId}

}}. (I believe this point is out of scope of this discussion. But I appreciate your input!)

2. Thanks for this clarification!

3a. Thank your for suggested the EntityCallback! This works much better for me than the ApplicationListener approach. Btw: I read the JavaDoc of your suggested BeforeSaveCallback and therefore (and after trying it out) I think the BeforeConvertCallback is the callback for my use-case (setting the ID)?

3c. Using spring-data-jpa I liked the approach to detect the entity state via the version-field! As I noticed that Optimistic Locking is not yet supported by spring-data-jdbc that was not an option for me anymore. I'm happy that you make progress in that point and as soon as it become stable I will give it a try!

I played with all this input in an example project. This example is written in kotlin. It extends your spring-data-example project.

• if you are interested in "my" approach using spring-data-jdbc please have a look into my fork.
• if you think this (koltin) example is good enough to help others I'm happy to share it (with a pull request..)

[spring-projects-issues]

Clemens Hahn commented

Sorry - Jira destroyed the format of my API-Endpoints (in 1.) and I'm not allowed to edit it:

GET /purchases/{purchaseOrderId}
GET /purchases/{purchaseOrderId}/items/{orderItemId}}} 
GET /orderitems/{orderItemId}