-
Notifications
You must be signed in to change notification settings - Fork 356
Add support for composite ids #1957
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
base: main
Are you sure you want to change the base?
Conversation
d2967c0
to
b931dcf
Compare
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
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
b931dcf
to
5527960
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
...data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/MappingJdbcConverter.java
Outdated
Show resolved
Hide resolved
...data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/MappingJdbcConverter.java
Outdated
Show resolved
Hide resolved
spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/SqlGenerator.java
Outdated
Show resolved
Hide resolved
spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/SqlGenerator.java
Show resolved
Hide resolved
spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/SqlGenerator.java
Outdated
Show resolved
Hide resolved
...nal/src/test/java/org/springframework/data/relational/core/mapping/ColumnInfosUnitTests.java
Outdated
Show resolved
Hide resolved
|
||
assertSoftly(softly -> { | ||
|
||
softly.assertThat(path("second").append(path()).toDotPath()).isEqualTo("second"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
...nal/src/test/java/org/springframework/data/relational/core/sql/TupleExpressionUnitTests.java
Show resolved
Hide resolved
...c/test/java/org/springframework/data/relational/core/sql/render/SelectRendererUnitTests.java
Show resolved
Hide resolved
[[entity-persistence.embedded-ids]] | ||
=== Embedded Ids | ||
|
||
Entities may be annotated with `@Id` and `@Embedded`, resulting in a composite id on the database side. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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
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
099c890
to
db06331
Compare
571fd96
to
1f2e694
Compare
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
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
Introduce ColumnInfos.reduce(…) operation.
8228915
to
404730f
Compare
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 propertiesfirstName
andlastName
will consist of the two columnsPERSON_FIRST_NAME
andPERSON_LAST_NAME
.This holds for directly referenced entities as well as
List
,Set
andMap
.Closes #574