Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
133 changes: 118 additions & 15 deletions core/arns/src/main/java/software/amazon/awssdk/arns/ArnResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
* within an Arn.
*
* <p>
* If {@link #resourceType} is not present, {@link #resource} will return the entire resource
* If {@link #resourceType} is not present, {@link #resourceId} will return the entire resource
* as a string the same as {@link Arn#resource()}.
*
* @see Arn
Expand All @@ -38,13 +38,34 @@
public final class ArnResource implements ToCopyableBuilder<ArnResource.Builder, ArnResource> {

private final String resourceType;
private final String resource;
private final String resourceId;
private final String qualifier;
private final ArnResourceFormat resourceFormat;

/**
* There are two valid ways to format a resource type paired with a resource ID:
* with a colon or with a slash.
*/
public enum ArnResourceFormat {
RESOURCE_WITH_SLASH('/'),
RESOURCE_WITH_COLON(':');

private final char seperator;

ArnResourceFormat(char seperator) {
this.seperator = seperator;
}

char getSeperator() {
return this.seperator;
}
}

private ArnResource(DefaultBuilder builder) {
this.resourceType = builder.resourceType;
this.resource = Validate.paramNotBlank(builder.resource, "resource");
this.resourceId = Validate.paramNotBlank(builder.resourceId, "resource");
this.qualifier = builder.qualifier;
this.resourceFormat = builder.resourceFormat;
}

/**
Expand All @@ -58,7 +79,14 @@ public Optional<String> resourceType() {
* @return the entire resource as a string
*/
public String resource() {
return resource;
return this.resourceId;
}

/**
* @return the resourceId
*/
public String resourceId() {
return resourceId;
}

/**
Expand Down Expand Up @@ -94,20 +122,23 @@ public static ArnResource fromString(String resource) {
Character splitter = StringUtils.findFirstOccurrence(resource, ':', '/');

if (splitter == null) {
return ArnResource.builder().resource(resource).build();
return ArnResource.builder().resourceId(resource).build();
}

int resourceTypeColonIndex = resource.indexOf(splitter);

ArnResource.Builder builder = ArnResource.builder().resourceType(resource.substring(0, resourceTypeColonIndex));
int resourceColonIndex = resource.indexOf(splitter, resourceTypeColonIndex);
int qualifierColonIndex = resource.indexOf(splitter, resourceColonIndex + 1);
int qualifierColonIndex = resource.indexOf(':', resourceColonIndex + 1);
if (qualifierColonIndex < 0) {
builder.resource(resource.substring(resourceTypeColonIndex + 1));
builder.resourceId(resource.substring(resourceTypeColonIndex + 1));
} else {
builder.resource(resource.substring(resourceTypeColonIndex + 1, qualifierColonIndex));
builder.resourceId(resource.substring(resourceTypeColonIndex + 1, qualifierColonIndex));
builder.qualifier(resource.substring(qualifierColonIndex + 1));
}
if (splitter.equals('/')) {
builder.resourceFormat(ArnResourceFormat.RESOURCE_WITH_SLASH);
}

return builder.build();
}
Expand All @@ -116,11 +147,21 @@ public static ArnResource fromString(String resource) {
public String toString() {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

return this.resourceType
+ ":"
+ this.resource
+ this.resourceId
+ ":"
+ this.qualifier;
}

public String toStringFormatted() {
if (this.resourceType == null) {
return this.resourceId;
}
if (this.qualifier == null) {
return this.resourceType + this.resourceFormat.getSeperator() + this.resourceId;
}
return this.resourceType + this.resourceFormat.getSeperator() + this.resourceId + ':' + qualifier;
}

@Override
public boolean equals(Object o) {
if (this == o) {
Expand All @@ -135,7 +176,7 @@ public boolean equals(Object o) {
if (!Objects.equals(resourceType, that.resourceType)) {
return false;
}
if (!Objects.equals(resource, that.resource)) {
if (!Objects.equals(resourceId, that.resourceId)) {
return false;
}
return Objects.equals(qualifier, that.qualifier);
Expand All @@ -144,17 +185,18 @@ public boolean equals(Object o) {
@Override
public int hashCode() {
int result = resourceType != null ? resourceType.hashCode() : 0;
result = 31 * result + (resource != null ? resource.hashCode() : 0);
result = 31 * result + (resourceId != null ? resourceId.hashCode() : 0);
result = 31 * result + (qualifier != null ? qualifier.hashCode() : 0);
return result;
}

@Override
public Builder toBuilder() {
return builder()
.resource(resource)
.resourceId(resourceId)
.resourceType(resourceType)
.qualifier(qualifier);
.qualifier(qualifier)
.resourceFormat(resourceFormat);
}


Expand All @@ -176,6 +218,15 @@ public interface Builder extends CopyableBuilder<ArnResource.Builder, ArnResourc
*/
Builder resource(String resource);

/**
* Define the resource ID. This has the same affect as {@link #resource(String)}, but
* renamed to better align with the ARN format spec.
*
* @param resourceId the entire resource
* @return Returns a reference to this builder
*/
Builder resourceId(String resourceId);

/**
* Define the qualifier of the resource.
*
Expand All @@ -184,6 +235,24 @@ public interface Builder extends CopyableBuilder<ArnResource.Builder, ArnResourc
*/
Builder qualifier(String qualifier);

/**
* For legacy AWS Arns not following the resourceType:resource:qualifier pattern,
* the qualifier field will contain everything after the first two sections separated
* by a slash character "/". The default is to use a colon.
*
* @param resourceFormat the resource format to use
* @return Returns a reference to this builder
*/
Builder resourceFormat(ArnResourceFormat resourceFormat);

/**
* Set the builder from an pre-existing {@link ArnResource}.
*
* @param arnResource The {@link ArnResource} to copy.
* @return Returns a reference to this builder
*/
Builder arnResource(ArnResource arnResource);

/**
* @return an instance of {@link ArnResource} that is created from the builder
*/
Expand All @@ -193,8 +262,9 @@ public interface Builder extends CopyableBuilder<ArnResource.Builder, ArnResourc

public static final class DefaultBuilder implements Builder {
private String resourceType;
private String resource;
private String resourceId;
private String qualifier;
private ArnResourceFormat resourceFormat = ArnResourceFormat.RESOURCE_WITH_COLON;

private DefaultBuilder() {
}
Expand All @@ -210,7 +280,7 @@ public Builder resourceType(String resourceType) {
}

public void setResource(String resource) {
this.resource = resource;
this.resourceId = resource;
}

@Override
Expand All @@ -229,6 +299,39 @@ public Builder qualifier(String qualifier) {
return this;
}

public void setResourceId(String resourceId) {
this.resourceId = resourceId;
}

@Override
public Builder resourceId(String resourceId) {
setResourceId(resourceId);
return this;
}

public void setResourceFormat(ArnResourceFormat resourceFormat) {
this.resourceFormat = resourceFormat;
}

@Override
public Builder resourceFormat(ArnResourceFormat resourceFormat) {
setResourceFormat(resourceFormat);
return this;
}

public void setArnResource(ArnResource arnResource) {
setResourceType(arnResource.resourceType);
setResourceId(arnResource.resourceId);
setQualifier(arnResource.qualifier);
setResourceFormat(arnResource.resourceFormat);
}

@Override
public Builder arnResource(ArnResource arnResource) {
setArnResource(arnResource);
return this;
}

@Override
public ArnResource build() {
return new ArnResource(this);
Expand Down
70 changes: 70 additions & 0 deletions core/arns/src/test/java/software/amazon/awssdk/arns/ArnResourceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
import java.util.Optional;
import org.junit.jupiter.api.Test;

import software.amazon.awssdk.arns.ArnResource.ArnResourceFormat;

public class ArnResourceTest {

@Test
Expand Down Expand Up @@ -64,4 +66,72 @@ public void hashCodeEquals_minimalProperties() {
assertThat(arnResource.equals(anotherResource)).isTrue();
assertThat(arnResource.hashCode()).isEqualTo(anotherResource.hashCode());
}

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

@belugabehr belugabehr Jun 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

}

@Test
public void toString_resourceType_resourceId() {
ArnResource arnResource = ArnResource.builder()
.resourceType("type")
.resourceId("resource")
.build();
assertThat("type:resource:null").isEqualTo(arnResource.toString());
}

@Test
public void toFormattedString_resourceType_resourceId_qualfiier() {
ArnResource arnResource = ArnResource.builder()
.resourceType("type")
.resourceId("resource")
.qualifier("qualifier")
.build();
assertThat("type:resource:qualifier").isEqualTo(arnResource.toStringFormatted());
}

@Test
public void toFormattedString_slash__resourceType_resourceId_qualfiier() {
ArnResource arnResource = ArnResource.builder()
.resourceType("type")
.resourceId("resource")
.qualifier("qualifier")
.resourceFormat(ArnResourceFormat.RESOURCE_WITH_SLASH)
.build();
assertThat("type/resource:qualifier").isEqualTo(arnResource.toStringFormatted());
}

@Test
public void toStringFormatted_resourceType_resourceId_parse_slash_seperator() {
ArnResource arnResource = ArnResource.builder()
.resourceType("vpc")
.resourceFormat(ArnResourceFormat.RESOURCE_WITH_SLASH)
.resourceId("vpc-0e9801d129EXAMPLE")
.build();
assertThat("vpc/vpc-0e9801d129EXAMPLE").isEqualTo(arnResource.toStringFormatted());
}

@Test
public void toStringFormatted_resourceType_resourceId_slash_seperator() {
ArnResource arnResource = ArnResource.fromString("vpc/vpc-0e9801d129EXAMPLE");
assertThat("vpc/vpc-0e9801d129EXAMPLE").isEqualTo(arnResource.toStringFormatted());
assertThat(arnResource.resourceType()).isEqualTo(Optional.of("vpc"));
assertThat(arnResource.resourceId()).isEqualTo("vpc-0e9801d129EXAMPLE");
assertThat(arnResource.qualifier()).isEqualTo(Optional.empty());
}

@Test
public void toStringFormatted_arn_resource() {
ArnResource arnResource = ArnResource.fromString("vpc/vpc-0e9801d129EXAMPLE:qualifier");
ArnResource arnResourceCpy = ArnResource.builder()
.arnResource(arnResource)
.build();
assertThat("vpc/vpc-0e9801d129EXAMPLE:qualifier").isEqualTo(arnResourceCpy.toStringFormatted());
assertThat(arnResourceCpy.resourceType()).isEqualTo(Optional.of("vpc"));
assertThat(arnResourceCpy.resourceId()).isEqualTo("vpc-0e9801d129EXAMPLE");
assertThat(arnResourceCpy.qualifier()).isEqualTo(Optional.of("qualifier"));
}

}
6 changes: 3 additions & 3 deletions core/arns/src/test/java/software/amazon/awssdk/arns/ArnTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

import java.util.Optional;

import org.junit.jupiter.api.Test;

public class ArnTest {
Expand All @@ -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());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

General cleanup

}

@Test
Expand Down Expand Up @@ -110,8 +111,7 @@ public void arnWithResourceTypeAndResourceAndQualifier_SlashSplitter_ParsesCorre
assertThat(arn.service()).isEqualTo("s3");
assertThat(arn.region()).hasValue("us-east-1");
assertThat(arn.resourceAsString()).isEqualTo("bucket/foobar/1");
verifyArnResource(arn.resource());
assertThat(arn.resource().qualifier().get()).isEqualTo("1");
assertThat(arn.resource().qualifier()).isEqualTo(Optional.empty());
Comment on lines -113 to +114
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

@Test
Expand Down