Cascading update isn't propagated - use of merge() vs "stateless update"

[viluon]

I believe we've come across a bug with EbeanServer.update(). In the provided test case, an update isn't propagated to linked entities (despite CascadeType.ALL). This happens on Ebeans 11.22.10.

Expected behavior

The provided unit test should pass.

Actual behavior

The update doesn't touch a list of entities deeper within the hierarchy.

Steps to reproduce

This gist contains a unit test showcasing the issue. The unit test fails with the following output:

java.lang.AssertionError: 
Expected :1
Actual   :2

	at org.junit.Assert.fail(Assert.java:88)
	at org.junit.Assert.failNotEquals(Assert.java:834)
	at org.junit.Assert.assertEquals(Assert.java:645)
	at org.junit.Assert.assertEquals(Assert.java:631)
	at org.somewhere.EbeanSoftDeleteTest.testEbeanBug(EbeanSoftDeleteTest.java:130)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
	at org.springframework.test.context.junit4.statements.RunBeforeTestMethodCallbacks.evaluate(RunBeforeTestMethodCallbacks.java:75)
	at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
	at org.springframework.test.context.junit4.statements.RunAfterTestMethodCallbacks.evaluate(RunAfterTestMethodCallbacks.java:86)
	at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:298)
	at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:292)
	at java.util.concurrent.FutureTask.run$$$capture(FutureTask.java:266)
	at java.util.concurrent.FutureTask.run(FutureTask.java)
	at java.lang.Thread.run(Thread.java:748)

[rbygrave]

A way to get the test to pass is to use merge rather than a stateless update.

That is:

    MergeOptions options = new MergeOptionsBuilder()
      .addPath("foo")
      .addPath("foo.children")
      .build();

    DB.merge(deserialized, options);

In short, stateless update has some limitations wrt knowing how far to propagate the removal of orphans. In this case the ToMany is not directly off the root type and instead off the "foo" path.

Merge can be used to any depth because we are explicitly telling Ebean the paths that should be included in the 'merge / update'. In the merge code above we explicitly specify the paths of foo and foo.children and that means that it is explicit that we expect the foo.children to be part of the graph and that if they are "not present" that means that they should be deleted / treated as orphans for removal.

So to me you have hit that limit of where stateless update doesn't work (as it can't differentiate between "not present" and "orphan removal").

Now I can understand that with this model there is the "orphanRemoval=true" on those paths so that implies that stateless update could reasonably expect to treat "not present" as orphan removal - I'm not sure if that can be safely done or not so that will take some investigation.

In the meantime you should consider using merge() instead.

Does that make sense?

[rbygrave]

@viluon did the above comment make sense?

Thanks, Rob.

[rbygrave]

@viluon ... I'll close this for now. I am assuming you are using merge() for this case rather than a stateless update.

[viluon]

@rbygrave thanks for the workaround, but a workaround isn't a fix. I would expect a stateless update to work with CascadeType.ALL. This bug is the reason why we are stuck with an older version of Ebeans, the newer versions are unusable for production due to this issue.

Note that a fix for this and another issue was proposed in #1403, but was rejected due to refactoring work. The provided unit test shows that a proper solution never made it to master.

[viluon]

@rbygrave could you please reopen this issue to reflect the state of the bug?

[rbygrave]

This bug is the reason why we are stuck with an older version of Ebeans

That doesn't make sense to me. You will need to expand on that ... (are you stuck on your own fork?)

fix for this and another issue was proposed in #1403, but was rejected

Well that PR was closed with agreement that the issue was resolved by other changes. That PR didn't contain ANY failing test case. If that PR contained "a fix" then you could create a PR with the failing test and the associated fix and I could review that - no problem with that - why don't you do that?

I would expect a stateless update to work with CascadeType.ALL

I believe it is more complex than that. More specifically the ability to differentiate between an empty collection for non-cascading and an empty collection that is "remove all as orphans". We have to be pretty careful with the behaviour here. I believe the interesting aspect here is the orphanRemoval=true setting on the relationships which might be enough to imply that a stateless update can be "aggressive" wrt cascading collections.

Another option would be to use MergeOptions/MergeOptionsBuilder to be explicit on the paths to cascade (rather than being implicit).

merge() is nice and explicit with no room for interpretation - it is not clear to me why you are rejecting that option.

The other workaround would be to perform a stateless update on entityRoot.foo (as then the collection is off the "root" bean being updated).

I'll re-open but not sure when I'll get to look at it. I would be good if you can clarify the points raised above.

Cheers, Rob.

[rbygrave]

Closing. No feed back, expecting people to use merge() when appropriate.
Cheers, Rob.

[viluon]

I think the disadvantages of using merge() are obvious: a person using it must know the precise nature of the entities they are dealing with (fields and annotations, not just columns and tables) and must make the decision of using a merge() vs using a regular update in the first place. Building the MergeOptions object is a potential source of hard-to-identify bugs if the supplied paths (strings) aren't correctly refactored when a data model change happens. Think about renaming the fields deep down these paths.

What's even worse is a situation where the programmer doesn't realise that a stateless update is not enough, this obscure Ebean bug isn't caught in testing and makes its way to production, where it causes serious trouble and requires a manual revert of the database to a previous state.

Using merge() is a workaround, not a fix. Another workaround would be writing raw SQL, but I think we can both agree that that would not be a wise course of action. merge() is definitely less ugly, but still not nearly as clean as a stateless update.

To summarise, using merge() is a verbose, unintuitive, error-prone way to construct update queries. The provided annotations should be enough for the ORM to determine the correct behaviour, especially since this wasn't a problem in Ebean 11.7.2.

I would consider this bug in violation of the guarantees promised by the Java Persistence API, but I am unsure of JPA's exact phrasings.

I have provided a test case showcasing the behaviour. If you insist on closing the issue, you are saying that this is in fact not a bug and that the test should never have passed in the first place. Therefore, a feature of Ebean 11.7.2 has been deprecated later on for no apparent reason.

This is also in violation of semantic versioning, which requires API removals to bump the major version number, but I'm not sure if Ebean aims to follow semver or not.

If you keep this issue open, you'll allow people to discover the bug and perhaps implement patches to fix it. It is a way of admitting that this is a real problem in search of a solution.

[rbygrave]

Ok, lets reopen.

[rbygrave]

Refer to #1824 ... which is the same issue.

PR pending.

This is considered as a breaking change, removing the deleteMissingChildren parameter for stateless updates in favour of always honouring orphanRemoval.