Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 338884

Summary:

CommitIntegrityCheck does not check inclusion of normal refTargets in NEW state

Product:

[Modeling] EMF

Reporter:

Caspar D. <caspar_d>

Component:

cdo.core

Assignee:

Caspar D. <caspar_d>

Status:

CLOSED FIXED

QA Contact:

Eike Stepper <stepper>

Severity:

normal

Priority:

CC:

saulius.tvarijonas

Version:

4.0

Flags:

stepper: review+
stepper: review+
stepper: review+

Target Milestone:

---

Hardware:

All

OS:

All

Whiteboard:

Attachments:

Description	Flags
Patch (including testcase)	none
Patch (incremental)	none
Patch (incremental, including testcase)	none

Description Caspar D.

2011-03-04 00:25:50 EST

When coding CommitIntegrityCheck, my understanding was that only
containment refs and refs with eOpposites would have to be checked.

What I overlooked, is that if the refTarget is a NEW object, then
it too must be included in the partial commit, even if the ref
is non-containment and has no eOpposite.

Comment 1 Caspar D.

2011-03-04 00:37:52 EST

Created attachment 190350 [details]
Patch (including testcase)

Comment 2 Caspar D.

2011-03-04 00:39:52 EST

Note that I've also sneaked in a minor improvement in the 
partial commit API. I'm changing the committables set from
Set<EObject> to Set<? extends EObject>. I should have done
it that way from the start, methinks.

Comment 3 Caspar D.

2011-03-09 01:38:09 EST

Committed revision 7425.

Comment 4 Caspar D.

2011-03-09 22:33:17 EST

Patch introduces a bug: CommitIntegrityCheck.checkFeatureDelta
will throw an exception at the bottom of the method, if 
containmentOrWithOpposite==false and the delta is not one either
CDOSetFeatureDelta or CDOAddFeatureDelta.

Comment 5 Caspar D.

2011-03-09 22:45:43 EST

Created attachment 190813 [details]
Patch (incremental)

Comment 6 Caspar D.

2011-03-09 22:50:34 EST

For your reviewing convenience: it looks like a big change in the
diff view, but it's not. I simply replaced the recurring

  else if (containmentOrWithOpposite && ...)

condition with a single

  if (containmentOrWithOpposite)

enclosing all relevant branches.

Comment 7 Caspar D.

2011-03-10 03:05:28 EST

Committed revision 7436.

Comment 8 Caspar D.

2011-04-20 05:20:58 EDT

Still not working perfectly.

The remaining problem (or rather: "one known remaining problem") is that
normal (non-bidi) refTargets of NEW objects that *are* included, are not
checked. This is a problem if those targets themselves are NEW, and its
a containment reference. In those cases, those targets must be included
in the commit, but currently the CommitIntegrityCheck doesn't detect the
problem.

Comment 9 Caspar D.

2011-04-20 06:30:53 EDT

Created attachment 193678 [details]
Patch (incremental, including testcase)

Comment 10 Caspar D.

2011-04-20 06:34:00 EDT

(In reply to comment #8)
> This is a problem if those targets themselves are NEW, and its
> a containment reference.

No no no Caspar, don't be silly!! Whether the ref is containment or not
doesn't matter; even if it's non-containment, the target must be committed
otherwise it'll be a dangling reference.

I see you got it right in the patch though.

Comment 11 Eike Stepper

2011-04-20 10:44:23 EDT

;-)

Comment 12 Caspar D.

2011-04-21 04:35:34 EDT

Committed revision 7635.

Comment 13 Eike Stepper

2011-06-23 03:41:04 EDT

Available in R20110608-1407