Make ARN Resource builder more expressive. Implement proper toString() #3683

belugabehr · 2023-01-11T19:49:05Z

Motivation and Context

The modeling in ArnResource does not well match the spec. [1]

The toString() method of the ArnResource does not produce a proper (i.e., to spec) value.

The current implementation uses the phrase resource whereas the spec calls it a resource-id. So, from the top-level Arn class, it's a bit awkward to access it: myArn.getResource().getResource().

The code currently does not support legacy '/' resource values. I currently have a case where I am trying to strip off the qualifier off an ARN with this legacy '/' and it fails when using the API. The '/' is replaced with a colon ':' and therefore the ARN produced removes the qualifier, but does not otherwise match the original ARN that I am passing into the API.

[1] https://docs.aws.amazon.com/general/latest/gr/aws-arns-and-namespaces.html

Modifications

Deprecate resource attribute -> resourceId
Update toString() to produce a properly formed output
Allow for specifying a legacy '/' character to separate resoure-type and resource-id

Testing

Added new unit tests

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Checklist

I have read the CONTRIBUTING document
Local run of mvn install succeeds
My code follows the code style of this project
My change requires a change to the Javadoc documentation
I have updated the Javadoc documentation accordingly
I have added tests to cover my changes
All new and existing tests passed
I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

I confirm that this pull request can be released under the Apache 2 license

zoewangg · 2023-03-20T23:01:54Z

core/arns/src/main/java/software/amazon/awssdk/arns/ArnResource.java

@@ -57,8 +59,16 @@ public Optional<String> resourceType() {
    /**
     * @return the entire resource as a string
     */
+    @Deprecated


Instead of deprecating it, could we clarify it in the Javadoc?

zoewangg · 2023-03-20T23:05:06Z

core/arns/src/main/java/software/amazon/awssdk/arns/ArnResource.java

            builder.qualifier(resource.substring(qualifierColonIndex + 1));
        }
+        if (splitter.equals('/')) {
+            builder.usingSlashSeperator();
+        }

        return builder.build();
    }

    @Override
    public String toString() {


This may potentially break customers who rely on existing behavior. Can we introduce a new method, toStringPretty or toStringFormatted?

zoewangg · 2023-03-20T23:05:53Z

core/arns/src/main/java/software/amazon/awssdk/arns/ArnResource.java

+         *
+         * @return Returns a reference to this builder
+         */
+        Builder usingSlashSeperator();


Should we just take a character as separator?

Builder seperator(Character char);

debora-ito · 2023-04-19T23:16:48Z

@belugabehr let us know if you want to keep this PR open.

belugabehr · 2023-04-21T03:12:49Z

@debora-ito Other priorities keeping me from finishing - but my intent is to see it through. Thanks for the patience.

cenedhryn · 2023-05-23T17:41:11Z

@belugabehr Any updates? Saw you updated that other PR :-)

belugabehr · 2023-06-23T03:35:06Z

@cenedhryn Hello - better late than never. I made the the requested changes. However, I noticed a small bug that is also fixed in this PR.

The qualifier of an ARN is always preceded by a colon ':'. However, the current code allows it to be a colon or a slash - this is incorrect.

aws-sdk-java-v2/core/arns/src/main/java/software/amazon/awssdk/arns/ArnResource.java

Line 104 in c2db6af

int qualifierColonIndex = resource.indexOf(splitter, resourceColonIndex + 1);

belugabehr · 2023-06-23T03:37:15Z

core/arns/src/test/java/software/amazon/awssdk/arns/ArnTest.java

-        verifyArnResource(arn.resource());
-        assertThat(arn.resource().qualifier().get()).isEqualTo("1");
+        assertThat(arn.resource().qualifier()).isEqualTo(Optional.empty());


As seen here, this is a valid S3 path, yet it is being treated as a qualifier.

belugabehr · 2023-06-23T03:37:31Z

core/arns/src/test/java/software/amazon/awssdk/arns/ArnTest.java

@@ -31,7 +33,6 @@ public void arnWithBasicResource_ParsesCorrectly() {
        assertThat(arn.region()).hasValue("us-east-1");
        assertThat(arn.accountId()).hasValue("12345678910");
        assertThat(arn.resourceAsString()).isEqualTo("myresource");
-        System.out.println(arn.resource());


General cleanup

belugabehr · 2023-06-23T03:38:08Z

core/arns/src/test/java/software/amazon/awssdk/arns/ArnResourceTest.java

+    @Test
+    public void toString_resourceIdOnly() {
+        ArnResource arnResource = ArnResource.builder().resourceId("resource").build();
+        assertThat("null:resource:null").isEqualTo(arnResource.toString());


Add unit test to demonstrate the unexpected 'null' behavior here of the toString and how it differs from toStringFormatted()

…e6f04ae87 Pull request: release <- staging/464ee5ef-a5af-45c3-ad48-294e6f04ae87

belugabehr requested a review from a team as a code owner January 11, 2023 19:49

debora-ito added the needs-review This issue or PR needs review from the team. label Jan 23, 2023

zoewangg reviewed Mar 20, 2023

View reviewed changes

debora-ito added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 10 days. needs-review This issue or PR needs review from the team. and removed needs-review This issue or PR needs review from the team. labels Apr 19, 2023

github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 10 days. label Apr 21, 2023

Make ARN Resource builder more expressive. Implement proper toString()

49ccea5

belugabehr force-pushed the arn-build-qualifier branch from 5c334d2 to 49ccea5 Compare June 23, 2023 03:32

belugabehr commented Jun 23, 2023

View reviewed changes

aws-sdk-java-automation added a commit that referenced this pull request Feb 24, 2025

Merge pull request #3683 from aws/staging/464ee5ef-a5af-45c3-ad48-294…

f89d49a

…e6f04ae87 Pull request: release <- staging/464ee5ef-a5af-45c3-ad48-294e6f04ae87

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make ARN Resource builder more expressive. Implement proper toString() #3683

Make ARN Resource builder more expressive. Implement proper toString() #3683

belugabehr commented Jan 11, 2023 •

edited

Loading

zoewangg Mar 20, 2023

zoewangg Mar 20, 2023

zoewangg Mar 20, 2023

debora-ito commented Apr 19, 2023

belugabehr commented Apr 21, 2023

cenedhryn commented May 23, 2023

belugabehr commented Jun 23, 2023 •

edited

Loading

belugabehr Jun 23, 2023

belugabehr Jun 23, 2023

belugabehr Jun 23, 2023 •

edited

Loading

Make ARN Resource builder more expressive. Implement proper toString() #3683

Are you sure you want to change the base?

Make ARN Resource builder more expressive. Implement proper toString() #3683

Conversation

belugabehr commented Jan 11, 2023 • edited Loading

Motivation and Context

Modifications

Testing

Types of changes

Checklist

License

zoewangg Mar 20, 2023

Choose a reason for hiding this comment

zoewangg Mar 20, 2023

Choose a reason for hiding this comment

zoewangg Mar 20, 2023

Choose a reason for hiding this comment

debora-ito commented Apr 19, 2023

belugabehr commented Apr 21, 2023

cenedhryn commented May 23, 2023

belugabehr commented Jun 23, 2023 • edited Loading

belugabehr Jun 23, 2023

Choose a reason for hiding this comment

belugabehr Jun 23, 2023

Choose a reason for hiding this comment

belugabehr Jun 23, 2023 • edited Loading

Choose a reason for hiding this comment

belugabehr commented Jan 11, 2023 •

edited

Loading

belugabehr commented Jun 23, 2023 •

edited

Loading

belugabehr Jun 23, 2023 •

edited

Loading