Task: Add IfAbsent to LinkPhysicalAddress #7480

N-o-Z · 2024-02-20T10:11:57Z

Closes #7479

Change Description

THIS PR REINTRODUCES SET IF-NON-MATCH to the upload object API

Background

Add IfAbsent to support LinkPhysicalAddress failure if path already exists

Testing Details

Added unit test

Breaking Change?

No

N-o-Z · 2024-02-20T10:12:12Z

Need 1 reviewer, thanks!

github-actions · 2024-02-20T10:13:02Z

♻️ PR Preview 4289191 has been successfully destroyed since this PR has been closed.

_{🤖 By surge-preview}

github-actions · 2024-02-20T10:18:47Z

E2E Test Results - DynamoDB Local - Local Block Adapter

arielshaqed

Thanks!

arielshaqed · 2024-02-20T10:18:25Z

api/swagger.yml

+        if_absent:
+          type: boolean
+          default: false


We use the HTTP header If-Match elsewhere in this API, and I believe that we should continue to do the same and use the HTTP header If-None-Match here. The only legal value will be *, of course.

Done + removed deprecation for post object

arielshaqed · 2024-02-20T10:19:55Z

pkg/graveler/graveler.go

@@ -1711,7 +1711,10 @@ func (g *Graveler) Set(ctx context.Context, repository *RepositoryRecord, branch

 		// verify the key not found
 		_, err := g.Get(ctx, repository, Ref(branchID), key)
-		if err == nil || !errors.Is(err, ErrNotFound) {
+		if err == nil { // Entry found, return precondition failed


This changes behaviour of regular putObject, doesn't it?

This was a bug

Please confirm with one of the authors of the original feature -- we've historically had this feature with this behaviour and the opposite successful behaviour. So I worry someone might depend on it.

https://hyrumslaw.com FTL.

arielshaqed

Thanks!

This is a minor version bump of course, so let's also document it in the CHANGELOG as upcoming, or otherwise mark.

arielshaqed · 2024-02-20T13:30:18Z

pkg/graveler/graveler.go

@@ -1711,7 +1711,10 @@ func (g *Graveler) Set(ctx context.Context, repository *RepositoryRecord, branch

 		// verify the key not found
 		_, err := g.Get(ctx, repository, Ref(branchID), key)
-		if err == nil || !errors.Is(err, ErrNotFound) {
+		if err == nil { // Entry found, return precondition failed


Please confirm with one of the authors of the original feature -- we've historically had this feature with this behaviour and the opposite successful behaviour. So I worry someone might depend on it.

https://hyrumslaw.com FTL.

api/swagger.yml

arielshaqed · 2024-02-20T13:33:19Z

api/swagger.yml

+      required: false
+      schema:
+        type: string
+        pattern: '^\*$' # Currently, only "*" is supported


Please avoid this: server limitations should be enforced on the server not on the client. Suppose I update my server to a version that also supports ETag. This is the only thing that prevents me from using an old client version.

I know the previous implementation also did this. I now believe that it was also wrong. But back then we didn't try hard enough to keep old clients compatible where possible.

Will remove pattern and add validation on server

Co-authored-by: Ariel Shaqed (Scolnicov) <[email protected]>

itaiad200 · 2024-02-20T16:26:48Z

api/swagger.yml

@@ -4164,6 +4173,10 @@ paths:
          application/json:
            schema:
              $ref: "#/components/schemas/StagingMetadata"
+
+      parameters:
+        - $ref: "#/components/parameters/IfNonMatch"


Suggested change

- $ref: "#/components/parameters/IfNonMatch"

- $ref: "#/components/parameters/IfNoneMatch"

itaiad200 · 2024-02-20T16:27:38Z

api/swagger.yml

@@ -69,6 +69,15 @@ components:
      description: delimiter used to group common prefixes by
      schema:
        type: string
+
+    IfNonMatch:


Suggested change

IfNonMatch:

IfNoneMatch:

itaiad200 · 2024-02-20T16:27:53Z

CHANGELOG.md

@@ -1,5 +1,10 @@
 # Changelog

+# Unreleased
+
+- Task: Add If-Non-Match to LinkPhysicalAddress (#7480)


Suggested change

- Task: Add If-Non-Match to LinkPhysicalAddress (#7480)

- Task: Add If-None-Match to LinkPhysicalAddress (#7480)

itaiad200 · 2024-02-20T16:28:03Z

CHANGELOG.md

+# Unreleased
+
+- Task: Add If-Non-Match to LinkPhysicalAddress (#7480)
+  Removes deprecation of If-Non-Match from upload object 


Suggested change

Removes deprecation of If-Non-Match from upload object

Removes deprecation of If-None-Match from upload object

itaiad200 · 2024-02-20T16:29:01Z

docs/assets/js/swagger.yml

+
+      parameters:
+        - $ref: "#/components/parameters/IfNonMatch"
+
      responses:


Don't you need to add 412 response for that?

itaiad200 · 2024-02-20T16:32:52Z

api/swagger.yml

@@ -4397,24 +4410,14 @@ paths:
              format: binary

      parameters:
+        - $ref: "#/components/parameters/IfNonMatch"


Making sure this isn't a breaking change for clients

itaiad200 · 2024-02-20T16:33:20Z

pkg/api/controller_test.go

@@ -3007,6 +3022,68 @@ func TestController_LinkPhysicalAddressHandler(t *testing.T) {
 		}
 	})

+	t.Run("link physical address twice if non match", func(t *testing.T) {


Great test!

Task: Add IfAbsent to LinkPhysicalAddress

625c07a

N-o-Z added area/API Improvements or additions to the API include-changelog PR description should be included in next release changelog labels Feb 20, 2024

N-o-Z requested review from arielshaqed, idanovo and guy-har February 20, 2024 10:11

N-o-Z self-assigned this Feb 20, 2024

arielshaqed requested changes Feb 20, 2024

View reviewed changes

CR Fixes

fc2b318

N-o-Z requested a review from arielshaqed February 20, 2024 12:01

arielshaqed approved these changes Feb 20, 2024

View reviewed changes

N-o-Z and others added 3 commits February 20, 2024 15:58

Update api/swagger.yml

ec2cc46

Co-authored-by: Ariel Shaqed (Scolnicov) <[email protected]>

CR Fixes 2

372024e

gen clients

4289191

itaiad200 reviewed Feb 20, 2024

View reviewed changes

N-o-Z merged commit d81a950 into master Feb 20, 2024
36 of 37 checks passed

N-o-Z deleted the task/add-ifabsent-to-link-physical-address-7479 branch February 20, 2024 16:31

itaiad200 reviewed Feb 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Task: Add IfAbsent to LinkPhysicalAddress #7480

Task: Add IfAbsent to LinkPhysicalAddress #7480

N-o-Z commented Feb 20, 2024 •

edited

Loading

N-o-Z commented Feb 20, 2024

github-actions bot commented Feb 20, 2024 •

edited

Loading

github-actions bot commented Feb 20, 2024

arielshaqed left a comment

arielshaqed Feb 20, 2024

N-o-Z Feb 20, 2024

arielshaqed Feb 20, 2024

N-o-Z Feb 20, 2024

arielshaqed Feb 20, 2024

arielshaqed left a comment

arielshaqed Feb 20, 2024

arielshaqed Feb 20, 2024

N-o-Z Feb 20, 2024

itaiad200 Feb 20, 2024

itaiad200 Feb 20, 2024

itaiad200 Feb 20, 2024

itaiad200 Feb 20, 2024

itaiad200 Feb 20, 2024

itaiad200 Feb 20, 2024

itaiad200 Feb 20, 2024

	- $ref: "#/components/parameters/IfNonMatch"
	- $ref: "#/components/parameters/IfNoneMatch"

	- Task: Add If-Non-Match to LinkPhysicalAddress (#7480)
	- Task: Add If-None-Match to LinkPhysicalAddress (#7480)

	Removes deprecation of If-Non-Match from upload object
	Removes deprecation of If-None-Match from upload object

Task: Add IfAbsent to LinkPhysicalAddress #7480

Task: Add IfAbsent to LinkPhysicalAddress #7480

Conversation

N-o-Z commented Feb 20, 2024 • edited Loading

Change Description

THIS PR REINTRODUCES SET IF-NON-MATCH to the upload object API

Background

Testing Details

Breaking Change?

N-o-Z commented Feb 20, 2024

github-actions bot commented Feb 20, 2024 • edited Loading

github-actions bot commented Feb 20, 2024

E2E Test Results - DynamoDB Local - Local Block Adapter

arielshaqed left a 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

arielshaqed left a 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

N-o-Z commented Feb 20, 2024 •

edited

Loading

github-actions bot commented Feb 20, 2024 •

edited

Loading