Added support for self references (#10)

[Namnodorel]

I found this project appealing enough to implement this myself :D

I added support for objects having M2M-relations to themselves by distinguishing the attributes of the join-table with "source" and "target" keywords, and changed buildToEntityMapFunc() to avoid infinite recursions in that case (and it also now passes the proper object references of related objects).

Because this PR changes how krush expects a join-table to look, this can be considered a breaking change.

Also: I did not do much testing of this change apart from running the unit tests and testing it with my use case (both work without issue Looks like I overlooked the "example" tests - will look into this), so this will likely need to be tested with more examples before it can be merged with confidence that there will be no unfortunate surprises.

[Namnodorel]

Okay, I know what the problem is: With the updated implementation, krush will return objects with "perfect" references - meaning multiple objects pointing to the same entity object will all point to the same object instance. Every object from the DB exists only once in memory as well.

The problem that arises is that in bidirectional relations, that means that toString(), equals() and hashCode() will just loop infinitely between two objects that reference each other. The first solution I have thought of would be to exclude the bidir-fields from these methods, but afaik the only way to do that in Kotlin is to override the methods with manual implementations.

I don't really know what to do about this, would appreciate any input.

[Pilleo]

Aren't only fields declared in constructor used in equals method?
https://kotlinlang.org/docs/reference/data-classes.html#properties-declared-in-the-class-body

[Namnodorel]

Aren't only fields declared in constructor used in equals method?
https://kotlinlang.org/docs/reference/data-classes.html#properties-declared-in-the-class-body

If you defined all bidirectional fields outside of the constructor, krush would have to require the user to make them mutable, or implement strange secondary constructors. Certainly works, but I'm not sure whether it's an okay restriction for krush to have.

[pjagielski]

Hi, thanks for your submission.
Yes, before your changes we prevented these problems by keeping the references "imperfect". Take a look at TreeTest: the Leafs have references to Branches with empty Leaf list. If you want to traverse whole graph of objects you have to start from the root, this will give you access to all fetched objects, without lazy fetching.
Is it possible to make this similar in you case? So the Tasks graph will rather be a tree, not a DAG?

[Namnodorel]

Well, my specific use case allows for using a tree-structure instead of a graph, yes. But my requirement is not that I can traverse the graph/tree arbitrarily(in fact, I don't use anything bidir), but that the references point to the right object.

My line of thought was that it would be unfortunate if the application made a change to a on object in one place, and then later had to work with another object that still holds the reference to the (now outdated) copy. To get around such bugs, the app would have to either reload data from the DB frequently, or, for every change, execute it for both the original object and all of its copies (which sounds awful to develop and maintain). I think this problem will tend to happen in other cases too when you're working with copies instead of references.

[pjagielski]

Well, the design of krush is mostly focused on fetching data (read model). Because of the data classes, the fetched model is immutable. For simple CRUD operations we provide a way of updating data by passing a object copy with modifications, but for complex updates we recommend using native Exposed API. This also worked really well in our projects.

What I'm thinking about is a configuration option when the user can choose whether references or copies are used in relations. For references, the relation fields should be removed from constructor for proper equals/hashCode as @Pilleo mentioned, to avoid infinite recursion. IMO the default should be copies to avoid user confusion regarding StackOverflow errors.

@sledzmateusz @Namnodorel what do you think?

[Namnodorel]

Sounds like a reasonable solution. Where would you want to put such a configuration option? My first thought would be to add an optional parameter like copyRelated: Boolean = true to the to...List() and to...Map() functions, move the implementations to their own private functions, and choosing which one to use based on the parameter.

[Namnodorel]

(I'll happily implement this, but am currently waiting for confirmation that it's the desired solution)

[pjagielski]

Hi, sorry for delay. I was rather thinking of some global configuration option for the source code generator. For now we use only kapt.kotlin.generated which is standard kapt option, we could introduced something like kapt.krush.references which could be set to copy (default) or real. This option should be pass then to MappingsGenerator. WDYT?
Generating multiple methods could be confusing for users.

[Namnodorel]

Okay, I have now added a kapt config option that lets you switch between the current and real-reference implementations. It can be used like

kapt {
    arguments {
        arg("krush.references", 'real')
    }
}

I had to comment out the test I added though, because AFAIK there is now way to set such Gradle options per-test. So to test that functionality, one has to set the Gradle option, uncomment the test and keep in mind that the BiDir-Tests won't be happy about the change.

[pjagielski]

please reformat if statements as in the rest of the code (space after "if", "else")

[pjagielski]

maybe it would be better to do it by inheritance? buildToEntityMapFunc would be abstract and we would have 2 implementations of MappingsGenerator - CopiedReferencesMappingsGenerator and RealReferencesMappingsGenerator

[Namnodorel]

Hm, I feel like creating two whole new classes for one changed method is a bit much. Well, it's your call if you prefer it this way ^^

[pjagielski]

please add (commented) the configuration you mentioned in comment into example/build.gradle