From 3dc398f346e9cc44ed66cbad83bc7ebfc2cace91 Mon Sep 17 00:00:00 2001 From: Alexandre Dutra Date: Mon, 31 Jul 2023 13:39:08 +0200 Subject: [PATCH 01/12] Changes to the model API --- .../api/v2/http/HttpTreeApi.java | 13 +++- .../api/v2/params/CommitLogParams.java | 12 ++- .../projectnessie/api/v2/params/Merge.java | 23 +++--- .../api/v2/params/Transplant.java | 27 +++++-- .../org/projectnessie/model/Detached.java | 15 +++- .../org/projectnessie/model/Reference.java | 9 ++- .../org/projectnessie/model/Validation.java | 55 ++++++++----- .../params/TestParamObjectsSerialization.java | 29 ++++++- .../projectnessie/model/TestValidation.java | 77 ++++++++----------- 9 files changed, 169 insertions(+), 91 deletions(-) diff --git a/api/model/src/main/java/org/projectnessie/api/v2/http/HttpTreeApi.java b/api/model/src/main/java/org/projectnessie/api/v2/http/HttpTreeApi.java index 27b3609ba2c..41e8b0051af 100644 --- a/api/model/src/main/java/org/projectnessie/api/v2/http/HttpTreeApi.java +++ b/api/model/src/main/java/org/projectnessie/api/v2/http/HttpTreeApi.java @@ -28,6 +28,7 @@ import com.fasterxml.jackson.annotation.JsonView; import java.util.List; +import javax.validation.constraints.Pattern; import javax.ws.rs.BeanParam; import javax.ws.rs.Consumes; import javax.ws.rs.DELETE; @@ -71,6 +72,7 @@ import org.projectnessie.model.Reference; import org.projectnessie.model.ReferencesResponse; import org.projectnessie.model.SingleReferenceResponse; +import org.projectnessie.model.Validation; import org.projectnessie.model.ser.Views; @Consumes(MediaType.APPLICATION_JSON) @@ -268,8 +270,8 @@ EntriesResponse getEntries( @jakarta.ws.rs.GET @Produces(MediaType.APPLICATION_JSON) @jakarta.ws.rs.Produces(jakarta.ws.rs.core.MediaType.APPLICATION_JSON) - @Path("{ref}/history") - @jakarta.ws.rs.Path("{ref}/history") + @Path("{ref:" + REF_NAME_PATH_ELEMENT_REGEX + "}/history") + @jakarta.ws.rs.Path("{ref:" + REF_NAME_PATH_ELEMENT_REGEX + "}/history") @Operation( summary = "Get commit log for a particular reference", description = @@ -603,6 +605,7 @@ ContentResponse getContent( @JsonView(Views.V2.class) GetMultipleContentsResponse getSeveralContents( @Parameter( + schema = @Schema(pattern = REF_NAME_PATH_ELEMENT_REGEX), description = REF_PARAMETER_DESCRIPTION, examples = { @ExampleObject(ref = "ref"), @@ -624,6 +627,12 @@ GetMultipleContentsResponse getSeveralContents( @ExampleObject(ref = "refDefault"), @ExampleObject(ref = "refDetached"), }) + @Pattern( + regexp = Validation.REF_NAME_PATH_REGEX, + message = Validation.REF_NAME_PATH_MESSAGE) + @jakarta.validation.constraints.Pattern( + regexp = Validation.REF_NAME_PATH_REGEX, + message = Validation.REF_NAME_PATH_MESSAGE) @PathParam("ref") @jakarta.ws.rs.PathParam("ref") String ref, diff --git a/api/model/src/main/java/org/projectnessie/api/v2/params/CommitLogParams.java b/api/model/src/main/java/org/projectnessie/api/v2/params/CommitLogParams.java index 1471054ac7c..31344a51e0c 100644 --- a/api/model/src/main/java/org/projectnessie/api/v2/params/CommitLogParams.java +++ b/api/model/src/main/java/org/projectnessie/api/v2/params/CommitLogParams.java @@ -15,6 +15,9 @@ */ package org.projectnessie.api.v2.params; +import static org.projectnessie.model.Validation.HASH_OR_RELATIVE_COMMIT_SPEC_MESSAGE; +import static org.projectnessie.model.Validation.HASH_OR_RELATIVE_COMMIT_SPEC_REGEX; + import javax.annotation.Nullable; import javax.validation.constraints.Pattern; import javax.ws.rs.QueryParam; @@ -22,7 +25,6 @@ import org.eclipse.microprofile.openapi.annotations.parameters.Parameter; import org.immutables.builder.Builder.Constructor; import org.projectnessie.model.FetchOption; -import org.projectnessie.model.Validation; /** * The purpose of this class is to include optional parameters that can be passed to {@code @@ -35,10 +37,12 @@ public class CommitLogParams extends AbstractParams { @Nullable @jakarta.annotation.Nullable - @Pattern(regexp = Validation.HASH_REGEX, message = Validation.HASH_MESSAGE) + @Pattern( + regexp = HASH_OR_RELATIVE_COMMIT_SPEC_REGEX, + message = HASH_OR_RELATIVE_COMMIT_SPEC_MESSAGE) @jakarta.validation.constraints.Pattern( - regexp = Validation.HASH_REGEX, - message = Validation.HASH_MESSAGE) + regexp = HASH_OR_RELATIVE_COMMIT_SPEC_REGEX, + message = HASH_OR_RELATIVE_COMMIT_SPEC_MESSAGE) @Parameter( description = "Hash on the given ref to identify the commit where the operation of fetching the log " diff --git a/api/model/src/main/java/org/projectnessie/api/v2/params/Merge.java b/api/model/src/main/java/org/projectnessie/api/v2/params/Merge.java index 413013cef23..62f48b19d82 100644 --- a/api/model/src/main/java/org/projectnessie/api/v2/params/Merge.java +++ b/api/model/src/main/java/org/projectnessie/api/v2/params/Merge.java @@ -21,8 +21,7 @@ import static org.projectnessie.api.v2.doc.ApiDoc.FROM_REF_NAME_DESCRIPTION; import static org.projectnessie.api.v2.doc.ApiDoc.KEY_MERGE_MODES_DESCRIPTION; import static org.projectnessie.api.v2.doc.ApiDoc.RETURN_CONFLICTS_AS_RESULT_DESCRIPTION; -import static org.projectnessie.model.Validation.validateHash; -import static org.projectnessie.model.Validation.validateNoRelativeSpec; +import static org.projectnessie.model.Validation.validateHashOrRelativeSpec; import com.fasterxml.jackson.annotation.JsonInclude; import com.fasterxml.jackson.annotation.JsonInclude.Include; @@ -89,24 +88,30 @@ public interface Merge extends BaseMergeTransplant { @JsonInclude(Include.NON_NULL) CommitMeta getCommitMeta(); + /** + * The hash of the commit from {@linkplain #getFromRefName() the source ref} to merge. + * + *

Since Nessie spec 2.1.1, the hash can be absolute or relative. + */ @NotBlank @jakarta.validation.constraints.NotBlank - @Pattern(regexp = Validation.HASH_REGEX, message = Validation.HASH_MESSAGE) + @Pattern( + regexp = Validation.HASH_OR_RELATIVE_COMMIT_SPEC_REGEX, + message = Validation.HASH_OR_RELATIVE_COMMIT_SPEC_MESSAGE) @jakarta.validation.constraints.Pattern( - regexp = Validation.HASH_REGEX, - message = Validation.HASH_MESSAGE) + regexp = Validation.HASH_OR_RELATIVE_COMMIT_SPEC_REGEX, + message = Validation.HASH_OR_RELATIVE_COMMIT_SPEC_MESSAGE) String getFromHash(); /** - * Validation rule using {@link org.projectnessie.model.Validation#validateHash(String)} - * (String)}. + * Validation rule using {@link + * org.projectnessie.model.Validation#validateHashOrRelativeSpec(String)} (String)}. */ @Value.Check default void checkHash() { String hash = getFromHash(); if (hash != null) { - validateNoRelativeSpec(hash); - validateHash(hash); + validateHashOrRelativeSpec(hash); } } } diff --git a/api/model/src/main/java/org/projectnessie/api/v2/params/Transplant.java b/api/model/src/main/java/org/projectnessie/api/v2/params/Transplant.java index ed71e3f599a..7292c5b1a1b 100644 --- a/api/model/src/main/java/org/projectnessie/api/v2/params/Transplant.java +++ b/api/model/src/main/java/org/projectnessie/api/v2/params/Transplant.java @@ -21,19 +21,20 @@ import static org.projectnessie.api.v2.doc.ApiDoc.FROM_REF_NAME_DESCRIPTION; import static org.projectnessie.api.v2.doc.ApiDoc.KEY_MERGE_MODES_DESCRIPTION; import static org.projectnessie.api.v2.doc.ApiDoc.RETURN_CONFLICTS_AS_RESULT_DESCRIPTION; -import static org.projectnessie.model.Validation.validateHash; -import static org.projectnessie.model.Validation.validateNoRelativeSpec; +import static org.projectnessie.model.Validation.validateHashOrRelativeSpec; import com.fasterxml.jackson.databind.annotation.JsonDeserialize; import com.fasterxml.jackson.databind.annotation.JsonSerialize; import java.util.List; import javax.annotation.Nullable; import javax.validation.constraints.NotNull; +import javax.validation.constraints.Pattern; import javax.validation.constraints.Size; import org.eclipse.microprofile.openapi.annotations.enums.SchemaType; import org.eclipse.microprofile.openapi.annotations.media.Schema; import org.eclipse.microprofile.openapi.annotations.media.SchemaProperty; import org.immutables.value.Value; +import org.projectnessie.model.Validation; @Schema( type = SchemaType.OBJECT, @@ -71,23 +72,35 @@ public interface Transplant extends BaseMergeTransplant { @jakarta.validation.constraints.Size(min = 1) String getMessage(); + /** + * Lists the hashes of commits that should be transplanted into the target branch. + * + *

Since Nessie spec 2.1.1, hashes can be absolute or relative. + */ @NotNull @jakarta.validation.constraints.NotNull @Size @jakarta.validation.constraints.Size(min = 1) - List getHashesToTransplant(); + List< + @Pattern( + regexp = Validation.HASH_OR_RELATIVE_COMMIT_SPEC_REGEX, + message = Validation.HASH_OR_RELATIVE_COMMIT_SPEC_MESSAGE) + @jakarta.validation.constraints.Pattern( + regexp = Validation.HASH_OR_RELATIVE_COMMIT_SPEC_REGEX, + message = Validation.HASH_OR_RELATIVE_COMMIT_SPEC_MESSAGE) + String> + getHashesToTransplant(); /** - * Validation rule using {@link org.projectnessie.model.Validation#validateHash(String)} - * (String)}. + * Validation rule using {@link + * org.projectnessie.model.Validation#validateHashOrRelativeSpec(String)} (String)}. */ @Value.Check default void checkHashes() { List hashes = getHashesToTransplant(); if (hashes != null) { for (String hash : hashes) { - validateNoRelativeSpec(hash); - validateHash(hash); + validateHashOrRelativeSpec(hash); } } } diff --git a/api/model/src/main/java/org/projectnessie/model/Detached.java b/api/model/src/main/java/org/projectnessie/model/Detached.java index d299474aa31..459fa8c7bdc 100644 --- a/api/model/src/main/java/org/projectnessie/model/Detached.java +++ b/api/model/src/main/java/org/projectnessie/model/Detached.java @@ -15,7 +15,7 @@ */ package org.projectnessie.model; -import static org.projectnessie.model.Validation.validateHash; +import static org.projectnessie.model.Validation.validateHashOrRelativeSpec; import com.fasterxml.jackson.annotation.JsonIgnore; import com.fasterxml.jackson.annotation.JsonTypeName; @@ -23,6 +23,7 @@ import com.fasterxml.jackson.databind.annotation.JsonSerialize; import javax.annotation.Nullable; import javax.validation.constraints.NotEmpty; +import javax.validation.constraints.Pattern; import org.eclipse.microprofile.openapi.annotations.enums.SchemaType; import org.eclipse.microprofile.openapi.annotations.media.Schema; import org.immutables.value.Value; @@ -52,6 +53,12 @@ default String getName() { @NotEmpty @jakarta.validation.constraints.NotEmpty @Value.Parameter(order = 1) + @Pattern( + regexp = Validation.HASH_OR_RELATIVE_COMMIT_SPEC_REGEX, + message = Validation.HASH_OR_RELATIVE_COMMIT_SPEC_MESSAGE) + @jakarta.validation.constraints.Pattern( + regexp = Validation.HASH_OR_RELATIVE_COMMIT_SPEC_REGEX, + message = Validation.HASH_OR_RELATIVE_COMMIT_SPEC_MESSAGE) String getHash(); @Nullable @@ -60,11 +67,13 @@ default String getName() { @Value.Parameter(order = 2) ReferenceMetadata getMetadata(); - /** Validation rule using {@link Validation#validateReferenceName(String)}. */ + /** Validation rule using {@link Validation#validateHashOrRelativeSpec(String)}. */ @Value.Check @Override default void checkHash() { - validateHash(getHash()); + // Note: a detached hash must have a start commit ID (absolute part); this is going to be + // checked by the service layer. + validateHashOrRelativeSpec(getHash()); } @Override diff --git a/api/model/src/main/java/org/projectnessie/model/Reference.java b/api/model/src/main/java/org/projectnessie/model/Reference.java index 11527067e22..86a35de005d 100644 --- a/api/model/src/main/java/org/projectnessie/model/Reference.java +++ b/api/model/src/main/java/org/projectnessie/model/Reference.java @@ -15,7 +15,7 @@ */ package org.projectnessie.model; -import static org.projectnessie.model.Validation.validateHash; +import static org.projectnessie.model.Validation.validateHashOrRelativeSpec; import com.fasterxml.jackson.annotation.JsonIgnore; import com.fasterxml.jackson.annotation.JsonInclude; @@ -78,12 +78,15 @@ public interface Reference extends Base { message = Validation.HASH_OR_RELATIVE_COMMIT_SPEC_MESSAGE) String getHash(); - /** Validation rule using {@link org.projectnessie.model.Validation#validateHash(String)}. */ + /** + * Validation rule using {@link + * org.projectnessie.model.Validation#validateHashOrRelativeSpec(String)}. + */ @Value.Check default void checkHash() { String hash = getHash(); if (hash != null) { - validateHash(hash); + validateHashOrRelativeSpec(hash); } } diff --git a/api/model/src/main/java/org/projectnessie/model/Validation.java b/api/model/src/main/java/org/projectnessie/model/Validation.java index ccc9c8961ff..c8383bae59d 100644 --- a/api/model/src/main/java/org/projectnessie/model/Validation.java +++ b/api/model/src/main/java/org/projectnessie/model/Validation.java @@ -130,12 +130,13 @@ public final class Validation { + "'^' + a number representing the n-th parent within a commit or " + "'*' + a number representing the created timestamp in milliseconds since epoch of a commit"; public static final String HASH_OR_RELATIVE_COMMIT_SPEC_RULE = - "consist of a valid commit hash (" + "consist of either a valid commit hash (" + HASH_RULE - + "), optionally followed by a " - + RELATIVE_COMMIT_SPEC_RULE; + + "), a valid relative part (" + + RELATIVE_COMMIT_SPEC_RULE + + "), or both"; public static final String HASH_OR_RELATIVE_COMMIT_SPEC_MESSAGE = - "Hash with optional timestamp with optional parent must " + HASH_OR_RELATIVE_COMMIT_SPEC_RULE; + "Hash with optional relative part must " + HASH_OR_RELATIVE_COMMIT_SPEC_RULE; public static final String REF_NAME_PATH_MESSAGE = "Reference name must " @@ -183,6 +184,21 @@ public static boolean isValidHash(String hash) { return matcher.matches(); } + /** + * Just checks whether a string is a valid hash and/or a relative spec, but doesn't throw an + * exception. + * + * @see #validateHashOrRelativeSpec(String) + */ + public static boolean isValidHashOrRelativeSpec(String hash) { + Objects.requireNonNull(hash, "hash must not be null"); + if (hash.isEmpty()) { + return false; + } + Matcher matcher = HASH_OR_RELATIVE_COMMIT_SPEC_PATTERN.matcher(hash); + return matcher.matches(); + } + /** * Just checks whether a string is a valid reference-name (as per {@link * #isValidReferenceName(String)}) or a valid hash (as per {@link #isValidHash(String)}). @@ -221,6 +237,22 @@ public static String validateHash(String referenceName) throws IllegalArgumentEx throw new IllegalArgumentException(HASH_MESSAGE + " - but was: " + referenceName); } + /** + * Validates whether a string is a valid hash. + * + *

The rules are: {@value #HASH_RULE} + * + * @param referenceName the reference name string to test. + */ + public static String validateHashOrRelativeSpec(String referenceName) + throws IllegalArgumentException { + if (isValidHashOrRelativeSpec(referenceName)) { + return referenceName; + } + throw new IllegalArgumentException( + HASH_OR_RELATIVE_COMMIT_SPEC_MESSAGE + " - but was: " + referenceName); + } + /** Just checks whether a string is a valid cut-off policy or not. */ private static boolean isValidDefaultCutoffPolicy(String policy) { Objects.requireNonNull(policy, "policy must not be null"); @@ -280,19 +312,4 @@ public static String validateForbiddenReferenceName(String ref) throws IllegalAr } return ref; } - - public static boolean hasRelativeSpec(String hash) { - if (hash == null) { - return false; - } - Matcher matcher = HASH_OR_RELATIVE_COMMIT_SPEC_PATTERN.matcher(hash); - return matcher.matches() && !matcher.group(2).isEmpty(); - } - - public static void validateNoRelativeSpec(String hash) throws IllegalArgumentException { - if (hasRelativeSpec(hash)) { - throw new IllegalArgumentException( - "Relative hash not allowed in commit, merge or transplant operations: " + hash); - } - } } diff --git a/api/model/src/test/java/org/projectnessie/api/v2/params/TestParamObjectsSerialization.java b/api/model/src/test/java/org/projectnessie/api/v2/params/TestParamObjectsSerialization.java index bc20682a2cc..362e03dbd30 100644 --- a/api/model/src/test/java/org/projectnessie/api/v2/params/TestParamObjectsSerialization.java +++ b/api/model/src/test/java/org/projectnessie/api/v2/params/TestParamObjectsSerialization.java @@ -148,7 +148,34 @@ static List goodCases() { objectNode() .set( "elements", - arrayNode().add("ignore").add("this"))))))); + arrayNode().add("ignore").add("this")))))), + // relative hashes + new Case(Merge.class) + .obj(ImmutableMerge.builder().fromRefName(branchName).fromHash("~1").build()) + .jsonNode(o -> o.put("fromRefName", "testBranch").put("fromHash", "~1")), + new Case(Merge.class) + .obj(ImmutableMerge.builder().fromRefName(branchName).fromHash("cafebabe~1").build()) + .jsonNode(o -> o.put("fromRefName", "testBranch").put("fromHash", "cafebabe~1")), + new Case(Transplant.class) + .obj( + ImmutableTransplant.builder() + .fromRefName(branchName) + .addHashesToTransplant("~1") + .build()) + .jsonNode( + o -> + o.put("fromRefName", "testBranch") + .set("hashesToTransplant", arrayNode().add("~1"))), + new Case(Transplant.class) + .obj( + ImmutableTransplant.builder() + .fromRefName(branchName) + .addHashesToTransplant("cafebabe~1") + .build()) + .jsonNode( + o -> + o.put("fromRefName", "testBranch") + .set("hashesToTransplant", arrayNode().add("cafebabe~1")))); } @SuppressWarnings("unused") // called by JUnit framework based on annotations in superclass diff --git a/api/model/src/test/java/org/projectnessie/model/TestValidation.java b/api/model/src/test/java/org/projectnessie/model/TestValidation.java index bd18cf4b8d1..162f9d88cc0 100644 --- a/api/model/src/test/java/org/projectnessie/model/TestValidation.java +++ b/api/model/src/test/java/org/projectnessie/model/TestValidation.java @@ -17,6 +17,7 @@ import static org.projectnessie.model.Validation.FORBIDDEN_REF_NAME_MESSAGE; import static org.projectnessie.model.Validation.HASH_MESSAGE; +import static org.projectnessie.model.Validation.HASH_OR_RELATIVE_COMMIT_SPEC_MESSAGE; import static org.projectnessie.model.Validation.HASH_OR_RELATIVE_COMMIT_SPEC_PATTERN; import static org.projectnessie.model.Validation.REF_NAME_MESSAGE; import static org.projectnessie.model.Validation.REF_NAME_PATH_PATTERN; @@ -24,6 +25,7 @@ import static org.projectnessie.model.Validation.isForbiddenReferenceName; import static org.projectnessie.model.Validation.validateForbiddenReferenceName; import static org.projectnessie.model.Validation.validateHash; +import static org.projectnessie.model.Validation.validateHashOrRelativeSpec; import static org.projectnessie.model.Validation.validateReferenceName; import static org.projectnessie.model.Validation.validateReferenceNameOrHash; @@ -111,6 +113,31 @@ void validHashes(String hash) { .hasMessageStartingWith(FORBIDDEN_REF_NAME_MESSAGE); } + @ParameterizedTest + @ValueSource( + strings = { + "1122334455667788990011223344556677889900112233445566778899001122", + "abcDEF4242424242424242424242BEEF00DEAD42112233445566778899001122", + "0011223344556677", + "11223344556677889900", + "cafebabedeadbeef", + "CAFEBABEDEADBEEF", + "caffee20", + "20caffee", + "20caff22", + "~2", + "^3", + "*1234567890", + "*2023-07-27T16:14:23.123456Z", + "1122334455667788990011223344556677889900112233445566778899001122~2", + "11223344556677889900^3", + "cafebabedeadbeef*1234567890", + "cafebabedeadbeef*2023-07-27T16:14:23.123456Z", + }) + void validHashOrRelativeSpecs(String hash) { + soft.assertThatCode(() -> validateHashOrRelativeSpec(hash)).doesNotThrowAnyException(); + } + @ParameterizedTest @ValueSource(strings = {"", "abc/", ".foo", "abc/def/../blah", "abc/de..blah", "abc/de@{blah"}) void invalidHashes(String hash) { @@ -118,12 +145,15 @@ void invalidHashes(String hash) { soft.assertThatIllegalArgumentException() .isThrownBy(() -> validateHash(hash)) .withMessage(HASH_MESSAGE + " - but was: " + hash); + soft.assertThatIllegalArgumentException() + .isThrownBy(() -> validateHashOrRelativeSpec(hash)) + .withMessage(HASH_OR_RELATIVE_COMMIT_SPEC_MESSAGE + " - but was: " + hash); soft.assertThatIllegalArgumentException() .isThrownBy(() -> Branch.of(referenceName, hash)) - .withMessage(HASH_MESSAGE + " - but was: " + hash); + .withMessage(HASH_OR_RELATIVE_COMMIT_SPEC_MESSAGE + " - but was: " + hash); soft.assertThatIllegalArgumentException() .isThrownBy(() -> Tag.of(referenceName, hash)) - .withMessage(HASH_MESSAGE + " - but was: " + hash); + .withMessage(HASH_OR_RELATIVE_COMMIT_SPEC_MESSAGE + " - but was: " + hash); } @ParameterizedTest @@ -156,10 +186,10 @@ void validNamesAndHashes(String referenceName, String hash) { void validNamesAndInvalidHashes(String referenceName, String hash) { soft.assertThatIllegalArgumentException() .isThrownBy(() -> Branch.of(referenceName, hash)) - .withMessage(HASH_MESSAGE + " - but was: " + hash); + .withMessage(HASH_OR_RELATIVE_COMMIT_SPEC_MESSAGE + " - but was: " + hash); soft.assertThatIllegalArgumentException() .isThrownBy(() -> Tag.of(referenceName, hash)) - .withMessage(HASH_MESSAGE + " - but was: " + hash); + .withMessage(HASH_OR_RELATIVE_COMMIT_SPEC_MESSAGE + " - but was: " + hash); } @ParameterizedTest @@ -251,45 +281,6 @@ void pathRefName(String rest, String ref, String hashOnRef, String relativeSpec) soft.assertThat(matcher.group(3)).isEqualTo(relativeSpec); } - @ParameterizedTest - @CsvSource({ - ",false", - "'',false", - "2e1cfa82b035c26cbbbdae632cea070514eb8b773f616aaeaf668e2f0be8f10d,false", - "~1,true", - "^2,true", - "*2021-04-07T14:42:25.534748Z,true", - "6dd38434e4520966085a2f428b6a9803358dd31997661e44a7038eb66018a5f1~1,true", - "6dd38434e4520966085a2f428b6a9803358dd31997661e44a7038eb66018a5f1^2,true", - "6dd38434e4520966085a2f428b6a9803358dd31997661e44a7038eb66018a5f1*2021-04-07T14:42:25.534748Z,true", - }) - void hasRelativeSpec(String hash, boolean expected) { - soft.assertThat(Validation.hasRelativeSpec(hash)).isEqualTo(expected); - } - - @ParameterizedTest - @CsvSource({ - ",false", - "'',false", - "2e1cfa82b035c26cbbbdae632cea070514eb8b773f616aaeaf668e2f0be8f10d,false", - "~1,true", - "^2,true", - "*2021-04-07T14:42:25.534748Z,true", - "6dd38434e4520966085a2f428b6a9803358dd31997661e44a7038eb66018a5f1~1,true", - "6dd38434e4520966085a2f428b6a9803358dd31997661e44a7038eb66018a5f1^2,true", - "6dd38434e4520966085a2f428b6a9803358dd31997661e44a7038eb66018a5f1*2021-04-07T14:42:25.534748Z,true", - }) - void validateNoRelativeSpec(String hash, boolean errorExpected) { - if (errorExpected) { - soft.assertThatIllegalArgumentException() - .isThrownBy(() -> Validation.validateNoRelativeSpec(hash)) - .withMessageContaining( - "Relative hash not allowed in commit, merge or transplant operations"); - } else { - soft.assertThatCode(() -> Validation.validateNoRelativeSpec(hash)).doesNotThrowAnyException(); - } - } - @ParameterizedTest @ValueSource( strings = { From 8746ea7ad4e46f1b7e9d1dd81591ae40d447e000 Mon Sep 17 00:00:00 2001 From: Alexandre Dutra Date: Mon, 31 Jul 2023 13:40:57 +0200 Subject: [PATCH 02/12] Changes to Service layer --- .../services/rest/RestV2TreeResource.java | 3 + .../services/hash/HashResolver.java | 161 +++++++++++ .../services/hash/HashValidators.java | 51 ++++ .../services/hash/ParsedHash.java | 80 ++++++ .../services/hash/ResolvedHash.java | 83 ++++++ .../services/impl/BaseApiImpl.java | 89 +------ .../services/impl/ContentApiImpl.java | 9 +- .../services/impl/DiffApiImpl.java | 11 +- .../services/impl/NamespaceApiImpl.java | 17 +- .../services/impl/TreeApiImpl.java | 252 ++++++++++-------- .../services/spi/TreeService.java | 36 ++- .../services/hash/TestParsedHash.java | 72 +++++ 12 files changed, 652 insertions(+), 212 deletions(-) create mode 100644 servers/services/src/main/java/org/projectnessie/services/hash/HashResolver.java create mode 100644 servers/services/src/main/java/org/projectnessie/services/hash/HashValidators.java create mode 100644 servers/services/src/main/java/org/projectnessie/services/hash/ParsedHash.java create mode 100644 servers/services/src/main/java/org/projectnessie/services/hash/ResolvedHash.java create mode 100644 servers/services/src/test/java/org/projectnessie/services/hash/TestParsedHash.java diff --git a/servers/rest-services/src/main/java/org/projectnessie/services/rest/RestV2TreeResource.java b/servers/rest-services/src/main/java/org/projectnessie/services/rest/RestV2TreeResource.java index 271bbfc4afb..475efd1e5f0 100644 --- a/servers/rest-services/src/main/java/org/projectnessie/services/rest/RestV2TreeResource.java +++ b/servers/rest-services/src/main/java/org/projectnessie/services/rest/RestV2TreeResource.java @@ -169,6 +169,9 @@ public SingleReferenceResponse createReference(String name, String type, Referen public SingleReferenceResponse getReferenceByName(GetReferenceParams params) throws NessieNotFoundException { ParsedReference reference = parseRefPathString(params.getRef()); + checkArgument( + reference.hashWithRelativeSpec() == null, + "Hashes are not allowed when fetching a reference by name"); return SingleReferenceResponse.builder() .reference(tree().getReferenceByName(reference.name(), params.fetchOption())) .build(); diff --git a/servers/services/src/main/java/org/projectnessie/services/hash/HashResolver.java b/servers/services/src/main/java/org/projectnessie/services/hash/HashResolver.java new file mode 100644 index 00000000000..93906ba73e5 --- /dev/null +++ b/servers/services/src/main/java/org/projectnessie/services/hash/HashResolver.java @@ -0,0 +1,161 @@ +/* + * Copyright (C) 2023 Dremio + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.projectnessie.services.hash; + +import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Preconditions.checkState; +import static org.projectnessie.services.hash.HashValidators.ANY_HASH; +import static org.projectnessie.services.hash.HashValidators.REQUIRED_UNAMBIGUOUS_HASH; + +import java.util.Collections; +import java.util.List; +import java.util.Objects; +import java.util.Optional; +import java.util.function.BiConsumer; +import javax.annotation.Nullable; +import org.projectnessie.error.NessieReferenceNotFoundException; +import org.projectnessie.model.CommitMeta; +import org.projectnessie.services.config.ServerConfig; +import org.projectnessie.versioned.DetachedRef; +import org.projectnessie.versioned.GetNamedRefsParams; +import org.projectnessie.versioned.Hash; +import org.projectnessie.versioned.NamedRef; +import org.projectnessie.versioned.ReferenceInfo; +import org.projectnessie.versioned.ReferenceNotFoundException; +import org.projectnessie.versioned.RelativeCommitSpec; +import org.projectnessie.versioned.VersionStore; + +public class HashResolver { + + private final ServerConfig config; + private final VersionStore store; + + public HashResolver(ServerConfig config, VersionStore store) { + this.config = config; + this.store = store; + } + + /** + * Resolves the given {@code namedRef} to its current HEAD. Throws if the reference does not + * exist, or if it's {@link DetachedRef}. + * + *

If {@code namedRef} is null, the default branch will be used. + */ + public ResolvedHash resolveToHead(@Nullable @jakarta.annotation.Nullable String namedRef) + throws NessieReferenceNotFoundException { + checkArgument( + namedRef == null || !namedRef.equals(DetachedRef.REF_NAME), + "Cannot resolve DETACHED to HEAD"); + return resolveHashOnRef(namedRef, null, "", ANY_HASH); + } + + /** + * Resolves the given {@code namedRef} to {@code hashOnRef} if present, otherwise to its current + * HEAD, if available. + * + *

Throws if the reference does not exist, the hash is invalid, or is not present on the + * reference. + * + *

If {@code namedRef} is {@link DetachedRef}, then a non-null {@code hashOnRef} is required. + * + *

If {@code namedRef} is null, the default branch will be used. + * + *

The parameter {name} is used for error messages. + * + *

A hash validator must be provided to perform extra validations on the parsed hashed. If no + * extra validation is required, {@link HashValidators#ANY_HASH} can be used. + */ + public ResolvedHash resolveHashOnRef( + @Nullable @jakarta.annotation.Nullable String namedRef, + @Nullable @jakarta.annotation.Nullable String hashOnRef, + String name, + BiConsumer validator) + throws NessieReferenceNotFoundException { + if (null == namedRef) { + namedRef = config.getDefaultBranch(); + } + try { + NamedRef ref; + Hash currentHead = null; + if (DetachedRef.REF_NAME.equals(namedRef)) { + ref = DetachedRef.INSTANCE; + } else { + ReferenceInfo refInfo = store.getNamedRef(namedRef, GetNamedRefsParams.DEFAULT); + ref = refInfo.getNamedRef(); + currentHead = refInfo.getHash(); + // Shortcut: use HEAD, if hashOnRef is null or empty + if (hashOnRef == null || hashOnRef.isEmpty()) { + return ResolvedHash.of(ref, Optional.empty(), Optional.of(currentHead)); + } + } + return resolveHashOnRef(ref, currentHead, hashOnRef, name, validator); + } catch (ReferenceNotFoundException e) { + throw new NessieReferenceNotFoundException(e.getMessage(), e); + } + } + + /** + * Resolves the given {@code hashOnRef} against another previously computed {@link ResolvedHash} + * pointing to a branch at its HEAD. + * + *

This is useful to compute more hashes against a first resolved hash, e.g. when + * transplanting. + * + *

See {@link #resolveHashOnRef(String, String, String, BiConsumer)} for important caveats. + */ + public ResolvedHash resolveHashOnRef( + ResolvedHash head, + @Nullable @jakarta.annotation.Nullable String hashOnRef, + String name, + BiConsumer validator) + throws ReferenceNotFoundException { + return resolveHashOnRef( + head.getNamedRef(), head.getHead().orElse(null), hashOnRef, name, validator); + } + + /** + * Resolves the given {@code namedRef} to {@code hashOnRef} if present, otherwise to {@code + * currentHead}, if available. + * + *

Either {@code currentHead} or {@code hashOnRef} must be non-null. It's the caller's + * responsibility to validate that any user-provided input meets this requirement. + * + *

See {@link #resolveHashOnRef(String, String, String, BiConsumer)} for important caveats. + */ + public ResolvedHash resolveHashOnRef( + NamedRef ref, + @Nullable @jakarta.annotation.Nullable Hash currentHead, + @Nullable @jakarta.annotation.Nullable String hashOnRef, + String name, + BiConsumer validator) + throws ReferenceNotFoundException { + checkState(currentHead != null || hashOnRef != null); + Optional parsed = ParsedHash.parse(hashOnRef, store.noAncestorHash()); + if (ref == DetachedRef.INSTANCE) { + validator = validator.andThen(REQUIRED_UNAMBIGUOUS_HASH); + } + validator.accept(name, parsed.orElse(null)); + Hash startOrHead = parsed.flatMap(ParsedHash::getAbsolutePart).orElse(currentHead); + checkState(startOrHead != null); + List relativeParts = + parsed.map(ParsedHash::getRelativeParts).orElse(Collections.emptyList()); + Optional resolved = Optional.of(startOrHead); + if (!Objects.equals(startOrHead, currentHead) || !relativeParts.isEmpty()) { + resolved = Optional.ofNullable(store.hashOnReference(ref, resolved, relativeParts)); + } + return ResolvedHash.of(ref, resolved, Optional.ofNullable(currentHead)); + } +} diff --git a/servers/services/src/main/java/org/projectnessie/services/hash/HashValidators.java b/servers/services/src/main/java/org/projectnessie/services/hash/HashValidators.java new file mode 100644 index 00000000000..986e3279e2c --- /dev/null +++ b/servers/services/src/main/java/org/projectnessie/services/hash/HashValidators.java @@ -0,0 +1,51 @@ +/* + * Copyright (C) 2023 Dremio + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.projectnessie.services.hash; + +import static com.google.common.base.Preconditions.checkArgument; + +import java.util.Objects; +import java.util.function.BiConsumer; + +public final class HashValidators { + + private HashValidators() {} + + /** Validates any parseable hash. */ + public static final BiConsumer ANY_HASH = (name, parsed) -> {}; + + /** Validates that a hash has been provided (absolute or relative). */ + public static final BiConsumer REQUIRED_HASH = + (name, parsed) -> Objects.requireNonNull(parsed, String.format("%s must be provided.", name)); + + /** + * Validates that, if a hash was provided, it is unambiguous. A hash is unambiguous if it has an + * absolute part, because it will always resolve to the same hash, even if it also has relative + * parts. + */ + public static final BiConsumer UNAMBIGUOUS_HASH = + (name, parsed) -> + checkArgument( + parsed == null || parsed.getAbsolutePart().isPresent(), + String.format("%s must contain a starting commit ID.", name)); + + /** + * Validates that a hash has been provided, and that it is unambiguous (that is, it contains an + * absolute part). + */ + public static final BiConsumer REQUIRED_UNAMBIGUOUS_HASH = + REQUIRED_HASH.andThen(UNAMBIGUOUS_HASH); +} diff --git a/servers/services/src/main/java/org/projectnessie/services/hash/ParsedHash.java b/servers/services/src/main/java/org/projectnessie/services/hash/ParsedHash.java new file mode 100644 index 00000000000..018e40e8303 --- /dev/null +++ b/servers/services/src/main/java/org/projectnessie/services/hash/ParsedHash.java @@ -0,0 +1,80 @@ +/* + * Copyright (C) 2023 Dremio + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.projectnessie.services.hash; + +import static com.google.common.base.Preconditions.checkArgument; +import static java.util.Objects.requireNonNull; +import static org.projectnessie.model.Validation.HASH_OR_RELATIVE_COMMIT_SPEC_MESSAGE; +import static org.projectnessie.model.Validation.HASH_OR_RELATIVE_COMMIT_SPEC_PATTERN; +import static org.projectnessie.versioned.RelativeCommitSpec.parseRelativeSpecs; + +import java.util.List; +import java.util.Optional; +import java.util.regex.Matcher; +import javax.annotation.Nonnull; +import javax.annotation.Nullable; +import org.immutables.value.Value; +import org.projectnessie.versioned.Hash; +import org.projectnessie.versioned.RelativeCommitSpec; + +/** + * A parsed hash, which can contain either an absolute hash, a relative commit spec, or both. + * + *

It is package-private, because it is only used by {@link HashResolver}. + */ +@Value.Immutable +interface ParsedHash { + + Optional getAbsolutePart(); + + List getRelativeParts(); + + static ParsedHash of(Hash absolutePart) { + return ImmutableParsedHash.builder().absolutePart(absolutePart).build(); + } + + static ParsedHash of(RelativeCommitSpec... relativeParts) { + return ImmutableParsedHash.builder().addRelativeParts(relativeParts).build(); + } + + static ParsedHash of(Hash absolutePart, RelativeCommitSpec... relativeParts) { + return ImmutableParsedHash.builder() + .absolutePart(absolutePart) + .addRelativeParts(relativeParts) + .build(); + } + + static Optional parse( + @Nullable @jakarta.annotation.Nullable String hashOrRelativeSpec, + @Nonnull @jakarta.annotation.Nonnull Hash noAncestorHash) { + requireNonNull(noAncestorHash, "noAncestorHash"); + if (hashOrRelativeSpec == null || hashOrRelativeSpec.isEmpty()) { + return Optional.empty(); + } + Matcher matcher = HASH_OR_RELATIVE_COMMIT_SPEC_PATTERN.matcher(hashOrRelativeSpec); + checkArgument(matcher.matches(), HASH_OR_RELATIVE_COMMIT_SPEC_MESSAGE, hashOrRelativeSpec); + Optional absolutePart = + Optional.ofNullable(matcher.group(1)) + .map(Hash::of) + .map(h -> h.equals(noAncestorHash) ? noAncestorHash : h); + List relativePart = parseRelativeSpecs(matcher.group(2)); + return Optional.of( + ImmutableParsedHash.builder() + .absolutePart(absolutePart) + .relativeParts(relativePart) + .build()); + } +} diff --git a/servers/services/src/main/java/org/projectnessie/services/hash/ResolvedHash.java b/servers/services/src/main/java/org/projectnessie/services/hash/ResolvedHash.java new file mode 100644 index 00000000000..e2c8e3c1c6f --- /dev/null +++ b/servers/services/src/main/java/org/projectnessie/services/hash/ResolvedHash.java @@ -0,0 +1,83 @@ +/* + * Copyright (C) 2023 Dremio + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.projectnessie.services.hash; + +import static com.google.common.base.Preconditions.checkState; + +import java.util.Optional; +import org.immutables.value.Value; +import org.projectnessie.versioned.DetachedRef; +import org.projectnessie.versioned.Hash; +import org.projectnessie.versioned.NamedRef; +import org.projectnessie.versioned.WithHash; + +/** + * A {@link Hash} that has been resolved against a {@link NamedRef}. + * + *

It extends {@link WithHash} to provide compatibility with legacy code. + */ +@Value.Immutable +@SuppressWarnings("immutables:subtype") +public interface ResolvedHash extends WithHash { + + /** The {@link NamedRef}; can be {@link DetachedRef}. */ + NamedRef getNamedRef(); + + /** + * The user-provided hash. Only present if a hash was actually provided and successfully parsed; + * will be empty if the hash was implicitly resolved to the ref's HEAD. + * + *

If this is empty, then {@link #getHead()} is guaranteed to be non-empty. + * + *

If the provided hash had relative specs, this hash will be the result of resolving those + * specs against its absolute part, or the ref's HEAD if it doesn't have an absolute part. + */ + Optional getProvidedHash(); + + /** + * The ref's HEAD, if available. Will always be empty for {@link DetachedRef}. Exposed mostly to + * avoid re-fetching the HEAD many times. + * + *

If this is empty, then {@link #getProvidedHash()} is guaranteed to be non-empty. + */ + Optional getHead(); + + /** + * The effective resolved hash; never null. Will either be {@link #getProvidedHash()} if present, + * or otherwise, {@link #getHead()}. + */ + @Override + Hash getHash(); + + @Value.Derived + @Override + default NamedRef getValue() { + return getNamedRef(); + } + + static ResolvedHash of(NamedRef ref, Optional providedHash, Optional head) { + checkState( + providedHash.isPresent() || head.isPresent(), + "Either providedHash or head must be present"); + Hash effective = providedHash.orElseGet(head::get); + return ImmutableResolvedHash.builder() + .namedRef(ref) + .providedHash(providedHash) + .head(head) + .hash(effective) + .build(); + } +} diff --git a/servers/services/src/main/java/org/projectnessie/services/impl/BaseApiImpl.java b/servers/services/src/main/java/org/projectnessie/services/impl/BaseApiImpl.java index 6aacae2f105..b283f38f2cb 100644 --- a/servers/services/src/main/java/org/projectnessie/services/impl/BaseApiImpl.java +++ b/servers/services/src/main/java/org/projectnessie/services/impl/BaseApiImpl.java @@ -15,56 +15,41 @@ */ package org.projectnessie.services.impl; -import static com.google.common.base.Preconditions.checkArgument; -import static org.projectnessie.model.Validation.HASH_OR_RELATIVE_COMMIT_SPEC_MESSAGE; -import static org.projectnessie.model.Validation.HASH_OR_RELATIVE_COMMIT_SPEC_PATTERN; import static org.projectnessie.services.cel.CELUtil.CONTAINER; import static org.projectnessie.services.cel.CELUtil.CONTENT_KEY_DECLARATIONS; import static org.projectnessie.services.cel.CELUtil.CONTENT_KEY_TYPES; import static org.projectnessie.services.cel.CELUtil.SCRIPT_HOST; import static org.projectnessie.services.cel.CELUtil.VAR_KEY; import static org.projectnessie.services.cel.CELUtil.forCel; -import static org.projectnessie.versioned.RelativeCommitSpec.parseRelativeSpecs; import com.google.common.base.Strings; import com.google.common.collect.ImmutableMap; import java.security.Principal; import java.time.Instant; -import java.util.List; -import java.util.Objects; -import java.util.Optional; import java.util.UUID; import java.util.function.IntFunction; import java.util.function.Predicate; import java.util.function.Supplier; -import java.util.regex.Matcher; import javax.annotation.Nullable; import org.projectnessie.cel.tools.Script; import org.projectnessie.cel.tools.ScriptException; -import org.projectnessie.error.NessieReferenceNotFoundException; import org.projectnessie.model.CommitMeta; import org.projectnessie.model.ContentKey; import org.projectnessie.services.authz.Authorizer; import org.projectnessie.services.authz.BatchAccessChecker; import org.projectnessie.services.authz.ServerAccessContext; import org.projectnessie.services.config.ServerConfig; +import org.projectnessie.services.hash.HashResolver; import org.projectnessie.versioned.DefaultMetadataRewriter; -import org.projectnessie.versioned.DetachedRef; -import org.projectnessie.versioned.GetNamedRefsParams; -import org.projectnessie.versioned.Hash; import org.projectnessie.versioned.MetadataRewriter; -import org.projectnessie.versioned.NamedRef; -import org.projectnessie.versioned.ReferenceInfo; -import org.projectnessie.versioned.ReferenceNotFoundException; -import org.projectnessie.versioned.RelativeCommitSpec; import org.projectnessie.versioned.VersionStore; -import org.projectnessie.versioned.WithHash; public abstract class BaseApiImpl { private final ServerConfig config; private final VersionStore store; private final Authorizer authorizer; private final Supplier principal; + private final HashResolver hashResolver; protected BaseApiImpl( ServerConfig config, @@ -75,6 +60,7 @@ protected BaseApiImpl( this.store = store; this.authorizer = authorizer; this.principal = principal; + this.hashResolver = new HashResolver(config, store); } /** @@ -108,71 +94,6 @@ static Predicate filterOnContentKey(String filter) { }; } - WithHash namedRefWithHashOrThrow( - @Nullable @jakarta.annotation.Nullable String namedRef, - @Nullable @jakarta.annotation.Nullable String hashOnRef) - throws NessieReferenceNotFoundException { - if (null == namedRef) { - namedRef = config.getDefaultBranch(); - } - - NamedRef ref; - Hash hash = null; - Hash knownValid = null; - if (DetachedRef.REF_NAME.equals(namedRef)) { - ref = DetachedRef.INSTANCE; - Objects.requireNonNull( - hashOnRef, String.format("hashOnRef must not be null for '%s'", DetachedRef.REF_NAME)); - } else { - try { - ReferenceInfo refInfo = - getStore().getNamedRef(namedRef, GetNamedRefsParams.DEFAULT); - ref = refInfo.getNamedRef(); - knownValid = hash = refInfo.getHash(); - - // Shortcut: use HEAD, if hashOnRef is null or empty - if (hashOnRef == null || hashOnRef.isEmpty()) { - return WithHash.of(refInfo.getHash(), ref); - } - } catch (ReferenceNotFoundException e) { - throw new NessieReferenceNotFoundException(e.getMessage(), e); - } - } - - try { - Matcher hashAndRelatives = HASH_OR_RELATIVE_COMMIT_SPEC_PATTERN.matcher(hashOnRef); - checkArgument(hashAndRelatives.find(), HASH_OR_RELATIVE_COMMIT_SPEC_MESSAGE); - - String s = hashAndRelatives.group(1); - if (s != null) { - hash = Hash.of(s); - } - - Objects.requireNonNull( - hash, // can only be null for DETACHED w/ hashOnRef w/o a starting commit ID - String.format( - "hashOnRef must contain a valid starting commit ID for '%s'", DetachedRef.REF_NAME)); - - List relativeSpecs = parseRelativeSpecs(hashAndRelatives.group(2)); - - if (store.noAncestorHash().equals(hash)) { - // hashOnRef might point to "no ancestor hash", but the actual HEAD of the reference is - // not necessarily the same, so construct a new instance to return. - return WithHash.of(store.noAncestorHash(), ref); - } - - // If the hash provided by the caller is not the current reference HEAD, verify that the hash - // is a valid parent. - if (!hash.equals(knownValid) || !relativeSpecs.isEmpty()) { - hash = getStore().hashOnReference(ref, Optional.of(hash), relativeSpecs); - } - - return WithHash.of(hash, ref); - } catch (ReferenceNotFoundException e) { - throw new NessieReferenceNotFoundException(e.getMessage(), e); - } - } - protected ServerConfig getServerConfig() { return config; } @@ -189,6 +110,10 @@ protected Authorizer getAuthorizer() { return authorizer; } + protected HashResolver getHashResolver() { + return hashResolver; + } + protected BatchAccessChecker startAccessCheck() { return getAuthorizer().startAccessCheck(createAccessContext()); } diff --git a/servers/services/src/main/java/org/projectnessie/services/impl/ContentApiImpl.java b/servers/services/src/main/java/org/projectnessie/services/impl/ContentApiImpl.java index ca2fe8967cd..b8bbfaeb273 100644 --- a/servers/services/src/main/java/org/projectnessie/services/impl/ContentApiImpl.java +++ b/servers/services/src/main/java/org/projectnessie/services/impl/ContentApiImpl.java @@ -15,6 +15,8 @@ */ package org.projectnessie.services.impl; +import static org.projectnessie.services.hash.HashValidators.ANY_HASH; + import java.security.Principal; import java.util.List; import java.util.Map; @@ -34,6 +36,7 @@ import org.projectnessie.services.authz.Authorizer; import org.projectnessie.services.authz.BatchAccessChecker; import org.projectnessie.services.config.ServerConfig; +import org.projectnessie.services.hash.ResolvedHash; import org.projectnessie.services.spi.ContentService; import org.projectnessie.versioned.BranchName; import org.projectnessie.versioned.ContentResult; @@ -58,7 +61,8 @@ public ContentApiImpl( public ContentResponse getContent( ContentKey key, String namedRef, String hashOnRef, boolean withDocumentation) throws NessieNotFoundException { - WithHash ref = namedRefWithHashOrThrow(namedRef, hashOnRef); + ResolvedHash ref = + getHashResolver().resolveHashOnRef(namedRef, hashOnRef, "Expected hash", ANY_HASH); try { ContentResult obj = getStore().getValue(ref.getHash(), key); BatchAccessChecker accessCheck = startAccessCheck(); @@ -78,7 +82,8 @@ public GetMultipleContentsResponse getMultipleContents( String namedRef, String hashOnRef, List keys, boolean withDocumentation) throws NessieNotFoundException { try { - WithHash ref = namedRefWithHashOrThrow(namedRef, hashOnRef); + ResolvedHash ref = + getHashResolver().resolveHashOnRef(namedRef, hashOnRef, "Expected hash", ANY_HASH); BatchAccessChecker check = startAccessCheck().canViewReference(ref.getValue()); diff --git a/servers/services/src/main/java/org/projectnessie/services/impl/DiffApiImpl.java b/servers/services/src/main/java/org/projectnessie/services/impl/DiffApiImpl.java index f5ced8b1f3d..a97ca609911 100644 --- a/servers/services/src/main/java/org/projectnessie/services/impl/DiffApiImpl.java +++ b/servers/services/src/main/java/org/projectnessie/services/impl/DiffApiImpl.java @@ -18,6 +18,7 @@ import static java.util.Collections.singleton; import static org.projectnessie.services.authz.Check.canReadContentKey; import static org.projectnessie.services.authz.Check.canViewReference; +import static org.projectnessie.services.hash.HashValidators.ANY_HASH; import com.google.common.base.Strings; import com.google.common.collect.ImmutableSet; @@ -37,6 +38,7 @@ import org.projectnessie.services.authz.AuthzPaginationIterator; import org.projectnessie.services.authz.Check; import org.projectnessie.services.config.ServerConfig; +import org.projectnessie.services.hash.ResolvedHash; import org.projectnessie.services.spi.DiffService; import org.projectnessie.services.spi.PagedResponseHandler; import org.projectnessie.versioned.Diff; @@ -73,10 +75,11 @@ public R getDiff( List requestedKeys, String filter) throws NessieNotFoundException { - WithHash from = namedRefWithHashOrThrow(fromRef, fromHash); - WithHash to = namedRefWithHashOrThrow(toRef, toHash); - NamedRef fromNamedRef = from.getValue(); - NamedRef toNamedRef = to.getValue(); + ResolvedHash from = + getHashResolver().resolveHashOnRef(fromRef, fromHash, "From hash", ANY_HASH); + ResolvedHash to = getHashResolver().resolveHashOnRef(toRef, toHash, "To hash", ANY_HASH); + NamedRef fromNamedRef = from.getNamedRef(); + NamedRef toNamedRef = to.getNamedRef(); fromReference.accept(from); toReference.accept(to); diff --git a/servers/services/src/main/java/org/projectnessie/services/impl/NamespaceApiImpl.java b/servers/services/src/main/java/org/projectnessie/services/impl/NamespaceApiImpl.java index 2a2e671ca51..cb84ec4558b 100644 --- a/servers/services/src/main/java/org/projectnessie/services/impl/NamespaceApiImpl.java +++ b/servers/services/src/main/java/org/projectnessie/services/impl/NamespaceApiImpl.java @@ -19,6 +19,7 @@ import static java.util.stream.StreamSupport.stream; import static org.projectnessie.error.ContentKeyErrorDetails.contentKeyErrorDetails; import static org.projectnessie.model.Validation.validateHash; +import static org.projectnessie.services.hash.HashValidators.ANY_HASH; import static org.projectnessie.services.impl.RefUtil.toReference; import static org.projectnessie.versioned.VersionStore.KeyRestrictions.NO_KEY_RESTRICTIONS; @@ -54,17 +55,16 @@ import org.projectnessie.services.authz.Authorizer; import org.projectnessie.services.authz.BatchAccessChecker; import org.projectnessie.services.config.ServerConfig; +import org.projectnessie.services.hash.ResolvedHash; import org.projectnessie.services.spi.NamespaceService; import org.projectnessie.versioned.BranchName; import org.projectnessie.versioned.ContentResult; import org.projectnessie.versioned.Hash; import org.projectnessie.versioned.KeyEntry; -import org.projectnessie.versioned.NamedRef; import org.projectnessie.versioned.Operation; import org.projectnessie.versioned.ReferenceConflictException; import org.projectnessie.versioned.ReferenceNotFoundException; import org.projectnessie.versioned.VersionStore; -import org.projectnessie.versioned.WithHash; import org.projectnessie.versioned.paging.PaginationIterator; public class NamespaceApiImpl extends BaseApiImpl implements NamespaceService { @@ -82,7 +82,7 @@ public Namespace createNamespace(String refName, Namespace namespace) throws NessieReferenceNotFoundException { Preconditions.checkArgument(!namespace.isEmpty(), "Namespace name must not be empty"); - WithHash refWithHash = namedRefWithHashOrThrow(refName, null); + ResolvedHash refWithHash = getHashResolver().resolveToHead(refName); try { try { @@ -128,7 +128,7 @@ public Namespace createNamespace(String refName, Namespace namespace) @Override public void deleteNamespace(String refName, Namespace namespaceToDelete) throws NessieReferenceNotFoundException, NessieNamespaceNotFoundException { - WithHash refWithHash = namedRefWithHashOrThrow(refName, null); + ResolvedHash refWithHash = getHashResolver().resolveToHead(refName); try { Namespace namespace = getNamespace(namespaceToDelete, refWithHash.getHash()); Delete delete = Delete.of(namespace.toContentKey()); @@ -163,7 +163,9 @@ public Namespace getNamespace(String refName, String hashOnRef, Namespace namesp if (hashOnRef != null) { validateHash(hashOnRef); } - return getNamespace(namespace, namedRefWithHashOrThrow(refName, hashOnRef).getHash()); + ResolvedHash resolved = + getHashResolver().resolveHashOnRef(refName, hashOnRef, "Hash", ANY_HASH); + return getNamespace(namespace, resolved.getHash()); } catch (ReferenceNotFoundException e) { throw refNotFoundException(e); } @@ -201,7 +203,8 @@ public GetNamespacesResponse getNamespaces(String refName, String hashOnRef, Nam if (hashOnRef != null) { validateHash(hashOnRef); } - WithHash refWithHash = namedRefWithHashOrThrow(refName, hashOnRef); + ResolvedHash refWithHash = + getHashResolver().resolveHashOnRef(refName, hashOnRef, "Hash", ANY_HASH); try { // Note: `Namespace` objects are supposed to get more attributes (e.g. a properties map) // which will make it impossible to use the `Namespace` object itself as an identifier to @@ -262,7 +265,7 @@ public void updateProperties( Set propertyRemovals) throws NessieNamespaceNotFoundException, NessieReferenceNotFoundException { try { - WithHash refWithHash = namedRefWithHashOrThrow(refName, null); + ResolvedHash refWithHash = getHashResolver().resolveToHead(refName); Namespace namespace = getNamespace(namespaceToUpdate, refWithHash.getHash()); Map properties = new HashMap<>(namespace.getProperties()); diff --git a/servers/services/src/main/java/org/projectnessie/services/impl/TreeApiImpl.java b/servers/services/src/main/java/org/projectnessie/services/impl/TreeApiImpl.java index c72b9a5e6a9..e5186f9c89a 100644 --- a/servers/services/src/main/java/org/projectnessie/services/impl/TreeApiImpl.java +++ b/servers/services/src/main/java/org/projectnessie/services/impl/TreeApiImpl.java @@ -23,8 +23,6 @@ import static java.util.Collections.singleton; import static java.util.function.Function.identity; import static org.projectnessie.model.CommitResponse.AddedContent.addedContent; -import static org.projectnessie.model.Validation.validateHash; -import static org.projectnessie.model.Validation.validateNoRelativeSpec; import static org.projectnessie.services.authz.Check.canReadContentKey; import static org.projectnessie.services.authz.Check.canReadEntries; import static org.projectnessie.services.authz.Check.canViewReference; @@ -42,12 +40,16 @@ import static org.projectnessie.services.cel.CELUtil.VAR_REF; import static org.projectnessie.services.cel.CELUtil.VAR_REF_META; import static org.projectnessie.services.cel.CELUtil.VAR_REF_TYPE; +import static org.projectnessie.services.hash.HashValidators.ANY_HASH; +import static org.projectnessie.services.hash.HashValidators.REQUIRED_UNAMBIGUOUS_HASH; +import static org.projectnessie.services.hash.HashValidators.UNAMBIGUOUS_HASH; import static org.projectnessie.services.impl.RefUtil.toNamedRef; import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import java.security.Principal; +import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.HashMap; @@ -63,7 +65,6 @@ import java.util.function.Predicate; import java.util.function.Supplier; import java.util.stream.Collectors; -import java.util.stream.Stream; import javax.annotation.Nullable; import org.projectnessie.cel.tools.Script; import org.projectnessie.cel.tools.ScriptException; @@ -77,7 +78,6 @@ import org.projectnessie.model.CommitResponse; import org.projectnessie.model.Content; import org.projectnessie.model.ContentKey; -import org.projectnessie.model.Detached; import org.projectnessie.model.EntriesResponse.Entry; import org.projectnessie.model.FetchOption; import org.projectnessie.model.IdentifiedContentKey; @@ -105,6 +105,7 @@ import org.projectnessie.services.authz.Check; import org.projectnessie.services.cel.CELUtil; import org.projectnessie.services.config.ServerConfig; +import org.projectnessie.services.hash.ResolvedHash; import org.projectnessie.services.spi.PagedResponseHandler; import org.projectnessie.services.spi.TreeService; import org.projectnessie.versioned.BranchName; @@ -264,10 +265,14 @@ public Reference createReference( "Tag-creation requires a target named-reference and hash."); } - BatchAccessChecker check = - startAccessCheck().canCreateReference(RefUtil.toNamedRef(type, refName)); + BatchAccessChecker check = startAccessCheck().canCreateReference(namedReference); + Optional targetHashObj = Optional.empty(); try { - check.canViewReference(namedRefWithHashOrThrow(sourceRefName, targetHash).getValue()); + ResolvedHash targetRef = + getHashResolver() + .resolveHashOnRef(sourceRefName, targetHash, "Target hash", UNAMBIGUOUS_HASH); + check.canViewReference(targetRef.getNamedRef()); + targetHashObj = targetRef.getProvidedHash(); } catch (NessieNotFoundException e) { // If the default-branch does not exist and hashOnRef points to the "beginning of time", // then do not throw a NessieNotFoundException, but re-create the default branch. In all @@ -281,7 +286,7 @@ public Reference createReference( check.checkAndThrow(); try { - Hash hash = getStore().create(namedReference, toHash(targetHash, false)).getHash(); + Hash hash = getStore().create(namedReference, targetHashObj).getHash(); return RefUtil.toReference(namedReference, hash); } catch (ReferenceNotFoundException e) { throw new NessieReferenceNotFoundException(e.getMessage(), e); @@ -302,25 +307,29 @@ public Reference assignReference( ReferenceType referenceType, String referenceName, String expectedHash, Reference assignTo) throws NessieNotFoundException, NessieConflictException { try { - ReferenceInfo resolved = - getStore().getNamedRef(referenceName, GetNamedRefsParams.DEFAULT); - NamedRef ref = resolved.getNamedRef(); + + ResolvedHash oldRef = + getHashResolver() + .resolveHashOnRef( + referenceName, expectedHash, "Expected hash", REQUIRED_UNAMBIGUOUS_HASH); + ResolvedHash newRef = + getHashResolver() + .resolveHashOnRef( + assignTo.getName(), assignTo.getHash(), "Target hash", UNAMBIGUOUS_HASH); checkArgument( - referenceType == null || referenceType == RefUtil.referenceType(ref), + referenceType == null || referenceType == RefUtil.referenceType(oldRef.getNamedRef()), "Expected reference type %s does not match existing reference %s", referenceType, - ref); + oldRef.getNamedRef()); startAccessCheck() - .canViewReference( - namedRefWithHashOrThrow(assignTo.getName(), assignTo.getHash()).getValue()) - .canAssignRefToHash(ref) + .canViewReference(newRef.getNamedRef()) + .canAssignRefToHash(oldRef.getNamedRef()) .checkAndThrow(); - Hash targetHash = toHash(assignTo.getName(), assignTo.getHash()); - getStore().assign(resolved.getNamedRef(), toHash(expectedHash, true), targetHash); - return RefUtil.toReference(ref, targetHash); + getStore().assign(oldRef.getNamedRef(), oldRef.getProvidedHash(), newRef.getHash()); + return RefUtil.toReference(oldRef.getNamedRef(), newRef.getHash()); } catch (ReferenceNotFoundException e) { throw new NessieReferenceNotFoundException(e.getMessage(), e); } catch (ReferenceConflictException e) { @@ -350,7 +359,16 @@ && getServerConfig().getDefaultBranch().equals(ref.getName())), startAccessCheck().canDeleteReference(ref).checkAndThrow(); - Hash deletedAthash = getStore().delete(ref, toHash(expectedHash, true)).getHash(); + ResolvedHash refToDelete = + getHashResolver() + .resolveHashOnRef( + resolved.getNamedRef(), + resolved.getHash(), + expectedHash, + "Expected hash", + REQUIRED_UNAMBIGUOUS_HASH); + + Hash deletedAthash = getStore().delete(ref, refToDelete.getProvidedHash()).getHash(); return RefUtil.toReference(ref, deletedAthash); } catch (ReferenceNotFoundException e) { throw new NessieReferenceNotFoundException(e.getMessage(), e); @@ -369,42 +387,61 @@ public R getCommitLog( String pageToken, PagedResponseHandler pagedResponseHandler) throws NessieNotFoundException { - // we should only allow named references when no paging is defined - WithHash endRef = - namedRefWithHashOrThrow(namedRef, null == pageToken ? youngestHash : pageToken); + // FIXME we should only allow named references when no paging is defined - startAccessCheck().canListCommitLog(endRef.getValue()).checkAndThrow(); + try { - boolean fetchAll = FetchOption.isFetchAll(fetchOption); - Set successfulChecks = new HashSet<>(); - Set failedChecks = new HashSet<>(); - try (PaginationIterator commits = getStore().getCommits(endRef.getHash(), fetchAll)) { + ResolvedHash endRef = + getHashResolver() + .resolveHashOnRef( + namedRef, + null == pageToken ? youngestHash : pageToken, + null == pageToken ? "Youngest hash" : "Token pagination hash", + ANY_HASH); + + startAccessCheck().canListCommitLog(endRef.getNamedRef()).checkAndThrow(); + + String stopHash = + oldestHashLimit == null + ? null + : getHashResolver() + .resolveHashOnRef(endRef, oldestHashLimit, "Oldest hash", ANY_HASH) + .getProvidedHash() + .map(Hash::asString) + .orElse(null); - Predicate predicate = filterCommitLog(filter); - while (commits.hasNext()) { - Commit commit = commits.next(); + boolean fetchAll = FetchOption.isFetchAll(fetchOption); + Set successfulChecks = new HashSet<>(); + Set failedChecks = new HashSet<>(); + try (PaginationIterator commits = getStore().getCommits(endRef.getHash(), fetchAll)) { - LogEntry logEntry = commitToLogEntry(fetchAll, commit); + Predicate predicate = filterCommitLog(filter); + while (commits.hasNext()) { + Commit commit = commits.next(); - String hash = logEntry.getCommitMeta().getHash(); + LogEntry logEntry = commitToLogEntry(fetchAll, commit); - if (!predicate.test(logEntry)) { - continue; - } + String hash = logEntry.getCommitMeta().getHash(); + + if (!predicate.test(logEntry)) { + continue; + } - boolean stop = Objects.equals(hash, oldestHashLimit); + boolean stop = Objects.equals(hash, stopHash); - logEntry = logEntryOperationsAccessCheck(successfulChecks, failedChecks, endRef, logEntry); + logEntry = + logEntryOperationsAccessCheck(successfulChecks, failedChecks, endRef, logEntry); - if (!pagedResponseHandler.addEntry(logEntry)) { - if (!stop) { - pagedResponseHandler.hasMore(commits.tokenForCurrent()); + if (!pagedResponseHandler.addEntry(logEntry)) { + if (!stop) { + pagedResponseHandler.hasMore(commits.tokenForCurrent()); + } + break; } - break; - } - if (stop) { - break; + if (stop) { + break; + } } } @@ -577,21 +614,32 @@ public MergeResponse transplantCommitsIntoBranch( try { checkArgument(!hashesToTransplant.isEmpty(), "No hashes given to transplant."); validateCommitMeta(commitMeta); - validateNoRelativeSpec(expectedHash); - BranchName targetBranch = BranchName.of(branchName); String lastHash = hashesToTransplant.get(hashesToTransplant.size() - 1); - validateHash(lastHash); - WithHash namedRefWithHash = namedRefWithHashOrThrow(fromRefName, lastHash); + ResolvedHash fromRef = + getHashResolver() + .resolveHashOnRef( + fromRefName, lastHash, "Hash to transplant", REQUIRED_UNAMBIGUOUS_HASH); + + ResolvedHash toRef = + getHashResolver() + .resolveHashOnRef( + branchName, expectedHash, "Expected hash", REQUIRED_UNAMBIGUOUS_HASH); + + checkArgument( + toRef.getNamedRef() instanceof BranchName, "Can only transplant into branches."); startAccessCheck() - .canViewReference(namedRefWithHash.getValue()) - .canCommitChangeAgainstReference(targetBranch) + .canViewReference(fromRef.getNamedRef()) + .canCommitChangeAgainstReference(toRef.getNamedRef()) .checkAndThrow(); - List transplants; - try (Stream s = hashesToTransplant.stream().map(Hash::of)) { - transplants = s.collect(Collectors.toList()); + List transplants = new ArrayList<>(hashesToTransplant.size()); + for (String hash : hashesToTransplant) { + transplants.add( + getHashResolver() + .resolveHashOnRef(fromRef, hash, "Hash to transplant", REQUIRED_UNAMBIGUOUS_HASH) + .getHash()); } if (transplants.size() > 1) { @@ -600,15 +648,13 @@ public MergeResponse transplantCommitsIntoBranch( commitMeta = null; } - Optional into = toHash(expectedHash, true); - MergeResult result = getStore() .transplant( TransplantOp.builder() - .fromRef(namedRefWithHash.getValue()) - .toBranch(targetBranch) - .expectedHash(into) + .fromRef(fromRef.getNamedRef()) + .toBranch((BranchName) toRef.getNamedRef()) + .expectedHash(toRef.getProvidedHash()) .sequenceToTransplant(transplants) .updateCommitMetadata( commitMetaUpdate( @@ -620,12 +666,15 @@ public MergeResponse transplantCommitsIntoBranch( fromRefName, lastHash, branchName, - into.map(h -> " at " + h.asString()).orElse("")))) + toRef + .getProvidedHash() + .map(h -> " at " + h.asString()) + .orElse("")))) .mergeKeyBehaviors(keyMergeBehaviors(keyMergeBehaviors)) .defaultMergeBehavior(defaultMergeBehavior(defaultMergeBehavior)) .dryRun(Boolean.TRUE.equals(dryRun)) .fetchAdditionalInfo(Boolean.TRUE.equals(fetchAdditionalInfo)) - .validator(createCommitValidator(targetBranch)) + .validator(createCommitValidator((BranchName) toRef.getNamedRef())) .build()); return createResponse(fetchAdditionalInfo, result); } catch (ReferenceNotFoundException e) { @@ -657,27 +706,30 @@ public MergeResponse mergeRefIntoBranch( throws NessieNotFoundException, NessieConflictException { try { validateCommitMeta(commitMeta); - validateNoRelativeSpec(fromHash); - validateNoRelativeSpec(expectedHash); - BranchName targetBranch = BranchName.of(branchName); - WithHash namedRefWithHash = namedRefWithHashOrThrow(fromRefName, fromHash); + ResolvedHash fromRef = + getHashResolver() + .resolveHashOnRef(fromRefName, fromHash, "Source hash", REQUIRED_UNAMBIGUOUS_HASH); + ResolvedHash toRef = + getHashResolver() + .resolveHashOnRef( + branchName, expectedHash, "Expected hash", REQUIRED_UNAMBIGUOUS_HASH); + + checkArgument(toRef.getNamedRef() instanceof BranchName, "Can only merge into branches."); + startAccessCheck() - .canViewReference(namedRefWithHash.getValue()) - .canCommitChangeAgainstReference(targetBranch) + .canViewReference(fromRef.getNamedRef()) + .canCommitChangeAgainstReference(toRef.getNamedRef()) .checkAndThrow(); - Hash from = toHash(fromRefName, fromHash); - Optional into = toHash(expectedHash, true); - MergeResult result = getStore() .merge( MergeOp.builder() - .fromRef(namedRefWithHash.getValue()) - .fromHash(toHash(fromRefName, fromHash)) - .toBranch(targetBranch) - .expectedHash(into) + .fromRef(fromRef.getNamedRef()) + .fromHash(fromRef.getHash()) + .toBranch((BranchName) toRef.getNamedRef()) + .expectedHash(toRef.getProvidedHash()) .updateCommitMetadata( commitMetaUpdate( commitMeta, @@ -686,14 +738,17 @@ public MergeResponse mergeRefIntoBranch( String.format( "Merged %s at %s into %s%s", fromRefName, - from.asString(), + fromRef.getHash().asString(), branchName, - into.map(h -> " at " + h.asString()).orElse("")))) + toRef + .getProvidedHash() + .map(h -> " at " + h.asString()) + .orElse("")))) .mergeKeyBehaviors(keyMergeBehaviors(keyMergeBehaviors)) .defaultMergeBehavior(defaultMergeBehavior(defaultMergeBehavior)) .dryRun(Boolean.TRUE.equals(dryRun)) .fetchAdditionalInfo(Boolean.TRUE.equals(fetchAdditionalInfo)) - .validator(createCommitValidator(targetBranch)) + .validator(createCommitValidator((BranchName) toRef.getNamedRef())) .build()); return createResponse(fetchAdditionalInfo, result); } catch (ReferenceNotFoundException e) { @@ -807,7 +862,8 @@ public R getEntries( ContentKey prefixKey, List requestedKeys) throws NessieNotFoundException { - WithHash refWithHash = namedRefWithHashOrThrow(namedRef, hashOnRef); + ResolvedHash refWithHash = + getHashResolver().resolveHashOnRef(namedRef, hashOnRef, "Expected hash", ANY_HASH); effectiveReference.accept(refWithHash); @@ -968,12 +1024,16 @@ protected Predicate filterEntries(String filter) { public CommitResponse commitMultipleOperations( String branch, String expectedHash, Operations operations) throws NessieNotFoundException, NessieConflictException { - BranchName branchName = BranchName.of(branch); - validateNoRelativeSpec(expectedHash); CommitMeta commitMeta = operations.getCommitMeta(); validateCommitMeta(commitMeta); + ResolvedHash toRef = + getHashResolver() + .resolveHashOnRef(branch, expectedHash, "Expected hash", REQUIRED_UNAMBIGUOUS_HASH); + + checkArgument(toRef.getNamedRef() instanceof BranchName, "Can only commit into branches."); + List ops = operations.getOperations().stream() .map(TreeApiImpl::toOp) @@ -985,14 +1045,12 @@ public CommitResponse commitMultipleOperations( Hash newHash = getStore() .commit( - BranchName.of(branch), - Optional.ofNullable(expectedHash).map(Hash::of), + (BranchName) toRef.getNamedRef(), + toRef.getProvidedHash(), commitMetaUpdate(null, numCommits -> null).rewriteSingle(commitMeta), ops, - createCommitValidator(branchName), - (key, cid) -> { - commitResponse.addAddedContents(addedContent(key, cid)); - }) + createCommitValidator((BranchName) toRef.getNamedRef()), + (key, cid) -> commitResponse.addAddedContents(addedContent(key, cid))) .getCommitHash(); return commitResponse.targetBranch(Branch.of(branch, newHash.asString())).build(); @@ -1029,28 +1087,6 @@ private CommitValidator createCommitValidator(BranchName branchName) { }; } - private Hash toHash(String referenceName, String hashOnReference) - throws ReferenceNotFoundException { - if (Detached.REF_NAME.equals(referenceName)) { - return Hash.of(hashOnReference); - } - if (hashOnReference == null) { - return getStore().getNamedRef(referenceName, GetNamedRefsParams.DEFAULT).getHash(); - } - return toHash(hashOnReference, true) - .orElseThrow(() -> new IllegalStateException("Required hash is missing")); - } - - private static Optional toHash(String hash, boolean required) { - if (hash == null || hash.isEmpty()) { - if (required) { - throw new IllegalArgumentException("Must provide expected hash value for operation."); - } - return Optional.empty(); - } - return Optional.of(Hash.of(hash)); - } - private static Reference makeReference( ReferenceInfo refWithHash, boolean fetchMetadata) { NamedRef ref = refWithHash.getNamedRef(); diff --git a/servers/services/src/main/java/org/projectnessie/services/spi/TreeService.java b/servers/services/src/main/java/org/projectnessie/services/spi/TreeService.java index 15464c6bff1..dce05894ab0 100644 --- a/servers/services/src/main/java/org/projectnessie/services/spi/TreeService.java +++ b/servers/services/src/main/java/org/projectnessie/services/spi/TreeService.java @@ -15,10 +15,8 @@ */ package org.projectnessie.services.spi; -import static org.projectnessie.model.Validation.HASH_MESSAGE; import static org.projectnessie.model.Validation.HASH_OR_RELATIVE_COMMIT_SPEC_MESSAGE; import static org.projectnessie.model.Validation.HASH_OR_RELATIVE_COMMIT_SPEC_REGEX; -import static org.projectnessie.model.Validation.HASH_REGEX; import static org.projectnessie.model.Validation.REF_NAME_MESSAGE; import static org.projectnessie.model.Validation.REF_NAME_REGEX; @@ -92,8 +90,12 @@ Reference createReference( ReferenceType type, @Valid @jakarta.validation.Valid - @Pattern(regexp = HASH_REGEX, message = HASH_MESSAGE) - @jakarta.validation.constraints.Pattern(regexp = HASH_REGEX, message = HASH_MESSAGE) + @Pattern( + regexp = HASH_OR_RELATIVE_COMMIT_SPEC_REGEX, + message = HASH_OR_RELATIVE_COMMIT_SPEC_MESSAGE) + @jakarta.validation.constraints.Pattern( + regexp = HASH_OR_RELATIVE_COMMIT_SPEC_REGEX, + message = HASH_OR_RELATIVE_COMMIT_SPEC_MESSAGE) String hash, @Valid @jakarta.validation.Valid @@ -162,8 +164,12 @@ R getCommitLog( FetchOption fetchOption, @Valid @jakarta.validation.Valid - @Pattern(regexp = HASH_REGEX, message = HASH_MESSAGE) - @jakarta.validation.constraints.Pattern(regexp = HASH_REGEX, message = HASH_MESSAGE) + @Pattern( + regexp = HASH_OR_RELATIVE_COMMIT_SPEC_REGEX, + message = HASH_OR_RELATIVE_COMMIT_SPEC_MESSAGE) + @jakarta.validation.constraints.Pattern( + regexp = HASH_OR_RELATIVE_COMMIT_SPEC_REGEX, + message = HASH_OR_RELATIVE_COMMIT_SPEC_MESSAGE) String oldestHashLimit, @Valid @jakarta.validation.Valid @@ -202,7 +208,15 @@ MergeResponse transplantCommitsIntoBranch( message = HASH_OR_RELATIVE_COMMIT_SPEC_MESSAGE) String expectedHash, @Nullable @jakarta.annotation.Nullable CommitMeta commitMeta, - List hashesToTransplant, + List< + @Pattern( + regexp = HASH_OR_RELATIVE_COMMIT_SPEC_REGEX, + message = HASH_OR_RELATIVE_COMMIT_SPEC_MESSAGE) + @jakarta.validation.constraints.Pattern( + regexp = HASH_OR_RELATIVE_COMMIT_SPEC_REGEX, + message = HASH_OR_RELATIVE_COMMIT_SPEC_MESSAGE) + String> + hashesToTransplant, @Valid @jakarta.validation.Valid @NotBlank @@ -255,8 +269,12 @@ MergeResponse mergeRefIntoBranch( @jakarta.validation.Valid @NotBlank @jakarta.validation.constraints.NotBlank - @Pattern(regexp = HASH_REGEX, message = HASH_MESSAGE) - @jakarta.validation.constraints.Pattern(regexp = HASH_REGEX, message = HASH_MESSAGE) + @Pattern( + regexp = HASH_OR_RELATIVE_COMMIT_SPEC_REGEX, + message = HASH_OR_RELATIVE_COMMIT_SPEC_MESSAGE) + @jakarta.validation.constraints.Pattern( + regexp = HASH_OR_RELATIVE_COMMIT_SPEC_REGEX, + message = HASH_OR_RELATIVE_COMMIT_SPEC_MESSAGE) String fromHash, @Nullable @jakarta.annotation.Nullable CommitMeta commitMeta, Collection keyMergeBehaviors, diff --git a/servers/services/src/test/java/org/projectnessie/services/hash/TestParsedHash.java b/servers/services/src/test/java/org/projectnessie/services/hash/TestParsedHash.java new file mode 100644 index 00000000000..a0011150b04 --- /dev/null +++ b/servers/services/src/test/java/org/projectnessie/services/hash/TestParsedHash.java @@ -0,0 +1,72 @@ +/* + * Copyright (C) 2023 Dremio + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.projectnessie.services.hash; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy; +import static org.projectnessie.versioned.RelativeCommitSpec.Type.N_TH_PREDECESSOR; +import static org.projectnessie.versioned.RelativeCommitSpec.Type.TIMESTAMP_MILLIS_EPOCH; +import static org.projectnessie.versioned.RelativeCommitSpec.relativeCommitSpec; + +import java.util.Optional; +import java.util.stream.Stream; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; +import org.junit.jupiter.params.provider.ValueSource; +import org.projectnessie.versioned.Hash; + +class TestParsedHash { + + private static final Hash NO_ANCESTOR = Hash.of("12345678"); + + public static Stream parseGoodCases() { + return Stream.of( + Arguments.of(null, null), + Arguments.of("", null), + Arguments.of("cafeBABE", ParsedHash.of(Hash.of("cafebabe"))), + Arguments.of("~1", ParsedHash.of(relativeCommitSpec(N_TH_PREDECESSOR, "1"))), + Arguments.of( + "cafeBABE*2023-07-29T12:34:56.789Z~1", + ParsedHash.of( + Hash.of("cafebabe"), + relativeCommitSpec(TIMESTAMP_MILLIS_EPOCH, "2023-07-29T12:34:56.789Z"), + relativeCommitSpec(N_TH_PREDECESSOR, "1")))); + } + + @ParameterizedTest + @MethodSource + void parseGoodCases(String hashOrRelativeSpec, ParsedHash expected) { + Optional parse = ParsedHash.parse(hashOrRelativeSpec, NO_ANCESTOR); + assertThat(parse.orElse(null)).isEqualTo(expected); + } + + @ParameterizedTest + @ValueSource(strings = {" ", "123", "cafeBABE?1"}) + void parseBadCases(String hashOrRelativeSpec) { + assertThatThrownBy(() -> ParsedHash.parse(hashOrRelativeSpec, NO_ANCESTOR)) + .isInstanceOf(IllegalArgumentException.class); + } + + @Test + void parseNoAncestor() { + Optional parsed = ParsedHash.parse(NO_ANCESTOR.asString() + "~1", NO_ANCESTOR); + assertThat(parsed).isPresent(); + assertThat(parsed.get().getAbsolutePart()).isPresent(); + assertThat(parsed.get().getAbsolutePart().get()).isSameAs(NO_ANCESTOR); + } +} From e4afc0ba7a1b0953825fde6c9006bba3502c9783 Mon Sep 17 00:00:00 2001 From: Alexandre Dutra Date: Mon, 31 Jul 2023 13:41:28 +0200 Subject: [PATCH 03/12] Changes to Nessie specs --- api/NESSIE-SPEC-2-0.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/api/NESSIE-SPEC-2-0.md b/api/NESSIE-SPEC-2-0.md index b001cccf5c9..5f7ce98a431 100644 --- a/api/NESSIE-SPEC-2-0.md +++ b/api/NESSIE-SPEC-2-0.md @@ -22,6 +22,18 @@ well-specified behaviours. Refer to the [Nessie API documentation](./README.md) for the meaning of Nessie-specific terms. +# 2.1.1 + +* Released with Nessie version 0.67.0. +* Support for relative hashes was extended to the entire v2 API. + * Path parameters and request entities, such as `Reference`, `Merge` and `Transplant`, now + consistently support relative hashes. + * Ambiguous hashes (that is, hashes that are implicitly resolved against the current HEAD, e.g. + `~1`) are not allowed in writing operations. +* API v2 `GetReferenceByName` endpoint now returns a `BAD_REQUEST` error if the reference name + contains any hash (absolute or relative). Previously, the server would silently ignore the + hash and return the reference if it existed. + # 2.1.0 * Released with Nessie version 0.61.0. From 243db21c52af9ffa46b1bdc3a2015e6b412613e6 Mon Sep 17 00:00:00 2001 From: Alexandre Dutra Date: Mon, 31 Jul 2023 13:41:53 +0200 Subject: [PATCH 04/12] Tests for relative hashes --- .../tests/AbstractRelativeReferences.java | 865 ++++++++++++++++++ .../jaxrs/tests/BaseTestNessieApi.java | 34 +- .../jaxrs/tests/BaseTestNessieRest.java | 164 +--- .../jaxrs/tests/AbstractTestRestApi.java | 5 + .../tests/AbstractTestRestApiPersist.java | 5 + .../server/TestQuarkusRestInMemory.java | 14 +- .../TestQuarkusRestInMemoryPersist.java | 11 + ...arkusWithOlderRestApiV1ClientInMemory.java | 8 +- 8 files changed, 975 insertions(+), 131 deletions(-) create mode 100644 servers/jax-rs-tests/src/main/java/org/projectnessie/jaxrs/tests/AbstractRelativeReferences.java diff --git a/servers/jax-rs-tests/src/main/java/org/projectnessie/jaxrs/tests/AbstractRelativeReferences.java b/servers/jax-rs-tests/src/main/java/org/projectnessie/jaxrs/tests/AbstractRelativeReferences.java new file mode 100644 index 00000000000..2cdc4fbc432 --- /dev/null +++ b/servers/jax-rs-tests/src/main/java/org/projectnessie/jaxrs/tests/AbstractRelativeReferences.java @@ -0,0 +1,865 @@ +/* + * Copyright (C) 2023 Dremio + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.projectnessie.jaxrs.tests; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assumptions.assumeTrue; +import static org.projectnessie.error.ErrorCode.BAD_REQUEST; +import static org.projectnessie.jaxrs.tests.BaseTestNessieRest.rest; + +import io.restassured.response.Response; +import io.restassured.response.ValidatableResponse; +import java.time.Instant; +import java.util.Arrays; +import java.util.List; +import java.util.Set; +import java.util.stream.Stream; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.projectnessie.client.ext.NessieApiVersion; +import org.projectnessie.client.ext.NessieApiVersions; +import org.projectnessie.error.ErrorCode; +import org.projectnessie.error.NessieError; +import org.projectnessie.model.Branch; +import org.projectnessie.model.CommitMeta; +import org.projectnessie.model.CommitResponse; +import org.projectnessie.model.ContentKey; +import org.projectnessie.model.ContentResponse; +import org.projectnessie.model.Detached; +import org.projectnessie.model.DiffResponse; +import org.projectnessie.model.EntriesResponse; +import org.projectnessie.model.GetMultipleContentsRequest; +import org.projectnessie.model.GetMultipleContentsResponse; +import org.projectnessie.model.IcebergTable; +import org.projectnessie.model.LogResponse; +import org.projectnessie.model.MergeResponse; +import org.projectnessie.model.Reference; +import org.projectnessie.model.SingleReferenceResponse; + +/** A set of tests specifically aimed at testing relative references in API v2. */ +public abstract class AbstractRelativeReferences { + + private final BaseTestNessieRest outer; + + private final ContentKey key1 = ContentKey.of("key1"); + private final ContentKey key2 = ContentKey.of("key2"); + private final ContentKey key3 = ContentKey.of("key3"); + private final ContentKey key4 = ContentKey.of("key4"); + private final ContentKey key5 = ContentKey.of("key5"); + + private final IcebergTable table1 = IcebergTable.of("1", 1, 1, 1, 1); + private final IcebergTable table2 = IcebergTable.of("2", 2, 2, 2, 2); + private final IcebergTable table3 = IcebergTable.of("3", 3, 3, 3, 3); + private final IcebergTable table4 = IcebergTable.of("4", 4, 4, 4, 4); + private final IcebergTable table5 = IcebergTable.of("5", 5, 5, 5, 5); + + private String c1; + private String c2; + private String c3; + private String c4; + private String c5; + + private Branch etl1; + + protected AbstractRelativeReferences(BaseTestNessieRest outer) { + this.outer = outer; + } + + @BeforeEach + @NessieApiVersions(versions = NessieApiVersion.V2) + void createFixtures() { + /* + base c1 -- c2 + \ + etl1 c3 + \ + etl2 c4 -- c5 + */ + assumeTrue(outer.isNewModel()); + Branch base = outer.createBranchV2("base"); + base = outer.commitV2(base, key1, table1); + c1 = base.getHash(); + base = outer.commitV2(base, key2, table2); + c2 = base.getHash(); + etl1 = outer.createBranchV2("etl1", base); + etl1 = outer.commitV2(etl1, key3, table3); + c3 = etl1.getHash(); + Branch etl2 = outer.createBranchV2("etl2", etl1); + etl2 = outer.commitV2(etl2, key4, table4); + c4 = etl2.getHash(); + etl2 = outer.commitV2(etl2, key5, table5); + c5 = etl2.getHash(); + } + + /** + * Can create a reference from a hash that is unambiguous. Expect a 200 response with the created + * reference. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + public void createReferenceUnambiguous() { + Reference reference = + expectReference( + rest() + .queryParam("name", "branch1") + .queryParam("type", "branch") + .body(Branch.of("base", c2 + "~1")) + .post("trees")); + assertThat(reference.getName()).isEqualTo("branch1"); + assertThat(reference.getHash()).isEqualTo(c1); + } + + /** + * Can create a reference from a hash that is unambiguous and DETACHED. Expect a 200 response with + * the created reference. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + public void createReferenceUnambiguousDetached() { + Reference reference = + expectReference( + rest() + .queryParam("name", "branch1") + .queryParam("type", "branch") + .body(Detached.of(c2 + "~1")) + .post("trees")); + assertThat(reference.getName()).isEqualTo("branch1"); + assertThat(reference.getHash()).isEqualTo(c1); + } + + /** + * Cannot create a reference from a hash that is ambiguous (writing operation). The hash must + * contain a starting commit ID. Expect a BAD_REQUEST error. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + public void createReferenceAmbiguous() { + NessieError error = + expectError( + rest() + .queryParam("name", "branch1") + .queryParam("type", "branch") + .body(Branch.of("base", "~1")) + .post("trees"), + 400); + checkError(error, BAD_REQUEST, "Target hash must contain a starting commit ID."); + } + + /** + * Can assign a reference from a hash that is unambiguous, but the hash is not the current hash of + * the reference. Expect a REFERENCE_CONFLICT error. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + public void assignReferenceFromUnambiguousConflict() { + NessieError error = expectError(rest().body(etl1).put("trees/base@{hash}", c2 + "~1"), 409); + checkError( + error, ErrorCode.REFERENCE_CONFLICT, "Named-reference 'base' is not at expected hash"); + } + + /** + * Can assign a reference from a hash that is unambiguous, and the hash is the current HEAD of the + * reference. Expect a 200 response. + * + *

This is a convoluted case as the only way to use a relative reference that resolves to the + * HEAD of a branch is to use a timestamp in the future that resolves to the HEAD itself. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + public void assignReferenceFromUnambiguousSuccess() { + Reference base = + expectReference( + rest().body(etl1).put("trees/base@{hash}", c2 + "*" + Instant.now().plusSeconds(60))); + outer.soft.assertThat(base.getHash()).isEqualTo(c3); + } + + /** + * Cannot assign a reference from a hash that is ambiguous (writing operation). The hash must + * contain a starting commit ID. Expect a BAD_REQUEST error. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + public void assignReferenceFromAmbiguous() { + NessieError error = expectError(rest().body(etl1).put("trees/base@{hash}", "^1"), 400); + checkError(error, BAD_REQUEST, "Expected hash must contain a starting commit ID."); + } + + /** + * Cannot assign a reference from a DETACHED hash that is ambiguous (DETACHED requires + * unambiguous). The hash must contain a starting commit ID. Expect a BAD_REQUEST error. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + public void assignReferenceFromAmbiguousDetached() { + NessieError error = expectError(rest().body(etl1).put("trees/@~1"), 400); + checkError(error, BAD_REQUEST, "Expected hash must contain a starting commit ID."); + } + + /** + * Can assign a reference to a hash that is unambiguous. Expect a 200 response with the updated + * reference. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + public void assignReferenceToUnambiguous() { + etl1 = + expectReference( + rest().body(Branch.of("base", c2 + "~1")).put("trees/etl1@{hash}", etl1.getHash())); + outer.soft.assertThat(etl1.getHash()).isEqualTo(c1); + } + + /** + * Can assign a reference to a hash that is unambiguous adn DETACHED. Expect a 200 response with + * the updated reference. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + public void assignReferenceToUnambiguousDetached() { + etl1 = + expectReference( + rest().body(Detached.of(c2 + "~1")).put("trees/etl1@{hash}", etl1.getHash())); + outer.soft.assertThat(etl1.getHash()).isEqualTo(c1); + } + + /** + * Cannot assign a reference to a hash that is ambiguous (writing operation). The hash must + * contain a starting commit ID. Expect a BAD_REQUEST error. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + public void assignReferenceToAmbiguous() { + NessieError error = + expectError( + rest() + .body(Branch.of("base", "*2023-01-01T00:00:00.000Z")) + .put("trees/etl1@{hash}", etl1.getHash()), + 400); + checkError(error, BAD_REQUEST, "Target hash must contain a starting commit ID."); + } + + /** + * Can delete a reference that is unambiguous, but the hash is not the current HEAD of the + * reference. Expect a REFERENCE_CONFLICT error. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + public void deleteReferenceUnambiguousConflict() { + NessieError error = expectError(rest().delete("trees/base@{hash}", c2 + "~1"), 409); + checkError( + error, ErrorCode.REFERENCE_CONFLICT, "Named-reference 'base' is not at expected hash"); + } + + /** + * Can delete a reference that is unambiguous, and the hash is the current HEAD of the reference. + * Expect a 200 response with the deleted reference. + * + *

This is a convoluted case as the only way to use a relative reference that resolves to the + * HEAD of a branch is to use a timestamp in the future that resolves to the HEAD itself. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + public void deleteReferenceUnambiguousSuccess() { + Reference base = + expectReference( + rest().delete("trees/base@{hash}", c2 + "*" + Instant.now().plusSeconds(60))); + outer.soft.assertThat(base.getHash()).isEqualTo(c2); + } + + /** + * Can commit to a reference that is unambiguous, even if the hash is not the current HEAD of the + * reference, since there are no conflicts between the expected hash and HEAD. Expect a 200 + * response with the add contents. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void commitUnambiguous() { + Set keys = + outer + .prepareCommitV2("base@" + c2 + "~1", key3, table3, 2, false) + .statusCode(200) + .extract() + .as(CommitResponse.class) + .toAddedContentsMap() + .keySet(); + assertThat(keys).containsOnly(key3); + } + + /** + * Cannot commit to a reference that is ambiguous (writing operation). The hash must contain a + * starting commit ID. Expect a BAD_REQUEST error. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void commitAmbiguous() { + NessieError error = + expectError( + outer.prepareCommitV2("base*2023-01-01T00:00:00.000Z", key3, table3, 2, false), 400); + checkError(error, BAD_REQUEST, "Expected hash must contain a starting commit ID."); + } + + /** + * Can merge from a reference that is unambiguous. Expect a 200 response with the merge result. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void mergeFromUnambiguous() { + MergeResponse response = expectMerge(outer.prepareMergeV2("base@" + c2, "etl2", c5 + "~2", 2)); + checkMerge(response, c2, c2); + } + + /** + * Cannot merge from a reference that is ambiguous (writing operation). The hash must contain a + * starting commit ID. Expect a BAD_REQUEST error. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void mergeFromAmbiguous() { + NessieError error = + expectError( + outer.prepareMergeV2("base@" + c2, "etl1", "*2023-01-01T00:00:00.000Z", 2), 400); + checkError(error, BAD_REQUEST, "Source hash must contain a starting commit ID."); + } + + /** + * Can merge to a reference that is unambiguous, even if the hash is not the current HEAD of the + * reference, since for merges and transplants, the expected hash is purely informational. Expect + * a 200 response with the merge result. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void mergeToUnambiguous() { + MergeResponse response = expectMerge(outer.prepareMergeV2("base@" + c2 + "~1", "etl1", c3, 2)); + checkMerge(response, c2, c2); + } + + /** + * Cannot merge to a reference that is ambiguous (writing operation). The hash must contain a + * starting commit ID. Expect a BAD_REQUEST error. + * + *

Note: since for merges and transplants, the expected hash is purely informational, ambiguous + * hashes could in theory be allowed, but this would be confusing to users. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void mergeToAmbiguous() { + NessieError error = + expectError(outer.prepareMergeV2("base@*2023-01-01T00:00:00.000Z", "etl1", c3, 2), 400); + checkError(error, BAD_REQUEST, "Expected hash must contain a starting commit ID."); + } + + /** + * Can transplant from a reference that is unambiguous. Expect a 200 response with the transplant + * result. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void transplantFromUnambiguous() { + List hashes = + Arrays.asList( + c5 + "~1", // c4 + c5 + "*" + Instant.now().plusSeconds(60)); // c5 + MergeResponse response = + expectMerge(outer.prepareTransplantV2("base@" + c2, "etl2", hashes, 2)); + outer.soft.assertThat(response.getEffectiveTargetHash()).isEqualTo(c2); + } + + /** + * Cannot transplant from a reference that is ambiguous (writing operation). The hash must contain + * a starting commit ID. Expect a BAD_REQUEST error. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void transplantFromAmbiguous() { + List hashes = Arrays.asList(c5, "~1"); + NessieError error = + expectError(outer.prepareTransplantV2("base@" + c2, "etl2", hashes, 2), 400); + outer + .soft + .assertThat(error.getMessage()) + .contains("Hash to transplant must contain a starting commit ID."); + } + + /** + * Can transplant to a reference that is unambiguous, even if the hash is not the current HEAD of + * the reference, since for merges and transplants, the expected hash is purely informational. + * Expect a 200 response with the transplant result. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void transplantToUnambiguous() { + List hashes = Arrays.asList(c3, c4, c5); + MergeResponse response = + expectMerge(outer.prepareTransplantV2("base@" + c2 + "~1", "etl2", hashes, 2)); + outer.soft.assertThat(response.getEffectiveTargetHash()).isEqualTo(c2); + } + + /** + * Cannot transplant to a reference that is ambiguous (writing operation). The hash must contain a + * starting commit ID. Expect a BAD_REQUEST error. + * + *

Note: since for merges and transplants, the expected hash is purely informational, ambiguous + * hashes could in theory be allowed, but this would be confusing to users. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void transplantToAmbiguous() { + List hashes = Arrays.asList(c5, c4); + NessieError error = + expectError( + outer.prepareTransplantV2("base@*2023-01-01T00:00:00.000Z", "etl2", hashes, 2), 400); + checkError(error, BAD_REQUEST, "Expected hash must contain a starting commit ID."); + } + + /** + * Can get contents for a reference that is unambiguous. Expect a 200 response with the contents. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void getSeveralContentsUnambiguous() { + Stream keys = + expectContents( + rest() + .queryParam("key", key1.toPathString(), key2.toPathString()) + .get("trees/base@{hash}/contents", c2 + "~1")); + assertThat(keys).containsOnly(key1); + } + + /** + * Can get contents for a DETACHED reference that is unambiguous. Expect a 200 response with the + * contents. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void getSeveralContentsUnambiguousDetached() { + Stream keys = + expectContents( + rest() + .queryParam("key", key1.toPathString(), key2.toPathString()) + .get("trees/@{hash}/contents", c3 + "~1")); + assertThat(keys).containsOnly(key1, key2); + } + + /** + * Can get contents for a reference that is ambiguous (reading operation). Expect a 200 response + * with the contents. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void getSeveralContentsAmbiguous() { + Stream keys = + expectContents( + rest() + .queryParam("key", key1.toPathString(), key2.toPathString()) + .get("trees/base@{hash}/contents", "~1")); + assertThat(keys).containsOnly(key1); + } + + /** + * Cannot get contents for a DETACHED reference that is ambiguous (DETACHED requires unambiguous). + * Expect a BAD_REQUEST error. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void getSeveralContentsAmbiguousDetached() { + NessieError error = + expectError( + rest() + .queryParam("key", key1.toPathString(), key2.toPathString()) + .get("trees/@{hash}/contents", "~1"), + 400); + checkError(error, BAD_REQUEST, "Expected hash must contain a starting commit ID."); + } + + /** + * Can get contents for a reference that is unambiguous. Expect a 200 response with the contents. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void getMultipleContentsUnambiguous() { + Stream keys = + expectContents( + rest() + .body(GetMultipleContentsRequest.of(key1, key2)) + .post("trees/base@{hash}/contents", c2 + "~1")); + assertThat(keys).containsOnly(key1); + } + + /** + * Can get contents for a DETACHED reference that is unambiguous. Expect a 200 response with the + * contents. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void getMultipleContentsUnambiguousDetached() { + Stream keys = + expectContents( + rest() + .body(GetMultipleContentsRequest.of(key1, key2)) + .post("trees/@{hash}/contents", c3 + "~1")); + assertThat(keys).containsOnly(key1, key2); + } + + /** + * Can get contents for a reference that is ambiguous (reading operation). Expect a 200 response + * with the contents. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void getMultipleContentsAmbiguous() { + Stream keys = + expectContents( + rest() + .body(GetMultipleContentsRequest.of(key1, key2)) + .post("trees/base@{hash}/contents", "~1")); + assertThat(keys).containsOnly(key1); + } + + /** + * Cannot get contents for a DETACHED reference that is ambiguous (DETACHED requires unambiguous). + * Expect a BAD_REQUEST error. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void getMultipleContentsAmbiguousDetached() { + NessieError error = + expectError( + rest() + .body(GetMultipleContentsRequest.of(key1, key2)) + .post("trees/@{hash}/contents", "~1"), + 400); + checkError(error, BAD_REQUEST, "Expected hash must contain a starting commit ID."); + } + + /** + * Can get the content for a reference that is unambiguous. Expect a 200 response with the + * content. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void getContentUnambiguous() { + IcebergTable content = + expectContent( + rest().get("trees/base@{hash}/contents/{key}", c2 + "~1", key1.toPathString())); + assertThat(content.getSpecId()).isEqualTo(table1.getSpecId()); + } + + /** + * Can get the content for a DETACHED reference that is unambiguous. Expect a 200 response with + * the content. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void getContentUnambiguousDetached() { + IcebergTable content = + expectContent(rest().get("trees/@{hash}/contents/{key}", c3 + "~1", key2.toPathString())); + assertThat(content.getSpecId()).isEqualTo(table2.getSpecId()); + } + + /** + * Can get the content for a reference that is ambiguous (reading operation). Expect a 200 + * response with the content. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void getContentAmbiguous() { + IcebergTable content = + expectContent(rest().get("trees/base@{hash}/contents/{key}", "~1", key1.toPathString())); + assertThat(content.getSpecId()).isEqualTo(table1.getSpecId()); + } + + /** + * Cannot get the content for a DETACHED reference that is ambiguous (DETACHED requires + * unambiguous). Expect a BAD_REQUEST error. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void getContentAmbiguousDetached() { + NessieError error = + expectError(rest().get("trees/@{hash}/contents/{key}", "~1", key1.toPathString()), 400); + checkError(error, BAD_REQUEST, "Expected hash must contain a starting commit ID."); + } + + /** + * Can get the entries for a reference that is unambiguous. Expect a 200 response with the + * entries. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void getEntriesUnambiguous() { + Stream keys = + expectEntries( + rest() + .queryParam("key", key1.toPathString(), key2.toPathString()) + .get("trees/base@{hash}/entries", c2 + "~1")); + assertThat(keys).containsOnly(key1); + } + + /** + * Can get the entries for a DETACHED reference that is unambiguous. Expect a 200 response with + * the entries. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void getEntriesUnambiguousDetached() { + Stream keys = + expectEntries( + rest() + .queryParam("key", key1.toPathString(), key2.toPathString()) + .get("trees/@{hash}/entries", c3 + "~1")); + assertThat(keys).containsOnly(key1, key2); + } + + /** + * Can get the entries for a reference that is ambiguous (reading operation). Expect a 200 + * response with the entries. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void getEntriesAmbiguous() { + Stream keys = + expectEntries( + rest() + .queryParam("key", key1.toPathString(), key2.toPathString()) + .get("trees/base@{hash}/entries", "~1")); + assertThat(keys).containsOnly(key1); + } + + /** + * Cannot get the entries for a DETACHED reference that is ambiguous (DETACHED requires + * unambiguous). Expect a BAD_REQUEST error. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void getEntriesAmbiguousDetached() { + NessieError error = + expectError( + rest() + .queryParam("key", key1.toPathString(), key2.toPathString()) + .get("trees/@{hash}/entries", "~1"), + 400); + checkError(error, BAD_REQUEST, "Expected hash must contain a starting commit ID."); + } + + /** + * Can get the commit log for a reference that is unambiguous. Expect a 200 response with the log. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void getCommitLogYoungestHashUnambiguous() { + Stream hashes = expectCommitLog(rest().get("trees/base@{hash}/history", c2 + "~1")); + assertThat(hashes).containsExactly(c1); + } + + /** + * Can get the commit log for a DETACHED reference that is unambiguous. Expect a 200 response with + * the log. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void getCommitLogYoungestHashUnambiguousDetached() { + Stream hashes = expectCommitLog(rest().get("trees/@{hash}/history", c3 + "~1")); + assertThat(hashes).containsExactly(c2, c1); + } + + /** + * Can get the commit log for a reference that is ambiguous (reading operation). Expect a 200 + * response with the log. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void getCommitLogYoungestHashAmbiguous() { + Stream hashes = expectCommitLog(rest().get("trees/base@{hash}/history", "~1")); + assertThat(hashes).containsExactly(c1); + } + + /** + * Cannot get the commit log for a DETACHED reference that is ambiguous (DETACHED requires + * unambiguous). Expect a BAD_REQUEST error. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void getCommitLogYoungestHashAmbiguousDetached() { + NessieError error = expectError(rest().get("trees/@{hash}/history", "~1"), 400); + checkError(error, BAD_REQUEST, "Youngest hash must contain a starting commit ID."); + } + + /** + * Can get the commit log with an oldest hash that is unambiguous. Expect a 200 response with the + * log. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void getCommitLogOldestHashUnambiguous() { + Stream hashes = + expectCommitLog(rest().queryParam("limit-hash", c4 + "~1").get("trees/etl2/history")); + assertThat(hashes).containsExactly(c5, c4, c3); + } + + /** + * Can get the commit log with an oldest hash that is unambiguous on a DETACHED ref. Expect a 200 + * response with the log. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void getCommitLogOldestHashUnambiguousDetached() { + Stream hashes = + expectCommitLog( + rest().queryParam("limit-hash", c4 + "~1").get("trees/@{hash}/history", c5)); + assertThat(hashes).containsExactly(c5, c4, c3); + } + + /** + * Can get the commit log with an oldest hash that is ambiguous (reading operation). Expect a 200 + * response with the log. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void getCommitLogOldestHashAmbiguous() { + Stream hashes = + expectCommitLog(rest().queryParam("limit-hash", "~2").get("trees/etl2/history")); + assertThat(hashes).containsExactly(c5, c4, c3); + } + + /** + * Cannot get the commit log with an oldest hash that is ambiguous on a DETACHED ref (DETACHED + * requires unambiguous). Expect a BAD_REQUEST error. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void getCommitLogOldestHashAmbiguousDetached() { + NessieError error = + expectError(rest().queryParam("limit-hash", "~2").get("trees/@{hash}/history", c5), 400); + checkError(error, BAD_REQUEST, "Oldest hash must contain a starting commit ID."); + } + + /** + * Can get a diff between two references that are unambiguous. Expect a 200 response with the + * diff. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void getDiffFromRefToRefUnambiguous() { + Stream keys = + expectDiff(rest().get("trees/etl1@{from}/diff/base@{to}", c3 + "~1", c2 + "~2")); + assertThat(keys).containsOnly(key1, key2); + } + + /** + * Can get a diff between two references that are ambiguous (reading operation). Expect a 200 + * response with the diff. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void getDiffFromRefToRefAmbiguous() { + Stream keys = + expectDiff(rest().get("trees/etl1@{from}/diff/base@{to}", "~1", "~2")); + assertThat(keys).containsOnly(key1, key2); + } + + /** + * Can get a diff between two DETACHED references that are unambiguous. Expect a 200 response with + * the diff. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void getDiffFromRefToRefUnambiguousDetached() { + Stream keys = + expectDiff(rest().get("trees/@{from}/diff/@{to}", c3 + "~1", c2 + "~2")); + assertThat(keys).containsOnly(key1, key2); + } + + /** + * Cannot get a diff between two references that are ambiguous on a DETACHED ref (DETACHED + * requires unambiguous). Expect a BAD_REQUEST error. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void getDiffFromRefAmbiguousDetached() { + NessieError error = expectError(rest().get("trees/@{from}/diff/base@{to}", "~1", "~2"), 400); + checkError(error, BAD_REQUEST, "From hash must contain a starting commit ID."); + } + + /** + * Cannot get a diff between two references that are ambiguous on a DETACHED ref (DETACHED + * requires unambiguous). Expect a BAD_REQUEST error. + */ + @Test + @NessieApiVersions(versions = NessieApiVersion.V2) + void getDiffToRefAmbiguousDetached() { + NessieError error = expectError(rest().get("trees/base@{from}/diff/@{to}", "~1", "~2"), 400); + checkError(error, BAD_REQUEST, "To hash must contain a starting commit ID."); + } + + private static Branch expectReference(Response base) { + return (Branch) + base.then().statusCode(200).extract().as(SingleReferenceResponse.class).getReference(); + } + + private static MergeResponse expectMerge(ValidatableResponse etl2) { + return etl2.statusCode(200).extract().as(MergeResponse.class); + } + + private static Stream expectContents(Response key) { + return key + .then() + .statusCode(200) + .extract() + .as(GetMultipleContentsResponse.class) + .getContents() + .stream() + .map(GetMultipleContentsResponse.ContentWithKey::getKey); + } + + private static IcebergTable expectContent(Response response) { + return (IcebergTable) + response.then().statusCode(200).extract().as(ContentResponse.class).getContent(); + } + + private static Stream expectEntries(Response key) { + return key.then().statusCode(200).extract().as(EntriesResponse.class).getEntries().stream() + .map(EntriesResponse.Entry::getName); + } + + private static Stream expectCommitLog(Response c2) { + return c2.then().statusCode(200).extract().as(LogResponse.class).getLogEntries().stream() + .map(LogResponse.LogEntry::getCommitMeta) + .map(CommitMeta::getHash); + } + + private static Stream expectDiff(Response response) { + return response.then().statusCode(200).extract().as(DiffResponse.class).getDiffs().stream() + .map(DiffResponse.DiffEntry::getKey); + } + + private static NessieError expectError(Response response, int expectedStatusCode) { + return expectError(response.then(), expectedStatusCode); + } + + private static NessieError expectError(ValidatableResponse response, int expectedStatusCode) { + return response.statusCode(expectedStatusCode).extract().as(NessieError.class); + } + + private void checkMerge( + MergeResponse response, String commonAncestor, String effectiveTargetHash) { + outer.soft.assertThat(response.getCommonAncestor()).isEqualTo(commonAncestor); + outer.soft.assertThat(response.getEffectiveTargetHash()).isEqualTo(effectiveTargetHash); + } + + private void checkError(NessieError error, ErrorCode code, String msg) { + outer.soft.assertThat(error.getErrorCode()).isEqualTo(code); + outer.soft.assertThat(error.getMessage()).contains(msg); + } +} diff --git a/servers/jax-rs-tests/src/main/java/org/projectnessie/jaxrs/tests/BaseTestNessieApi.java b/servers/jax-rs-tests/src/main/java/org/projectnessie/jaxrs/tests/BaseTestNessieApi.java index ca02e10d2ce..4d2819c808e 100644 --- a/servers/jax-rs-tests/src/main/java/org/projectnessie/jaxrs/tests/BaseTestNessieApi.java +++ b/servers/jax-rs-tests/src/main/java/org/projectnessie/jaxrs/tests/BaseTestNessieApi.java @@ -79,6 +79,7 @@ import org.projectnessie.error.NessieNamespaceNotFoundException; import org.projectnessie.error.NessieNotFoundException; import org.projectnessie.error.NessieReferenceConflictException; +import org.projectnessie.error.NessieReferenceNotFoundException; import org.projectnessie.error.ReferenceConflicts; import org.projectnessie.model.Branch; import org.projectnessie.model.CommitMeta; @@ -373,10 +374,36 @@ public void references() throws Exception { api().deleteTag().tag(tag).delete(); } + // We cannot delete a branch if the expected hash is not reachable from its HEAD. Here, + // the expected hash is not reachable anymore, because the branch was reassigned to main + // previously. + // In such cases we expect a not-found error. AbstractThrowableAssert deleteConflict = isV2() ? soft.assertThatThrownBy(() -> api().deleteBranch().branch(branch).getAndDelete()) : soft.assertThatThrownBy(() -> api().deleteBranch().branch(branch).delete()); + deleteConflict + .isInstanceOf(NessieReferenceNotFoundException.class) + .hasMessageContaining("Could not find commit"); + + // Move the HEAD of the branch from main to a new commit. + Branch branchWithNewCommit = + prepCommit( + branchAssigned, + "commit", + Put.of(ContentKey.of("key"), Namespace.of("key")), + dummyPut("key", "foo")) + .commit(); + + // We cannot delete a branch if the expected hash is reachable from HEAD, but is not HEAD. Here, + // the expected hash is not the HEAD anymore, because the branch was updated to + // branchWithNewCommit above. + // In such cases we expect a conflict. + deleteConflict = + isV2() + ? soft.assertThatThrownBy( + () -> api().deleteBranch().branch(branchAssigned).getAndDelete()) + : soft.assertThatThrownBy(() -> api().deleteBranch().branch(branchAssigned).delete()); deleteConflict .isInstanceOf(NessieReferenceConflictException.class) .asInstanceOf(type(NessieReferenceConflictException.class)) @@ -385,11 +412,12 @@ public void references() throws Exception { .extracting(Conflict::conflictType) .containsExactly(ConflictType.UNEXPECTED_HASH); + // Delete branch with new commit as expected HEAD: OK if (isV2()) { - Branch deleted = api().deleteBranch().branch(branchAssigned).getAndDelete(); - soft.assertThat(deleted).isEqualTo(branchAssigned); + Branch deleted = api().deleteBranch().branch(branchWithNewCommit).getAndDelete(); + soft.assertThat(deleted).isEqualTo(branchWithNewCommit); } else { - api().deleteBranch().branch(branchAssigned).delete(); + api().deleteBranch().branch(branchWithNewCommit).delete(); } soft.assertThat(api().getAllReferences().get().getReferences()) diff --git a/servers/jax-rs-tests/src/main/java/org/projectnessie/jaxrs/tests/BaseTestNessieRest.java b/servers/jax-rs-tests/src/main/java/org/projectnessie/jaxrs/tests/BaseTestNessieRest.java index 034a62167dc..12540214590 100644 --- a/servers/jax-rs-tests/src/main/java/org/projectnessie/jaxrs/tests/BaseTestNessieRest.java +++ b/servers/jax-rs-tests/src/main/java/org/projectnessie/jaxrs/tests/BaseTestNessieRest.java @@ -32,7 +32,6 @@ import java.net.HttpURLConnection; import java.net.URI; import java.net.URL; -import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -41,6 +40,7 @@ import org.assertj.core.data.MapEntry; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.CsvSource; @@ -78,6 +78,8 @@ public abstract class BaseTestNessieRest extends BaseTestNessieApi { protected URI clientUri; + protected abstract boolean isNewModel(); + @BeforeEach public void enableLogging() { RestAssured.enableLoggingOfRequestAndResponseIfValidationFails(); @@ -409,7 +411,7 @@ public void testInvalidTag(String invalidTagName) { "Could not resolve type id 'FOOBAR' as a subtype of `org.projectnessie.model.Reference`")); } - private Branch createBranchV2(String branchName) { + Branch createBranchV2(String branchName) { // Note: no request body means creating the new branch from the HEAD of the default branch. return (Branch) rest() @@ -423,7 +425,21 @@ private Branch createBranchV2(String branchName) { .getReference(); } - private ValidatableResponse prepareCommitV2( + Branch createBranchV2(String branchName, Reference ref) { + return (Branch) + rest() + .queryParam("name", branchName) + .queryParam("type", Reference.ReferenceType.BRANCH.name()) + .body(ref) + .post("trees") + .then() + .statusCode(200) + .extract() + .as(SingleReferenceResponse.class) + .getReference(); + } + + ValidatableResponse prepareCommitV2( Branch branch, ContentKey key, IcebergTable table, @@ -432,7 +448,7 @@ private ValidatableResponse prepareCommitV2( return prepareCommitV2(branch.toPathString(), key, table, clientSpec, buildMissingNamespaces); } - private ValidatableResponse prepareCommitV2( + ValidatableResponse prepareCommitV2( String ref, ContentKey key, IcebergTable table, @@ -456,7 +472,7 @@ private ValidatableResponse prepareCommitV2( return resp.post("trees/{ref}/history/commit", ref).then(); } - private ValidatableResponse prepareMergeV2( + ValidatableResponse prepareMergeV2( String targetRefAndHash, String fromRefName, String fromHash, int clientSpec) { Map body = new HashMap<>(); body.put("fromRefName", fromRefName); @@ -468,7 +484,7 @@ private ValidatableResponse prepareMergeV2( return resp.post("trees/{ref}/history/merge", targetRefAndHash).then(); } - private ValidatableResponse prepareTransplantV2( + ValidatableResponse prepareTransplantV2( String targetRefAndHash, String fromRefName, List hashesToTransplant, @@ -483,7 +499,7 @@ private ValidatableResponse prepareTransplantV2( return resp.post("trees/{ref}/history/transplant", targetRefAndHash).then(); } - private Branch commitV2(Branch branch, ContentKey key, IcebergTable table) { + Branch commitV2(Branch branch, ContentKey key, IcebergTable table) { return prepareCommitV2(branch, key, table, 2, true) .statusCode(200) .extract() @@ -707,128 +723,24 @@ public void referenceTypeInvalidValue() { .contains("Reference type name must be either 'branch' or 'tag'"); } + @Test @NessieApiVersions(versions = {NessieApiVersion.V2}) - @ParameterizedTest - @CsvSource({ - "-", "main", - }) - public void commitWithoutHashNotAllowed(String ref) { - ContentKey key1 = ContentKey.of("test", "Key"); - IcebergTable table1 = IcebergTable.of("loc1", 1, 2, 3, 4); - NessieError error = - prepareCommitV2(ref, key1, table1, 2, true).statusCode(400).extract().as(NessieError.class); - assertThat(error.getMessage()) - .isEqualTo("commitMultipleOperations.expectedHash: must not be null"); - } - - @NessieApiVersions(versions = {NessieApiVersion.V2}) - @ParameterizedTest - @CsvSource({ - "-~1", - "main~1", - "main@cafebabe~1", - "-^2", - "main^2", - "main@cafebabe^2", - "-*2021-04-07T14:42:25.534748Z", - "main*2021-04-07T14:42:25.534748Z", - "main@cafebabe*2021-04-07T14:42:25.534748Z", - }) - public void commitWithRelativeHashesNotAllowed(String ref) { - ContentKey key1 = ContentKey.of("test", "Key"); - IcebergTable table1 = IcebergTable.of("loc1", 1, 2, 3, 4); - NessieError error = - prepareCommitV2(ref, key1, table1, 2, true).statusCode(400).extract().as(NessieError.class); - assertThat(error.getMessage()) - .startsWith("Relative hash not allowed in commit, merge or transplant operations"); - } - - @NessieApiVersions(versions = {NessieApiVersion.V2}) - @ParameterizedTest - @CsvSource({ - "-", "main", - }) - public void mergeWithoutHashNotAllowed(String targetRefAndHash) { - NessieError error = - prepareMergeV2(targetRefAndHash, "irrelevant", "cafebabe", 2) - .statusCode(400) - .extract() - .as(NessieError.class); - assertThat(error.getMessage()).contains("mergeRefIntoBranch.expectedHash: must not be null"); - } - - @NessieApiVersions(versions = {NessieApiVersion.V2}) - @ParameterizedTest - @CsvSource({ - // relative hashes in target ref (path param) - "-~1,cafebabe", - "main~1,cafebabe", - "main@cafebabe~1,cafebabe", - "-^2,cafebabe", - "main^2,cafebabe", - "main@cafebabe^2,cafebabe", - "-*2021-04-07T14:42:25.534748Z,cafebabe", - "main*2021-04-07T14:42:25.534748Z,cafebabe", - "main@cafebabe*2021-04-07T14:42:25.534748Z,cafebabe", - // relative hashes in source hash (request body) - "main@cafebabe,cafebabe~1", - "main@cafebabe,cafebabe^2", - "main@cafebabe,cafebabe*2021-04-07T14:42:25.534748Z", - }) - public void mergeWithRelativeHashesNotAllowed(String targetRefAndHash, String fromHash) { - NessieError error = - prepareMergeV2(targetRefAndHash, "source", fromHash, 2) - .statusCode(400) - .extract() - .as(NessieError.class); - assertThat(error.getMessage()) - .contains("Relative hash not allowed in commit, merge or transplant operations"); - } - - @NessieApiVersions(versions = {NessieApiVersion.V2}) - @ParameterizedTest - @CsvSource({ - "-", "main", - }) - public void transplantWithoutHashNotAllowed(String targetRefAndHash) { - NessieError error = - prepareTransplantV2( - targetRefAndHash, "irrelevant", Collections.singletonList("cafebabe"), 2) - .statusCode(400) - .extract() - .as(NessieError.class); - assertThat(error.getMessage()) - .contains("transplantCommitsIntoBranch.expectedHash: must not be null"); + public void getRefByNameHashNotAllowed() { + NessieError nessieError = + rest().get("trees/main@12345678").then().statusCode(400).extract().as(NessieError.class); + soft.assertThat(nessieError) + .extracting(NessieError::getStatus, NessieError::getErrorCode) + .containsExactly(400, ErrorCode.BAD_REQUEST); + soft.assertThat(nessieError.getMessage()) + .contains("Hashes are not allowed when fetching a reference by name"); } - @NessieApiVersions(versions = {NessieApiVersion.V2}) - @ParameterizedTest - @CsvSource({ - // relative hashes in target ref (path param) - "-~1,cafebabe", - "main~1,cafebabe", - "main@cafebabe~1,cafebabe", - "-^2,cafebabe", - "main^2,cafebabe", - "main@cafebabe^2,cafebabe", - "-*2021-04-07T14:42:25.534748Z,cafebabe", - "main*2021-04-07T14:42:25.534748Z,cafebabe", - "main@cafebabe*2021-04-07T14:42:25.534748Z,cafebabe", - // relative hashes in hashes to transplant (request body) - "main@cafebabe,cafebabe~1", - "main@cafebabe,cafebabe^2", - "main@cafebabe,cafebabe*2021-04-07T14:42:25.534748Z", - }) - public void transplantWithRelativeHashesNotAllowed( - String targetRefAndHash, String hashToTransplant) { - NessieError error = - prepareTransplantV2( - targetRefAndHash, "source", Collections.singletonList(hashToTransplant), 2) - .statusCode(400) - .extract() - .as(NessieError.class); - assertThat(error.getMessage()) - .contains("Relative hash not allowed in commit, merge or transplant operations"); + @Nested + @NessieApiVersions(versions = NessieApiVersion.V2) + public class RelativeReferences extends AbstractRelativeReferences { + protected RelativeReferences() { + super(BaseTestNessieRest.this); + } } @NessieApiVersions(versions = {NessieApiVersion.V2}) diff --git a/servers/jax-rs-tests/src/test/java/org/projectnessie/jaxrs/tests/AbstractTestRestApi.java b/servers/jax-rs-tests/src/test/java/org/projectnessie/jaxrs/tests/AbstractTestRestApi.java index d238fa361a3..ea911ab6903 100644 --- a/servers/jax-rs-tests/src/test/java/org/projectnessie/jaxrs/tests/AbstractTestRestApi.java +++ b/servers/jax-rs-tests/src/test/java/org/projectnessie/jaxrs/tests/AbstractTestRestApi.java @@ -31,4 +31,9 @@ abstract class AbstractTestRestApi extends BaseTestNessieRest { @RegisterExtension static NessieJaxRsExtension server = jaxRsExtensionForDatabaseAdapter(() -> databaseAdapter); + + @Override + protected boolean isNewModel() { + return false; + } } diff --git a/servers/jax-rs-tests/src/test/java/org/projectnessie/jaxrs/tests/AbstractTestRestApiPersist.java b/servers/jax-rs-tests/src/test/java/org/projectnessie/jaxrs/tests/AbstractTestRestApiPersist.java index e1b9c9a6bbc..2337eb3d996 100644 --- a/servers/jax-rs-tests/src/test/java/org/projectnessie/jaxrs/tests/AbstractTestRestApiPersist.java +++ b/servers/jax-rs-tests/src/test/java/org/projectnessie/jaxrs/tests/AbstractTestRestApiPersist.java @@ -35,4 +35,9 @@ abstract class AbstractTestRestApiPersist extends BaseTestNessieRest { protected boolean fullPagingSupport() { return true; } + + @Override + protected boolean isNewModel() { + return true; + } } diff --git a/servers/quarkus-server/src/test/java/org/projectnessie/server/TestQuarkusRestInMemory.java b/servers/quarkus-server/src/test/java/org/projectnessie/server/TestQuarkusRestInMemory.java index 9e80b7b97b5..96552cd732e 100644 --- a/servers/quarkus-server/src/test/java/org/projectnessie/server/TestQuarkusRestInMemory.java +++ b/servers/quarkus-server/src/test/java/org/projectnessie/server/TestQuarkusRestInMemory.java @@ -17,8 +17,20 @@ import io.quarkus.test.junit.QuarkusTest; import io.quarkus.test.junit.TestProfile; +import org.junit.jupiter.api.Nested; +import org.projectnessie.jaxrs.tests.BaseTestNessieRest; import org.projectnessie.quarkus.tests.profiles.QuarkusTestProfileInmemory; @QuarkusTest @TestProfile(QuarkusTestProfileInmemory.class) -class TestQuarkusRestInMemory extends AbstractQuarkusRestWithMetrics {} +class TestQuarkusRestInMemory extends AbstractQuarkusRestWithMetrics { + + @Override + protected boolean isNewModel() { + return false; + } + + // See https://github.com/quarkusio/quarkus/issues/35104 + @Nested + class RelativeReferences extends BaseTestNessieRest.RelativeReferences {} +} diff --git a/servers/quarkus-server/src/test/java/org/projectnessie/server/TestQuarkusRestInMemoryPersist.java b/servers/quarkus-server/src/test/java/org/projectnessie/server/TestQuarkusRestInMemoryPersist.java index 557d3a33895..e4c0c461c6d 100644 --- a/servers/quarkus-server/src/test/java/org/projectnessie/server/TestQuarkusRestInMemoryPersist.java +++ b/servers/quarkus-server/src/test/java/org/projectnessie/server/TestQuarkusRestInMemoryPersist.java @@ -17,7 +17,9 @@ import io.quarkus.test.junit.QuarkusTest; import io.quarkus.test.junit.TestProfile; +import org.junit.jupiter.api.Nested; import org.projectnessie.client.ext.NessieApiVersions; +import org.projectnessie.jaxrs.tests.BaseTestNessieRest; import org.projectnessie.quarkus.tests.profiles.QuarkusTestProfilePersistInmemory; @QuarkusTest @@ -29,4 +31,13 @@ class TestQuarkusRestInMemoryPersist extends AbstractQuarkusRestWithMetrics { protected boolean fullPagingSupport() { return true; } + + @Override + protected boolean isNewModel() { + return true; + } + + // See https://github.com/quarkusio/quarkus/issues/35104 + @Nested + class RelativeReferences extends BaseTestNessieRest.RelativeReferences {} } diff --git a/servers/quarkus-server/src/test/java/org/projectnessie/server/TestQuarkusWithOlderRestApiV1ClientInMemory.java b/servers/quarkus-server/src/test/java/org/projectnessie/server/TestQuarkusWithOlderRestApiV1ClientInMemory.java index 9c5436a028d..24fb9f67609 100644 --- a/servers/quarkus-server/src/test/java/org/projectnessie/server/TestQuarkusWithOlderRestApiV1ClientInMemory.java +++ b/servers/quarkus-server/src/test/java/org/projectnessie/server/TestQuarkusWithOlderRestApiV1ClientInMemory.java @@ -31,4 +31,10 @@ @TestProfile(QuarkusTestProfileInmemory.class) @NessieApiVersions(versions = NessieApiVersion.V1) @NessieClientResponseFactory(ValidatingApiV1ResponseFactory.class) -class TestQuarkusWithOlderRestApiV1ClientInMemory extends AbstractQuarkusRest {} +class TestQuarkusWithOlderRestApiV1ClientInMemory extends AbstractQuarkusRest { + + @Override + protected boolean isNewModel() { + return false; + } +} From c8ffca12212e37c8d5e9cd236aea73664ea8080e Mon Sep 17 00:00:00 2001 From: Alexandre Dutra Date: Mon, 31 Jul 2023 16:07:13 +0200 Subject: [PATCH 05/12] Update CHANGELOG.md --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b0e4878191..3ba96ccb77e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,12 +8,14 @@ as necessary. Empty sections will not end in the release notes. ### Highlights ### Upgrade notes +- This release upgrades the Nessie API spec to 2.1.1. ### Breaking changes ### New Features ### Changes +- Support for relative hashes has been standardized and is now allowed in all v2 endpoints ### Deprecations From 66b10eb65ccac18cb17bffc898583025e9c63348 Mon Sep 17 00:00:00 2001 From: Alexandre Dutra Date: Mon, 31 Jul 2023 18:12:25 +0200 Subject: [PATCH 06/12] Attempt to fix NesqueIT failures --- .../extensions/AbstractNessieSparkSqlExtensionTest.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/integrations/spark-extensions-basetests/src/main/java/org/projectnessie/spark/extensions/AbstractNessieSparkSqlExtensionTest.java b/integrations/spark-extensions-basetests/src/main/java/org/projectnessie/spark/extensions/AbstractNessieSparkSqlExtensionTest.java index 2c04da61971..5a1a1fc69b2 100644 --- a/integrations/spark-extensions-basetests/src/main/java/org/projectnessie/spark/extensions/AbstractNessieSparkSqlExtensionTest.java +++ b/integrations/spark-extensions-basetests/src/main/java/org/projectnessie/spark/extensions/AbstractNessieSparkSqlExtensionTest.java @@ -270,7 +270,8 @@ additionalRefName, defaultBranch(), currentHash)) "ASSIGN BRANCH %s TO %s AT %s IN nessie", additionalRefName, defaultBranch(), invalidHash)) .isInstanceOf(IllegalArgumentException.class) - .hasMessage(Validation.HASH_MESSAGE + " - but was: " + invalidHash); + .hasMessageContaining(Validation.HASH_RULE) + .hasMessageContaining(" - but was: " + invalidHash); assertThatThrownBy( () -> sql( @@ -320,7 +321,8 @@ additionalRefName, defaultBranch(), currentHash)) "ASSIGN TAG %s TO %s AT %s IN nessie", additionalRefName, defaultBranch(), invalidHash)) .isInstanceOf(IllegalArgumentException.class) - .hasMessage(Validation.HASH_MESSAGE + " - but was: " + invalidHash); + .hasMessageContaining(Validation.HASH_RULE) + .hasMessageContaining(" - but was: " + invalidHash); assertThatThrownBy( () -> sql( From ed99173fbc12439626ef4a10003a84bbb4d21c76 Mon Sep 17 00:00:00 2001 From: Alexandre Dutra Date: Mon, 31 Jul 2023 18:55:25 +0200 Subject: [PATCH 07/12] Fix validation messages --- .../org/projectnessie/model/Validation.java | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/api/model/src/main/java/org/projectnessie/model/Validation.java b/api/model/src/main/java/org/projectnessie/model/Validation.java index c8383bae59d..826f9fe6bbb 100644 --- a/api/model/src/main/java/org/projectnessie/model/Validation.java +++ b/api/model/src/main/java/org/projectnessie/model/Validation.java @@ -124,17 +124,16 @@ public final class Validation { public static final String HASH_MESSAGE = "Hash must " + HASH_RULE; public static final String RELATIVE_COMMIT_SPEC_RULE = - "numeric timestamp (milliseconds since epoch), " - + "optionally followed relative pointers: " + "be either " + "'~' + a number representing the n-th predecessor of a commit, " - + "'^' + a number representing the n-th parent within a commit or " - + "'*' + a number representing the created timestamp in milliseconds since epoch of a commit"; + + "'^' + a number representing the n-th parent within a commit, or " + + "'*' + a number representing the created timestamp of a commit, in milliseconds since epoch or in ISO-8601 format"; public static final String HASH_OR_RELATIVE_COMMIT_SPEC_RULE = - "consist of either a valid commit hash (" + "consist of either a valid commit hash (which in turn must " + HASH_RULE - + "), a valid relative part (" + + "), or a valid relative part (which must " + RELATIVE_COMMIT_SPEC_RULE - + "), or both"; + + "), or both."; public static final String HASH_OR_RELATIVE_COMMIT_SPEC_MESSAGE = "Hash with optional relative part must " + HASH_OR_RELATIVE_COMMIT_SPEC_RULE; @@ -142,10 +141,8 @@ public final class Validation { "Reference name must " + REF_RULE + ", optionally followed " - + "by @ and a commit hash, which must " - + HASH_RULE - + ", optionally followed by a " - + RELATIVE_COMMIT_SPEC_RULE; + + "by @ and a hash with optional relative part. " + + HASH_OR_RELATIVE_COMMIT_SPEC_MESSAGE; public static final String REF_NAME_MESSAGE = "Reference name must " + REF_RULE; public static final String REF_TYPE_RULE = "be either 'branch' or 'tag'"; From 6f4a1bcca5ad05ebd28dd8774ecf7568e4afe6a2 Mon Sep 17 00:00:00 2001 From: Alexandre Dutra Date: Tue, 1 Aug 2023 10:28:32 +0200 Subject: [PATCH 08/12] Resolve hashes against DETACHED --- .../AbstractNessieSparkSqlExtensionTest.java | 14 +++----- .../jaxrs/tests/BaseTestNessieApi.java | 34 ++----------------- .../services/hash/HashResolver.java | 9 +++-- .../impl/AbstractTestInvalidRefs.java | 16 +++------ 4 files changed, 17 insertions(+), 56 deletions(-) diff --git a/integrations/spark-extensions-basetests/src/main/java/org/projectnessie/spark/extensions/AbstractNessieSparkSqlExtensionTest.java b/integrations/spark-extensions-basetests/src/main/java/org/projectnessie/spark/extensions/AbstractNessieSparkSqlExtensionTest.java index 5a1a1fc69b2..044d916e510 100644 --- a/integrations/spark-extensions-basetests/src/main/java/org/projectnessie/spark/extensions/AbstractNessieSparkSqlExtensionTest.java +++ b/integrations/spark-extensions-basetests/src/main/java/org/projectnessie/spark/extensions/AbstractNessieSparkSqlExtensionTest.java @@ -278,9 +278,7 @@ additionalRefName, defaultBranch(), invalidHash)) "ASSIGN BRANCH %s TO %s AT %s IN nessie", additionalRefName, defaultBranch(), unknownHash)) .isInstanceOf(NessieNotFoundException.class) - .hasMessage( - String.format( - "Could not find commit '%s' in reference '%s'.", unknownHash, defaultBranch())); + .hasMessage(String.format("Commit '%s' not found", unknownHash)); assertThatThrownBy( () -> sql( @@ -329,9 +327,7 @@ additionalRefName, defaultBranch(), invalidHash)) "ASSIGN TAG %s TO %s AT %s IN nessie", additionalRefName, defaultBranch(), unknownHash)) .isInstanceOf(NessieNotFoundException.class) - .hasMessage( - String.format( - "Could not find commit '%s' in reference '%s'.", unknownHash, defaultBranch())); + .hasMessage(String.format("Commit '%s' not found", unknownHash)); assertThatThrownBy( () -> sql( @@ -458,8 +454,7 @@ void useShowReferencesAtWithFailureConditions() assertThatThrownBy(() -> sql("USE REFERENCE %s AT %s IN nessie ", refName, randomHash)) .isInstanceOf(NessieNotFoundException.class) - .hasMessage( - String.format("Could not find commit '%s' in reference '%s'.", randomHash, refName)); + .hasMessage(String.format("Commit '%s' not found", randomHash)); assertThatThrownBy(() -> sql("USE REFERENCE %s AT `%s` IN nessie ", refName, invalidTimestamp)) .isInstanceOf(NessieNotFoundException.class) @@ -469,8 +464,7 @@ void useShowReferencesAtWithFailureConditions() assertThatThrownBy(() -> sql("USE REFERENCE %s AT %s IN nessie ", refName, invalidHash)) .isInstanceOf(NessieNotFoundException.class) - .hasMessageStartingWith( - String.format("Could not find commit '%s' in reference '%s'", invalidHash, refName)); + .hasMessage(String.format("Commit '%s' not found", invalidHash)); } @Test diff --git a/servers/jax-rs-tests/src/main/java/org/projectnessie/jaxrs/tests/BaseTestNessieApi.java b/servers/jax-rs-tests/src/main/java/org/projectnessie/jaxrs/tests/BaseTestNessieApi.java index 4d2819c808e..ca02e10d2ce 100644 --- a/servers/jax-rs-tests/src/main/java/org/projectnessie/jaxrs/tests/BaseTestNessieApi.java +++ b/servers/jax-rs-tests/src/main/java/org/projectnessie/jaxrs/tests/BaseTestNessieApi.java @@ -79,7 +79,6 @@ import org.projectnessie.error.NessieNamespaceNotFoundException; import org.projectnessie.error.NessieNotFoundException; import org.projectnessie.error.NessieReferenceConflictException; -import org.projectnessie.error.NessieReferenceNotFoundException; import org.projectnessie.error.ReferenceConflicts; import org.projectnessie.model.Branch; import org.projectnessie.model.CommitMeta; @@ -374,36 +373,10 @@ public void references() throws Exception { api().deleteTag().tag(tag).delete(); } - // We cannot delete a branch if the expected hash is not reachable from its HEAD. Here, - // the expected hash is not reachable anymore, because the branch was reassigned to main - // previously. - // In such cases we expect a not-found error. AbstractThrowableAssert deleteConflict = isV2() ? soft.assertThatThrownBy(() -> api().deleteBranch().branch(branch).getAndDelete()) : soft.assertThatThrownBy(() -> api().deleteBranch().branch(branch).delete()); - deleteConflict - .isInstanceOf(NessieReferenceNotFoundException.class) - .hasMessageContaining("Could not find commit"); - - // Move the HEAD of the branch from main to a new commit. - Branch branchWithNewCommit = - prepCommit( - branchAssigned, - "commit", - Put.of(ContentKey.of("key"), Namespace.of("key")), - dummyPut("key", "foo")) - .commit(); - - // We cannot delete a branch if the expected hash is reachable from HEAD, but is not HEAD. Here, - // the expected hash is not the HEAD anymore, because the branch was updated to - // branchWithNewCommit above. - // In such cases we expect a conflict. - deleteConflict = - isV2() - ? soft.assertThatThrownBy( - () -> api().deleteBranch().branch(branchAssigned).getAndDelete()) - : soft.assertThatThrownBy(() -> api().deleteBranch().branch(branchAssigned).delete()); deleteConflict .isInstanceOf(NessieReferenceConflictException.class) .asInstanceOf(type(NessieReferenceConflictException.class)) @@ -412,12 +385,11 @@ public void references() throws Exception { .extracting(Conflict::conflictType) .containsExactly(ConflictType.UNEXPECTED_HASH); - // Delete branch with new commit as expected HEAD: OK if (isV2()) { - Branch deleted = api().deleteBranch().branch(branchWithNewCommit).getAndDelete(); - soft.assertThat(deleted).isEqualTo(branchWithNewCommit); + Branch deleted = api().deleteBranch().branch(branchAssigned).getAndDelete(); + soft.assertThat(deleted).isEqualTo(branchAssigned); } else { - api().deleteBranch().branch(branchWithNewCommit).delete(); + api().deleteBranch().branch(branchAssigned).delete(); } soft.assertThat(api().getAllReferences().get().getReferences()) diff --git a/servers/services/src/main/java/org/projectnessie/services/hash/HashResolver.java b/servers/services/src/main/java/org/projectnessie/services/hash/HashResolver.java index 93906ba73e5..0ecc52bac52 100644 --- a/servers/services/src/main/java/org/projectnessie/services/hash/HashResolver.java +++ b/servers/services/src/main/java/org/projectnessie/services/hash/HashResolver.java @@ -22,7 +22,6 @@ import java.util.Collections; import java.util.List; -import java.util.Objects; import java.util.Optional; import java.util.function.BiConsumer; import javax.annotation.Nullable; @@ -153,8 +152,12 @@ public ResolvedHash resolveHashOnRef( List relativeParts = parsed.map(ParsedHash::getRelativeParts).orElse(Collections.emptyList()); Optional resolved = Optional.of(startOrHead); - if (!Objects.equals(startOrHead, currentHead) || !relativeParts.isEmpty()) { - resolved = Optional.ofNullable(store.hashOnReference(ref, resolved, relativeParts)); + if (!relativeParts.isEmpty()) { + // Resolve the hash against DETACHED because we are only interested in + // resolving the hash, not checking if it is on the branch. This will + // be done later on. + resolved = + Optional.ofNullable(store.hashOnReference(DetachedRef.INSTANCE, resolved, relativeParts)); } return ResolvedHash.of(ref, resolved, Optional.ofNullable(currentHead)); } diff --git a/servers/services/src/testFixtures/java/org/projectnessie/services/impl/AbstractTestInvalidRefs.java b/servers/services/src/testFixtures/java/org/projectnessie/services/impl/AbstractTestInvalidRefs.java index 9d044146b1d..33ab2d3a501 100644 --- a/servers/services/src/testFixtures/java/org/projectnessie/services/impl/AbstractTestInvalidRefs.java +++ b/servers/services/src/testFixtures/java/org/projectnessie/services/impl/AbstractTestInvalidRefs.java @@ -37,29 +37,21 @@ public void testUnknownHashesOnValidNamedRefs() throws BaseNessieClientServerExc createCommits(branch, 1, commits, currentHash); assertThatThrownBy(() -> commitLog(branch.getName(), MINIMAL, null, invalidHash, null)) .isInstanceOf(NessieNotFoundException.class) - .hasMessageContaining( - String.format( - "Could not find commit '%s' in reference '%s'.", invalidHash, branch.getName())); + .hasMessageContaining(String.format("Commit '%s' not found", invalidHash)); assertThatThrownBy(() -> entries(branch.getName(), invalidHash)) .isInstanceOf(NessieNotFoundException.class) - .hasMessageContaining( - String.format( - "Could not find commit '%s' in reference '%s'.", invalidHash, branch.getName())); + .hasMessageContaining(String.format("Commit '%s' not found", invalidHash)); assertThatThrownBy(() -> contents(branch.getName(), invalidHash, ContentKey.of("table0"))) .isInstanceOf(NessieNotFoundException.class) - .hasMessageContaining( - String.format( - "Could not find commit '%s' in reference '%s'.", invalidHash, branch.getName())); + .hasMessageContaining(String.format("Commit '%s' not found", invalidHash)); assertThatThrownBy( () -> contentApi() .getContent(ContentKey.of("table0"), branch.getName(), invalidHash, false)) .isInstanceOf(NessieNotFoundException.class) - .hasMessageContaining( - String.format( - "Could not find commit '%s' in reference '%s'.", invalidHash, branch.getName())); + .hasMessageContaining(String.format("Commit '%s' not found", invalidHash)); } } From f1392f7855496ab95cbd401b2cc1c4a4fc2372d2 Mon Sep 17 00:00:00 2001 From: Alexandre Dutra Date: Tue, 1 Aug 2023 10:40:03 +0200 Subject: [PATCH 09/12] Add TODO for when Nessie lib will be >= 0.61 --- .../AbstractNessieSparkSqlExtensionTest.java | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/integrations/spark-extensions-basetests/src/main/java/org/projectnessie/spark/extensions/AbstractNessieSparkSqlExtensionTest.java b/integrations/spark-extensions-basetests/src/main/java/org/projectnessie/spark/extensions/AbstractNessieSparkSqlExtensionTest.java index 044d916e510..0798c61d59e 100644 --- a/integrations/spark-extensions-basetests/src/main/java/org/projectnessie/spark/extensions/AbstractNessieSparkSqlExtensionTest.java +++ b/integrations/spark-extensions-basetests/src/main/java/org/projectnessie/spark/extensions/AbstractNessieSparkSqlExtensionTest.java @@ -270,6 +270,11 @@ additionalRefName, defaultBranch(), currentHash)) "ASSIGN BRANCH %s TO %s AT %s IN nessie", additionalRefName, defaultBranch(), invalidHash)) .isInstanceOf(IllegalArgumentException.class) + // TODO enable this when Nessie lib will be >= 0.61 + // .hasMessage( + // Validation.HASH_OR_RELATIVE_COMMIT_SPEC_MESSAGE + // + " - but was: " + // + invalidHash) .hasMessageContaining(Validation.HASH_RULE) .hasMessageContaining(" - but was: " + invalidHash); assertThatThrownBy( @@ -319,6 +324,11 @@ additionalRefName, defaultBranch(), currentHash)) "ASSIGN TAG %s TO %s AT %s IN nessie", additionalRefName, defaultBranch(), invalidHash)) .isInstanceOf(IllegalArgumentException.class) + // TODO enable this when Nessie lib will be >= 0.61 + // .hasMessage( + // Validation.HASH_OR_RELATIVE_COMMIT_SPEC_MESSAGE + // + " - but was: " + // + invalidHash) .hasMessageContaining(Validation.HASH_RULE) .hasMessageContaining(" - but was: " + invalidHash); assertThatThrownBy( From 5b214dcec1f1c0ef7118b74debf364cb9cc218b1 Mon Sep 17 00:00:00 2001 From: Alexandre Dutra Date: Tue, 1 Aug 2023 13:17:17 +0200 Subject: [PATCH 10/12] Fix javadocs --- .../src/main/java/org/projectnessie/model/Validation.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/model/src/main/java/org/projectnessie/model/Validation.java b/api/model/src/main/java/org/projectnessie/model/Validation.java index 826f9fe6bbb..0486383007a 100644 --- a/api/model/src/main/java/org/projectnessie/model/Validation.java +++ b/api/model/src/main/java/org/projectnessie/model/Validation.java @@ -235,9 +235,9 @@ public static String validateHash(String referenceName) throws IllegalArgumentEx } /** - * Validates whether a string is a valid hash. + * Validates whether a string is a valid hash with optional relative specs. * - *

The rules are: {@value #HASH_RULE} + *

The rules are: {@value #HASH_OR_RELATIVE_COMMIT_SPEC_RULE} * * @param referenceName the reference name string to test. */ From 8e945f2520e53f63d1420046c5052764f0cc1297 Mon Sep 17 00:00:00 2001 From: Alexandre Dutra Date: Tue, 1 Aug 2023 15:49:53 +0200 Subject: [PATCH 11/12] Address review comments --- CHANGELOG.md | 2 +- .../api/v2/params/Transplant.java | 2 +- .../services/hash/HashResolver.java | 25 +++++------ ...HashValidators.java => HashValidator.java} | 44 +++++++++++++------ .../services/hash/ResolvedHash.java | 4 +- .../services/impl/BaseApiImpl.java | 6 ++- .../services/impl/ContentApiImpl.java | 2 +- .../services/impl/DiffApiImpl.java | 2 +- .../services/impl/NamespaceApiImpl.java | 2 +- .../services/impl/TreeApiImpl.java | 9 ++-- .../services/hash/TestParsedHash.java | 4 +- 11 files changed, 59 insertions(+), 43 deletions(-) rename servers/services/src/main/java/org/projectnessie/services/hash/{HashValidators.java => HashValidator.java} (54%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ba96ccb77e..0a9a5e36f5b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,13 +8,13 @@ as necessary. Empty sections will not end in the release notes. ### Highlights ### Upgrade notes -- This release upgrades the Nessie API spec to 2.1.1. ### Breaking changes ### New Features ### Changes +- Nessie API spec upgraded to 2.1.1 - Support for relative hashes has been standardized and is now allowed in all v2 endpoints ### Deprecations diff --git a/api/model/src/main/java/org/projectnessie/api/v2/params/Transplant.java b/api/model/src/main/java/org/projectnessie/api/v2/params/Transplant.java index 7292c5b1a1b..6e47a0556b5 100644 --- a/api/model/src/main/java/org/projectnessie/api/v2/params/Transplant.java +++ b/api/model/src/main/java/org/projectnessie/api/v2/params/Transplant.java @@ -73,7 +73,7 @@ public interface Transplant extends BaseMergeTransplant { String getMessage(); /** - * Lists the hashes of commits that should be transplanted into the target branch. + * The hashes of commits that should be transplanted into the target branch. * *

Since Nessie spec 2.1.1, hashes can be absolute or relative. */ diff --git a/servers/services/src/main/java/org/projectnessie/services/hash/HashResolver.java b/servers/services/src/main/java/org/projectnessie/services/hash/HashResolver.java index 0ecc52bac52..a122d83448f 100644 --- a/servers/services/src/main/java/org/projectnessie/services/hash/HashResolver.java +++ b/servers/services/src/main/java/org/projectnessie/services/hash/HashResolver.java @@ -17,13 +17,12 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkState; -import static org.projectnessie.services.hash.HashValidators.ANY_HASH; -import static org.projectnessie.services.hash.HashValidators.REQUIRED_UNAMBIGUOUS_HASH; +import static org.projectnessie.services.hash.HashValidator.ANY_HASH; +import static org.projectnessie.services.hash.HashValidator.REQUIRED_UNAMBIGUOUS_HASH; import java.util.Collections; import java.util.List; import java.util.Optional; -import java.util.function.BiConsumer; import javax.annotation.Nullable; import org.projectnessie.error.NessieReferenceNotFoundException; import org.projectnessie.model.CommitMeta; @@ -37,7 +36,7 @@ import org.projectnessie.versioned.RelativeCommitSpec; import org.projectnessie.versioned.VersionStore; -public class HashResolver { +public final class HashResolver { private final ServerConfig config; private final VersionStore store; @@ -72,16 +71,16 @@ public ResolvedHash resolveToHead(@Nullable @jakarta.annotation.Nullable String * *

If {@code namedRef} is null, the default branch will be used. * - *

The parameter {name} is used for error messages. + *

The parameter {@code name} is used for error messages. * *

A hash validator must be provided to perform extra validations on the parsed hashed. If no - * extra validation is required, {@link HashValidators#ANY_HASH} can be used. + * extra validation is required, {@link HashValidator#ANY_HASH} can be used. */ public ResolvedHash resolveHashOnRef( @Nullable @jakarta.annotation.Nullable String namedRef, @Nullable @jakarta.annotation.Nullable String hashOnRef, String name, - BiConsumer validator) + HashValidator validator) throws NessieReferenceNotFoundException { if (null == namedRef) { namedRef = config.getDefaultBranch(); @@ -113,13 +112,13 @@ public ResolvedHash resolveHashOnRef( *

This is useful to compute more hashes against a first resolved hash, e.g. when * transplanting. * - *

See {@link #resolveHashOnRef(String, String, String, BiConsumer)} for important caveats. + *

See {@link #resolveHashOnRef(String, String, String, HashValidator)} for important caveats. */ public ResolvedHash resolveHashOnRef( ResolvedHash head, @Nullable @jakarta.annotation.Nullable String hashOnRef, String name, - BiConsumer validator) + HashValidator validator) throws ReferenceNotFoundException { return resolveHashOnRef( head.getNamedRef(), head.getHead().orElse(null), hashOnRef, name, validator); @@ -132,21 +131,21 @@ public ResolvedHash resolveHashOnRef( *

Either {@code currentHead} or {@code hashOnRef} must be non-null. It's the caller's * responsibility to validate that any user-provided input meets this requirement. * - *

See {@link #resolveHashOnRef(String, String, String, BiConsumer)} for important caveats. + *

See {@link #resolveHashOnRef(String, String, String, HashValidator)} for important caveats. */ public ResolvedHash resolveHashOnRef( NamedRef ref, @Nullable @jakarta.annotation.Nullable Hash currentHead, @Nullable @jakarta.annotation.Nullable String hashOnRef, String name, - BiConsumer validator) + HashValidator validator) throws ReferenceNotFoundException { checkState(currentHead != null || hashOnRef != null); Optional parsed = ParsedHash.parse(hashOnRef, store.noAncestorHash()); if (ref == DetachedRef.INSTANCE) { - validator = validator.andThen(REQUIRED_UNAMBIGUOUS_HASH); + validator = validator.and(REQUIRED_UNAMBIGUOUS_HASH); } - validator.accept(name, parsed.orElse(null)); + validator.validate(name, parsed.orElse(null)); Hash startOrHead = parsed.flatMap(ParsedHash::getAbsolutePart).orElse(currentHead); checkState(startOrHead != null); List relativeParts = diff --git a/servers/services/src/main/java/org/projectnessie/services/hash/HashValidators.java b/servers/services/src/main/java/org/projectnessie/services/hash/HashValidator.java similarity index 54% rename from servers/services/src/main/java/org/projectnessie/services/hash/HashValidators.java rename to servers/services/src/main/java/org/projectnessie/services/hash/HashValidator.java index 986e3279e2c..3d100f8355f 100644 --- a/servers/services/src/main/java/org/projectnessie/services/hash/HashValidators.java +++ b/servers/services/src/main/java/org/projectnessie/services/hash/HashValidator.java @@ -18,34 +18,50 @@ import static com.google.common.base.Preconditions.checkArgument; import java.util.Objects; -import java.util.function.BiConsumer; +import javax.annotation.Nullable; -public final class HashValidators { +@FunctionalInterface +public interface HashValidator { - private HashValidators() {} - - /** Validates any parseable hash. */ - public static final BiConsumer ANY_HASH = (name, parsed) -> {}; + /** No-op, for any parseable hash. */ + HashValidator ANY_HASH = (name, parsed) -> {}; /** Validates that a hash has been provided (absolute or relative). */ - public static final BiConsumer REQUIRED_HASH = + HashValidator REQUIRED_HASH = (name, parsed) -> Objects.requireNonNull(parsed, String.format("%s must be provided.", name)); /** - * Validates that, if a hash was provided, it is unambiguous. A hash is unambiguous if it has an - * absolute part, because it will always resolve to the same hash, even if it also has relative - * parts. + * Validates that, if a hash was provided, it is unambiguous. A hash is unambiguous if it starts + * with an absolute part, because it will always resolve to the same hash, even if it also has + * relative parts. */ - public static final BiConsumer UNAMBIGUOUS_HASH = + HashValidator UNAMBIGUOUS_HASH = (name, parsed) -> checkArgument( parsed == null || parsed.getAbsolutePart().isPresent(), String.format("%s must contain a starting commit ID.", name)); /** - * Validates that a hash has been provided, and that it is unambiguous (that is, it contains an + * Validates that a hash has been provided, and that it is unambiguous (that is, it starts with an * absolute part). */ - public static final BiConsumer REQUIRED_UNAMBIGUOUS_HASH = - REQUIRED_HASH.andThen(UNAMBIGUOUS_HASH); + HashValidator REQUIRED_UNAMBIGUOUS_HASH = REQUIRED_HASH.and(UNAMBIGUOUS_HASH); + + /** + * Validates the provided hash. + * + * @param name the name of the hash parameter, for error messages + * @param parsed the parsed hash, or {@code null} if no hash was provided + */ + void validate(String name, @Nullable @jakarta.annotation.Nullable ParsedHash parsed); + + /** + * Returns a new {@link HashValidator} that validates against both {@code this} and {@code other}. + */ + default HashValidator and(HashValidator other) { + return (name, parsed) -> { + validate(name, parsed); + other.validate(name, parsed); + }; + } } diff --git a/servers/services/src/main/java/org/projectnessie/services/hash/ResolvedHash.java b/servers/services/src/main/java/org/projectnessie/services/hash/ResolvedHash.java index e2c8e3c1c6f..5ed0006b3fe 100644 --- a/servers/services/src/main/java/org/projectnessie/services/hash/ResolvedHash.java +++ b/servers/services/src/main/java/org/projectnessie/services/hash/ResolvedHash.java @@ -56,8 +56,8 @@ public interface ResolvedHash extends WithHash { Optional getHead(); /** - * The effective resolved hash; never null. Will either be {@link #getProvidedHash()} if present, - * or otherwise, {@link #getHead()}. + * The effective resolved hash, never {@code null}. Will either be {@link #getProvidedHash()} if + * present, or {@link #getHead()} otherwise. */ @Override Hash getHash(); diff --git a/servers/services/src/main/java/org/projectnessie/services/impl/BaseApiImpl.java b/servers/services/src/main/java/org/projectnessie/services/impl/BaseApiImpl.java index b283f38f2cb..1175b7e97ee 100644 --- a/servers/services/src/main/java/org/projectnessie/services/impl/BaseApiImpl.java +++ b/servers/services/src/main/java/org/projectnessie/services/impl/BaseApiImpl.java @@ -49,7 +49,7 @@ public abstract class BaseApiImpl { private final VersionStore store; private final Authorizer authorizer; private final Supplier principal; - private final HashResolver hashResolver; + private HashResolver hashResolver; protected BaseApiImpl( ServerConfig config, @@ -60,7 +60,6 @@ protected BaseApiImpl( this.store = store; this.authorizer = authorizer; this.principal = principal; - this.hashResolver = new HashResolver(config, store); } /** @@ -111,6 +110,9 @@ protected Authorizer getAuthorizer() { } protected HashResolver getHashResolver() { + if (hashResolver == null) { + this.hashResolver = new HashResolver(config, store); + } return hashResolver; } diff --git a/servers/services/src/main/java/org/projectnessie/services/impl/ContentApiImpl.java b/servers/services/src/main/java/org/projectnessie/services/impl/ContentApiImpl.java index b8bbfaeb273..f812860dcef 100644 --- a/servers/services/src/main/java/org/projectnessie/services/impl/ContentApiImpl.java +++ b/servers/services/src/main/java/org/projectnessie/services/impl/ContentApiImpl.java @@ -15,7 +15,7 @@ */ package org.projectnessie.services.impl; -import static org.projectnessie.services.hash.HashValidators.ANY_HASH; +import static org.projectnessie.services.hash.HashValidator.ANY_HASH; import java.security.Principal; import java.util.List; diff --git a/servers/services/src/main/java/org/projectnessie/services/impl/DiffApiImpl.java b/servers/services/src/main/java/org/projectnessie/services/impl/DiffApiImpl.java index a97ca609911..cc8375d3759 100644 --- a/servers/services/src/main/java/org/projectnessie/services/impl/DiffApiImpl.java +++ b/servers/services/src/main/java/org/projectnessie/services/impl/DiffApiImpl.java @@ -18,7 +18,7 @@ import static java.util.Collections.singleton; import static org.projectnessie.services.authz.Check.canReadContentKey; import static org.projectnessie.services.authz.Check.canViewReference; -import static org.projectnessie.services.hash.HashValidators.ANY_HASH; +import static org.projectnessie.services.hash.HashValidator.ANY_HASH; import com.google.common.base.Strings; import com.google.common.collect.ImmutableSet; diff --git a/servers/services/src/main/java/org/projectnessie/services/impl/NamespaceApiImpl.java b/servers/services/src/main/java/org/projectnessie/services/impl/NamespaceApiImpl.java index cb84ec4558b..3a7abc8a497 100644 --- a/servers/services/src/main/java/org/projectnessie/services/impl/NamespaceApiImpl.java +++ b/servers/services/src/main/java/org/projectnessie/services/impl/NamespaceApiImpl.java @@ -19,7 +19,7 @@ import static java.util.stream.StreamSupport.stream; import static org.projectnessie.error.ContentKeyErrorDetails.contentKeyErrorDetails; import static org.projectnessie.model.Validation.validateHash; -import static org.projectnessie.services.hash.HashValidators.ANY_HASH; +import static org.projectnessie.services.hash.HashValidator.ANY_HASH; import static org.projectnessie.services.impl.RefUtil.toReference; import static org.projectnessie.versioned.VersionStore.KeyRestrictions.NO_KEY_RESTRICTIONS; diff --git a/servers/services/src/main/java/org/projectnessie/services/impl/TreeApiImpl.java b/servers/services/src/main/java/org/projectnessie/services/impl/TreeApiImpl.java index e5186f9c89a..e5931d24931 100644 --- a/servers/services/src/main/java/org/projectnessie/services/impl/TreeApiImpl.java +++ b/servers/services/src/main/java/org/projectnessie/services/impl/TreeApiImpl.java @@ -40,9 +40,9 @@ import static org.projectnessie.services.cel.CELUtil.VAR_REF; import static org.projectnessie.services.cel.CELUtil.VAR_REF_META; import static org.projectnessie.services.cel.CELUtil.VAR_REF_TYPE; -import static org.projectnessie.services.hash.HashValidators.ANY_HASH; -import static org.projectnessie.services.hash.HashValidators.REQUIRED_UNAMBIGUOUS_HASH; -import static org.projectnessie.services.hash.HashValidators.UNAMBIGUOUS_HASH; +import static org.projectnessie.services.hash.HashValidator.ANY_HASH; +import static org.projectnessie.services.hash.HashValidator.REQUIRED_UNAMBIGUOUS_HASH; +import static org.projectnessie.services.hash.HashValidator.UNAMBIGUOUS_HASH; import static org.projectnessie.services.impl.RefUtil.toNamedRef; import com.google.common.base.Strings; @@ -266,7 +266,7 @@ public Reference createReference( } BatchAccessChecker check = startAccessCheck().canCreateReference(namedReference); - Optional targetHashObj = Optional.empty(); + Optional targetHashObj; try { ResolvedHash targetRef = getHashResolver() @@ -282,6 +282,7 @@ public Reference createReference( && (null == targetHash || getStore().noAncestorHash().asString().equals(targetHash)))) { throw e; } + targetHashObj = Optional.empty(); } check.checkAndThrow(); diff --git a/servers/services/src/test/java/org/projectnessie/services/hash/TestParsedHash.java b/servers/services/src/test/java/org/projectnessie/services/hash/TestParsedHash.java index a0011150b04..7e2ca195adb 100644 --- a/servers/services/src/test/java/org/projectnessie/services/hash/TestParsedHash.java +++ b/servers/services/src/test/java/org/projectnessie/services/hash/TestParsedHash.java @@ -65,8 +65,6 @@ void parseBadCases(String hashOrRelativeSpec) { @Test void parseNoAncestor() { Optional parsed = ParsedHash.parse(NO_ANCESTOR.asString() + "~1", NO_ANCESTOR); - assertThat(parsed).isPresent(); - assertThat(parsed.get().getAbsolutePart()).isPresent(); - assertThat(parsed.get().getAbsolutePart().get()).isSameAs(NO_ANCESTOR); + assertThat(parsed.flatMap(ParsedHash::getAbsolutePart)).containsSame(NO_ANCESTOR); } } From 4d41eeac1261b126d719b2228f7d1b7d871a7a40 Mon Sep 17 00:00:00 2001 From: Alexandre Dutra Date: Tue, 1 Aug 2023 17:56:17 +0200 Subject: [PATCH 12/12] More code reviews --- .../main/java/org/projectnessie/services/hash/ResolvedHash.java | 2 +- .../main/java/org/projectnessie/services/impl/TreeApiImpl.java | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/servers/services/src/main/java/org/projectnessie/services/hash/ResolvedHash.java b/servers/services/src/main/java/org/projectnessie/services/hash/ResolvedHash.java index 5ed0006b3fe..bf498b604cf 100644 --- a/servers/services/src/main/java/org/projectnessie/services/hash/ResolvedHash.java +++ b/servers/services/src/main/java/org/projectnessie/services/hash/ResolvedHash.java @@ -62,7 +62,7 @@ public interface ResolvedHash extends WithHash { @Override Hash getHash(); - @Value.Derived + @Value.NonAttribute @Override default NamedRef getValue() { return getNamedRef(); diff --git a/servers/services/src/main/java/org/projectnessie/services/impl/TreeApiImpl.java b/servers/services/src/main/java/org/projectnessie/services/impl/TreeApiImpl.java index e5931d24931..4893ad058b2 100644 --- a/servers/services/src/main/java/org/projectnessie/services/impl/TreeApiImpl.java +++ b/servers/services/src/main/java/org/projectnessie/services/impl/TreeApiImpl.java @@ -388,7 +388,6 @@ public R getCommitLog( String pageToken, PagedResponseHandler pagedResponseHandler) throws NessieNotFoundException { - // FIXME we should only allow named references when no paging is defined try {