added support for put-if-absent operations #1823
Conversation
| @@ -70,6 +70,12 @@ components: | |||
| application/json: | |||
| schema: | |||
| $ref: "#/components/schemas/Error" | |||
| PreconditionFailed: | |||
| // before writing body, ensure preconditions - this means we essentially check for object existence twice: | ||
| // once before uploading the body to save resources and time, | ||
| // and then graveler will check again when passed a WriteCondition. | ||
| allowOverwrite := true |
There was a problem hiding this comment.
- Swagger validator should catch invalid value for
IfNoneMatch- part of the JSON schema validation (if not ignore this comment) - Why do we need to write code at this level to verify the existence and not just pass the boolean value to the underlying catalog? if WriteCondition members were public this will be easier to pass in this case.
There was a problem hiding this comment.
The idea is to test this specific condition outside of Graveler - why wait for the user to upload the body only to fail at CreateEntry if I already know this write is going to fail?
pkg/api/controller_test.go
Outdated
|
|
||
| t.Run("overwrite", func(t *testing.T) { | ||
| // write | ||
| contentType, buf := writeMultipart("content", "bar", "hello world!") |
There was a problem hiding this comment.
Since this is a separate test, you shouldn't count on the object from the previous test already being written. I would upload the object twice here.
| @@ -25,7 +25,7 @@ func TestSetGet(t *testing.T) { | |||
| t.Fatalf("error different than expected. expected=%v, got=%v", graveler.ErrNotFound, err) | |||
| } | |||
| value := newTestValue("identity1", "value1") | |||
| err = s.Set(ctx, "t1", []byte("a/b/c/"), value) | |||
| err = s.Set(ctx, "t1", []byte("a/b/c/"), value, true) | |||
There was a problem hiding this comment.
Can we add a case to this test with overwrite=false?
| required: false | ||
| schema: | ||
| type: string | ||
| pattern: '^\*$' # Currently, only "*" is supported |
There was a problem hiding this comment.
@nopcoder I hope our generated clients don't validate. Otherwise when we add other values, an old client library won't work with an application that wants to use a non-* value.
| @@ -1714,7 +1736,12 @@ func (c *Controller) UploadObject(w http.ResponseWriter, r *http.Request, reposi | |||
| Size: blob.Size, | |||
| Checksum: blob.Checksum, | |||
| } | |||
| err = c.Catalog.CreateEntry(ctx, repo.Name, branch, entry) | |||
|
|
|||
| err = c.Catalog.CreateEntry(ctx, repo.Name, branch, entry, graveler.IfAbsent(!allowOverwrite)) | |||
There was a problem hiding this comment.
Cool, double-checked locking actually works here! (The previous test is only an optimization, but still nice to know.)
| @@ -671,7 +671,7 @@ func (c *Catalog) CreateEntry(ctx context.Context, repository string, branch str | |||
| if err != nil { | |||
| return err | |||
| } | |||
| return c.Store.Set(ctx, repositoryID, branchID, key, *value) | |||
| return c.Store.Set(ctx, repositoryID, branchID, key, *value, writeConditions...) | |||
There was a problem hiding this comment.
Style nit: no real reason to have varargs here, this API only ever gets generated slices (here on the server side). varargs now means we cannot varargs later, when we might really want it.
No description provided.