-
Notifications
You must be signed in to change notification settings - Fork 134
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
Changes from 1 commit
3dc398f
8746ea7
e4afc0b
243db21
c8ffca1
66b10eb
ed99173
6f4a1bc
f1392f7
5b214dc
8e945f2
4d41eea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. LGTM 👍 |
||
|
||
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)); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
main@c2
should legitimately throw not-found.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.assignReference
ordeleteReference
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dimas-b Seems like CI is happy with option 2!