-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Add full conditional write support to S3 test fixture #138542
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
Changes from all commits
258602c
8b14e66
3b4477f
5a9eace
86363d1
185a32e
0030552
5c0542c
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 |
|---|---|---|
|
|
@@ -45,6 +45,7 @@ | |
| import java.util.Set; | ||
| import java.util.concurrent.ConcurrentHashMap; | ||
| import java.util.concurrent.ConcurrentMap; | ||
| import java.util.concurrent.atomic.AtomicReference; | ||
| import java.util.regex.Matcher; | ||
| import java.util.regex.Pattern; | ||
|
|
||
|
|
@@ -248,6 +249,9 @@ public void handle(final HttpExchange exchange) throws IOException { | |
| if (isProtectOverwrite(exchange)) { | ||
| throw new AssertionError("If-None-Match: * header is not supported here"); | ||
| } | ||
| if (getRequiredExistingETag(exchange) != null) { | ||
| throw new AssertionError("If-Match: * header is not supported here"); | ||
| } | ||
|
|
||
| var sourceBlob = blobs.get(copySource); | ||
| if (sourceBlob == null) { | ||
|
|
@@ -406,8 +410,9 @@ public void handle(final HttpExchange exchange) throws IOException { | |
| * Update the blob contents if and only if the preconditions in the request are satisfied. | ||
| * | ||
| * @return {@link RestStatus#OK} if the blob contents were updated, or else a different status code to indicate the error: possibly | ||
| * {@link RestStatus#CONFLICT} or {@link RestStatus#PRECONDITION_FAILED} if the object exists and the precondition requires it | ||
| * not to. | ||
| * {@link RestStatus#CONFLICT} in any case, but if not then either {@link RestStatus#PRECONDITION_FAILED} if the object exists | ||
| * but doesn't match the specified precondition, or {@link RestStatus#NOT_FOUND} if the object doesn't exist but is required to | ||
| * do so by the precondition. | ||
| * | ||
| * @see <a href="https://docs.aws.amazon.com/AmazonS3/latest/userguide/conditional-writes.html#conditional-error-response">AWS docs</a> | ||
| */ | ||
|
|
@@ -418,6 +423,25 @@ private RestStatus updateBlobContents(HttpExchange exchange, String path, BytesR | |
| : ESTestCase.randomFrom(RestStatus.PRECONDITION_FAILED, RestStatus.CONFLICT); | ||
| } | ||
|
|
||
| final var requireExistingETag = getRequiredExistingETag(exchange); | ||
| if (requireExistingETag != null) { | ||
| final var responseCode = new AtomicReference<>(RestStatus.OK); | ||
| blobs.compute(path, (ignoredPath, existingContents) -> { | ||
| if (existingContents != null && requireExistingETag.equals(getEtagFromContents(existingContents))) { | ||
| return newContents; | ||
| } | ||
|
|
||
| responseCode.set( | ||
| ESTestCase.randomFrom( | ||
| existingContents == null ? RestStatus.NOT_FOUND : RestStatus.PRECONDITION_FAILED, | ||
| RestStatus.CONFLICT | ||
| ) | ||
| ); | ||
|
Comment on lines
+434
to
+439
Contributor
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. Why do you need randomization for the conflict? Is it because we
Contributor
Author
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. The AWS docs are not 100% clear on exactly what conditions lead to a 409 here, but it doesn't really matter from our point of view anyway: we just need to make sure we handle both 409 and 412s as acceptable failures. |
||
| return existingContents; | ||
| }); | ||
| return responseCode.get(); | ||
| } | ||
|
|
||
| blobs.put(path, newContents); | ||
| return RestStatus.OK; | ||
| } | ||
|
|
@@ -598,6 +622,9 @@ private static boolean isProtectOverwrite(final HttpExchange exchange) { | |
| return false; | ||
| } | ||
|
|
||
| if (exchange.getRequestHeaders().get("If-Match") != null) { | ||
| throw new AssertionError("Handling both If-None-Match and If-Match headers is not supported"); | ||
| } | ||
| if (ifNoneMatch.size() != 1) { | ||
| throw new AssertionError("multiple If-None-Match headers found: " + ifNoneMatch); | ||
| } | ||
|
|
@@ -609,6 +636,29 @@ private static boolean isProtectOverwrite(final HttpExchange exchange) { | |
| throw new AssertionError("invalid If-None-Match header: " + ifNoneMatch); | ||
| } | ||
|
|
||
| @Nullable // if no If-Match header found | ||
| private static String getRequiredExistingETag(final HttpExchange exchange) { | ||
| final var ifMatch = exchange.getRequestHeaders().get("If-Match"); | ||
|
|
||
| if (ifMatch == null) { | ||
| return null; | ||
| } | ||
|
|
||
| if (exchange.getRequestHeaders().get("If-None-Match") != null) { | ||
| throw new AssertionError("Handling both If-None-Match and If-Match headers is not supported"); | ||
| } | ||
|
|
||
| final var iterator = ifMatch.iterator(); | ||
| if (iterator.hasNext()) { | ||
| final var result = iterator.next(); | ||
| if (iterator.hasNext() == false) { | ||
| return result; | ||
| } | ||
| } | ||
|
Comment on lines
+651
to
+657
Contributor
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. nit: I think it should be |
||
|
|
||
| throw new AssertionError("multiple If-Match headers found: " + ifMatch); | ||
| } | ||
|
|
||
| MultipartUpload putUpload(String path) { | ||
| final var upload = new MultipartUpload(UUIDs.randomBase64UUID(), path); | ||
| synchronized (uploads) { | ||
|
|
||
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.
If there is no existing object and it's
If-Matchit should return 404. I assume in this case it would return 412 precondition failed.https://docs.aws.amazon.com/AmazonS3/latest/userguide/conditional-writes.html#conditional-error-response
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.
Maybe a test for If-Match for non-existing object.
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.
Ah thanks, well spotted. Bit of an odd choice tbh but I'll try and find a way to match that behaviour.