Skip to content

Commit 4642335

Browse files
kstichkstich
committed
Address more CR feedback
1 parent 3e03cb8 commit 4642335

File tree

36 files changed

+144
-141
lines changed

36 files changed

+144
-141
lines changed

‎smithy-aws-rules-engine/src/main/java/software/amazon/smithy/rulesengine/aws/language/functions/AwsArn.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ public String toString() {
147147
+ "service=" + service + ", "
148148
+ "region=" + region + ", "
149149
+ "accountId=" + accountId + ", "
150-
+ "resource=" + builder + ']';
150+
+ "resource=" + builder + "]";
151151
}
152152

153153
@Override

‎smithy-aws-rules-engine/src/main/java/software/amazon/smithy/rulesengine/aws/language/functions/AwsPartition.java‎

Lines changed: 33 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -5,17 +5,14 @@
55

66
package software.amazon.smithy.rulesengine.aws.language.functions;
77

8-
import java.util.ArrayList;
98
import java.util.Collections;
109
import java.util.HashMap;
11-
import java.util.Iterator;
1210
import java.util.LinkedHashMap;
1311
import java.util.List;
1412
import java.util.Map;
15-
import java.util.ServiceLoader;
1613
import java.util.regex.Pattern;
14+
import software.amazon.smithy.model.node.Node;
1715
import software.amazon.smithy.rulesengine.aws.language.functions.partition.Partition;
18-
import software.amazon.smithy.rulesengine.aws.language.functions.partition.PartitionDataProvider;
1916
import software.amazon.smithy.rulesengine.aws.language.functions.partition.PartitionOutputs;
2017
import software.amazon.smithy.rulesengine.aws.language.functions.partition.Partitions;
2118
import software.amazon.smithy.rulesengine.language.evaluation.type.Type;
@@ -42,7 +39,19 @@ public final class AwsPartition extends LibraryFunction {
4239
public static final Identifier INFERRED = Identifier.of("inferred");
4340

4441
private static final Definition DEFINITION = new Definition();
45-
private static final PartitionData PARTITION_DATA = loadPartitionData();
42+
private static final List<Partition> PARTITIONS;
43+
private static final Map<String, Partition> REGION_MAP = new HashMap<>();
44+
45+
static {
46+
PARTITIONS = Partitions.fromNode(Node.parse(Partitions.class.getResourceAsStream("partitions.json")))
47+
.getPartitions();
48+
49+
for (Partition partition : PARTITIONS) {
50+
for (String region : partition.getRegions().keySet()) {
51+
REGION_MAP.put(region, partition);
52+
}
53+
}
54+
}
4655

4756
private AwsPartition(FunctionNode functionNode) {
4857
super(DEFINITION, functionNode);
@@ -62,37 +71,22 @@ public <T> T accept(ExpressionVisitor<T> visitor) {
6271
return visitor.visitLibraryFunction(DEFINITION, getArguments());
6372
}
6473

65-
private static PartitionData loadPartitionData() {
66-
Iterator<PartitionDataProvider> iter = ServiceLoader.load(PartitionDataProvider.class).iterator();
67-
if (!iter.hasNext()) {
68-
throw new RuntimeException("Unable to locate partition data");
69-
}
70-
71-
PartitionDataProvider provider = iter.next();
72-
73-
Partitions partitions = provider.loadPartitions();
74-
75-
PartitionData partitionData = new PartitionData();
76-
77-
partitions.getPartitions().forEach(part -> {
78-
partitionData.partitions.add(part);
79-
part.getRegions().forEach((name, override) -> {
80-
partitionData.regionMap.put(name, part);
81-
});
82-
});
83-
84-
return partitionData;
85-
}
86-
87-
private static class PartitionData {
88-
private final List<Partition> partitions = new ArrayList<>();
89-
private final Map<String, Partition> regionMap = new HashMap<>();
90-
}
91-
9274
/**
9375
* A {@link FunctionDefinition} for the {@link AwsPartition} function.
9476
*/
9577
public static final class Definition implements FunctionDefinition {
78+
private final Type returnType;
79+
80+
private Definition() {
81+
Map<Identifier, Type> type = new LinkedHashMap<>();
82+
type.put(NAME, Type.stringType());
83+
type.put(DNS_SUFFIX, Type.stringType());
84+
type.put(DUAL_STACK_DNS_SUFFIX, Type.stringType());
85+
type.put(SUPPORTS_DUAL_STACK, Type.booleanType());
86+
type.put(SUPPORTS_FIPS, Type.booleanType());
87+
returnType = Type.optionalType(Type.recordType(type));
88+
}
89+
9690
@Override
9791
public String getId() {
9892
return ID;
@@ -105,13 +99,7 @@ public List<Type> getArguments() {
10599

106100
@Override
107101
public Type getReturnType() {
108-
Map<Identifier, Type> type = new LinkedHashMap<>();
109-
type.put(NAME, Type.stringType());
110-
type.put(DNS_SUFFIX, Type.stringType());
111-
type.put(DUAL_STACK_DNS_SUFFIX, Type.stringType());
112-
type.put(SUPPORTS_DUAL_STACK, Type.booleanType());
113-
type.put(SUPPORTS_FIPS, Type.booleanType());
114-
return Type.optionalType(Type.recordType(type));
102+
return returnType;
115103
}
116104

117105
@Override
@@ -121,21 +109,22 @@ public Value evaluate(List<Value> arguments) {
121109
boolean inferred = false;
122110

123111
// Known region
124-
matchedPartition = PARTITION_DATA.regionMap.get(regionName);
112+
matchedPartition = REGION_MAP.get(regionName);
125113
if (matchedPartition == null) {
126114
// Try matching on region name pattern
127-
for (Partition p : PARTITION_DATA.partitions) {
128-
Pattern regex = Pattern.compile(p.getRegionRegex());
115+
for (Partition partition : PARTITIONS) {
116+
Pattern regex = Pattern.compile(partition.getRegionRegex());
129117
if (regex.matcher(regionName).matches()) {
130-
matchedPartition = p;
118+
matchedPartition = partition;
131119
inferred = true;
132120
break;
133121
}
134122
}
135123
}
136124

125+
// Default to the `aws` partition.
137126
if (matchedPartition == null) {
138-
for (Partition partition : PARTITION_DATA.partitions) {
127+
for (Partition partition : PARTITIONS) {
139128
if (partition.getId().equals("aws")) {
140129
matchedPartition = partition;
141130
break;

‎smithy-aws-rules-engine/src/main/java/software/amazon/smithy/rulesengine/aws/language/functions/IsVirtualHostableS3Bucket.java‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77

88
import java.util.Arrays;
99
import java.util.List;
10+
import java.util.regex.Pattern;
1011
import software.amazon.smithy.rulesengine.language.evaluation.type.Type;
1112
import software.amazon.smithy.rulesengine.language.evaluation.value.Value;
1213
import software.amazon.smithy.rulesengine.language.syntax.expressions.ExpressionVisitor;
@@ -45,6 +46,13 @@ public <T> T accept(ExpressionVisitor<T> visitor) {
4546
* A {@link FunctionDefinition} for the {@link IsVirtualHostableS3Bucket} function.
4647
*/
4748
public static final class Definition implements FunctionDefinition {
49+
private static final Pattern DOTS_ALLOWED = Pattern.compile("[a-z\\d][a-z\\d\\-.]{1,61}[a-z\\d]");
50+
private static final Pattern DOTS_DISALLOWED = Pattern.compile("[a-z\\d][a-z\\d\\-]{1,61}[a-z\\d]");
51+
private static final Pattern IP_ADDRESS = Pattern.compile("(\\d+\\.){3}\\d+");
52+
private static final Pattern DASH_DOT_SEPARATOR = Pattern.compile(".*[.-]{2}.*");
53+
54+
private Definition() {}
55+
4856
@Override
4957
public String getId() {
5058
return ID;
@@ -66,12 +74,14 @@ public Value evaluate(List<Value> arguments) {
6674
boolean allowDots = arguments.get(1).expectBooleanValue().getValue();
6775
if (allowDots) {
6876
return Value.booleanValue(
69-
hostLabel.matches("[a-z\\d][a-z\\d\\-.]{1,61}[a-z\\d]")
70-
&& !hostLabel.matches("(\\d+\\.){3}\\d+") // don't allow ip address
71-
&& !hostLabel.matches(".*[.-]{2}.*") // don't allow names like bucket-.name or bucket.-name
77+
DOTS_ALLOWED.matcher(hostLabel).matches()
78+
// Don't allow ip address
79+
&& !IP_ADDRESS.matcher(hostLabel).matches()
80+
// Don't allow names like bucket-.name or bucket.-name
81+
&& !DASH_DOT_SEPARATOR.matcher(hostLabel).matches()
7282
);
7383
} else {
74-
return Value.booleanValue(hostLabel.matches("[a-z\\d][a-z\\d\\-]{1,61}[a-z\\d]"));
84+
return Value.booleanValue(DOTS_DISALLOWED.matcher(hostLabel).matches());
7585
}
7686
}
7787

‎smithy-aws-rules-engine/src/main/java/software/amazon/smithy/rulesengine/aws/language/functions/ParseArn.java‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,9 @@
77

88
import java.util.ArrayList;
99
import java.util.Collections;
10+
import java.util.LinkedHashMap;
1011
import java.util.List;
12+
import java.util.Map;
1113
import java.util.Optional;
1214
import software.amazon.smithy.rulesengine.language.evaluation.type.Type;
1315
import software.amazon.smithy.rulesengine.language.evaluation.value.Value;
@@ -56,6 +58,18 @@ public <T> T accept(ExpressionVisitor<T> visitor) {
5658
* A {@link FunctionDefinition} for the {@link ParseArn} function.
5759
*/
5860
public static final class Definition implements FunctionDefinition {
61+
private final Type returnType;
62+
63+
private Definition() {
64+
Map<Identifier, Type> types = new LinkedHashMap<>();
65+
types.put(PARTITION, Type.stringType());
66+
types.put(SERVICE, Type.stringType());
67+
types.put(REGION, Type.stringType());
68+
types.put(ACCOUNT_ID, Type.stringType());
69+
types.put(RESOURCE_ID, Type.arrayType(Type.stringType()));
70+
returnType = Type.optionalType(Type.recordType(types));
71+
}
72+
5973
@Override
6074
public String getId() {
6175
return ID;
@@ -68,13 +82,7 @@ public List<Type> getArguments() {
6882

6983
@Override
7084
public Type getReturnType() {
71-
return Type.optionalType(Type.recordType(MapUtils.of(
72-
PARTITION, Type.stringType(),
73-
SERVICE, Type.stringType(),
74-
REGION, Type.stringType(),
75-
ACCOUNT_ID, Type.stringType(),
76-
RESOURCE_ID, Type.arrayType(Type.stringType())
77-
)));
85+
return returnType;
7886
}
7987

8088
@Override

‎smithy-aws-rules-engine/src/main/java/software/amazon/smithy/rulesengine/aws/language/functions/partition/DefaultPartitionDataProvider.java‎

Lines changed: 0 additions & 19 deletions
This file was deleted.

‎smithy-aws-rules-engine/src/main/java/software/amazon/smithy/rulesengine/aws/language/functions/partition/Partition.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ public static Partition fromNode(Node node) {
6565
ObjectNode objectNode = node.expectObjectNode();
6666
objectNode.expectObjectNode().expectNoAdditionalProperties(PROPERTIES);
6767

68-
objectNode.getStringMember(ID, builder::id);
68+
objectNode.expectStringMember(ID, builder::id);
6969
objectNode.getStringMember(REGION_REGEX, builder::regionRegex);
7070
objectNode.getObjectMember(REGIONS, regionsNode -> regionsNode.getMembers().forEach((k, v) ->
7171
builder.putRegion(k.toString(), RegionOverride.fromNode(v))));

‎smithy-aws-rules-engine/src/main/java/software/amazon/smithy/rulesengine/aws/language/functions/partition/PartitionDataProvider.java‎

Lines changed: 0 additions & 13 deletions
This file was deleted.

‎smithy-aws-rules-engine/src/main/resources/META-INF/services/software.amazon.smithy.rulesengine.aws.language.functions.partition.PartitionDataProvider‎

Lines changed: 0 additions & 1 deletion
This file was deleted.

‎smithy-aws-rules-engine/src/test/java/software/amazon/smithy/rulesengine/aws/language/functions/AwsPartitionFunctionTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ public void eval_regionNotEnumerated_inferredIsTrue() {
3737

3838

3939
private RecordValue evalWithRegion(String region) {
40-
AwsPartition fn = new AwsPartition.Definition()
40+
AwsPartition fn = AwsPartition.getDefinition()
4141
.createFunction(FunctionNode.ofExpressions(AwsPartition.ID, Expression.of(region)));
4242
return fn.accept(new RuleEvaluator()).expectRecordValue();
4343
}

‎smithy-aws-rules-engine/src/test/java/software/amazon/smithy/rulesengine/aws/language/functions/IntegrationTest.java‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@
55

66
import java.io.FileInputStream;
77
import java.io.FileNotFoundException;
8-
import java.io.IOException;
98
import java.nio.file.Files;
109
import java.nio.file.Path;
1110
import java.nio.file.Paths;
@@ -22,9 +21,9 @@
2221
import software.amazon.smithy.utils.Pair;
2322

2423
public class IntegrationTest {
25-
public static List<Pair<Node, String>> invalidRules() throws IOException {
24+
public static List<Pair<Node, String>> invalidRules() throws Exception {
2625
try (Stream<Path> paths = Files.list(
27-
Paths.get(IntegrationTest.class.getResource("invalid-rules").getPath()))
26+
Paths.get(IntegrationTest.class.getResource("invalid-rules/").toURI()))
2827
) {
2928
return paths.map(path -> {
3029
try {
@@ -40,7 +39,7 @@ public static List<Pair<Node, String>> invalidRules() throws IOException {
4039
}
4140
}
4241

43-
return Pair.of(content, String.join("\n", commentLines));
42+
return Pair.of(content, String.join(System.lineSeparator(), commentLines));
4443
} catch (FileNotFoundException e) {
4544
throw new RuntimeException(e);
4645
}

0 commit comments

Comments
 (0)