Fix the prefix requirement in ListObjects #3005
Conversation
alyssaxu333
force-pushed
the
S3ListObjectPrefix
branch
from
3e87cce to
8d3a18d
Compare
alyssaxu333
force-pushed
the
S3ListObjectPrefix
branch
from
8d3a18d to
5f1c6fa
Compare
| * @throws RestServiceException | ||
| */ | ||
| public static NamedBlobPath parse(RestRequest restRequest) throws RestServiceException { | ||
| restRequest.setArg(InternalKeys.REST_METHOD, restRequest.getRestMethod()); |
There was a problem hiding this comment.
May I know why do we need this extra internal header?
If you want the rest method you can directly get it right?
If you want to know which request is S3 request, you can use isS3Request(restRequest).
There was a problem hiding this comment.
So in parseS3, I added a check for whether it's a listObject request or not. To determine if it's a listObject request, the rest method should be "Get" and the blob uri should be s3/account/container. But parseS3(String path, Map<String, Object> args) doesn't have rest method passed into it. I was thinking about changing it to parseS3(RestRequest restRequest) or adding rest method header in args beforehand. Changing signatures seems to be a bigger change thus I chose the second way. Pls let me know which way do you think is better, or if there's other ways to revise it that I didn't consider.
There was a problem hiding this comment.
Changing signatures seem to be cleaner approach. But, if it involves changing too many files, may be adding the Rest method to args is also okay.
I think we could also add Rest method to args in FrontendRestRequestService#preProcessAndRouteRequest so that it is available to all requests.
| @@ -522,6 +527,11 @@ public static final class InternalKeys { | |||
| * content-length header | |||
| */ | |||
| public static final String CONTENT_RANGE_LENGTH = KEY_PREFIX + "content-range-length"; | |||
|
|
|||
| /** | |||
| * Boolean field set to "true" if this is a S3 request. | |||
There was a problem hiding this comment.
Nit: Looks like copy-paste typo
| * @throws RestServiceException | ||
| */ | ||
| public static NamedBlobPath parse(RestRequest restRequest) throws RestServiceException { | ||
| restRequest.setArg(InternalKeys.REST_METHOD, restRequest.getRestMethod()); |
There was a problem hiding this comment.
Changing signatures seem to be cleaner approach. But, if it involves changing too many files, may be adding the Rest method to args is also okay.
I think we could also add Rest method to args in FrontendRestRequestService#preProcessAndRouteRequest so that it is available to all requests.
Summary
ListObjects request without prefix will throw 400 error. But Amazon S3 ListObject request doesn't require prefix, it will list all objects in the bucket. Fix the request requirement.
Testing Done
Unit test