feat: EXPOSED-77 Support entity class for table with composite primary key #1987
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
e5l
requested changes
Feb 13, 2024
| } | ||
|
|
||
| override fun equals(other: Any?): Boolean { | ||
| if (other !is CompositeID) return false |
There was a problem hiding this comment.
Should we compare CompositeId with one mapping with EntityId?
| ) : HashMap<Column<*>, Comparable<*>?>(idMapping), Comparable<CompositeID> { | ||
| override fun toString(): String = "CompositeID(${entries.joinToString { "${it.key.name}=${it.value}" }})" | ||
|
|
||
| override fun hashCode(): Int = entries.fold(0) { acc, entry -> |
There was a problem hiding this comment.
Single entry CompositeId will always have the same value as EntityId of same type
| // SQLite excluded from all tests as it only allows auto-increment on single column PKs. | ||
| // SQL Server is sometimes excluded because it doesn't allow inserting explicit values for identity columns. | ||
| class CompositeIdTableEntityTest : DatabaseTestsBase() { | ||
| object Publishers : CompositeIdTable("publishers") { |
There was a problem hiding this comment.
I'm thinking if we can try make CompositeIdTable to IdTable by default, let's discuss
…y key - Add base objects for new CompositeEntity and CompositeIdTable. - Use existing implementation via EntityID to wrap new comparable type, CompositeID. - Retain use of IdTable.id via a placeholder column that can deconstruct to its component columns in DSL and ResultRow accessors. - Add base unit tests.
…y key - Extract logic between regular id columns and composite id columns to lower locations. - Add KDocs and change some object locations. - Flesh out unit tests
…y key - Remove changes to reference logic. - Small change to unit test. - Rebase from main.
…y key - Update KDocs to reflect merged changes - Add CompositeID equality comparison for EntityID<*>
…y key Clean up some logic Add first example of supporting a simple referencedOn relation to a composite primary key.
bog-walk
force-pushed
the
bog-walk/support-composite-entityid
branch
from
a0630fd
to
54b8d89
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Phase 1:
CompositeIdTablethat allows id columns to be marked forEntityIDinclusion usingColumn.compositeEntityid().CompositeIDthat will be wrapped byEntityIDfor value comparison.CompositeIdTable.idcan be used in DSL in the same way anyIdTable.idcan (for example, in WHERE or SELECT clauses). This means users can easily include all deconstructed PK columns in their queries.ResultRow[CompositeIdTable.id]is also possible. Even though the result set returns each PK column value separately, using the previous accessor returns a newEntityID<CompositeID>, just as for a standard entity id column.IdTablesubtype has been extracted (in most places) to the lowest common point possible.Phase 2 - About References:
This PR currently includes minimal changes to Reference logic (only for
referencedOn). The better option seems to be to add a property to all reference classes. This new property would provide the full set of reference columns, while leaving the original single columnreferenceproperty as an internal delegate (like an ambassador for the other columns), especially when it comes to managing the reference entity in the cache.Ideally I'd split this PR into 2, but support isn't really complete until referencing is working, so I'd like for this to be an iterative process, if possible.
Once everything is agreed on with the first phase, I'll commit the improved reference changes for review.