Support multi-column foreign keys in SQL#1494
Conversation
… into shared base classes (MYSQL, POSTGRES and H2)
… `NULL` values. In SQL, As `NULL` values are considered unknown (`NULL=NULL` evaluates to false),
… keys in the same table.
…eys in `SqlForeignKeyGene`
…ignKeyGene` creation
…bleString` validate both primary key ID and column name, and add tests for multi-column foreign key support.
…i-column foreign keys
…de missing parameter
| */ | ||
| sourceColumn: String, | ||
| /** | ||
| * The position in the list of SQL insertion actions |
There was a problem hiding this comment.
position??? if that is indeed the case, it seems very brittle, as SQL actions can be inserted/deleted during mutation
There was a problem hiding this comment.
you are right. This description does not correctly explains what uniqueId is.
| * names of columns that form a composite foreign key | ||
| * together with this one. | ||
| */ | ||
| val otherSourceColumnsInCompositeFK: List<String> = emptyList() |
There was a problem hiding this comment.
does it contain itself? i.e., sourceColumn? clarify
There was a problem hiding this comment.
also, shouldn't it rather be a Set?
There was a problem hiding this comment.
I clarified and changed the field from List to Set.
| .flatMap { it.flatView().asSequence() } | ||
| .filterIsInstance<SqlForeignKeyGene>() | ||
| .filter { it.uniqueId == uniqueId && it !== this && it.targetTable == targetTable } | ||
| .filter { otherSourceColumnsInCompositeFK.contains(it.name) } |
There was a problem hiding this comment.
is the name the same as sourceColumn??? i mean, here it is a FK. shouldn't be something like otherTargetColumnsInCompositeFK? bit confused here... maybe add some more comments, and rename variables if needed
There was a problem hiding this comment.
I will refactor this code to make it more readable
|
|
||
| val sourceColumns: Set<Column>, | ||
| /** | ||
| * The columns in the source table that are referenced by this foreign key |
There was a problem hiding this comment.
are referenced by? you mean something like define?
There was a problem hiding this comment.
I will improve the Javadoc
| * The columns in the target table that are referenced by this foreign key. | ||
| * The order should match the order of the source columns. | ||
| */ | ||
| val targetColumns: List<Column> |
There was a problem hiding this comment.
shouldn't track sourceTableId as well?
There was a problem hiding this comment.
The ForeignKey is contained within a data class Table. Therefore I think it is redundant to include the sourceTableId here.
|
|
||
| val fks = actions[i].seeTopGenes() | ||
| val currentAction = actions[i] | ||
| val currentFkGenes = currentAction.seeTopGenes() |
There was a problem hiding this comment.
ok. I will remove the flatview then
| if (type == DatabaseType.POSTGRES) { | ||
| sql += " CASCADE"; | ||
| } | ||
| statement.executeUpdate(sql); |
There was a problem hiding this comment.
thx. this fix seems not related to PK/FK, isn't it? for future, it would had been better to have in a separate PR
There was a problem hiding this comment.
agree. Unfortunately it was blocking my progress, so I pushed as a change here. But next time I will try to have a separate small PR.
…SqlForeignKeyGene`.
…e `Set` for filtering operations.
…improved readability and efficiency.
2 participants
support for multi-column foreign keys in SQL.