Support ListObjectsV2 in S3HttpHandler
#126189
Conversation
`ListObjects` and `ListObjectsV2` only really differ in their approach to pagination, but today `S3HttpHandler` does not simulate pagination anyway so we can use the same handling code for both APIs. The only practical difference is that the v2 SDK requires the `<IsTruncated>` element in a `ListObjectsV2` response, but this element is permitted in both APIs so we add it here.
DaveCTurner
added
>test
elasticsearchmachine
added
the
Team:Distributed Coordination
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
DiannaHohensee
left a comment
DiannaHohensee
left a comment
There was a problem hiding this comment.
Clarification nits, otherwise LGTM
| } else if (request.isListObjectsRequest()) { | ||
| if (request.queryParameters().containsKey("list-type")) { | ||
| throw new AssertionError("Test must be adapted for GET Bucket (List Objects) Version 2"); | ||
| final var listType = request.queryParameters().getOrDefault("list-type", List.of("1")).get(0); |
There was a problem hiding this comment.
Could you clarify with a comment that V2 requests newly have this extra version (?) field, so we're adding the check in case the version unexpectedly changes in future? That's my understanding, anyway, from looking around on the internet.
There was a problem hiding this comment.
I think we can replace it something like
final var listType = request.getOptionalQueryParam("list-type");
assert listType.isEmpty() || "2".equals(listType.get());Seems more explicit with the intention and also ensures the parameter has a single value?
There was a problem hiding this comment.
All fair points. Previously I was doing something with the listType value so it was useful to distinguish "1" from "2", but we don't need this right now so we can just drop this check entirely -> 5974795
| list.append("<Delimiter>").append(delimiter).append("</Delimiter>"); | ||
| } | ||
| // Would be good to test pagination here (the only real difference between ListObjects and ListObjectsV2) but for now | ||
| // we return all the results at once. |
There was a problem hiding this comment.
Should this explanation be partly phrased as a TODO?
There was a problem hiding this comment.
I hold no hope that we'll actually do it so I'm going to just leave it as a comment.
| } else if (request.isListObjectsRequest()) { | ||
| if (request.queryParameters().containsKey("list-type")) { | ||
| throw new AssertionError("Test must be adapted for GET Bucket (List Objects) Version 2"); | ||
| final var listType = request.queryParameters().getOrDefault("list-type", List.of("1")).get(0); |
There was a problem hiding this comment.
Also, injecting "1" is a little misleading: it made me look around for what "1" could represent and it appears to be meaningless (since V1 doesn't have the field)?
Might it be better to add a not-null check to the middle of the if-statement (to avoid the mystery default number confusion)?
| } else if (request.isListObjectsRequest()) { | ||
| if (request.queryParameters().containsKey("list-type")) { | ||
| throw new AssertionError("Test must be adapted for GET Bucket (List Objects) Version 2"); | ||
| final var listType = request.queryParameters().getOrDefault("list-type", List.of("1")).get(0); |
There was a problem hiding this comment.
I think we can replace it something like
final var listType = request.getOptionalQueryParam("list-type");
assert listType.isEmpty() || "2".equals(listType.get());Seems more explicit with the intention and also ensures the parameter has a single value?
DaveCTurner
added
auto-merge-without-approval
`ListObjects` and `ListObjectsV2` only really differ in their approach to pagination, but today `S3HttpHandler` does not simulate pagination anyway so we can use the same handling code for both APIs. The only practical difference is that the v2 SDK requires the `<IsTruncated>` element in a `ListObjectsV2` response, but this element is permitted in both APIs so we add it here.
💚 Backport successful
|
`ListObjects` and `ListObjectsV2` only really differ in their approach to pagination, but today `S3HttpHandler` does not simulate pagination anyway so we can use the same handling code for both APIs. The only practical difference is that the v2 SDK requires the `<IsTruncated>` element in a `ListObjectsV2` response, but this element is permitted in both APIs so we add it here.
`ListObjects` and `ListObjectsV2` only really differ in their approach to pagination, but today `S3HttpHandler` does not simulate pagination anyway so we can use the same handling code for both APIs. The only practical difference is that the v2 SDK requires the `<IsTruncated>` element in a `ListObjectsV2` response, but this element is permitted in both APIs so we add it here.
4 participants
ListObjectsandListObjectsV2only really differ in their approachto pagination, but today
S3HttpHandlerdoes not simulate paginationanyway so we can use the same handling code for both APIs. The only
practical difference is that the v2 SDK requires the
<IsTruncated>element in a
ListObjectsV2response, but this element is permitted inboth APIs so we add it here.