Skip to content

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented May 17, 2023

This PR fixes #8325
This bug seemed really weird, as our response was exactly the same as AWS, format was the same.

However, after trying to debug the issue in Java, it seemed the Owner field was properly parsed by the SDK.

After looking at the code of the SDK, it seemed the order of the XML tag was important for the way they populate each bucket with the owner. Basically, if the Owner tag is second, they first populate every bucket with an empty owner, even though the owner was properly parsed at the end. (I'll put the Java code snippet from the SDK at the end of the PR).

We keep the order from the specs when we serialize the response: we use and iterate over the shapes members.
In the specs, Buckets is first and Owner is second. The trick is to remove Buckets from the specs and add it back, to put it as the last member. Dirty, but it works.

I could test the fix in the localstack-java-utils repo, by adding a test case in S3FeaturesTest.

@Test
public void testGetBucketOwner() {
	AmazonS3 s3 = TestUtils.getClientS3();

	String bucketName = UUID.randomUUID().toString();
	Bucket bucket = s3.createBucket(bucketName);

	List<Bucket> buckets = s3.listBuckets();
	System.out.println("buckets " + buckets);

}

I've also added an integration test that is AWS validated to keep the order.

SDK snippet:

protected void doStartElement(String uri, String name, String qName, Attributes attrs) {
    if (this.in(new String[]{"ListAllMyBucketsResult"})) {
        if (name.equals("Owner")) {
            this.bucketsOwner = new Owner();
        }
    } else if (this.in(new String[]{"ListAllMyBucketsResult", "Buckets"}) && name.equals("Bucket")) {
        this.currentBucket = new Bucket();
        this.currentBucket.setOwner(this.bucketsOwner);
    }

}

protected void doEndElement(String uri, String name, String qName) {
    if (this.in(new String[]{"ListAllMyBucketsResult", "Owner"})) {
        if (name.equals("ID")) {
            this.bucketsOwner.setId(this.getText());
        } else if (name.equals("DisplayName")) {
            this.bucketsOwner.setDisplayName(this.getText());
        }
    } else if (this.in(new String[]{"ListAllMyBucketsResult", "Buckets"})) {
        if (name.equals("Bucket")) {
            this.buckets.add(this.currentBucket);
            this.currentBucket = null;
        }
    } else if (this.in(new String[]{"ListAllMyBucketsResult", "Buckets", "Bucket"})) {
        if (name.equals("Name")) {
            this.currentBucket.setName(this.getText());
        } else if (name.equals("CreationDate")) {
            Date creationDate = DateUtils.parseISO8601Date(this.getText());
            this.currentBucket.setCreationDate(creationDate);
        }
    }

}

@bentsku bentsku added aws:s3 Amazon Simple Storage Service semver: patch Non-breaking changes which can be included in patch releases labels May 17, 2023
@bentsku bentsku self-assigned this May 17, 2023
@bentsku bentsku requested review from macnev2013 and thrau as code owners May 17, 2023 19:03
@bentsku bentsku requested a review from alexrashed May 17, 2023 19:03
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.

😱 what a catch! 🏅 thanks @bentsku, the change looks good to me! glad we have the jsonpatch in place. some comments would be useful to understand why patches were made, but i know it's not supported in json. at least we have the blame history and a great PR description.

💯

@coveralls
Copy link

Coverage Status

Coverage: 82.244% (-0.006%) from 82.249% when pulling 92823db on fix-list-buckets-order into a3145fd on master.

@github-actions
Copy link

LocalStack Community integration with Pro

2 044 tests   1 761 ✔️  1h 21m 50s ⏱️
       2 suites     283 💤
       2 files           0

Results for commit 92823db.

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 semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: S3 bucket owner's name is too long for the API to read. A null exception raised.
3 participants