Adding entity to @OneToMany property doesn't create the relationship

[Incanus3]

Expected behavior

When I add entry to @OneToMany property or reset it to a new list containing the entry and save the referencing entity, I'd expect this new relationship to be persisted.

Actual behavior

Neither the referencing entity's @OneToMany property, nor the referenced entity's @ManyToOne property reflects the new relationship after reload.

Steps to reproduce

// the entities

@Entity
public class Customer extends Model {
  @Id
  long id;

  String name;

  public Customer(String name) {
    this.name = name;
  }

  public long getId() {
    return id;
  }

  public void setId(long id) {
    this.id = id;
  }

  public String getName() {
    return name;
  }

  public void setName(String name) {
    this.name = name;
  }

  public Group getGroup() {
    return group;
  }

  public void setGroup(Group group) {
    this.group = group;
  }

  @ManyToOne(optional = true, cascade = {CascadeType.ALL})
  @JoinColumn(name = "group_id", nullable = true, insertable = true, updatable = true)
  private Group group = null;
}

@Entity
@Table(name = "groups")
public class Group extends Model {
  @Id
  long id;

  String name;

  public Group(String name) {
    this.name = name;
  }

  public long getId() {
    return id;
  }

  public void setId(long id) {
    this.id = id;
  }

  public String getName() {
    return name;
  }

  public void setName(String name) {
    this.name = name;
  }

  public List<Customer> getMembers() {
    return members;
  }

  public void setMembers(List<Customer> members) {
    this.members = members;
  }

  public void addMember(Customer member) {
    this.members.add(member);
  }

  @OneToMany(mappedBy = "group", cascade = {CascadeType.ALL})
  private List<Customer> members = new ArrayList<>();
}

// the test

@Test
void add_from_one_to_many_side() {
  Database server = DB.getDefault();

  Customer rob = new Customer("Rob");
  server.save(rob);

  Group devs = new Group("Devs");
  server.save(devs);

  devs.addMember(rob);
  server.save(devs);

  Customer reloadedRob = server.find(Customer.class, rob.getId());
  Group reloadedDevs = server.find(Group.class, devs.getId());

  // THESE FAIL
  assertThat(reloadedRob.getGroup()).isEqualTo(devs);
  assertThat(reloadedDevs.getMembers()).isEqualTo(List.of(rob));
}

Expected :Group@0(id:1, name:Devs, members:[Customer@1(id:1, name:Rob)])
Actual   :null
Expected: [Customer@0(id:1, name:Rob)]
 but was: []

• I've created a repository reproducing this problem here, the failing test is here
• after reading through this issue we added the second test that adds the member into the BeanList instead of replacing the whole list, but it doesn't solve the issue
• we've traced through most of server.save(devs) call and the save correctly cascades to the DefaultPersister.saveAssocMany() method where the members property is processed and saveRecurse() is called with rob, but when the Customer.group property is processed here, the prop.valueAsEntityBean(request.entityBean()) call returns null, which means the reversed property's value is never set.

[Incanus3]

From reading through the mentioned issue I understand that removing items from the @OneToMany side is a use-case that's not supported intentionally, does the same hold for adding to the list?

[jnehlmeier]

@Incanus3 I think this is similar to standard JPA. Bidirectional relations are usually persisted/updated from the owning side. In your case that would be Customer as it contains the group_id column and owns the foreign key pointing to the group.

In standard JPA you would always write code like this:

public void addMember(Customer member) {
    this.members.add(member);
    member.setGroup(this);
  }

if you want to call group.addMember() instead of customer.setGroup()

[rbygrave]

@jnehlmeier Is right but there is something subtle that I can explain and in that sense explain how this could be considered a bug.

With OneToMany with a cascade=PERSIST | ALL ... Ebean WILL automatically set the bi-directional relationship (so I this case set the Group on the Customer) IF the beans being cascaded to [Customer in this case] is in a NEW state or DIRTY state.

That is, it is not working as you had expected here because the Customer bean instance is in LOADED state and not DIRTY.

---

What you might have desired would be [when the cascaded bean is in loaded state and not dirty] for ebean to then check the relationship property, check the Group on the Customer instance and if that Group on the Customer was either NULL or different [by id value] to the cascading Group instance ... then automatically set that Group.

That would make the Group property dirty on Customer and we'd see a update customer set group_id = ? where id = ?

---

So maybe this is a bug in that sense. Hmm.

[Incanus3]

Hi guys and thanks for the quick replies.

@rbygrave

What you might have desired would be [when the cascaded bean is in loaded state and not dirty] for ebean to then check the relationship property, check the Group on the Customer instance and if that Group on the Customer was either NULL or different [by id value] to the cascading Group instance ... then automatically set that Group.

Yeah, that's exactly what I have desired :)

With OneToMany with a cascade=PERSIST | ALL ... Ebean WILL automatically set the bi-directional relationship (so I this case set the Group on the Customer) IF the beans being cascaded to [Customer in this case] is in a NEW state or DIRTY state.
That is, it is not working as you had expected here because the Customer bean instance is in LOADED state and not DIRTY.

Hmm, I didn't know about this and it does seem kinda inconsistent - why should it set the reversed (mappedBy) property in some states but not in others? On the other hand, if it does set it (doesn't really matter if just in some cases or all), wouldn't it also be reasonable to expect the reversed property to be set to null for items that were removed from the list, which is something you said here "does not seem sensible to you"? Otherwise it again seems a bit inconsistent that adding to the list would do the "right thing" (at least in NEW and DIRTY states and possibly in all if you consider this a bug and "fix" it), but removing from it wouldn't.

But, and this is also in reaction to @jnehlmeier, there is actually a problem with this removal and that's actually a bigger problem for us (meaning harder to work around). We solved the reported problem by making the "primary" ebean-handled property private and adding a second property using a simple delegate:

// in the entity:

@OneToMany(mappedBy = "category", cascade = [CascadeType.ALL])
private var _products: List<ProductResource> = arrayListOf()

@delegate:Transient
var products by OneToManyRelationship(
    oneToManyProperty = ProductCategoryResource::_products,
    manyToOneProperty = ProductResource::category,
)

// the delegate definition:
class OneToManyRelationship<OtM, MtO>(
    private val oneToManyProperty: KMutableProperty1<OtM, List<MtO>>,
    private val manyToOneProperty: KMutableProperty1<MtO, OtM?>,
) {
    operator fun getValue(owner: OtM, property: KProperty<*>): List<MtO> = oneToManyProperty.get(owner)

    operator fun setValue(owner: OtM, property: KProperty<*>, newValue: List<MtO>) {
        val oldValue = oneToManyProperty.get(owner)

        oneToManyProperty.set(owner, newValue)

        val add = newValue - oldValue.toSet()
        val remove = oldValue - newValue.toSet()

        // these will be saved automatically when the OtM side is saved
        add.forEach {
            manyToOneProperty.set(it, owner)
        }

        // these entities will not be saved automatically when the OtM side is saved,
        // because ebean save doesn't cascade into removed related entities
        remove.forEach {
            manyToOneProperty.set(it, null)
        }
    }
}

As you can notice in the last comment, this works and correctly updates the related (many) entities, but the removed related entities aren't saved when the "primary" (one) entity is saved, so the relationship is not persistently removed. On one hand this is kinda to be expected, because the save only cascades to the related entities referenced by the current value (after removal) of the @ManyToOne property. On the other hand this is a problem that's not easy to fix with our architecture - it would seem that in the delegate's setValue method we could just call save() on those removed related entities, but we can't really do that in this case. Of course, the fact that it's a problem for us doesn't mean that ebean should solve it, but I'd like to sketch out why it's a problem for us, because I think our architecture (or some of its aspects) is not all that uncommon, so there may be lots of other ebean users that will hit a similar problem.

There are two main reasons that make this a problem for us:

• we have a generic (storage-independent) layer on top of ebean database (or actually our db repositories that then interact with the Database) and also other types of storage. this higher-level layer works with domain (not db) repositories of domain (not ebean) entities, which have properties implemented typically as delegates and for each property delegate type (string, int, (domain) entity reference or list of those) we have a subclass for the specific storage type, which in case of the ebean db "storage" just delegate to the ebean entity's property getters and setters (after some simple conversions on the way there and back). these delegates have access to the owning property and instance, but nothing else, including the db repository or the reversed property.
• the create/update process (in this case implemented by a completely generic (storage-independent) set of spring endpoints) works in two steps - 1) setting the properties (thus calling the setters of the domain entity delegates and also some related hooks) and 2) saving the main entity being created/updated. these two steps don't know much about each other and the first step can't really pass any data to the second step (since delegate setters can't return any value), so the second step doesn't know it should also save the removed related entities. we could of course store some transient data that needs to be passed from the first step to the second in some semi-global store, but this would get extremely messy and also as I said, from the higher-level layer (which is what calls these two steps in sequence), all the storage details are hidden. and finally, the first step - setting the properties - should never have persistent side effects, which is why we can't easily call save in the delegate setter, as I said earlier.

Again, I'd like to stress I definitely don't expect the ebean maintainers to go and "fix" this for us, just because it would be convenient for us, but if you don't consider this an inconsistency that should be fixed, I'd really like to know if there is some kind of solution to work around this problem without mixing the "set properties" and the "save" steps.

Also, as a side not, I noticed in some other issues (mainly #431) that there is (or used to be) a @PrivateOwned annotation that could potentially solve this problem for us, but I can't find this annotation anywhere in the ebean code, I only found mentions of it in a few comments and test class names, most interestingly here. I walked through github releases since 2015, but didn't find any mention of it being removed, but I presume it has been. Right above this linked line, there's also a mention of this ModifyListenMode.REMOVALS, which seems like it could be useful, but I have no idea how to work with it from the outside. Is this something that could help and I should spend some time investigating.

Sorry for this really long reaction, it got a bit out of hand, and thank you guys for all the great work you're doing, we really appreciate it.

[rbygrave]

Ok, quite a lot here - I'm not sure I'll cover it all. Firstly we need some "big picture" about when cascading a OneToMany is appropriate I think.

"Ownership" type relationship vs "Assignment" type relationship

For myself I look to classify OneToMany relationships as "Ownership" or "Assignment". TLDR Ownership type relationships are appropriate for cascading (and orphan removal).

"Ownership" type relationships can be identified by asking a few questions. Taking Orders and Order Lines as a classic example:

• Q: Can Order lines be created without an Order? No.
• Q: If an Order is deleted, must the related Order lines also be deleted? Yes
• Q: Can an Order Line be "re-assigned" or "moved" to a different Order? No

There is a strong "Ownership" relationship - An Order Line is "owned" by its associated Order. It frequently means the parent and child have are created at the exact same time and deleted at the exact same time / the parent and child share the same "Lifecycle".

For "Assignment" type relationships we ask the same questions and get different answers. For the purpose of explaining this I'm going to use Customer and Group from above and I'll make up the answers.

• Q: Can Customer be created without a Group? Yes
• Q: If a Group is deleted, must the related Customers also be deleted? No, we instead "unassign" those Customers by setting their group to null.
• Q: Can a Customer be "re-assigned" or "moved" to a different Group? Yes.

In-Memory modify and Flush

With JPA, the approach in general terms is to modify the in-memory model (modify beans, add/remove them from collections etc) and then Flush the entire model to the database. This tends to encourage developers to think about adding/removing beans from collections and using cascade persist ... even for "Assignment type" relationships ... and even when this is a relatively bad idea.

The problem with the "add/remove from collection and flush/cascade" type approach is that it requires the collection to be loaded into memory. If the collection is large this can really hurt but it can actually be inefficient even when the collection is small. If the only thing we are doing is "assignment" or "unassignment" [set the foreign key value] this can be done with a "stateless update" / single sql update statement and without loading any beans or collections into memory.

If we are cascade persisting from Group to its associated Customers in order to unassigned a customer from the group that is a really inefficient way to do it compared to:

customer.setGroup(null);
customer.save();

... or a stateless update.

@PrivateOwned

PrivateOwned was the old feature that became JPA "Orphan Removal" which is an attribute of @OneToMany and specified like @OneToMany(cascade = ALL, orphanRemoval = true).

Orphan removal almost always should go along with cascade = PERSIST or cascade = ALL. Orphan removal also should ONLY be used with a "Ownership" type of relationship.

why should it set the reversed (mappedBy) property in some states but not in others?

I'd say because this is an "unassignment". It is the case of using cascade=PERSIST on a "Assignment" type relationship where removing from the collection is expected to mean "unassignment" [set the foreign key to null]. This is what you are expecting but in general:

• JPA folks will tend to follow Jen's approach
• Ebean orientated folks are not use cascade on the collection for this because it tends to be inefficient and requires the entire collection to be held in memory.

This is maybe why we don't have many people asking about this.

That all said, we still could consider this a bug and perform the "unassignment" on cascade case.

lots of other ebean users that will hit a similar problem.

If they are they are being pretty quite about it.

I think the main point is to see if you can see "Ownership" vs "Assignment" type relationships and maybe see why I think its a bad idea to lean on cascade PERSIST for "Assignment" type relationships.

For example, if a Group can have thousands of Customers in its collection then it's pretty obvious adding/removing from the Group customers collection is getting expensive and doing "assignment" / "unassignment" via that approach is not good.

---

Note for JPA folks. With JPA we generally always need the in-memory model to reflect what we want to persist. With Ebean we explicitly specify what we persist (insert, update, delete) and can even turn off cascade on a per transaction basis.

That means with Ebean we only care about the in-memory model state of the beans that we are explicitly persisting. For example, we can add/remove beans from collections and that only matters if we cascade persist that collection.

This is why JPA folks will tend to follow Jen's example for maintaining bi-directional relationships correctly all the time but some Ebean folks will not bother doing that.

[Incanus3]

Hi guys, I'm sorry for the long delay in responding, I was away for a few weeks and then I completely forgot about this.

As for @rbygrave's response, everything you say makes perfect sense to me, when viewed with the mindset you describe, especially when talking about "assignment" type relationship, although I'm a bit surprised there aren't more people struggling with this. Maybe people just understand this distinction without having to be told like me :).

But, if I were sure my specific relationship (not the customer-group one I've used in the example, I've only used that for the sake of familiarity, our domain in this case is actually document-subdocuments, which is basically the order-items type), and if I didn't mind the inefficiency of having to load the whole collection into memory, am I to understand that if I annotate it with @OneToMany(cascade = ALL, orphanRemoval = true), it should work as I expected? Because I think I tried that and the save didn't cascade on the removal either.

Thank you again for your patience with me.