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

Add conflict response and use it when creating an existing resource #1900

Merged
merged 4 commits into from
May 6, 2021

Conversation

itaiad200
Copy link
Contributor

closes #1898

@itaiad200 itaiad200 requested a review from nopcoder May 6, 2021 11:04
@ozkatz
Copy link
Collaborator

ozkatz commented May 6, 2021

@itaiad200 you're checking in the webui/build dir - I'm assuming you didn't intend to do that.
I think this happened because vite now uses webui/dist instead, so webui/build is no longer part of .gitignore. Unintended consequences from my PR..

Copy link
Collaborator

@ozkatz ozkatz left a comment

Choose a reason for hiding this comment

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

Please remove webui/build and fix auth/service - Other than that, looks good! :)

pkg/db/tx.go Outdated
@@ -121,6 +121,9 @@ func (d *dbTx) Exec(query string, args ...interface{}) (pgconn.CommandTag, error
"query": queryToString(query),
"took": time.Since(start),
})
if IsUniqueViolation(err) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This breaks pkg/auth/service.go - there are several places that check for db.IsUniqueViolation(..) - Having a good error value is indeed better but please fix the auth service to check for this error instead..
Better yet, make this check unexported to make sure no-one else is relying on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -1515,6 +1504,11 @@ func handleAPIError(w http.ResponseWriter, err error) bool {
errors.Is(err, model.ErrValidationError):
writeError(w, http.StatusBadRequest, err)

case errors.Is(err, graveler.ErrBranchExists),
errors.Is(err, graveler.ErrTagAlreadyExists),
errors.Is(err, graveler.ErrNotUnique):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be more readable if ErrBranchExists and ErrTagAlreadyExists were both wrapping graveler.ErrNotUnique - could make this check a little more generic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

if err != nil {
if errors.Is(err, db.ErrAlreadyExists) {
return nil, graveler.ErrNotUnique
Copy link
Collaborator

Choose a reason for hiding this comment

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

We rely on graveler to now return ErrAlreadyExists for these methods - I think the interface should document that (i.e. an implementation of it is now expected to return this error for duplicate repos, branches, tags).
Go makes this pretty hard to express in code, so let's at least document it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -1718,6 +1726,8 @@ paths:
$ref: "#/components/responses/Unauthorized"
404:
$ref: "#/components/responses/NotFound"
409:
Copy link
Collaborator

Choose a reason for hiding this comment

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

In Git you can git tag --force to overwrite an existing tag - In lakectl this happened implicitly for all tag creations, but it won't anymore. Maybe add --force to the lakcetl command? (could simply delete and recreate it to simulate the current behavior).

(This could be done in another PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@itaiad200 itaiad200 requested a review from ozkatz May 6, 2021 12:22
Copy link
Collaborator

@ozkatz ozkatz left a comment

Choose a reason for hiding this comment

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

I think there's a bug in error handling for lakectl tags create --force..

DieOnResponseError(res, err)

resp, err := client.DeleteTagWithResponse(ctx, tagURI.Repository, tagURI.Ref)
if err != nil && resp != nil && resp.JSON404 == nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't you want to die if there's an error but the response is nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, done.

@@ -236,6 +242,10 @@ components:
size_bytes:
type: integer
format: int64
mtime:
Copy link
Collaborator

Choose a reason for hiding this comment

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

you're going to publish an API that hasn't been released yet (well, same for yours)...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted.

@itaiad200 itaiad200 force-pushed the 1898-branch-exists-error branch from c66eb86 to 36254b6 Compare May 6, 2021 13:41
@itaiad200 itaiad200 requested a review from ozkatz May 6, 2021 13:41
Copy link
Collaborator

@ozkatz ozkatz left a comment

Choose a reason for hiding this comment

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

Looks great, thanks :)

@itaiad200 itaiad200 merged commit 4ea6571 into master May 6, 2021
@itaiad200 itaiad200 deleted the 1898-branch-exists-error branch May 6, 2021 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CreateBranch "branch already exists" error is returned as 500
2 participants