Skip to content

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented Mar 27, 2023

This is a follow up from #7545, a user reported an issue in our Community Slack. The test is AWS validated, and once again the specs were not compliant with what S3 really returns.

We also reapply the logic that was in a patch previously set, moto would have raised an error if using some request parameters that were not implemented. It would raise a NotImplemented exception. The previous provider was swallowing the exception and letting it pass anyway without applying the parameters. We can keep the same behaviour for now and come back later and actually implement the right behaviour.

related: #7981, localstack/docs#539

\cc @steffyP

@coveralls
Copy link

coveralls commented Mar 27, 2023

Coverage Status

Coverage: 81.873% (-0.003%) from 81.876% when pulling 0cef404 on fix-list-multipart-result into 649e77a on master.

@bentsku bentsku changed the title fix response structure for S3 ListMultipartUploads wip: fix response structure for S3 ListMultipartUploads Mar 28, 2023
@bentsku bentsku self-assigned this Mar 28, 2023
@bentsku bentsku added the aws:s3 Amazon Simple Storage Service label Mar 28, 2023
@bentsku bentsku force-pushed the fix-list-multipart-result branch from aec8162 to 72afb68 Compare March 28, 2023 16:21
@bentsku bentsku changed the title wip: fix response structure for S3 ListMultipartUploads fix response structure for S3 ListMultipartUploads Mar 28, 2023
@bentsku bentsku force-pushed the fix-list-multipart-result branch from 72afb68 to 338ca3c Compare March 28, 2023 17:17
@github-actions
Copy link

github-actions bot commented Mar 28, 2023

LocalStack integration with Pro

1 867 tests  +1   1 675 ✔️ ±0   1h 29m 56s ⏱️ + 7m 55s
       1 suites ±0      192 💤 +1 
       1 files   ±0          0 ±0 

Results for commit 0cef404. ± Comparison against base commit 649e77a.

♻️ This comment has been updated with latest results.

Copy link
Member

@thrau thrau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! another nice catch :-) just wondering about whether we need moto here

Comment on lines 571 to 573
try:
response: ListMultipartUploadsResult = call_moto(context)
except NotImplementedError:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a question: do we need this try/except? is this method implemented in moto, and are we only using it partially? or can we just replace call_moto with our implementation all together?

Copy link
Contributor Author

@bentsku bentsku Mar 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's partially implemented in moto, and raises only if some parameters are passed:
https://github.com/localstack/moto/blob/58830863f04661fd32d72a79c0046cb667068a34/moto/s3/responses.py#L428-L432

We could replace it I suppose because I think if there's an exception, we're basically doing what it would do if there wasn't an exception. Good idea!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it was in the case they would add more things, because right now our implementation is incomplete, and if moto adds feature we wouldn't get them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aws:s3 Amazon Simple Storage Service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants