Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Extend relative hash support to whole API v2 #7308

Merged
merged 12 commits into from
Aug 1, 2023
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ as necessary. Empty sections will not end in the release notes.
### 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

Expand Down
12 changes: 12 additions & 0 deletions api/NESSIE-SPEC-2-0.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -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"),
Expand All @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,16 @@
*/
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;
import org.eclipse.microprofile.openapi.annotations.media.ExampleObject;
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
Expand All @@ -35,10 +37,12 @@ public class CommitLogParams extends AbstractParams<CommitLogParams> {

@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 "
Expand Down
23 changes: 14 additions & 9 deletions api/model/src/main/java/org/projectnessie/api/v2/params/Merge.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
*
* <p>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);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -71,23 +72,35 @@ public interface Transplant extends BaseMergeTransplant {
@jakarta.validation.constraints.Size(min = 1)
String getMessage();

/**
* The hashes of commits that should be transplanted into the target branch.
*
* <p>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<String> getHashesToTransplant();
List<
@Pattern(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noting: I think I came across a constellation where annotated generic types didn't work. But since there's a test for this, it seems to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I manually validated that this annotation works both with Jaxrs/Weld and Quarkus.

(For Jaxrs, I added Hibernate Validator recently in #7193, for Quarkus it's already present in quarkus-common.)

The problem is, the bean is being validated by the @Check method too, and that method is executed first.

If you remove the @Check method, then you would observe how Hibernate Validator validates that:

transplantCommitsIntoBranch.transplant.hashesToTransplant[0].<list element>: Hash with optional relative part must consist of either ... [etc.]

To be fair, I think we don't need the @Check method anymore, and besides, the HV error message is more explicit because it mentions where the problem was found. But maybe keeping it is safer, because we own the validation code. Wdyt?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since HV is doing the job, I'm okay removing the check-method

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK but let's do it in a follow-up PR. I fear that some tests could fail, e.g. if they expect an exact error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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<String> hashes = getHashesToTransplant();
if (hashes != null) {
for (String hash : hashes) {
validateNoRelativeSpec(hash);
validateHash(hash);
validateHashOrRelativeSpec(hash);
}
}
}
Expand Down
15 changes: 12 additions & 3 deletions api/model/src/main/java/org/projectnessie/model/Detached.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,15 @@
*/
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;
import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
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;
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
}

Expand Down
68 changes: 41 additions & 27 deletions api/model/src/main/java/org/projectnessie/model/Validation.java
Original file line number Diff line number Diff line change
Expand Up @@ -124,27 +124,25 @@ public final class Validation {
public static final String HASH_MESSAGE = "Hash must " + HASH_RULE;

public static final String RELATIVE_COMMIT_SPEC_RULE =
snazy marked this conversation as resolved.
Show resolved Hide resolved
"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 a valid commit hash ("
"consist of either a valid commit hash (which in turn must "
+ HASH_RULE
+ "), optionally followed by a "
+ RELATIVE_COMMIT_SPEC_RULE;
+ "), or a valid relative part (which must "
+ 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 "
+ 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'";
Expand Down Expand Up @@ -183,6 +181,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)}).
Expand Down Expand Up @@ -221,6 +234,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 with optional relative specs.
*
* <p>The rules are: <em>{@value #HASH_OR_RELATIVE_COMMIT_SPEC_RULE}</em>
*
* @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");
Expand Down Expand Up @@ -280,19 +309,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);
}
}
}
Loading