Skip to content

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

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

Add support for composite ids #1957

wants to merge 19 commits into from

Conversation

schauder
Copy link
Contributor

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

@schauder schauder requested a review from mp911de December 12, 2024 13:20
@schauder schauder force-pushed the issue/574-composite-id branch from d2967c0 to b931dcf Compare December 23, 2024 10:50
@mp911de mp911de self-assigned this Jan 30, 2025
schauder added a commit that referenced this pull request Mar 13, 2025
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
schauder added a commit that referenced this pull request Mar 13, 2025
Original pull request #1957
See #574
schauder added a commit that referenced this pull request Mar 13, 2025
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
schauder added a commit that referenced this pull request Mar 13, 2025
Removed unused assignments.

Original pull request #1957
See #574
@schauder schauder force-pushed the issue/574-composite-id branch from b931dcf to 5527960 Compare March 13, 2025 11:55
Copy link
Member

@mp911de mp911de left a 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.


assertSoftly(softly -> {

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

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.

[[entity-persistence.embedded-ids]]
=== Embedded Ids

Entities may be annotated with `@Id` and `@Embedded`, resulting in a composite id on the database side.
Copy link
Member

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.

schauder added a commit that referenced this pull request Apr 2, 2025
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
schauder added a commit that referenced this pull request Apr 2, 2025
Original pull request #1957
See #574
schauder added a commit that referenced this pull request Apr 2, 2025
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
schauder added a commit that referenced this pull request Apr 2, 2025
Removed unused assignments.

Original pull request #1957
See #574
@schauder schauder force-pushed the issue/574-composite-id branch from 099c890 to db06331 Compare April 2, 2025 11:09
@mp911de mp911de force-pushed the main branch 2 times, most recently from 571fd96 to 1f2e694 Compare April 9, 2025 13:29
schauder and others added 12 commits April 16, 2025 13:05
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
Removed unused assignments.

Original pull request #1957
See #574
Introduce ColumnInfos.reduce(…) operation.
@schauder schauder force-pushed the issue/574-composite-id branch from 8228915 to 404730f Compare April 16, 2025 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for Composite Keys using @Embeded and @Id
2 participants