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

Bug 324756

Summary:

NPE in TransactionCommitContext with re-attached object on branch.

Product:

[Modeling] EMF

Reporter:

Pascal Lehmann <pascal.lehmann>

Component:

cdo.core

Assignee:

Caspar D. <caspar_d>

Status:

CLOSED FIXED

QA Contact:

Eike Stepper <stepper>

Severity:

normal

Priority:

CC:

caspar_d, cyril.jaquier, saulius.tvarijonas

Version:

4.0

Flags:

stepper: review+

Target Milestone:

---

Hardware:

All

OS:

All

Whiteboard:

offline-02

Attachments:

Description	Flags
Testcase	none
Proposed patch	none
Patch v2	none
Patch v3 - ready to be committed	none

Description Pascal Lehmann

2010-09-08 11:11:05 EDT

Build Identifier: 4.0

When trying to commit a change for a re-attached object on a branch you will get either an 'NPE' in case of a containment change, or an 'ISE: Origin revision not found' in case of an attribute change.

The problem is that the ReattachTransition will use the version from the branch the object was branched from, instead of a new version.

Reproducible: Always

Steps to Reproduce:
1. Create an (containment) object, do some changes/commit to increase the version on the object.
2. Create a branch, detach the object (containment), reattach it and commit.
3. Do another change on the object (containment) to create a RevisionDelta with the faulty version, try to commit.

Comment 1 Pascal Lehmann

2010-09-08 11:13:31 EDT

Created attachment 178417 [details]
Testcase

Comment 2 Pascal Lehmann

2010-09-08 11:32:33 EDT

Created attachment 178418 [details]
Proposed patch

Resetting version in case of branch change on re-attach.

Comment 3 Pascal Lehmann

2010-09-08 11:33:02 EDT

1) The number of lines that you changed is smaller than 250.
confirmed.
2) You are the only author of these changed lines.
confirmed.
3) You apply the EPL to these changed lines.
confirmed.

Comment 4 Caspar D.

2010-09-09 01:43:12 EDT

The patch avoids the exceptions, but I'm not sure it addresses 
the problem in the best possible way, because the fix performs
work that should IMO be done by AbstractCDORevision#adjustForCommit.
I'm still debugging to understand why this doesn't work.. perhaps
Pascal has already investigated this?

Flagging for Eike's review anyway, but I don't recommend committing
this yet.

(BTW, this problem is also partially due to the fact that our 
client-side logic doesn't net out redundant deltas -- but that's a 
more general issue that we can address separately.)

Comment 5 Caspar D.

2010-09-09 03:07:41 EDT

Created attachment 178485 [details]
Patch v2

Here's my own stab at a patch.

I believe the problem was not that the ReattachTransition DIDN'T
change the branch, but rather that it DID. In other words, it was
prematurely setting the branch of the reattached revision to the
transaction's *current* branch. But this adjustment shouldn't be
made until postCommit time, by AbstractCDORevision#adjustForCommit.

Comment 6 Pascal Lehmann

2010-09-09 03:09:56 EDT

I completely agree that the version adjustments should be handled in adjustForCommit, so I took another look:
The reason it wasn't caught by the code already present in adjustForCommit is that the branch in the revision for reattached objects will always be the current branch (Line 693: revision.setBranchPoint(transaction.getBranch().getHead());) no matter what branch the revKey acutally came from.

ahh cheers, you beat me :)

Comment 7 Eike Stepper

2010-09-11 05:27:26 EDT

Created attachment 178675 [details]
Patch v3 - ready to be committed

I included the tests in AllConfigs

Comment 8 Caspar D.

2010-09-12 22:17:56 EDT

Committed to HEAD