feat!: EXPOSED-320 Many-to-many relation with extra columns

[bog-walk]

Description

Summary of the change:
Provides the functionality for DAO entities to set/get additional data to/from extra columns (non-referencing) in the intermediate table of a many-to-many relation.

Detailed description:

• Why:
  Additional columns (beyond the 2 reference columns) are possible on intermediate tables defined in many-to-many relations. But these columns cannot be accessed by the referencing DAO entities involved because via() creates an InnerTableLink class that ignores these columns. Workarounds include adding a 'fake' id primary key to the intermediate table and creating an associated entity, or duplicating inner logic with custom classes. The latter only partially works, however, because internal cache logic requires that the target of via() is an Entity.

• What:

  • This PR allows via() to be used in the same way as before by introducing a new entity class InnerTableLinkEntity that wraps the referenced entity (and delegates to it's id and table) along with any additional data in the intermediate table row.
  • Additional columns are no longer ignored by default and values are required to be used in the generated SQL whenever a new reference collection is set.
  • Additional column data can be accessed from either (or both) the source or target entity.

• How:

  • Add abstract InnerTableLinkEntity, with associated InnerTableLinkEntityClass, that forces 2 overrides so users define how to properly set and get any additional data. Subclass can be a class or data class as needed.
  • via() and InnerTableLink now accept a list of columns as arguments to specify which additional columns should be included. This allows users to opt-out of new functionality, by providing emptyList(), if it breaks any current workaround.
  • InnerTableLink internal logic uses additional columns to generate triggered delete and insert SQL. Previously, statements would only be generated if there was a change in the reference id. Now they will be generated even if the reference id is identical, as long as any of the additional data changes. This is necessary to allow reference collections to be updated properly.
  • Add new internal EntityCache map just for InnerTableLinkEntity. Because the latter delegates to the wrapped entity, if the regular data cache is used, this special entity may override its cached wrapped entity or be incorrectly retrieved on find(). This map stores each entity by its target column and source id, as the intermediate table is expected to have a contract of uniqueness on the 2 reference columns.

---

Note: The original plan was to have this implementation alongside a more standard approach, which would get/set the additional data as a delegate field on an existing entity, for example:

class Project(id: EntityID<Int>) : IntEntity(id) {
    // ...
    var tasks by Task via ProjectTasks
}
class Task(id: EntityID<Int>) : IntEntity(id) {
    // ...
    var approved by ProjectTasks.approved
}

This worked well for safe setting and getting, but started raising questions when updates were introduced:

1. Should it be possible to set the field approved in isolation? Meaning not as part of SizedCollection, but in new {} or through a standard task.approved = true?
2. Would setting the field in isolation be considered an update and should it then trigger a cascade by causing the reference field to also update? And if so, how?
3. If a Task was already cached with its approved field set by references loaded from the intermediate table, then something like Task.all() was loaded, the new task would override the cached task and trying to access the approved field would cause an exception as it would rightly not have any value. Would it be expected that this shouldn't happen?

So I opted to go for the safer implementation and if users come forward requesting the design above, I'm hopeful that their use cases will answer some of these questions.

---

Type of Change

Please mark the relevant options with an "X":

• New feature

Updates/remove existing public API methods:

• Is breaking change

Affected databases:

• All

Checklist

• Unit tests are in place
• The build is green (including the Detekt check)
• All public methods affected by my PR has up to date API docs
• Documentation for my change is up to date

---

Related Issues

EXPOSED-320, EXPOSED-443

[bog-walk]

Even though an entity class is being used to accomplish this feature, the original behavior and usage of via() entities should most likely be the same. I think it would be best to override all standard entity functions, like delete(), all(), new(), findById(), etc since their use wouldn't work (since the intermediate table does not have to be an IdTable).

This would mean that attempting to call them on an InnerTableLinkEntity throws an error instead. So the only way to insert or delete or retrieve these linked values would be by setting/getting the parent/child entity field declared with via(). Basically these entities would only exist or be used through the via field. Does that make sense?

[bog-walk]

Most ideally, there should be a cache that stores the link entities without any relation to their referenced counterparts, solely based on some special id, which could be retrieved from the ResultRow in wrapLinkRow(). This is how the data cache is for example set up for all regular entities.
This would mean either a brand new id for the entity (defeats the purpose as the point is to not introduce a new/fake id column in the intermediate table, since uniqueness is based on the 2 referencing columns) or some way to check ResultRow values against entity values. For the latter, I did consider forcing another override where the user defines some sort of equality match between ResultRow and InnerTableLinkEntity, but it got a bit messy.

What the above cache does is store all InnerTableLinkEntitys for a target column and source (column) id, so uniqueness essentially relies on 3 values: target column (e.g. task in ProjectTasks), source id (e.g. project value in ProjectTasks), and target id (e.g. TaskWithData.id stored in the entity itself).

[bog-walk]

@obabichevjb I refactored this PR (and added more tests) as original cache was failing if the same wrapped entity was used with different additional data (for example, updating TaskWithData(Task(11), true, 1) to TaskWithData(Task(11), false, 1) would not trigger the cache to update). Now the cache stores these special entities based on target column, source id, and the target (wrapped) id.

Please let me know if any API improvements could be considered, and what you think about overriding the standard entity functions to throw an error (like new() etc) so that the entity isn't accidentally used like a standard entity.

[obabichevjb]

I see that this parameter is a way to redefine default list of extra columns ((table.columns - sourceColumn - targetColumn) with some filters), but it's not used now anywhere, probably we could a test for it also.

I don't expect that something bad could happen, but what if user add in this list mentioned sourceColumn or targetColumn.

[obabichevjb]

Must user handle null rows here?

As I can see from tests the instances of TaskWithData are created inside the application logic. When 2 entities are connected user can create link with extra data between them.

Probably I just missed something, but it's not clear for me, when I should expect the call of createdInstance with null row.

I replaced ?: TaskWithData(Task(entityId), false, 0) with ?: error("") and tests continued to be green.

[bog-walk]

I agree that I'm not a fan of the null handling.
createInstance() is an existing open method of EntityClass that I chose to force an override for, since it's the important function that is called every time we try to wrap a row for a link entity that doesn't already exist in the cache.

I could create a new method/overload that doesn't have a nullable row, which just calls createInstance() under the hood and we handle null ourselves.

[obabichevjb]

In terms of API defining of custom InnerTableLinkEntity looks quite verbose to me.

I expect that it's not the first variant and if all other options are not better this variant is ok.

I also tried to make something with it, and I have a feeling that it could be made in a similar way with original entity.

The thing I tried to do is to make it looks like:

class ProjectWithData2(
    val project: Project,
) : InnerTableLinkEntity<Int>(project) {
    val approved by ProjectTasks.approved
    val sprint by ProjectTasks.sprint

    companion object : InnerTableLinkEntityClass<Int, ProjectWithData>(Projects, ProjectTasks)
}

The main change here - I added ProjectTasks as a second paramerter of InnerTableLinkEntityClass. It allows to get the same data that was taken from row inside createInstance() but on the InnerTableLinkEntityClass level. Looks like:

abstract class InnerTableLinkEntityClass<WID : Comparable<WID>, out E : InnerTableLinkEntity<WID>>(
    table: IdTable<WID>,
    val intermediateTable: Table
) : EntityClass<WID, E>(table, null, null) {

fun createInstance2(entityId: EntityID<WID>, row: ResultRow?): E {
        if (row == null) error("Row is empty. How do we handle it?")
        val entity = wrapRow(row) // same as Project.wrapRow(it)
        val values = intermediateTable.columns.map { it to row[it] } // map with values it[ProjectTasks.approved], it[ProjectTasks.sprint]
// The question is what should be returned...
    }
    
}

Looking at the Entity class there is used some reflection:

private val entityPrimaryCtor: KFunction<T> by lazy { 
  klass.kotlin.primaryConstructor as KFunction<T> 
}

private val entityCtor: (EntityID<ID>) -> T = entityCtor ?: { entityID -> entityPrimaryCtor.call(entityID) }

Probably something similar could be done with links:

To the class InnerTableLinkEntity I added

internal val extraValues: MutableMap<Column<*>, Any?> = mutableMapOf()

So InnerTableLinkEntityClass looks like:

abstract class InnerTableLinkEntityClass<WID : Comparable<WID>, out E : InnerTableLinkEntity<WID>>(
    table: IdTable<WID>,
    val intermediateTable: Table
) : EntityClass<WID, E>(table, null, null) {

  private val entityPrimaryCtor: KFunction<E> by lazy { klass.kotlin.primaryConstructor as KFunction<E> }
  
  private val entityCtor: (Entity<WID>) -> E =  { entity -> entityPrimaryCtor.call(entity) }
  
  fun createInstance2(entityId: EntityID<WID>, row: ResultRow?): E {
      if (row == null) error("Row is empty. How do we handle it?")
      val entity = wrapRow(row)
      val values = intermediateTable.columns.map { it to row[it] }
      val resultEntity = entityCtor(entity)
      values.forEach {
          resultEntity.extraValues[it.first] = it.second
      }
      return resultEntity
  }
}

It compiles but I haven't tested it, so it's just more or less an idea of how API could look like. No idea which problems I can face up with such a change...

If you already tried that way, feel free to ignore it of course)

[bog-walk]

I can try adding a second parameter and give this shot. One of the reasons I kept moving away from a more traditional (albeit less verbose) entity with delegates was because of the questions I mentioned in the PR description. For example, let's say the above works and the new API means it looks like what you suggested above:

class ProjectWithData2(
    val project: Project,
) : InnerTableLinkEntity<Int>(project) {
   // what if user instead accidentally/purposefully made these mutable?
    var approved by ProjectTasks.approved
    var sprint by ProjectTasks.sprint

    companion object : InnerTableLinkEntityClass<Int, ProjectWithData>(Projects, ProjectTasks)
}

1. Using delegates means that a new link entity would be created, not by a constructor, but by new()? I thought maybe this would be confusing for users, because they might assume that it schedules an insert statement like a regular entity. It also means we would need to go into new() and adjust logic for this entity specifically.
2. What if, like above, user made the additional fields mutable? And then they assigned a new value after setting a new entity to the link field. This would be like trying to do an update, but we couldn't schedule an update like for a regular entity because it has to go through InnerTableLink.setValue. So what would happen? We'd have to adjust logic in Column.setValue to trigger an update of the via field?

Tbh we need this class to be some sort of entity so it fits the contract for our caches, find methods, and wrapRow etc. But it can't be too much like a traditional entity because it doesn't fit the contract for insertion/update/deletion etc. It should only exist (and be get/set) within the context of the other entity's via field.