-
Notifications
You must be signed in to change notification settings - Fork 358
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: 4.0.x
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.
@@ -403,6 +388,29 @@ public <T> T getPropertyValue(RelationalPersistentProperty property) { | |||
return (T) delegate.getValue(aggregatePath); | |||
} | |||
|
|||
private Identifier constructIdentifier(AggregatePath aggregatePath) { | |||
|
|||
Identifier identifierToUse = this.identifier; |
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 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.
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.
done
@@ -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()); |
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.
Related: ColumnInfos
methods should be documented.
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.
done
|
||
/** | ||
* Constructs a function for constructing a where condition. The where condition will be of the form | ||
* {@literal <columns> IN :bind-marker} |
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.
lt/gt signs conflict with Javadoc. These should be escaped.
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.
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.
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.
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.
|
||
/** | ||
* 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 ...} |
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.
See above for Javadoc lt/gt chars.
this.columns = new Columns(entity, mappingContext, converter); | ||
this.queryMapper = new QueryMapper(converter); | ||
this.dialect = dialect; | ||
|
||
inCondition = inCondition(); |
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.
Why are these modelled as functions? Generally, functions are neat, however these introduce another indirection to already hard-to-read code.
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.
Changed to methods returning conditions, and method references
record CompositeId(String one, String two) { | ||
} | ||
|
||
record DummyEntityWithCompositeId(@Id @Embedded.Nullable CompositeId id, String name) { |
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.
Dummy
🙈
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.
Done.
|
||
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.
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 tried to improve the readability of the tests with a custom assertion.
|
||
import org.junit.jupiter.api.Test; | ||
|
||
class TupleExpressionUnitTests { |
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.
Nit: Javadoc
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.
Done
* Tests for rendering joins. | ||
*/ | ||
@Nested | ||
class JoinsTests { |
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.
Are these tests actually invoked during mvn verify
?
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.
Yes. Verified by adding a failing test temporarily.
[[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.
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 agree on the documentation part and will tackle that.
I don't like the idea of automatically turning an entity into an embedded when an @Id
annotation is placed on it. If users want this kind of behavior they could create their own meta annotation.
Also the question of Embedded.onEmpty()
gets risen by an implicit conversion to an embedded also.
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
8228915
to
404730f
Compare
Fix invalid Javadoc references. See #1944
Kotlin reacts to JSpecify with increased nullability requirements. See #1980
We now raise the exceptions from `NamedParameterJdbcTemplate` directly. If you used to extract the `cause` of a `DbActionExecutionException` you should now catch that Exception directly. Original pull request #1956 Closes #831 Signed-off-by: mipo256 <[email protected]>
Query methods that are not derived methods now cause an exception, when they are annotated with `@Lock`, since this combination is not supported. Before they just logged a warning. Comment edited by Jens Schauder Signed-off-by: mipo256 <[email protected]>
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.
22b23d3
to
f4cbeca
Compare
8d6a98a
to
ffab841
Compare
Ok, finally ready for another round of review. |
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