Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make dry run merge / transplant throw exceptions #6685

Merged
merged 2 commits into from
Apr 25, 2023
Merged

Make dry run merge / transplant throw exceptions #6685

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fd56031
Make dry run merge / transplant throw exceptions
dimas-b Apr 25, 2023
d282e42
fix mergeKeyBehaviorValidation
dimas-b Apr 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions ...rc/main/java/org/projectnessie/versioned/persist/adapter/spi/AbstractDatabaseAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -574,7 +574,7 @@ protected Hash mergeTransplantCommon(

mergeResult.wasSuccessful(!hasCollisions);

if (hasCollisions && !params.isDryRun()) {
if (hasCollisions) {
MergeResult<CommitLogEntry> result = mergeResult.resultantTargetHash(toHead).build();
throw new MergeConflictException(
format(
Expand All @@ -588,7 +588,7 @@ protected Hash mergeTransplantCommon(
result);
}

if (params.isDryRun() || hasCollisions) {
if (params.isDryRun()) {
return toHead;
}

Expand Down
21 changes: 13 additions & 8 deletions ...ests/src/main/java/org/projectnessie/versioned/persist/tests/AbstractMergeTransplant.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,14 +392,19 @@ void mergeTransplantConflict(

// Merge/transplant w/ conflict / dry run

MergeResult<CommitLogEntry> mergeResult =
mergeOrTransplant.apply(
configurer
.apply(commits, 2)
.toBranch(conflict)
.expectedHead(Optional.of(conflictBase))
.isDryRun(true));
assertThat(mergeResult).isEqualTo(expectedMergeResult);
assertThatThrownBy(
() ->
mergeOrTransplant.apply(
configurer
.apply(commits, 2)
.toBranch(conflict)
.expectedHead(Optional.of(conflictBase))
.isDryRun(true)))
.isInstanceOf(MergeConflictException.class)
.hasMessage("The following keys have been changed in conflict: 'key-0', 'key-1'")
.asInstanceOf(InstanceOfAssertFactories.throwable(MergeConflictException.class))
.extracting(MergeConflictException::getMergeResult)
.isEqualTo(expectedMergeResult);

// Merge/transplant w/ conflict

Expand Down
10 changes: 5 additions & 5 deletions ...tore/src/main/java/org/projectnessie/versioned/storage/versionstore/VersionStoreImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -631,7 +631,7 @@ public MergeResult<Commit> merge(
(merge, retryState) ->
merge.merge(retryState, fromHash, updateCommitMetadata, mergeBehaviors, dryRun));

return mergeTransplantResponse(dryRun, mergeResult);
return mergeTransplantResponse(mergeResult);
}

@Override
Expand Down Expand Up @@ -668,12 +668,12 @@ public MergeResult<Commit> transplant(
mergeBehaviors,
dryRun));

return mergeTransplantResponse(dryRun, mergeResult);
return mergeTransplantResponse(mergeResult);
}

private MergeResult<Commit> mergeTransplantResponse(
boolean dryRun, MergeResult<Commit> mergeResult) throws MergeConflictException {
if (!dryRun && !mergeResult.wasSuccessful()) {
private MergeResult<Commit> mergeTransplantResponse(MergeResult<Commit> mergeResult)
throws MergeConflictException {
if (!mergeResult.wasSuccessful()) {
throw new MergeConflictException(
String.format(
"The following keys have been changed in conflict: %s",
Expand Down
157 changes: 88 additions & 69 deletions versioned/tests/src/main/java/org/projectnessie/versioned/tests/AbstractMerge.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,9 @@
import org.assertj.core.api.junit.jupiter.InjectSoftAssertions;
import org.assertj.core.api.junit.jupiter.SoftAssertionsExtension;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.CsvSource;
import org.junit.jupiter.params.provider.ValueSource;
import org.projectnessie.model.CommitMeta;
import org.projectnessie.model.Conflict;
Expand Down Expand Up @@ -202,52 +202,36 @@ protected void mergeKeyBehaviorValidation(boolean dryRun) throws Exception {
Content c11 = store().getValue(firstCommit, ContentKey.of("t1"));

for (MergeBehavior mergeBehavior : MergeBehavior.values()) {
if (dryRun) {
soft.assertThat(
store()
.merge(
thirdCommit,
targetBranch,
Optional.empty(),
metadataRewriter,
false,
ImmutableMap.of(
keyT3, MergeKeyBehavior.of(keyT3, mergeBehavior, c11, null)),
MergeBehavior.NORMAL,
dryRun,
false))
.describedAs("MergeBehavior.%s", mergeBehavior)
.extracting(
MergeResult::wasApplied, MergeResult::wasSuccessful, r -> r.getDetails().get(keyT3))
.containsExactly(
false,
false,
KeyDetails.keyDetails(
mergeBehavior,
MergeResult.ConflictType.UNRESOLVABLE,
Conflict.conflict(
ConflictType.VALUE_DIFFERS,
keyT3,
"values of existing and expected content for key 't3' are different")));
} else {
soft.assertThatThrownBy(
() ->
store()
.merge(
thirdCommit,
targetBranch,
Optional.empty(),
metadataRewriter,
false,
ImmutableMap.of(
keyT3, MergeKeyBehavior.of(keyT3, mergeBehavior, c11, null)),
MergeBehavior.NORMAL,
dryRun,
false))
.describedAs("MergeBehavior.%s", mergeBehavior)
.isInstanceOf(MergeConflictException.class)
.hasMessage("The following keys have been changed in conflict: 't3'");
}
soft.assertThatThrownBy(
() ->
store()
.merge(
thirdCommit,
targetBranch,
Optional.empty(),
metadataRewriter,
false,
ImmutableMap.of(
keyT3, MergeKeyBehavior.of(keyT3, mergeBehavior, c11, null)),
MergeBehavior.NORMAL,
dryRun,
false))
.describedAs("MergeBehavior.%s", mergeBehavior)
.hasMessage("The following keys have been changed in conflict: 't3'")
.asInstanceOf(type(MergeConflictException.class))
.extracting(MergeConflictException::getMergeResult)
.extracting(
MergeResult::wasApplied, MergeResult::wasSuccessful, r -> r.getDetails().get(keyT3))
.containsExactly(
false,
false,
KeyDetails.keyDetails(
mergeBehavior,
MergeResult.ConflictType.UNRESOLVABLE,
Conflict.conflict(
ConflictType.VALUE_DIFFERS,
keyT3,
"values of existing and expected content for key 't3' are different")));
soft.assertAll();
}
}
Expand Down Expand Up @@ -491,6 +475,8 @@ void compareDryAndEffectiveMergeResults(boolean individualCommits) throws Versio
store().create(newBranch, Optional.of(initialHash));
MetadataRewriter<CommitMeta> metadataRewriter = createMetadataRewriter("");

Hash origHead = store().getNamedRef(newBranch.getName(), GetNamedRefsParams.DEFAULT).getHash();

MergeResult<Commit> dryMergeResult =
store()
.merge(
Expand All @@ -514,6 +500,10 @@ void compareDryAndEffectiveMergeResults(boolean individualCommits) throws Versio
MergeResult::getExpectedHash)
.containsExactly(false, true, initialHash, newBranch, initialHash, initialHash);

// Dry merges should not advance HEAD
soft.assertThat(store().getNamedRef(newBranch.getName(), GetNamedRefsParams.DEFAULT).getHash())
.isEqualTo(origHead);

MergeResult<Commit> mergeResult =
store()
.merge(
Expand All @@ -528,6 +518,7 @@ void compareDryAndEffectiveMergeResults(boolean individualCommits) throws Versio
true);

Hash head = store().getNamedRef(newBranch.getName(), GetNamedRefsParams.DEFAULT).getHash();
soft.assertThat(head).isNotEqualTo(origHead);

soft.assertThat(mergeResult.getSourceCommits())
.satisfiesAnyOf(
Expand Down Expand Up @@ -878,8 +869,14 @@ protected void mergeWithCommonAncestor(boolean individualCommits) throws Version
}

@ParameterizedTest
@ValueSource(booleans = {false, true})
protected void mergeWithConflictingKeys(boolean individualCommits) throws VersionStoreException {
@CsvSource({
"false,false",
"false,true",
"true,false",
"true,true",
})
protected void mergeWithConflictingKeys(boolean individualCommits, boolean dryRun)
throws VersionStoreException {
final BranchName mergeInto = BranchName.of("foofoo");
final BranchName mergeFrom = BranchName.of("barbar");
store().create(mergeInto, Optional.of(this.initialHash));
Expand Down Expand Up @@ -931,7 +928,7 @@ protected void mergeWithConflictingKeys(boolean individualCommits) throws Versio
individualCommits,
Collections.emptyMap(),
MergeBehavior.NORMAL,
false,
dryRun,
false))
.isInstanceOf(ReferenceConflictException.class)
.hasMessageContaining("The following keys have been changed in conflict:")
Expand All @@ -952,7 +949,7 @@ protected void mergeWithConflictingKeys(boolean individualCommits) throws Versio
conflictingKey2,
MergeKeyBehavior.of(conflictingKey2, MergeBehavior.DROP)),
MergeBehavior.NORMAL,
false,
dryRun,
false))
.isInstanceOf(ReferenceConflictException.class)
.hasMessageContaining("The following keys have been changed in conflict:")
Expand All @@ -973,7 +970,7 @@ protected void mergeWithConflictingKeys(boolean individualCommits) throws Versio
conflictingKey1,
MergeKeyBehavior.of(conflictingKey1, MergeBehavior.NORMAL)),
MergeBehavior.DROP,
false,
dryRun,
false))
.isInstanceOf(ReferenceConflictException.class)
.hasMessageContaining("The following keys have been changed in conflict:")
Expand All @@ -994,7 +991,7 @@ protected void mergeWithConflictingKeys(boolean individualCommits) throws Versio
conflictingKey1,
MergeKeyBehavior.of(conflictingKey1, MergeBehavior.FORCE)),
MergeBehavior.NORMAL,
false,
dryRun,
false))
.isInstanceOf(ReferenceConflictException.class)
.hasMessageContaining("The following keys have been changed in conflict:")
Expand Down Expand Up @@ -1065,8 +1062,13 @@ protected void mergeWithConflictingKeys(boolean individualCommits) throws Versio
}

@ParameterizedTest
@ValueSource(booleans = {false, true})
protected void mergeIntoConflictingBranch(boolean individualCommits)
@CsvSource({
"false,false",
"false,true",
"true,false",
"true,true",
})
protected void mergeIntoConflictingBranch(boolean individualCommits, boolean dryRun)
throws VersionStoreException {
final BranchName newBranch = BranchName.of("bar_3");
store().create(newBranch, Optional.of(initialHash));
Expand All @@ -1083,14 +1085,19 @@ protected void mergeIntoConflictingBranch(boolean individualCommits)
individualCommits,
Collections.emptyMap(),
MergeBehavior.NORMAL,
false,
dryRun,
false))
.isInstanceOf(ReferenceConflictException.class);
}

@ParameterizedTest
@ValueSource(booleans = {false, true})
protected void mergeIntoNonExistingBranch(boolean individualCommits) {
@CsvSource({
"false,false",
"false,true",
"true,false",
"true,true",
})
protected void mergeIntoNonExistingBranch(boolean individualCommits, boolean dryRun) {
final BranchName newBranch = BranchName.of("bar_5");
soft.assertThatThrownBy(
() ->
Expand All @@ -1103,14 +1110,19 @@ protected void mergeIntoNonExistingBranch(boolean individualCommits) {
individualCommits,
Collections.emptyMap(),
MergeBehavior.NORMAL,
false,
dryRun,
false))
.isInstanceOf(ReferenceNotFoundException.class);
}

@ParameterizedTest
@ValueSource(booleans = {false, true})
protected void mergeIntoNonExistingReference(boolean individualCommits)
@CsvSource({
"false,false",
"false,true",
"true,false",
"true,true",
})
protected void mergeIntoNonExistingReference(boolean individualCommits, boolean dryRun)
throws VersionStoreException {
final BranchName newBranch = BranchName.of("bar_6");
store().create(newBranch, Optional.of(initialHash));
Expand All @@ -1125,14 +1137,20 @@ protected void mergeIntoNonExistingReference(boolean individualCommits)
individualCommits,
Collections.emptyMap(),
MergeBehavior.NORMAL,
false,
dryRun,
false))
.isInstanceOf(ReferenceNotFoundException.class);
}

@ParameterizedTest
@ValueSource(booleans = {false, true})
protected void mergeEmptyCommit(boolean individualCommits) throws VersionStoreException {
@CsvSource({
"false,false",
"false,true",
"true,false",
"true,true",
})
protected void mergeEmptyCommit(boolean individualCommits, boolean dryRun)
throws VersionStoreException {
BranchName source = BranchName.of("source");
BranchName target = BranchName.of("target");
store().create(source, Optional.of(this.initialHash));
Expand Down Expand Up @@ -1192,7 +1210,7 @@ protected void mergeEmptyCommit(boolean individualCommits) throws VersionStoreEx
key2,
MergeKeyBehavior.of(key2, MergeBehavior.DROP)),
MergeBehavior.NORMAL,
false,
dryRun,
false);

// No new commit should have been created in the target branch
Expand All @@ -1202,8 +1220,9 @@ protected void mergeEmptyCommit(boolean individualCommits) throws VersionStoreEx
}
}

@Test
public void mergeFromAndIntoHead() throws Exception {
@ParameterizedTest
@ValueSource(booleans = {false, true})
public void mergeFromAndIntoHead(boolean dryRun) throws Exception {
BranchName branch = BranchName.of("source");
store().create(branch, Optional.of(this.initialHash));

Expand Down Expand Up @@ -1238,7 +1257,7 @@ public void mergeFromAndIntoHead() throws Exception {
false,
emptyMap(),
MergeBehavior.NORMAL,
false,
dryRun,
false));

soft.assertThatIllegalArgumentException()
Expand All @@ -1253,7 +1272,7 @@ public void mergeFromAndIntoHead() throws Exception {
false,
emptyMap(),
MergeBehavior.NORMAL,
false,
dryRun,
false));
}
}