From ecc47ac2e61f347b1c4c6f3bf4035b786ecf6f0e Mon Sep 17 00:00:00 2001 From: Chantal Loncle <82039410+bog-walk@users.noreply.github.com> Date: Thu, 15 Aug 2024 20:53:37 -0400 Subject: [PATCH] feat!: EXPOSED-320 Many-to-many relation with extra columns - Remove approach to set/get additional data from an existing entity object field. This requires some UX concerns answered, for example, concerning caching. Would the wrapped entity (if loaded from a query of its own table) override the entity+data loaded from the many-to-many query? Would updating the field mean the reference should also be trigger a delete+insert? - Fix issue with updating and caching new additional data --- .../Writerside/topics/Breaking-Changes.md | 2 +- .../Writerside/topics/Deep-Dive-into-DAO.md | 33 ++------ exposed-dao/api/exposed-dao.api | 1 - .../org/jetbrains/exposed/dao/Entity.kt | 15 ---- .../org/jetbrains/exposed/dao/EntityCache.kt | 2 +- .../jetbrains/exposed/dao/InnerTableLink.kt | 45 ++++++++--- .../sql/tests/shared/entities/ViaTest.kt | 80 +++---------------- 7 files changed, 55 insertions(+), 123 deletions(-) diff --git a/documentation-website/Writerside/topics/Breaking-Changes.md b/documentation-website/Writerside/topics/Breaking-Changes.md index c3a447cf1d..9868f669c7 100644 --- a/documentation-website/Writerside/topics/Breaking-Changes.md +++ b/documentation-website/Writerside/topics/Breaking-Changes.md @@ -4,7 +4,7 @@ * All objects that are part of the sealed class `ForUpdateOption` are now converted to `data object`. * Additional columns from intermediate tables (defined for use with DAO `via()` for many-to-many relations) are no longer ignored on batch insert of references. - These columns are now included in the generated SQL and a value will be required when setting references, unless column defaults are defined. + These columns are now included and, unless column defaults are defined, values will be required when setting references by passing `InnerTableLinkEntity` instances. To continue to ignore these columns, use the non-infix version of `via()` and provide an empty list to `additionalColumns` (or a list of specific columns to include): ```kotlin diff --git a/documentation-website/Writerside/topics/Deep-Dive-into-DAO.md b/documentation-website/Writerside/topics/Deep-Dive-into-DAO.md index 9a55afba24..d28a9e296b 100644 --- a/documentation-website/Writerside/topics/Deep-Dive-into-DAO.md +++ b/documentation-website/Writerside/topics/Deep-Dive-into-DAO.md @@ -299,9 +299,9 @@ class User(id: EntityID) : IntEntity(id) { ... ``` -### Many-To-Many reference +### many-to-many reference In some cases, a many-to-many reference may be required. -Let's assume you want to add a reference to the following `Actors` table to the `StarWarsFilm` class: +Assuming that you want to add a reference to the following `Actors` table to the previous `StarWarsFilm` class: ```kotlin object Actors : IntIdTable() { val firstname = varchar("firstname", 50) @@ -344,7 +344,8 @@ Now you can access all actors (and their fields) for a `StarWarsFilm` object, `f film.actors.first() // returns an Actor object film.actors.map { it.lastname } // returns a List ``` -If the intermediate table is defined with more than just the two reference columns, these additional columns can be accessed in two ways, both detailed below. +If the intermediate table is defined with more than just the two reference columns, these additional columns can also be accessed by +calling `via()` on a special wrapping entity class, `InnerTableLinkEntity`, as shown below. Given a `StarWarsFilmActors` table with the extra column `roleName`: ```kotlin @@ -355,31 +356,7 @@ object StarWarsFilmActors : Table() { override val primaryKey = PrimaryKey(starWarsFilm, actor) } ``` -**The first approach** assumes that the value stored in this extra column will be accessed from the `Actor` class: -```kotlin -class Actor(id: EntityID) : IntEntity(id) { - companion object : IntEntityClass(Actors) - var firstname by Actors.firstname - var lastname by Actors.lastname - var roleName by StarWarsFilmActors.roleName -} -``` -This extra value can then be set, for example, when a new `Actor` is created or when it is provided to the parent entity's field, -and accessed like any other field: -```kotlin -val actor1 = Actor.new { - firstname = "Harrison" - lastname = "Ford" - roleName = "Han Solo" -} -// or -film.actors = SizedCollection(actor1, actor2.apply { roleName = "Ben Solo" }) - -StarWarsFilm.all().first.actors.map { it.roleName } -``` -**The second approach** assumes that the `Actor` class should not be given an extra field and that the extra value stored should -be accessed through an object that holds both the child entity and the additional data. - +The extra value stored can be accessed through an object that holds both the child entity and the additional data. To both allow this and still take advantage of the underlying DAO cache, a new entity class has to be defined using `InnerTableLinkEntity`, which details how to get and set the additional column values from the intermediate table through two overrides: ```kotlin diff --git a/exposed-dao/api/exposed-dao.api b/exposed-dao/api/exposed-dao.api index c6e515c35b..8c0714b7ed 100644 --- a/exposed-dao/api/exposed-dao.api +++ b/exposed-dao/api/exposed-dao.api @@ -23,7 +23,6 @@ public class org/jetbrains/exposed/dao/Entity { public static synthetic fun flush$default (Lorg/jetbrains/exposed/dao/Entity;Lorg/jetbrains/exposed/dao/EntityBatchUpdate;ILjava/lang/Object;)Z public final fun getDb ()Lorg/jetbrains/exposed/sql/Database; public final fun getId ()Lorg/jetbrains/exposed/dao/id/EntityID; - public fun getInnerTableLinkValue (Lorg/jetbrains/exposed/sql/Column;)Ljava/lang/Object; public final fun getKlass ()Lorg/jetbrains/exposed/dao/EntityClass; public final fun getReadValues ()Lorg/jetbrains/exposed/sql/ResultRow; public final fun getValue (Lorg/jetbrains/exposed/dao/EntityFieldWithTransform;Lorg/jetbrains/exposed/dao/Entity;Lkotlin/reflect/KProperty;)Ljava/lang/Object; diff --git a/exposed-dao/src/main/kotlin/org/jetbrains/exposed/dao/Entity.kt b/exposed-dao/src/main/kotlin/org/jetbrains/exposed/dao/Entity.kt index 38985a554e..ae5376e615 100644 --- a/exposed-dao/src/main/kotlin/org/jetbrains/exposed/dao/Entity.kt +++ b/exposed-dao/src/main/kotlin/org/jetbrains/exposed/dao/Entity.kt @@ -78,16 +78,6 @@ open class Entity>(val id: EntityID) { private val referenceCache by lazy { HashMap, Any?>() } - private val writeInnerTableLinkValues by lazy { HashMap, Any?>() } - - /** - * Returns the initial column-value mapping for an entity involved in an [InnerTableLink] relation - * before being flushed and inserted into the database. - * - * @sample org.jetbrains.exposed.sql.tests.shared.entities.ViaTests.ProjectWithApproval - */ - open fun getInnerTableLinkValue(column: Column<*>): Any? = writeInnerTableLinkValues[column] - internal fun isNewEntity(): Boolean { val cache = TransactionManager.current().entityCache return cache.inserts[klass.table]?.contains(this) ?: false @@ -283,7 +273,6 @@ open class Entity>(val id: EntityID) { @Suppress("UNCHECKED_CAST", "USELESS_CAST") fun Column.lookup(): T = when { writeValues.containsKey(this as Column) -> writeValues[this as Column] as T - writeInnerTableLinkValues.containsKey(this) -> getInnerTableLinkValue(this) as T id._value == null && _readValues?.hasValue(this)?.not() ?: true -> defaultValueFun?.invoke() as T columnType.nullable -> readValues[this] else -> readValues[this]!! @@ -291,10 +280,6 @@ open class Entity>(val id: EntityID) { operator fun Column.setValue(o: Entity, desc: KProperty<*>, value: T) { klass.invalidateEntityInCache(o) - if (this !in klass.table.columns) { - writeInnerTableLinkValues[this] = value - return - } val currentValue = _readValues?.getOrNull(this) if (writeValues.containsKey(this as Column) || currentValue != value) { val entityCache = TransactionManager.current().entityCache diff --git a/exposed-dao/src/main/kotlin/org/jetbrains/exposed/dao/EntityCache.kt b/exposed-dao/src/main/kotlin/org/jetbrains/exposed/dao/EntityCache.kt index 58fc1dfa8d..ab5871d875 100644 --- a/exposed-dao/src/main/kotlin/org/jetbrains/exposed/dao/EntityCache.kt +++ b/exposed-dao/src/main/kotlin/org/jetbrains/exposed/dao/EntityCache.kt @@ -23,7 +23,7 @@ class EntityCache(private val transaction: Transaction) { internal val inserts = LinkedHashMap, MutableSet>>() private val updates = LinkedHashMap, MutableSet>>() internal val referrers = HashMap, MutableMap, SizedIterable<*>>>() - private val innerTableLinks by lazy { LinkedHashMap, MutableMap>>() } + internal val innerTableLinks by lazy { LinkedHashMap, MutableMap>>() } /** * The amount of entities to store in this [EntityCache] per [Entity] class. diff --git a/exposed-dao/src/main/kotlin/org/jetbrains/exposed/dao/InnerTableLink.kt b/exposed-dao/src/main/kotlin/org/jetbrains/exposed/dao/InnerTableLink.kt index 16e9e1c254..2d81bd470d 100644 --- a/exposed-dao/src/main/kotlin/org/jetbrains/exposed/dao/InnerTableLink.kt +++ b/exposed-dao/src/main/kotlin/org/jetbrains/exposed/dao/InnerTableLink.kt @@ -78,8 +78,10 @@ class InnerTableLink, Source : Entity, ID : Comparabl columns to entityTables } - private val additionalColumns = additionalColumns - ?: (table.columns - sourceColumn - targetColumn).filter { !it.columnType.isAutoInc } + private val additionalColumns = (additionalColumns + ?: (table.columns - sourceColumn - targetColumn).filter { !it.columnType.isAutoInc }) + .takeIf { it.isEmpty() || target is InnerTableLinkEntityClass } + ?: error("Target entity must extend InnerTableLinkEntity to properly store and cache additional column data") override operator fun getValue(o: Source, unused: KProperty<*>): SizedIterable { if (o.id._value == null && !o.isNewEntity()) return emptySized() @@ -117,16 +119,33 @@ class InnerTableLink, Source : Entity, ID : Comparabl val entityCache = tx.entityCache entityCache.flush() val oldValue = getValue(o, unused) - val existingIds = oldValue.map { it.id }.toSet() + val existingValues = oldValue.mapIdToAdditionalValues() + val existingIds = existingValues.keys + val additionalColumnsExist = additionalColumns.isNotEmpty() + if (additionalColumnsExist) { + entityCache.innerTableLinks[target.table]?.remove(o.id.value) + } entityCache.referrers[sourceColumn]?.remove(o.id) - val targetIds = value.map { it.id } - val targetValues = value.map { target -> - target.id to additionalColumns.associateWith { target.getInnerTableLinkValue(it) } - } + val targetValues = value.mapIdToAdditionalValues() + val targetIds = targetValues.keys executeAsPartOfEntityLifecycle { - table.deleteWhere { (sourceColumn eq o.id) and (targetColumn notInList targetIds) } - table.batchInsert(targetValues.filter { !existingIds.contains(it.first) }, shouldReturnGeneratedValues = false) { (targetId, additionalValues) -> + val deleteCondition = if (additionalColumnsExist) { + val targetAdditionalValues = targetValues.map { it.value.values.toList() + it.key } + (sourceColumn eq o.id) and (additionalColumns + targetColumn notInList targetAdditionalValues) + } else { + (sourceColumn eq o.id) and (targetColumn notInList targetIds) + } + val newTargets = targetValues.filter { (targetId, additionalValues) -> + if (additionalColumnsExist) { + targetId !in existingIds || + existingValues[targetId]?.entries?.containsAll(additionalValues.entries) == false + } else { + targetId !in existingIds + } + } + table.deleteWhere { deleteCondition } + table.batchInsert(newTargets.entries, shouldReturnGeneratedValues = false) { (targetId, additionalValues) -> this[sourceColumn] = o.id this[targetColumn] = targetId additionalValues.forEach { (column, value) -> @@ -149,6 +168,12 @@ class InnerTableLink, Source : Entity, ID : Comparabl } } + private fun SizedIterable.mapIdToAdditionalValues(): Map, Map, Any?>> { + return associate { target -> + target.id to additionalColumns.associateWith { (target as InnerTableLinkEntity).getInnerTableLinkValue(it) } + } + } + /** Modifies this reference to sort entities based on multiple columns as specified in [order]. **/ infix fun orderBy(order: List, SortOrder>>) = this.also { orderByExpressions.addAll(order) @@ -180,7 +205,7 @@ abstract class InnerTableLinkEntity>(val wrapped: Entity): Any? + abstract fun getInnerTableLinkValue(column: Column<*>): Any? } /** diff --git a/exposed-tests/src/test/kotlin/org/jetbrains/exposed/sql/tests/shared/entities/ViaTest.kt b/exposed-tests/src/test/kotlin/org/jetbrains/exposed/sql/tests/shared/entities/ViaTest.kt index ba8cb3ccbe..becec80025 100644 --- a/exposed-tests/src/test/kotlin/org/jetbrains/exposed/sql/tests/shared/entities/ViaTest.kt +++ b/exposed-tests/src/test/kotlin/org/jetbrains/exposed/sql/tests/shared/entities/ViaTest.kt @@ -10,6 +10,8 @@ import org.jetbrains.exposed.sql.tests.DatabaseTestsBase import org.jetbrains.exposed.sql.tests.shared.assertEqualCollections import org.jetbrains.exposed.sql.tests.shared.assertEqualLists import org.jetbrains.exposed.sql.tests.shared.assertEquals +import org.jetbrains.exposed.sql.tests.shared.assertFalse +import org.jetbrains.exposed.sql.tests.shared.assertTrue import org.jetbrains.exposed.sql.transactions.TransactionManager import org.jetbrains.exposed.sql.transactions.inTopLevelTransaction import org.junit.Test @@ -287,11 +289,11 @@ class ViaTests : DatabaseTestsBase() { object Projects : IntIdTable("projects") { val name = varchar("name", 50) } - class ProjectUsingEntity(id: EntityID) : IntEntity(id) { - companion object : IntEntityClass(Projects) + class Project(id: EntityID) : IntEntity(id) { + companion object : IntEntityClass(Projects) var name by Projects.name - var tasks by TaskUsingEntity via ProjectTasks + var tasks by TaskWithApproval via ProjectTasks } object ProjectTasks : Table("project_tasks") { @@ -306,67 +308,13 @@ class ViaTests : DatabaseTestsBase() { object Tasks : IntIdTable("tasks") { val title = varchar("title", 64) } - class TaskUsingEntity(id: EntityID) : IntEntity(id) { - companion object : IntEntityClass(Tasks) + class Task(id: EntityID) : IntEntity(id) { + companion object : IntEntityClass(Tasks) var title by Tasks.title - var approved by ProjectTasks.approved - var sprint by ProjectTasks.sprint - } - - @Test - fun testAdditionalLinkDataUsingOriginalEntities() { - withTables(Projects, Tasks, ProjectTasks) { - val p1 = ProjectUsingEntity.new { name = "Project 1" } - val p2 = ProjectUsingEntity.new { name = "Project 2" } - val t1 = TaskUsingEntity.new { title = "Task 1" } - // additional fields can be set in new() - val t2 = TaskUsingEntity.new { - title = "Task 2" - approved = true - sprint = 2 - } - val t3 = TaskUsingEntity.new { - title = "Task 3" - approved = false - sprint = 3 - } - - // or additional fields can be applied directly once setting the parent reference - p1.tasks = SizedCollection( - t1.apply { - approved = true - sprint = 1 - } - ) - p2.tasks = SizedCollection(t2, t3) - - commit() - - inTopLevelTransaction(Connection.TRANSACTION_SERIALIZABLE) { - maxAttempts = 1 - ProjectUsingEntity.all().with(ProjectUsingEntity::tasks) - val cache = TransactionManager.current().entityCache - - val p1Task = cache.getReferrers(p1.id, ProjectTasks.project)?.single() - assertEquals(t1.id, p1Task?.id) - assertEquals(true, p1Task?.approved) - assertEquals(1, p1Task?.sprint) - - val p2Tasks = cache.getReferrers(p2.id, ProjectTasks.project)?.toList().orEmpty() - assertEqualLists(p2Tasks.map { it.id }, listOf(t2.id, t3.id)) - assertEqualLists(p2Tasks.map { it.approved }, listOf(true, false)) - assertEqualLists(p2Tasks.map { it.sprint }, listOf(2, 3)) - } - } + var projects by ProjectWithApproval via ProjectTasks } - class Project(id: EntityID) : IntEntity(id) { - companion object : IntEntityClass(Projects) - - var name by Projects.name - var tasks by TaskWithApproval via ProjectTasks - } class ProjectWithApproval( val project: Project, val approved: Boolean, @@ -387,12 +335,6 @@ class ViaTests : DatabaseTestsBase() { } } - class Task(id: EntityID) : IntEntity(id) { - companion object : IntEntityClass(Tasks) - - var title by Tasks.title - var projects by ProjectWithApproval via ProjectTasks - } class TaskWithApproval( val task: Task, val approved: Boolean, @@ -422,9 +364,13 @@ class ViaTests : DatabaseTestsBase() { val t2 = Task.new { title = "Task 2" } val t3 = Task.new { title = "Task 3" } - p1.tasks = SizedCollection(TaskWithApproval(t1, true, 1)) + p1.tasks = SizedCollection(TaskWithApproval(t1, false, 1)) p2.tasks = SizedCollection(TaskWithApproval(t2, true, 2), TaskWithApproval(t3, false, 3)) + assertFalse(p1.tasks.single().approved) + p1.tasks = SizedCollection(TaskWithApproval(t1, true, 1)) + assertTrue(p1.tasks.single().approved) + commit() // test that all child entities set on the parent can be loaded by parent