diff options
author | Philip Langer | 2014-10-11 18:14:20 +0000 |
---|---|---|
committer | Laurent Goubet | 2014-10-14 09:49:39 +0000 |
commit | b32dfec137438c9242a1e674e9fec19ac659681d (patch) | |
tree | 6f79cc0bad79974dbda14052cecda5c3d88425c4 | |
parent | 223fb7874e850c3e8d267bd96fae09d3ee6938c7 (diff) | |
download | org.eclipse.emf.compare-b32dfec137438c9242a1e674e9fec19ac659681d.tar.gz org.eclipse.emf.compare-b32dfec137438c9242a1e674e9fec19ac659681d.tar.xz org.eclipse.emf.compare-b32dfec137438c9242a1e674e9fec19ac659681d.zip |
[446739] Fixes obtaining the target feature in three-way merge of a move
We now make sure that the target feature is correctly determined. This
prevents a NullPointerException thrown during a three-way merge of a
move, in which the target containment feature is not available in the
type of the original containment feature.
Bug: 446739
Signed-off-by: Philip Langer <planger@eclipsesource.com>
Change-Id: I98a7f37bb5e43f10ddaf4a042893ed4773bf25b9
7 files changed, 231 insertions, 13 deletions
diff --git a/plugins/org.eclipse.emf.compare.tests/src/org/eclipse/emf/compare/tests/merge/ThreeWayBatchMergingTest.java b/plugins/org.eclipse.emf.compare.tests/src/org/eclipse/emf/compare/tests/merge/ThreeWayBatchMergingTest.java new file mode 100644 index 000000000..8c5659cfc --- /dev/null +++ b/plugins/org.eclipse.emf.compare.tests/src/org/eclipse/emf/compare/tests/merge/ThreeWayBatchMergingTest.java @@ -0,0 +1,150 @@ +/******************************************************************************* + * Copyright (c) 2014 EclipseSource Muenchen GmbH and others. + * All rights reserved. This program and the accompanying materials + * are made available under the terms of the Eclipse Public License v1.0 + * which accompanies this distribution, and is available at + * http://www.eclipse.org/legal/epl-v10.html + * + * Contributors: + * Philip Langer - initial API and implementation + *******************************************************************************/ +package org.eclipse.emf.compare.tests.merge; + +import static org.eclipse.emf.compare.DifferenceSource.LEFT; +import static org.eclipse.emf.compare.DifferenceSource.RIGHT; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; + +import org.eclipse.emf.common.notify.Notifier; +import org.eclipse.emf.common.util.BasicMonitor; +import org.eclipse.emf.common.util.EList; +import org.eclipse.emf.compare.Comparison; +import org.eclipse.emf.compare.ConflictKind; +import org.eclipse.emf.compare.Diff; +import org.eclipse.emf.compare.DifferenceSource; +import org.eclipse.emf.compare.EMFCompare; +import org.eclipse.emf.compare.merge.BatchMerger; +import org.eclipse.emf.compare.merge.IBatchMerger; +import org.eclipse.emf.compare.merge.IMerger; +import org.eclipse.emf.compare.scope.DefaultComparisonScope; +import org.eclipse.emf.compare.scope.IComparisonScope; +import org.eclipse.emf.compare.tests.merge.data.ThreeWayMergeInputData; +import org.eclipse.emf.compare.tests.nodes.NodesPackage; +import org.eclipse.emf.ecore.resource.Resource; +import org.junit.Test; + +/** + * Tests three-way comparison of {@link NodesPackage nodes models} and subsequent merging using the + * {@link BatchMerger}. + * + * @author Philip Langer <planger@eclipsesource.com> + */ +public class ThreeWayBatchMergingTest { + + final private ThreeWayMergeInputData input = new ThreeWayMergeInputData(); + + final private IMerger.Registry mergerRegistry = IMerger.RegistryImpl.createStandaloneInstance(); + + final private IBatchMerger merger = new BatchMerger(mergerRegistry); + + /** + * Tests a scenario in which a model element is moved from one container into another, whereas the + * original container's type does not provide the containment reference that is used as the target of the + * move. This lead to an NPE (cf. bug #446739). In this test, the move is applied on the left-hand side. + * + * @throws IOException + * if {@link ThreeWayMergeInputData} fails to load the test models. + */ + @Test + public void mergingMoveToDifferentContainmentFeatureLeft() throws IOException { + final Resource origin = input.getMoveToDifferentContainmentFeatureOrigin(); + final Resource left = input.getMoveToDifferentContainmentFeatureMove(); + final Resource right = input.getMoveToDifferentContainmentFeatureUnchanged(); + assertUnchanged(new DefaultComparisonScope(left, right, origin), RIGHT); + batchMergeAndAssertEquality(new DefaultComparisonScope(left, right, origin), LEFT); + } + + /** + * Tests a scenario in which a model element is moved from one container into another, whereas the + * original container's type does not provide the containment reference that is used as the target of the + * move. This lead to an NPE (cf. bug #446739). In this test, the move is applied on the right-hand side. + * + * @throws IOException + * if {@link ThreeWayMergeInputData} fails to load the test models. + */ + @Test + public void mergingMoveToDifferentContainmentFeatureRight() throws IOException { + final Resource origin = input.getMoveToDifferentContainmentFeatureOrigin(); + final Resource left = input.getMoveToDifferentContainmentFeatureUnchanged(); + final Resource right = input.getMoveToDifferentContainmentFeatureMove(); + assertUnchanged(new DefaultComparisonScope(left, right, origin), LEFT); + batchMergeAndAssertEquality(new DefaultComparisonScope(left, right, origin), RIGHT); + } + + private void assertUnchanged(IComparisonScope scope, DifferenceSource side) { + final Notifier supposeUnchanged; + if (LEFT.equals(side)) { + supposeUnchanged = scope.getLeft(); + } else { + supposeUnchanged = scope.getRight(); + } + final IComparisonScope scopeForAssert = createTwoWayScopeWithOrigin(scope, supposeUnchanged); + EList<Diff> diffsForAssertion = compare(scopeForAssert).getDifferences(); + assertEquals(0, diffsForAssertion.size()); + } + + private IComparisonScope createTwoWayScopeWithOrigin(IComparisonScope scope, final Notifier notifier) { + return new DefaultComparisonScope(scope.getOrigin(), notifier, null); + } + + private void batchMergeAndAssertEquality(IComparisonScope scope, DifferenceSource side) { + batchMerge(scope, side); + assertEqualityOfLeftAndRight(scope); + } + + private void batchMerge(IComparisonScope scope, DifferenceSource side) { + Comparison comparison = compare(scope); + final EList<Diff> differences = comparison.getDifferences(); + + if (LEFT.equals(side)) { + final List<Diff> leftDiffs = filterDiffs(differences, LEFT, false); + merger.copyAllLeftToRight(leftDiffs, new BasicMonitor()); + } else { + final List<Diff> rightDiffs = filterDiffs(differences, RIGHT, false); + merger.copyAllRightToLeft(rightDiffs, new BasicMonitor()); + } + } + + private Comparison compare(IComparisonScope scope) { + return EMFCompare.builder().build().compare(scope); + } + + private List<Diff> filterDiffs(List<Diff> diffs, DifferenceSource source, boolean filterConflicting) { + List<Diff> filteredDiffs = new ArrayList<Diff>(); + for (Diff diff : diffs) { + if (source.equals(diff.getSource()) && (!isConflicting(diff) || !filterConflicting)) { + filteredDiffs.add(diff); + } + } + return filteredDiffs; + } + + private boolean isConflicting(Diff diff) { + return diff.getConflict() != null && ConflictKind.REAL.equals(diff.getConflict().getKind()); + } + + private void assertEqualityOfLeftAndRight(IComparisonScope scope) { + final Notifier left = scope.getLeft(); + final Notifier right = scope.getRight(); + final IComparisonScope assertScope = new DefaultComparisonScope(left, right, null); + final Comparison assertComparison = compare(assertScope); + final EList<Diff> assertDiffs = assertComparison.getDifferences(); + + assertTrue(assertDiffs.isEmpty()); + } + +} diff --git a/plugins/org.eclipse.emf.compare.tests/src/org/eclipse/emf/compare/tests/merge/data/ThreeWayMergeInputData.java b/plugins/org.eclipse.emf.compare.tests/src/org/eclipse/emf/compare/tests/merge/data/ThreeWayMergeInputData.java new file mode 100644 index 000000000..2ad71d797 --- /dev/null +++ b/plugins/org.eclipse.emf.compare.tests/src/org/eclipse/emf/compare/tests/merge/data/ThreeWayMergeInputData.java @@ -0,0 +1,31 @@ +/******************************************************************************* + * Copyright (c) 2014 EclipseSource Muenchen GmbH and others. + * All rights reserved. This program and the accompanying materials + * are made available under the terms of the Eclipse Public License v1.0 + * which accompanies this distribution, and is available at + * http://www.eclipse.org/legal/epl-v10.html + * + * Contributors: + * Philip Langer - initial API and implementation + *******************************************************************************/ +package org.eclipse.emf.compare.tests.merge.data; + +import java.io.IOException; + +import org.eclipse.emf.compare.tests.framework.AbstractInputData; +import org.eclipse.emf.ecore.resource.Resource; + +@SuppressWarnings("nls") +public class ThreeWayMergeInputData extends AbstractInputData { + public Resource getMoveToDifferentContainmentFeatureOrigin() throws IOException { + return loadFromClassLoader("threeway/movedifferentcontainmentfeature/origin.nodes"); + } + + public Resource getMoveToDifferentContainmentFeatureMove() throws IOException { + return loadFromClassLoader("threeway/movedifferentcontainmentfeature/side-of-move.nodes"); + } + + public Resource getMoveToDifferentContainmentFeatureUnchanged() throws IOException { + return loadFromClassLoader("threeway/movedifferentcontainmentfeature/unchanged.nodes"); + } +} diff --git a/plugins/org.eclipse.emf.compare.tests/src/org/eclipse/emf/compare/tests/merge/data/threeway/movedifferentcontainmentfeature/origin.nodes b/plugins/org.eclipse.emf.compare.tests/src/org/eclipse/emf/compare/tests/merge/data/threeway/movedifferentcontainmentfeature/origin.nodes new file mode 100644 index 000000000..5771610c5 --- /dev/null +++ b/plugins/org.eclipse.emf.compare.tests/src/org/eclipse/emf/compare/tests/merge/data/threeway/movedifferentcontainmentfeature/origin.nodes @@ -0,0 +1,9 @@ +<?xml version="1.0" encoding="ASCII"?>
+<nodes:NodeMultipleContainment
+ xmi:version="2.0"
+ xmlns:xmi="http://www.omg.org/XMI"
+ xmlns:nodes="http://www.eclipse.org/emf/compare/tests/nodes"
+ name="Root">
+ <containmentRef2 xsi:type="nodes:Node" name="A"/>
+ <containmentRef2 xsi:type="nodes:Node" name="B"/>
+</nodes:NodeMultipleContainment>
diff --git a/plugins/org.eclipse.emf.compare.tests/src/org/eclipse/emf/compare/tests/merge/data/threeway/movedifferentcontainmentfeature/side-of-move.nodes b/plugins/org.eclipse.emf.compare.tests/src/org/eclipse/emf/compare/tests/merge/data/threeway/movedifferentcontainmentfeature/side-of-move.nodes new file mode 100644 index 000000000..0ddb5e4db --- /dev/null +++ b/plugins/org.eclipse.emf.compare.tests/src/org/eclipse/emf/compare/tests/merge/data/threeway/movedifferentcontainmentfeature/side-of-move.nodes @@ -0,0 +1,10 @@ +<?xml version="1.0" encoding="ASCII"?>
+<nodes:NodeMultipleContainment
+ xmi:version="2.0"
+ xmlns:xmi="http://www.omg.org/XMI"
+ xmlns:nodes="http://www.eclipse.org/emf/compare/tests/nodes"
+ name="Root">
+ <containmentRef2 xsi:type="nodes:Node" name="B">
+ <containmentRef1 xsi:type="nodes:Node" name="A"/>
+ </containmentRef2>
+</nodes:NodeMultipleContainment>
diff --git a/plugins/org.eclipse.emf.compare.tests/src/org/eclipse/emf/compare/tests/merge/data/threeway/movedifferentcontainmentfeature/unchanged.nodes b/plugins/org.eclipse.emf.compare.tests/src/org/eclipse/emf/compare/tests/merge/data/threeway/movedifferentcontainmentfeature/unchanged.nodes new file mode 100644 index 000000000..5771610c5 --- /dev/null +++ b/plugins/org.eclipse.emf.compare.tests/src/org/eclipse/emf/compare/tests/merge/data/threeway/movedifferentcontainmentfeature/unchanged.nodes @@ -0,0 +1,9 @@ +<?xml version="1.0" encoding="ASCII"?>
+<nodes:NodeMultipleContainment
+ xmi:version="2.0"
+ xmlns:xmi="http://www.omg.org/XMI"
+ xmlns:nodes="http://www.eclipse.org/emf/compare/tests/nodes"
+ name="Root">
+ <containmentRef2 xsi:type="nodes:Node" name="A"/>
+ <containmentRef2 xsi:type="nodes:Node" name="B"/>
+</nodes:NodeMultipleContainment>
diff --git a/plugins/org.eclipse.emf.compare.tests/src/org/eclipse/emf/compare/tests/suite/AllTests.java b/plugins/org.eclipse.emf.compare.tests/src/org/eclipse/emf/compare/tests/suite/AllTests.java index ca5805b02..01df42167 100644 --- a/plugins/org.eclipse.emf.compare.tests/src/org/eclipse/emf/compare/tests/suite/AllTests.java +++ b/plugins/org.eclipse.emf.compare.tests/src/org/eclipse/emf/compare/tests/suite/AllTests.java @@ -41,6 +41,7 @@ import org.eclipse.emf.compare.tests.merge.IndividualMergeOutOfScopeValuesTest; import org.eclipse.emf.compare.tests.merge.IndividualMergeTest; import org.eclipse.emf.compare.tests.merge.MultipleMergeTest; import org.eclipse.emf.compare.tests.merge.PseudoConflictMergeTest; +import org.eclipse.emf.compare.tests.merge.ThreeWayBatchMergingTest; import org.eclipse.emf.compare.tests.merge.TwoWayBatchMergingTest; import org.eclipse.emf.compare.tests.nodes.NodesPackage; import org.eclipse.emf.compare.tests.nodes.util.NodesResourceFactoryImpl; @@ -72,7 +73,7 @@ import org.junit.runners.Suite.SuiteClasses; ConflictMergeTest.class, PseudoConflictMergeTest.class, ProximityIndexTest.class, AllRCPTests.class, FeatureMaps2wayMergeTest.class, FeatureMaps3wayMergeTest.class, FeatureMapsConflictsMergeTest.class, FeatureMapsPseudoConflictsMergeTest.class, TwoWayBatchMergingTest.class, EqualityHelperTest.class, - FeatureFilterTest.class }) + FeatureFilterTest.class, ThreeWayBatchMergingTest.class }) public class AllTests { /** * Standalone launcher for all of compare's tests. diff --git a/plugins/org.eclipse.emf.compare/src/org/eclipse/emf/compare/internal/utils/DiffUtil.java b/plugins/org.eclipse.emf.compare/src/org/eclipse/emf/compare/internal/utils/DiffUtil.java index 1b1118320..c0d038807 100644 --- a/plugins/org.eclipse.emf.compare/src/org/eclipse/emf/compare/internal/utils/DiffUtil.java +++ b/plugins/org.eclipse.emf.compare/src/org/eclipse/emf/compare/internal/utils/DiffUtil.java @@ -7,7 +7,7 @@ * * Contributors: * Obeo - initial API and implementation - * Philip Langer - Fixes for bug 440679, 441258, 442439, 443504, and refactorings + * Philip Langer - Fixes for bug 440679, 441258, 442439, 443504, 446739, and refactorings *******************************************************************************/ package org.eclipse.emf.compare.internal.utils; @@ -1065,18 +1065,9 @@ public final class DiffUtil { final EStructuralFeature diffFeature = getChangedFeature(diff); final Object diffValue = getChangedValue(diff); - if (isContainmentReferenceMove(diff)) { + if (isContainmentReferenceMove(diff) && isTargetOnTheRight(diff, rightToLeft)) { final Match valueMatch = comparison.getMatch((EObject)diffValue); - if (rightToLeft && DifferenceSource.LEFT == diff.getSource() && valueMatch.getRight() != null) { - targetFeature = valueMatch.getRight().eContainingFeature(); - } else if (!rightToLeft && DifferenceSource.RIGHT == diff.getSource() - && valueMatch.getLeft() != null) { - targetFeature = valueMatch.getRight().eContainingFeature(); - } else if (valueMatch.getOrigin() != null) { - targetFeature = valueMatch.getOrigin().eContainingFeature(); - } else { - targetFeature = diffFeature; - } + targetFeature = valueMatch.getRight().eContainingFeature(); } else { targetFeature = diffFeature; } @@ -1100,6 +1091,23 @@ public final class DiffUtil { } /** + * Specifies whether the target container, target feature, and target value of the current merging is the + * right-hand side. This depends on the source of the {@code diff} and the direction of merging as + * specified in {@code rightToLeft}. + * + * @param diff + * The diff to check. + * @param rightToLeft + * Direction of the merging. {@code true} if the merge is to be done on the left side, making + * 'target' the right side, {@code false} otherwise. + * @return <code>true</code> if the target is on the right side, <code>false</code> otherwise. + */ + private static boolean isTargetOnTheRight(Diff diff, boolean rightToLeft) { + return (rightToLeft && DifferenceSource.LEFT == diff.getSource()) + || (!rightToLeft && DifferenceSource.RIGHT == diff.getSource()); + } + + /** * When computing the insertion index of an element in a list, we need to ignore all elements present in * that list that feature unresolved Diffs on the same feature. * |