Add support for composite ids #1957

schauder · 2024-12-12T13:20:55Z

Entities may be annotated with @Id and @Embedded, resulting in a composite id on the database side.
The full embedded entity is considered the id, and therefore the check for determining if an aggregate is considered a new aggregate requiring an insert or an existing one, asking for an update is based on that entity, not its elements.
Most use cases will require a custom BeforeConvertCallback to set the id for new aggregate.

For an entity with @Embedded id, the back reference used in tables for referenced entities consists of multiple columns, each named by a concatenation of + _ + .
E.g. the back reference to a Person entity, with a composite id with the properties firstName and lastName will consist of the two columns PERSON_FIRST_NAME and PERSON_LAST_NAME.
This holds for directly referenced entities as well as List, Set and Map.

Closes #574

Entities may be annotated with `@Id` and `@Embedded`, resulting in a composite id on the database side. The full embedded entity is considered the id, and therefore the check for determining if an aggregate is considered a new aggregate requiring an insert or an existing one, asking for an update is based on that entity, not its elements. Most use cases will require a custom `BeforeConvertCallback` to set the id for new aggregate. For an entity with `@Embedded` id, the back reference used in tables for referenced entities consists of multiple columns, each named by a concatenation of <table-name> + `_` + <column-name>. E.g. the back reference to a `Person` entity, with a composite id with the properties `firstName` and `lastName` will consist of the two columns `PERSON_FIRST_NAME` and `PERSON_LAST_NAME`. This holds for directly referenced entities as well as `List`, `Set` and `Map`. Closes #574 Original pull request #1957

Original pull request #1957 See #574

Adds support for id generation by sequence as part of a composite id. Added a proper test for sorting by composite id element. Added a stand in test for projection by composite id element. The latter does not test the actual intended behaviour since projection don't work as intended yet. See #1821 Original pull request #1957 See #574

Removed unused assignments. Original pull request #1957 See #574

mp911de

This is quite a PR with a lot of changes. I did a review pass on it capturing most impressions as comments. This PR introduces a lot of accidental complexity making the actual changes much more difficult to understand. I think rewriting some parts to simpler forms that make concepts more explicit (instead of implicit ifs or for-loops that do something) helps to understand the code.

Assertions that assert more than three items in a row exhaust my capability to understand these, also asserting tuples might technically work but that leads to test code that is much more difficult to understand than what is the actual functionality tested. If we continue that way, you'll be the sole person that is able to maintain the project.

I tried to make suggestions as concise as possible for improvements to reduce conceptual (and code) duplications.

mp911de · 2025-03-19T13:23:23Z

...data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/MappingJdbcConverter.java

@@ -403,6 +388,29 @@ public <T> T getPropertyValue(RelationalPersistentProperty property) {
 			return (T) delegate.getValue(aggregatePath);
 		}

+		private Identifier constructIdentifier(AggregatePath aggregatePath) {
+
+			Identifier identifierToUse = this.identifier;


This looks pretty similar to what JdbcIdentifierBuilder is doing. There's a high likelihood of duplicating code. Please check what parts could be refactored to avoid duplication of concepts.

mp911de · 2025-03-19T13:24:24Z

...data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/MappingJdbcConverter.java

@@ -449,7 +457,7 @@ public boolean hasNonEmptyValue(RelationalPersistentProperty property) {
 					return delegate.hasValue(toUse);
 				}

-				return delegate.hasValue(aggregatePath.getTableInfo().reverseColumnInfo().alias());
+				return delegate.hasValue(aggregatePath.getTableInfo().reverseColumnInfos().any().alias());


Related: ColumnInfos methods should be documented.

mp911de · 2025-03-19T13:32:26Z

spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/SqlGenerator.java

+
+	/**
+	 * Constructs a function for constructing a where condition. The where condition will be of the form
+	 * {@literal <columns> IN :bind-marker}


lt/gt signs conflict with Javadoc. These should be escaped.

It is escaped by the @literal if I'm not mistaken.

From the What's new announcement, when it was introduced:

New tags {@literal} and {@code} The {@literal} tag denotes literal text. The enclosed text is interpreted as not containing HTML markup or nested javadoc tags. For example, the doc comment text: {@literal ac} displays in the generated HTML page unchanged: ac -- that is, the is not interpreted as bold.

IntelliJs Javadoc renderer seems to agree with me.

Note that the comment above does not get rendered correctly. See the source for what it is trying to say.

mp911de · 2025-03-19T13:32:41Z

spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/SqlGenerator.java

+
+	/**
+	 * Constructs a function for constructing a where. The where condition will be of the form
+	 * {@literal <column-a> = :bind-marker-a AND <column-b> = :bind-marker-b ...}


See above for Javadoc lt/gt chars.

mp911de · 2025-03-19T13:35:55Z

spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/SqlGenerator.java

 		this.columns = new Columns(entity, mappingContext, converter);
 		this.queryMapper = new QueryMapper(converter);
 		this.dialect = dialect;
+
+		inCondition = inCondition();


Why are these modelled as functions? Generally, functions are neat, however these introduce another indirection to already hard-to-read code.

mp911de · 2025-03-19T14:25:44Z

...nal/src/test/java/org/springframework/data/relational/core/mapping/ColumnInfosUnitTests.java

+	record CompositeId(String one, String two) {
+	}
+
+	record DummyEntityWithCompositeId(@Id @Embedded.Nullable CompositeId id, String name) {


Dummy 🙈

mp911de · 2025-03-19T14:27:20Z

...est/java/org/springframework/data/relational/core/mapping/DefaultAggregatePathUnitTests.java

+
+		assertSoftly(softly -> {
+
+			softly.assertThat(path("second").append(path()).toDotPath()).isEqualTo("second");


There's so much to read in these test style that the actual test fixture and test code becomes invisible.

Please use much simpler assertions, otherwise I don't understand what is happening here.

Applies also for many other places in this project.

mp911de · 2025-03-19T14:27:32Z

...nal/src/test/java/org/springframework/data/relational/core/sql/TupleExpressionUnitTests.java

+
+import org.junit.jupiter.api.Test;
+
+class TupleExpressionUnitTests {


Nit: Javadoc

mp911de · 2025-03-19T14:28:07Z

...c/test/java/org/springframework/data/relational/core/sql/render/SelectRendererUnitTests.java

+	 * Tests for rendering joins.
+	 */
+	@Nested
+	class JoinsTests {


Are these tests actually invoked during mvn verify?

mp911de · 2025-03-19T14:32:53Z

src/main/antora/modules/ROOT/partials/mapping.adoc

+[[entity-persistence.embedded-ids]]
+=== Embedded Ids
+
+Entities may be annotated with `@Id` and `@Embedded`, resulting in a composite id on the database side.


I wonder whether we could simplify our detection. If a simple type is annotated with @Id, we consider it a simple identifier. Any non-simple type annotated with @Id is automatically considered an embedded/composite Id without the need to use @Embedded. @Embedded raises the question of Embedded.onEmpty(). Being able to provide more detail via @Embedded is a nice detail.

It would be also neat to have Mapping Annotation Overview explaining that @Id can be simple or embedded/composite identifiers along with embedded Id examples. So far, @Id is only used with simple types in code samples.

Introduce ColumnInfos.reduce(…) operation.

schauder requested a review from mp911de December 12, 2024 13:20

schauder force-pushed the issue/574-composite-id branch from d2967c0 to b931dcf Compare December 23, 2024 10:50

schauder mentioned this pull request Jan 2, 2025

Value object as aggregate id without custom converter #1964

Closed

schauder mentioned this pull request Jan 29, 2025

Add support for @JoinColumn #1952

Open

mp911de self-assigned this Jan 30, 2025

schauder added 5 commits March 12, 2025 10:32

574-composite-id - Prepare branch

5fb0aae

Polishing.

0117231

Original pull request #1957 See #574

Polishing.

5527960

Removed unused assignments. Original pull request #1957 See #574

schauder force-pushed the issue/574-composite-id branch from b931dcf to 5527960 Compare March 13, 2025 11:55

mp911de requested changes Mar 19, 2025

View reviewed changes

Polishing.

099c890

Introduce ColumnInfos.reduce(…) operation.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for composite ids #1957

Add support for composite ids #1957

schauder commented Dec 12, 2024

mp911de left a comment

mp911de Mar 19, 2025

mp911de Mar 19, 2025

schauder Mar 20, 2025

mp911de Mar 19, 2025

schauder Mar 20, 2025

schauder Mar 20, 2025

mp911de Mar 19, 2025

mp911de Mar 19, 2025

mp911de Mar 19, 2025

mp911de Mar 19, 2025

mp911de Mar 19, 2025

mp911de Mar 19, 2025

mp911de Mar 19, 2025


		assertSoftly(softly -> {

		softly.assertThat(path("second").append(path()).toDotPath()).isEqualTo("second");


		import org.junit.jupiter.api.Test;

		class TupleExpressionUnitTests {

Add support for composite ids #1957

Are you sure you want to change the base?

Add support for composite ids #1957

Conversation

schauder commented Dec 12, 2024

mp911de left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment