Extend relative hash support to whole API v2 #7308

adutra · 2023-07-31T14:19:59Z

Changes are dispatched in different commits for easier reviews.

Highlights:

A new component, org.projectnessie.services.hash.HashResolver is responsible for resolving all hashes. It replaces the namedRefWithHashOrThrow method previously in BaseApiImpl.
Most changes are in TreeApiImpl. The most difficult endpoints to adapt were assignReference and deleteReference because of their strict checking of the expected hash.
- Note that because TreeApiImpl is shared between v1 and v2, some changes may affect v1's behavior (but not its API). Since v1 is deprecated at this point, I think this is acceptable.
The REST layer is designed in such a way that relative hashes are accepted even if they are not particularly useful, or even desired, in certain cases. This is by design to avoid returning cryptic validation errors to clients.
As a general rule, ambiguous hases are only accepted in reading operations (see below for examples).
A (hopefully) thorough set of tests can be found in org.projectnessie.jaxrs.tests.AbstractRelativeReferences.

Note on hash ambiguity: it is my understanding that, in order for a hash to be unambiguous (i.e. it will be resolved deterministically), it is enough for it to have an absolute part (aka start commit ID). This is valid also for hashes with timestamps, including those in the future, because no relative spec can be resolved to a child of the start commit, only its parents. Thus the following are all unambiguous:

1234
1234~1
1234^1~2
1234*1970-01-01T00:00:00.000Z
1234*2050-01-01T00:00:00.000Z => will always resolve to 1234, assuming 1234 was committed before 2050.

The following however are considered ambiguous and thus rejected for all writing operations:

~1
^1~2
*1970-01-01T00:00:00.000Z
*2050-01-01T00:00:00.000Z

Given the above conclusions, this PR makes no special treatment for clock drifts.

adutra · 2023-07-31T14:26:38Z

servers/jax-rs-tests/src/main/java/org/projectnessie/jaxrs/tests/BaseTestNessieApi.java

    AbstractThrowableAssert<?, ? extends Throwable> deleteConflict =
        isV2()
            ? soft.assertThatThrownBy(() -> api().deleteBranch().branch(branch).getAndDelete())
            : soft.assertThatThrownBy(() -> api().deleteBranch().branch(branch).delete());
+    deleteConflict


This is the only test that failed after the changes. It is testing an edge case: the branch is at c1, but the provided expected hash is c2.

Previously there was no hash resolution, so the endpoint would throw a CONFLICT error since c2 is not the expected hash.

With the new hash resolution in place, the endpoint now throws NOT_FOUND because c2 is not reachable from c1 at all.

I added another test case below that generates a CONFLICT as before.

I'm not sure about the new behavour here. In this case, the hash is valid, but it is not in the current commit chain of the reference. This is exactly the case when one client assigns the reference, while another client attempts to delete it using the old HEAD. I tend to think that a "conflict" is more appropriate from the high-level perspective.

We seems to have two general approaches. Read operations throw "not found" when the hash is not found on the branch. Write operations throw "conflict" when the "expected" hash does not match server-side expectations. The relative hash case kind of mixes both cases.

Since write operations now require a concrete base hash, could we make the resolution more lenient and defer the (final) exception throwing to the highest-level method that knows the operation's end-user semantics?

On the other hand, all non-trivial unambiguous relative hash references resolve to something other than HEAD. Perhaps, we could simplify the parsing of the expected hash parameter for assign/deleteReference requests and respond with a "bad request" if a relative spec is used. If the hash is absolute we could avoid checking it against the history, but do "conflict" by comparing it to current HEAD right away (also requires less backend work).

Your comments are 100% spot on. I hesitated a long time myself before picking one solution.

To summarize, we'd have 3 ways to move forward with this issue:

Accept the behavior change: if c2 is not in main, then main@c2 should legitimately throw not-found.

Use DETACHED to resolve relative hashes.
a. Using DETACHED, VersionStore.hashOnReference() will simply lookup the commit but won't validate that it belongs to any branch.

respond with a "bad request" if a relative spec is used in assignReference or deleteReference
a. This would work too, but it might not solve the issue completely because instead of conflict, we would throw a bad-request error.

Regarding option 2: this was actually my first idea. It solves the issue at hand, and doesn't seem to have any concerning downsides. I gave up on it, thinking that conceptually, option 1 was better than 2... but now I am not sure anymore :-)

Let's try option 2 first and see how it goes. If it doesn't look good, then I will go with your suggestion (option 3).

@dimas-b Seems like CI is happy with option 2!

...ts/src/main/java/org/projectnessie/spark/extensions/AbstractNessieSparkSqlExtensionTest.java

api/model/src/main/java/org/projectnessie/model/Validation.java

dimas-b · 2023-08-01T00:14:24Z

servers/jax-rs-tests/src/main/java/org/projectnessie/jaxrs/tests/BaseTestNessieApi.java

    AbstractThrowableAssert<?, ? extends Throwable> deleteConflict =
        isV2()
            ? soft.assertThatThrownBy(() -> api().deleteBranch().branch(branch).getAndDelete())
            : soft.assertThatThrownBy(() -> api().deleteBranch().branch(branch).delete());
+    deleteConflict


I'm not sure about the new behavour here. In this case, the hash is valid, but it is not in the current commit chain of the reference. This is exactly the case when one client assigns the reference, while another client attempts to delete it using the old HEAD. I tend to think that a "conflict" is more appropriate from the high-level perspective.

We seems to have two general approaches. Read operations throw "not found" when the hash is not found on the branch. Write operations throw "conflict" when the "expected" hash does not match server-side expectations. The relative hash case kind of mixes both cases.

Since write operations now require a concrete base hash, could we make the resolution more lenient and defer the (final) exception throwing to the highest-level method that knows the operation's end-user semantics?

On the other hand, all non-trivial unambiguous relative hash references resolve to something other than HEAD. Perhaps, we could simplify the parsing of the expected hash parameter for assign/deleteReference requests and respond with a "bad request" if a relative spec is used. If the hash is absolute we could avoid checking it against the history, but do "conflict" by comparing it to current HEAD right away (also requires less backend work).

dimas-b · 2023-08-01T00:47:15Z

servers/services/src/main/java/org/projectnessie/services/impl/NamespaceApiImpl.java

@@ -82,7 +82,7 @@ public Namespace createNamespace(String refName, Namespace namespace)
      throws NessieReferenceNotFoundException {
    Preconditions.checkArgument(!namespace.isEmpty(), "Namespace name must not be empty");

-    WithHash<NamedRef> refWithHash = namedRefWithHashOrThrow(refName, null);
+    ResolvedHash refWithHash = getHashResolver().resolveToHead(refName);


The Namespaces endpoint is not supported in API v2, so relative hashes normally cannot make their way here. It's probably a moot point, but if there is an easier way to get HEAD, it might be worth using it in this class.

The change here is purely cosmetic. getHashResolver().resolveToHead(refName) does exactly the same thing as namedRefWithHashOrThrow(refName, null);. But the new name is more meaningful imho. Do you see any other easier way to resolve the HEAD of a ref?

adutra · 2023-08-01T08:58:07Z

.../services/src/testFixtures/java/org/projectnessie/services/impl/AbstractTestInvalidRefs.java

-        .hasMessageContaining(
-            String.format(
-                "Could not find commit '%s' in reference '%s'.", invalidHash, branch.getName()));
+        .hasMessageContaining(String.format("Commit '%s' not found", invalidHash));


@dimas-b FYI this is the only downside so far of resolving hashes against DETACHED: the error returned is still not-found but the message changes slightly.

CHANGELOG.md

...ts/src/main/java/org/projectnessie/spark/extensions/AbstractNessieSparkSqlExtensionTest.java

servers/services/src/test/java/org/projectnessie/services/hash/TestParsedHash.java

...rs/quarkus-server/src/test/java/org/projectnessie/server/TestQuarkusRestInMemoryPersist.java

servers/services/src/main/java/org/projectnessie/services/hash/HashResolver.java

snazy · 2023-08-01T12:06:40Z

servers/services/src/main/java/org/projectnessie/services/impl/TreeApiImpl.java

-    // we should only allow named references when no paging is defined
-    WithHash<NamedRef> endRef =
-        namedRefWithHashOrThrow(namedRef, null == pageToken ? youngestHash : pageToken);
+    // FIXME we should only allow named references when no paging is defined


TBH; I'm not sure that comment is still valid (aka: I think, it can just go away)

I thought about that too and came to the conclusion that it could still be relevant. If the named reference is reassigned between two pages to a completely different hash, paging could break I guess. Wdyt?

Paging for commit-log uses a commit-hash passed around as the offset for the next page. It could be an issue when the reference is deleted, but then it's deleted, so fine to fail IMO.

OK let's remove the comment then.

servers/services/src/main/java/org/projectnessie/services/hash/HashValidators.java

snazy · 2023-08-01T15:12:32Z

api/model/src/main/java/org/projectnessie/api/v2/params/Transplant.java

  @NotNull
  @jakarta.validation.constraints.NotNull
  @Size
  @jakarta.validation.constraints.Size(min = 1)
-  List<String> getHashesToTransplant();
+  List<
+          @Pattern(


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

snazy · 2023-08-01T15:13:31Z

servers/services/src/main/java/org/projectnessie/services/hash/HashValidator.java

+import javax.annotation.Nullable;
+
+@FunctionalInterface
+public interface HashValidator {


Thanks! This looks much better!

snazy · 2023-08-01T15:15:32Z

servers/services/src/main/java/org/projectnessie/services/hash/ResolvedHash.java

+  @Override
+  Hash getHash();
+
+  @Value.Derived


Suggested change

@Value.Derived

@Value.NonAttribute

otherwise the value would be stored twice (in two fields): in WithHash and in this one.

snazy · 2023-08-01T15:19:00Z

servers/services/src/main/java/org/projectnessie/services/impl/TreeApiImpl.java

-    // we should only allow named references when no paging is defined
-    WithHash<NamedRef> endRef =
-        namedRefWithHashOrThrow(namedRef, null == pageToken ? youngestHash : pageToken);
+    // FIXME we should only allow named references when no paging is defined


Paging for commit-log uses a commit-hash passed around as the offset for the next page. It could be an issue when the reference is deleted, but then it's deleted, so fine to fail IMO.

snazy

LGTM! Thanks!

The assertion was modified in projectnessie#7308 with a workaround to avoid compilation failures. This commit reintroduces the right assertion, which became possible when Iceberg updated Nessie to 0.61+.

This commit removes the workaround introduced in #7308. The workaround is not required anymore, now that this issue has been fixed: quarkusio/quarkus#35104

The assertion was modified in projectnessie#7308 with a workaround to avoid compilation failures. This commit reintroduces the right assertion, which became possible when Iceberg updated Nessie to 0.61+.

adutra commented Jul 31, 2023

View reviewed changes

...ts/src/main/java/org/projectnessie/spark/extensions/AbstractNessieSparkSqlExtensionTest.java Show resolved Hide resolved

adutra commented Jul 31, 2023

View reviewed changes

api/model/src/main/java/org/projectnessie/model/Validation.java Show resolved Hide resolved

dimas-b reviewed Aug 1, 2023

View reviewed changes

adutra added 9 commits August 1, 2023 10:42

Changes to the model API

3dc398f

Changes to Service layer

8746ea7

Changes to Nessie specs

e4afc0b

Tests for relative hashes

243db21

Update CHANGELOG.md

c8ffca1

Attempt to fix NesqueIT failures

66b10eb

Fix validation messages

ed99173

Resolve hashes against DETACHED

6f4a1bc

Add TODO for when Nessie lib will be >= 0.61

f1392f7

adutra force-pushed the relative-hash-support branch from b22da40 to f1392f7 Compare August 1, 2023 08:45

adutra commented Aug 1, 2023

View reviewed changes

Fix javadocs

5b214dc

snazy reviewed Aug 1, 2023

View reviewed changes

Address review comments

8e945f2

snazy reviewed Aug 1, 2023

View reviewed changes

More code reviews

4d41eea

snazy approved these changes Aug 1, 2023

View reviewed changes

adutra merged commit fac4ad0 into projectnessie:main Aug 1, 2023

adutra deleted the relative-hash-support branch August 1, 2023 16:43

adutra added a commit that referenced this pull request Aug 4, 2023

Remove workaround for Quarkus #35104 (#7315)

2831276

This commit removes the workaround introduced in #7308. The workaround is not required anymore, now that this issue has been fixed: quarkusio/quarkus#35104

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend relative hash support to whole API v2 #7308

Extend relative hash support to whole API v2 #7308

adutra commented Jul 31, 2023 •

edited

Loading

adutra Jul 31, 2023

dimas-b Aug 1, 2023

adutra Aug 1, 2023

adutra Aug 1, 2023

dimas-b Aug 1, 2023

dimas-b Aug 1, 2023

adutra Aug 1, 2023

adutra Aug 1, 2023

dimas-b Aug 7, 2023

snazy Aug 1, 2023

adutra Aug 1, 2023

snazy Aug 1, 2023

adutra Aug 1, 2023

snazy Aug 1, 2023

snazy Aug 1, 2023

snazy Aug 1, 2023

snazy Aug 1, 2023

snazy left a comment

Extend relative hash support to whole API v2 #7308

Extend relative hash support to whole API v2 #7308

Conversation

adutra commented Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snazy left a comment

Choose a reason for hiding this comment

adutra commented Jul 31, 2023 •

edited

Loading