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.

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

Cleanup V2 MergeResponse #6747

Merged
merged 1 commit into from
May 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -73,12 +73,14 @@ default boolean wasSuccessful() {

@Deprecated // for removal and replaced with something else
@Schema(deprecated = true, hidden = true)
@JsonView(Views.V1.class)
List<LogEntry> getSourceCommits();

@Nullable
@jakarta.annotation.Nullable
@Deprecated // for removal and replaced with something else
@Schema(deprecated = true, hidden = true)
@JsonView(Views.V1.class)
List<LogEntry> getTargetCommits();

/** Details of all keys encountered during the merge or transplant operation. */
Expand All @@ -93,18 +95,23 @@ interface ContentKeyDetails {

MergeBehavior getMergeBehavior();

@Deprecated // for removal, #getConflict() is a proper replacement
@Value.Default
@JsonDeserialize(using = ContentKeyConflict.Deserializer.class)
@Schema(deprecated = true, hidden = true)
@JsonView(Views.V1.class)
default ContentKeyConflict getConflictType() {
return ContentKeyConflict.NONE;
}

@Deprecated // for removal and replaced with something else
@Schema(deprecated = true, hidden = true)
@JsonView(Views.V1.class)
List<String> getSourceCommits();

@Deprecated // for removal and replaced with something else
@Schema(deprecated = true, hidden = true)
@JsonView(Views.V1.class)
List<String> getTargetCommits();

/** {@link Conflict} details, if available. */
Expand All @@ -115,6 +122,7 @@ default ContentKeyConflict getConflictType() {
Conflict getConflict();
}

@Deprecated // for removal
enum ContentKeyConflict {
NONE,
UNRESOLVABLE;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import java.util.stream.Stream;
import javax.validation.constraints.NotNull;
import org.assertj.core.api.AbstractThrowableAssert;
import org.assertj.core.api.ListAssert;
import org.assertj.core.api.SoftAssertions;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
Expand Down Expand Up @@ -87,7 +88,8 @@
import org.projectnessie.model.ImmutableReferenceMetadata;
import org.projectnessie.model.LogResponse;
import org.projectnessie.model.LogResponse.LogEntry;
import org.projectnessie.model.MergeResponse;
import org.projectnessie.model.MergeResponse.ContentKeyConflict;
import org.projectnessie.model.MergeResponse.ContentKeyDetails;
import org.projectnessie.model.Namespace;
import org.projectnessie.model.NessieConfiguration;
import org.projectnessie.model.Operation;
Expand Down Expand Up @@ -572,26 +574,36 @@ public void mergeTransplantDryRunWithConflictInResult() throws Exception {
Branch main =
prepCommit(main0, "main", dummyPut("conflictingKey1"), dummyPut("mainKey")).commit();

soft.assertThat(
ListAssert<ContentKeyDetails> mergeAssert =
soft.assertThat(
api()
.mergeRefIntoBranch()
.fromRef(branch)
.branch(main)
.returnConflictAsResult(true) // adds coverage on top of AbstractMerge tests
.dryRun(true)
.merge()
.getDetails())
.extracting(
MergeResponse.ContentKeyDetails::getKey,
MergeResponse.ContentKeyDetails::getConflictType)
.contains(
tuple(ContentKey.of("branchKey"), MergeResponse.ContentKeyConflict.NONE),
tuple(ContentKey.of("conflictingKey1"), MergeResponse.ContentKeyConflict.UNRESOLVABLE));
.getDetails());
if (isV2()) {
mergeAssert
// old model returns "UNKNOWN" for Conflict.conflictType(), new model returns KEY_EXISTS
.extracting(ContentKeyDetails::getKey, d -> d.getConflict() != null)
.contains(
tuple(ContentKey.of("branchKey"), false),
tuple(ContentKey.of("conflictingKey1"), true));
} else {
mergeAssert
.extracting(ContentKeyDetails::getKey, ContentKeyDetails::getConflictType)
.contains(
tuple(ContentKey.of("branchKey"), ContentKeyConflict.NONE),
tuple(ContentKey.of("conflictingKey1"), ContentKeyConflict.UNRESOLVABLE));
}

// Assert no change to the ref HEAD
soft.assertThat(api().getReference().refName(main.getName()).get()).isEqualTo(main);

soft.assertThat(
mergeAssert =
soft.assertThat(
api()
.transplantCommitsIntoBranch()
.fromRefName(branch.getName())
Expand All @@ -600,14 +612,21 @@ public void mergeTransplantDryRunWithConflictInResult() throws Exception {
.returnConflictAsResult(true) // adds coverage on top of AbstractTransplant tests
.dryRun(true)
.transplant()
.getDetails())
.extracting(
MergeResponse.ContentKeyDetails::getKey,
MergeResponse.ContentKeyDetails::getConflictType)
.contains(
tuple(ContentKey.of("branchKey"), MergeResponse.ContentKeyConflict.NONE),
tuple(ContentKey.of("conflictingKey1"), MergeResponse.ContentKeyConflict.UNRESOLVABLE));

.getDetails());
if (isV2()) {
mergeAssert
// old model returns "UNKNOWN" for Conflict.conflictType(), new model returns KEY_EXISTS
.extracting(ContentKeyDetails::getKey, d -> d.getConflict() != null)
.contains(
tuple(ContentKey.of("branchKey"), false),
tuple(ContentKey.of("conflictingKey1"), true));
} else {
mergeAssert
.extracting(ContentKeyDetails::getKey, ContentKeyDetails::getConflictType)
.contains(
tuple(ContentKey.of("branchKey"), ContentKeyConflict.NONE),
tuple(ContentKey.of("conflictingKey1"), ContentKeyConflict.UNRESOLVABLE));
}
// Assert no change to the ref HEAD
soft.assertThat(api().getReference().refName(main.getName()).get()).isEqualTo(main);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2046,7 +2046,12 @@ protected boolean hasKeyCollisions(
removeKeyCollisionsForNamespaces(
ctx, refHead, commitsChronological.get(0).getHash(), keyCollisions);
if (!keyCollisions.isEmpty()) {
keyCollisions.forEach(key -> keyDetails.apply(key).conflictType(ConflictType.UNRESOLVABLE));
keyCollisions.forEach(
key ->
keyDetails
.apply(key)
.conflict(Conflict.conflict(Conflict.ConflictType.UNKNOWN, key, "UNRESOLVABLE"))
.conflictType(ConflictType.UNRESOLVABLE));
return true;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import org.assertj.core.api.InstanceOfAssertFactories;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.CsvSource;
import org.projectnessie.model.Conflict;
import org.projectnessie.model.ContentKey;
import org.projectnessie.model.MergeBehavior;
import org.projectnessie.nessie.relocated.protobuf.ByteString;
Expand Down Expand Up @@ -458,7 +459,10 @@ private MergeResult<CommitLogEntry> conflictExpectedMergeResult(
.collect(Collectors.toList()));

if (k < 2) {
details.conflictType(ConflictType.UNRESOLVABLE).addTargetCommits(conflictHead);
details
.conflict(Conflict.conflict(Conflict.ConflictType.UNKNOWN, key, "UNRESOLVABLE"))
.conflictType(ConflictType.UNRESOLVABLE)
.addTargetCommits(conflictHead);
}

expectedMergeResult.putDetails(key, details.build());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,7 @@ private static List<Conflict> asReferenceConflicts(MergeResult<?> mergeResult) {
Conflict conflict = keyDetails.getConflict();
return conflict != null
? conflict
: Conflict.conflict(
ConflictType.KEY_CONFLICT, e.getKey(), e.getValue().getConflictType().name());
: Conflict.conflict(ConflictType.KEY_CONFLICT, e.getKey(), "UNKNOWN");
})
.collect(Collectors.toList());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ interface KeyDetails {
@Value.Parameter(order = 1)
MergeBehavior getMergeBehavior();

@Deprecated // for removal, #getConflict() is a proper replacement
@Value.Default
@Value.Parameter(order = 2)
default ConflictType getConflictType() {
Expand All @@ -102,16 +103,15 @@ static ImmutableKeyDetails.Builder builder() {
return ImmutableKeyDetails.builder();
}

static KeyDetails keyDetails(MergeBehavior mergeBehavior, ConflictType conflictType) {
return keyDetails(mergeBehavior, conflictType, null);
}

static KeyDetails keyDetails(
MergeBehavior mergeBehavior, ConflictType conflictType, Conflict conflict) {
return ImmutableKeyDetails.of(mergeBehavior, conflictType, conflict);
static KeyDetails keyDetails(MergeBehavior mergeBehavior, Conflict conflict) {
return ImmutableKeyDetails.of(
mergeBehavior,
conflict != null ? ConflictType.UNRESOLVABLE : ConflictType.NONE,
conflict);
}
}

@Deprecated // for removal
enum ConflictType {
NONE,
UNRESOLVABLE
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@
import org.projectnessie.versioned.Hash;
import org.projectnessie.versioned.ImmutableMergeResult;
import org.projectnessie.versioned.MergeResult;
import org.projectnessie.versioned.MergeResult.ConflictType;
import org.projectnessie.versioned.MergeResult.KeyDetails;
import org.projectnessie.versioned.ReferenceConflictException;
import org.projectnessie.versioned.ReferenceNotFoundException;
Expand Down Expand Up @@ -530,7 +529,7 @@ ImmutableMergeResult<Commit> mergeSquashFastForward(
ContentKey key = storeKeyToKey(k);
// Note: key==null, if not the "main universe" or not a "content" discriminator
if (key != null) {
result.putDetails(key, keyDetails(mergeBehaviors.mergeBehavior(key), ConflictType.NONE));
result.putDetails(key, keyDetails(mergeBehaviors.mergeBehavior(key), null));
}
}

Expand Down Expand Up @@ -606,19 +605,15 @@ && contentTypeForPayload((byte) op.payload()) == NAMESPACE) {
// Do not plain ignore (due to FORCE) or drop (DROP), when the caller provided an
// expectedTargetContent.
if (mergeKeyBe.getExpectedTargetContent() == null) {
keyDetailsMap.put(key, keyDetails(mergeBehavior, ConflictType.NONE));
keyDetailsMap.put(key, keyDetails(mergeBehavior, null));
return mergeBehavior == MergeBehavior.FORCE
? ConflictResolution.ADD
: ConflictResolution.DROP;
}
// fall through
case NORMAL:
keyDetailsMap.put(
key,
keyDetails(
mergeBehavior,
ConflictType.UNRESOLVABLE,
commitConflictToConflict(conflict)));
key, keyDetails(mergeBehavior, commitConflictToConflict(conflict)));
return ConflictResolution.ADD;
default:
throw new IllegalStateException("Unknown merge behavior " + mergeBehavior);
Expand All @@ -633,8 +628,7 @@ && contentTypeForPayload((byte) op.payload()) == NAMESPACE) {
ContentKey key = storeKeyToKey(storeKey);
// Note: key==null, if not the "main universe" or not a "content" discriminator
if (key != null) {
keyDetailsMap.putIfAbsent(
key, keyDetails(mergeBehaviors.mergeBehavior(key), ConflictType.NONE));
keyDetailsMap.putIfAbsent(key, keyDetails(mergeBehaviors.mergeBehavior(key), null));
}
},
/*
Expand Down Expand Up @@ -709,9 +703,7 @@ boolean recordKeyDetailsAndCheckConflicts(
boolean hasConflicts = false;
for (Entry<ContentKey, KeyDetails> keyDetail : keyDetailsMap.entrySet()) {
KeyDetails details = keyDetail.getValue();
if (details.getConflictType() == ConflictType.UNRESOLVABLE) {
hasConflicts = true;
}
hasConflicts |= details.getConflict() != null;
mergeResult.putDetails(keyDetail.getKey(), details);
}
return hasConflicts;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,6 @@
import org.projectnessie.versioned.KeyEntry;
import org.projectnessie.versioned.MergeConflictException;
import org.projectnessie.versioned.MergeResult;
import org.projectnessie.versioned.MergeResult.ConflictType;
import org.projectnessie.versioned.MetadataRewriter;
import org.projectnessie.versioned.NamedRef;
import org.projectnessie.versioned.Operation;
Expand Down Expand Up @@ -687,7 +686,7 @@ private MergeResult<Commit> mergeTransplantResponse(MergeResult<Commit> mergeRes
String.format(
"The following keys have been changed in conflict: %s",
mergeResult.getDetails().entrySet().stream()
.filter(e -> e.getValue().getConflictType() != ConflictType.NONE)
.filter(e -> e.getValue().getConflict() != null)
.map(Map.Entry::getKey)
.sorted()
.map(key -> String.format("'%s'", key))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,6 @@ protected void mergeKeyBehaviorValidation(boolean dryRun) throws Exception {
false,
KeyDetails.keyDetails(
mergeBehavior,
MergeResult.ConflictType.UNRESOLVABLE,
Conflict.conflict(
ConflictType.VALUE_DIFFERS,
keyT3,
Expand Down