-
Notifications
You must be signed in to change notification settings - Fork 25.7k
Return ETag from S3 fixture GetObject API
#138514
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
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 |
|---|---|---|
|
|
@@ -84,6 +84,8 @@ public S3HttpHandler(final String bucket, @Nullable final String basePath) { | |
| */ | ||
| private static final Set<String> METHODS_HAVING_NO_REQUEST_BODY = Set.of("GET", "HEAD", "DELETE"); | ||
|
|
||
| private static final String SHA_256_ETAG_PREFIX = "es-test-sha-256-"; | ||
|
|
||
| @Override | ||
| public void handle(final HttpExchange exchange) throws IOException { | ||
| // Remove custom query parameters before processing the request. This simulates how S3 ignores them. | ||
|
|
@@ -322,6 +324,9 @@ public void handle(final HttpExchange exchange) throws IOException { | |
| exchange.sendResponseHeaders(RestStatus.NOT_FOUND.getStatus(), -1); | ||
| return; | ||
| } | ||
|
|
||
| exchange.getResponseHeaders().add("ETag", getEtagFromContents(blob)); | ||
|
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. Could we have a brief comment here explaining what an
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. I added a comment in da4e70e. Not sure where to put it really,
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. AFAIK S3 returns ETag on creation(PutObject, MPU) too, but our fixture does not return ETag unless there is precondition failure. Should we add ETag to all operations that suppose to have one? Also can compute ETag once on creation and store together with blob
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. We have no plans to use the ETag returned by the PutObject and CompleteMultipartUpload APIs so I'm hesitant to add them now. It'd be easy enough to add later if needed. I did consider tracking etags alongside the object contents but it's a bigger change and not really necessary. This is purely test fixture code, and indeed a bit of latency here and there is helpful for test coverage. |
||
|
|
||
| final String rangeHeader = exchange.getRequestHeaders().getFirst("Range"); | ||
| if (rangeHeader == null) { | ||
| exchange.getResponseHeaders().add("Content-Type", "application/octet-stream"); | ||
|
|
@@ -413,6 +418,15 @@ private boolean updateBlobContents(HttpExchange exchange, String path, BytesRefe | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Etags are opaque identifiers for the contents of an object. | ||
| * | ||
| * @see <a href="https://en.wikipedia.org/wiki/HTTP_ETag">HTTP ETag on Wikipedia</a>. | ||
| */ | ||
| public static String getEtagFromContents(BytesReference blobContents) { | ||
|
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. Is there any value adding a specific unit test for this method?
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. Eh possibly, I meant to at least. Added in 6e98292. |
||
| return '"' + SHA_256_ETAG_PREFIX + MessageDigests.toHexString(MessageDigests.digest(blobContents, MessageDigests.sha256())) + '"'; | ||
| } | ||
|
|
||
| public Map<String, BytesReference> blobs() { | ||
| return blobs; | ||
| } | ||
|
|
@@ -490,7 +504,7 @@ private static Tuple<String, BytesReference> parseRequestBody(final HttpExchange | |
| ); | ||
| } | ||
| } | ||
| return Tuple.tuple(MessageDigests.toHexString(MessageDigests.digest(bytesReference, MessageDigests.md5())), bytesReference); | ||
| return Tuple.tuple(getEtagFromContents(bytesReference), bytesReference); | ||
| } catch (Exception e) { | ||
| logger.error("exception in parseRequestBody", e); | ||
| exchange.sendResponseHeaders(500, 0); | ||
|
|
||
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.
Is this string going to be visible?
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.
No, this is only in the test fixture, I'm using something descriptive here just to aid future troubleshooting. It's treated as an opaque string everywhere else.