From f5f2cfad7d38a312a9415075564ec1ff60e9a637 Mon Sep 17 00:00:00 2001 From: Alex Woods Date: Wed, 26 Aug 2026 09:01:12 -0700 Subject: [PATCH 1/8] feat(endpoints): Add a result cache to the BDD endpoint provider Generate a single-entry (params -> endpoint) cache into each Default{Service}EndpointProvider produced by the BDD codegen path, so a client resolving the same endpoint repeatedly skips the BDD walk after the first call. Scoped to the BDD path only; the rules2 path that every shipped service uses is untouched, because no service model ships an endpoint-bdd-1.json yet and that keeps a caching defect away from customers while the approach is evaluated. The cache is a volatile field holding an immutable CacheEntry. Racing threads compute equivalent entries for equal params, so a lost write costs one re-resolution and needs no further synchronisation. Only successful resolutions are stored: a rule error or a no-match leaves the previous entry in place, so a bad call neither poisons the cache nor gets replayed from it. Cache-key comparison is generated per parameter from a codegen-time classification (EndpointCacheKeyClassification, computed by EndpointProviderCacheIndex), ordered cheapest check first with an early exit on mismatch: BOOLEAN - identity, which is complete for Boolean rather than merely fast, since autoboxing returns the TRUE and FALSE singletons CLIENT_STATIC_REF - identity (AWS::Region, clientContextParams) OPERATION_STATIC - identity (staticContextParams literals) SEMI_STABLE - identity then equals (SDK::Endpoint, AccountIdEndpointMode) IDENTITY_DERIVED - identity then equals (AWS::Auth::AccountId) REQUEST_DYNAMIC - identity then equals (contextParam, JMESPath) REQUEST_LIST - size-capped element-wise identity/equals Classifications are read from the BDD model's parameters, not the rule set's. The generated provider evaluates the BDD, nothing in codegen enforces that the two files agree, and a parameter absent from the key is the one defect here that returns an endpoint resolved for different inputs. Codegen fails outright rather than skipping a parameter it cannot place. Reference stability, which raises the hit rate but is not required for correctness since every string tier keeps an equals fallback: - StaticClientEndpointProvider sanitizes the client endpoint once at construction instead of rebuilding the URI per request, exposed through a new ClientEndpointProvider#sanitizedEndpointString() that AwsEndpointProviderUtils#endpointBuiltIn now delegates to. The transform has a single definition so the cached and recomputed forms cannot drift. - AccountIdEndpointMode#endpointModeValue() returns an interned literal from a field rather than name().toLowerCase(), and EndpointParamsKnowledgeIndex emits it. - EndpointResolverUtilsSpec hoists staticContextParams array values to static final unmodifiable lists. The last two also remove a per-request allocation on the rules2 path. Testing: - BddEndpointProviderCacheTest, 30 tests over the bddendpoints service: one no-stale-hit test per parameter, hit assertions via instance identity, unset transitions, equals fallbacks, the list size cap, errors never cached, and 16-thread concurrent resolution of two parameter sets. Mutation-checked: emptying the key fails 21 of 30, and dropping only clientStringParam fails exactly the test that names it. - EndpointProviderCacheIndexTest pins each parameter's tier, the comparison order, and that classification reads the BDD rather than the rule set. Tier assignment is invisible at runtime, so it is asserted here or not at all. - bddendpoints and the default-regional codegen models declare parameters their BDD graphs never read, which is how all seven tiers get covered without a BDD compiler: the node graph indexes conditions rather than naming parameters, and the cache key spans every declared parameter. - queryServiceModelsWithBddEndpoints now pairs the S3 BDD with the S3 rule set instead of the four-parameter default-regional one, so the 17 parameters in the golden file match the params class. --- .../feature-AWSSDKforJavav2-e54c03a.json | 6 + .../rules/EndpointParamsKnowledgeIndex.java | 5 +- .../poet/rules/EndpointResolverUtilsSpec.java | 61 ++- .../rules2/bdd/BddEndpointProviderSpec.java | 156 +++++++ .../bdd/EndpointCacheKeyClassification.java | 91 ++++ .../bdd/EndpointProviderCacheIndex.java | 282 ++++++++++++ .../awssdk/codegen/poet/ClientTestModels.java | 5 +- .../bdd/EndpointProviderCacheIndexTest.java | 136 ++++++ .../query/endpoint-bdd-default-regional.json | 20 + .../endpoint-rule-set-default-regional.json | 20 + ...esolver-utils-with-endpointsbasedauth.java | 2 +- ...point-resolver-utils-with-stringarray.java | 9 +- .../poet/rules/endpoint-resolver-utils.java | 2 +- .../bdd/endpoint-provider-bdd-class.java | 51 +++ .../bdd/endpoint-provider-bdd-s3-class.java | 60 +++ .../endpoints/AccountIdEndpointMode.java | 24 +- .../endpoints/AwsEndpointProviderUtils.java | 18 +- .../awssdk/core/ClientEndpointProvider.java | 15 + .../StaticClientEndpointProvider.java | 40 ++ .../bddendpoints/endpoint-bdd-1.json | 32 ++ .../bddendpoints/endpoint-rule-set.json | 32 ++ .../bddendpoints/service-2.json | 88 ++++ .../BddEndpointProviderCacheTest.java | 412 ++++++++++++++++++ 23 files changed, 1545 insertions(+), 22 deletions(-) create mode 100644 .changes/next-release/feature-AWSSDKforJavav2-e54c03a.json create mode 100644 codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/EndpointCacheKeyClassification.java create mode 100644 codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/EndpointProviderCacheIndex.java create mode 100644 codegen/src/test/java/software/amazon/awssdk/codegen/poet/rules2/bdd/EndpointProviderCacheIndexTest.java create mode 100644 test/codegen-generated-classes-test/src/test/java/software/amazon/awssdk/services/bddendpoints/BddEndpointProviderCacheTest.java diff --git a/.changes/next-release/feature-AWSSDKforJavav2-e54c03a.json b/.changes/next-release/feature-AWSSDKforJavav2-e54c03a.json new file mode 100644 index 000000000000..1d94afc6e63a --- /dev/null +++ b/.changes/next-release/feature-AWSSDKforJavav2-e54c03a.json @@ -0,0 +1,6 @@ +{ + "type": "feature", + "category": "AWS SDK for Java v2", + "contributor": "", + "description": "Remove two per-request allocations from endpoint parameter construction. A client configured with an endpoint override no longer rebuilds and re-stringifies the sanitized override URI on every request; the value is now computed once when the client endpoint is set. Operations that declare a `staticContextParams` array value now pass a shared immutable list instead of constructing an equal list per request." +} diff --git a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules/EndpointParamsKnowledgeIndex.java b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules/EndpointParamsKnowledgeIndex.java index 7d200aeae0b5..cecd08f2d2c5 100644 --- a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules/EndpointParamsKnowledgeIndex.java +++ b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules/EndpointParamsKnowledgeIndex.java @@ -191,7 +191,10 @@ public MethodSpec recordAccountIdEndpointModeMethod() { + ".ifPresent(m -> executionAttributes.getAttribute($T.BUSINESS_METRICS).addMetric(m))", BusinessMetricsUtils.class, SdkInternalExecutionAttribute.class); - builder.addStatement("return mode.name().toLowerCase()"); + // Use endpointModeValue() rather than name().toLowerCase() so that the returned String is an interned + // compile-time literal. This makes the reference stable across calls, enabling identity (==) comparison + // in the endpoint-provider result cache key check. + builder.addStatement("return mode.endpointModeValue()"); return builder.build(); } diff --git a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules/EndpointResolverUtilsSpec.java b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules/EndpointResolverUtilsSpec.java index 84677fcae0cd..6ca5cd0b80e6 100644 --- a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules/EndpointResolverUtilsSpec.java +++ b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules/EndpointResolverUtilsSpec.java @@ -22,11 +22,13 @@ import com.fasterxml.jackson.jr.stree.JrsString; import com.squareup.javapoet.ClassName; import com.squareup.javapoet.CodeBlock; +import com.squareup.javapoet.FieldSpec; import com.squareup.javapoet.MethodSpec; import com.squareup.javapoet.ParameterizedTypeName; import com.squareup.javapoet.TypeName; import com.squareup.javapoet.TypeSpec; import com.squareup.javapoet.TypeVariableName; +import java.util.Collections; import java.util.Iterator; import java.util.List; import java.util.Locale; @@ -34,6 +36,8 @@ import java.util.Objects; import java.util.Optional; import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; import javax.lang.model.element.Modifier; import software.amazon.awssdk.annotations.SdkInternalApi; import software.amazon.awssdk.awscore.AwsExecutionAttribute; @@ -119,6 +123,7 @@ public TypeSpec poetSpec() { b.addMethod(setStaticContextParamsMethod()); addStaticContextParamMethods(b); + addStaticListFields(b); b.addMethod(authSchemeWithEndpointSignerPropertiesMethod()); @@ -342,9 +347,10 @@ private MethodSpec addStaticContextParamsMethod(OperationModel opModel) { b.addStatement("params.$N($L)", setterName, ((JrsBoolean) value).booleanValue()); break; case START_ARRAY: - JrsArray arrayValue = (JrsArray) value; - CodeBlock arrayCode = endpointRulesSpecUtils.treeNodeToLiteral(arrayValue); - b.addStatement("params.$N($L)", setterName, arrayCode); + // Reference the hoisted static final field instead of constructing a new list. + // This guarantees reference stability for the endpoint-provider cache key check. + String fieldName = staticListFieldName(opModel, n); + b.addStatement("params.$N($N)", setterName, fieldName); break; default: throw new RuntimeException("Don't know how to set parameter of type " + value.asToken()); @@ -358,6 +364,55 @@ private String staticContextParamsMethodName(OperationModel opModel) { return opModel.getMethodName() + "StaticContextParams"; } + /** + * Generates the name of the {@code static final List} field holding the static array value of + * {@code paramName} for {@code opModel}. + * + *

Format: {@code STATIC_LIST_{OPERATION}_{PARAM}}, both parts in screaming snake case. + */ + private static String staticListFieldName(OperationModel opModel, String paramName) { + return "STATIC_LIST_" + screamCase(opModel.getOperationName()) + "_" + screamCase(paramName); + } + + private static String screamCase(String word) { + return Stream.of(CodegenNamingUtils.splitOnWordBoundaries(word)) + .map(s -> s.toUpperCase(Locale.US)) + .collect(Collectors.joining("_")); + } + + /** + * Generates a {@code private static final List} field for every {@code staticContextParams} entry whose + * value is an array, so that {@code setStaticContextParams} hands the same list reference to the endpoint params + * builder on every call rather than constructing an equal list each time. + * + *

Reference stability is what lets a generated endpoint provider settle a list-valued cache-key check with an + * identity comparison instead of walking the elements. It also removes a per-request list allocation on the + * request path for every operation that declares a static array parameter, which stands on its own regardless of + * whether the provider caches. + */ + private void addStaticListFields(TypeSpec.Builder classBuilder) { + ParameterizedTypeName listOfString = ParameterizedTypeName.get(List.class, String.class); + + model.getOperations().forEach((opName, opModel) -> { + Map statics = opModel.getStaticContextParams(); + if (CollectionUtils.isNullOrEmpty(statics)) { + return; + } + statics.forEach((paramName, scp) -> { + TreeNode value = scp.getValue(); + if (value.asToken() != JsonToken.START_ARRAY) { + return; + } + CodeBlock arrayCode = endpointRulesSpecUtils.treeNodeToLiteral((JrsArray) value); + FieldSpec field = FieldSpec.builder(listOfString, staticListFieldName(opModel, paramName), + Modifier.PRIVATE, Modifier.STATIC, Modifier.FINAL) + .initializer("$T.unmodifiableList($L)", Collections.class, arrayCode) + .build(); + classBuilder.addField(field); + }); + }); + } + private boolean hasStaticContextParams(OperationModel opModel) { Map staticContextParams = opModel.getStaticContextParams(); return staticContextParams != null && !staticContextParams.isEmpty(); diff --git a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpec.java b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpec.java index 3fb181eaa637..5a5e3d287a5e 100644 --- a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpec.java +++ b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpec.java @@ -87,8 +87,10 @@ public class BddEndpointProviderSpec implements ClassSpec { private final RuleRuntimeTypeMirror typeMirror; private final Map registerInfoMap; private final ClassName evaluatorType; + private final ClassName cacheEntryType; private final List bddNodes; private final List conditionTypes; + private final EndpointProviderCacheIndex cacheIndex; public BddEndpointProviderSpec(IntermediateModel intermediateModel) { this.intermediateModel = intermediateModel; @@ -99,8 +101,10 @@ public BddEndpointProviderSpec(IntermediateModel intermediateModel) { this.knownEndpointAttributes = knownEndpointAttributes(intermediateModel); this.registerInfoMap = buildRegisterInfoMap(); this.evaluatorType = className().nestedClass("Evaluator"); + this.cacheEntryType = className().nestedClass("CacheEntry"); this.bddNodes = endpointBddModel.getDecodedNodes(); this.conditionTypes = analyzeConditions(); + this.cacheIndex = EndpointProviderCacheIndex.of(intermediateModel); } @Override @@ -110,12 +114,152 @@ public TypeSpec poetSpec() { .addSuperinterface(endpointRulesSpecUtils.providerInterfaceName()) .addAnnotation(SdkInternalApi.class); + builder.addField(cacheField()); builder.addType(evaluatorClass()); + builder.addType(cacheEntryClass()); builder.addMethod(resolveEndpointMethod()); + builder.addMethod(cacheParamsMatchMethod()); return builder.build(); } + // ---- Single-entry result cache ---- + + /** + * Generates {@code private volatile CacheEntry cache;}. + * + *

One entry, holding the most recent successfully resolved {@code (params, endpoint)} pair. A single entry is + * enough because the overwhelmingly common shape is a client resolving the same endpoint repeatedly: same region, + * same flags, and for most services no request-derived parameters at all. A service whose endpoint genuinely varies + * per request simply misses every time and pays only the key check. + * + *

{@code volatile} is the whole of the synchronisation. Racing threads compute equivalent entries for equal + * params, so a lost write costs one re-resolution and nothing more; {@code CacheEntry} is immutable with final + * fields, so a thread that reads the reference sees fully initialised contents. + */ + private FieldSpec cacheField() { + return FieldSpec.builder(cacheEntryType, "cache") + .addModifiers(Modifier.PRIVATE, Modifier.VOLATILE) + .build(); + } + + /** + * Generates the immutable {@code CacheEntry} holding one {@code (params, endpoint)} snapshot. + */ + private TypeSpec cacheEntryClass() { + ClassName paramsClass = endpointRulesSpecUtils.parametersClassName(); + return TypeSpec.classBuilder(cacheEntryType) + .addModifiers(Modifier.PRIVATE, Modifier.STATIC, Modifier.FINAL) + .addField(paramsClass, "params", Modifier.FINAL) + .addField(Endpoint.class, "endpoint", Modifier.FINAL) + .addMethod(MethodSpec.constructorBuilder() + .addParameter(paramsClass, "params") + .addParameter(Endpoint.class, "endpoint") + .addStatement("this.params = params") + .addStatement("this.endpoint = endpoint") + .build()) + .build(); + } + + /** + * Generates {@code cacheParamsMatch(a, b)}: true when the two parameter objects are interchangeable as far as + * endpoint resolution is concerned. + * + *

Every parameter the BDD model declares is compared. Parameters are ordered by + * {@link EndpointCacheKeyClassification}, cheapest comparison first, and the method returns on the first mismatch. + * + *

A parameter that {@link EndpointProviderCacheIndex} classified but that the model no longer declares would be + * a hole in the key, so that combination fails codegen instead of generating a comparison that quietly skips it. + */ + private MethodSpec cacheParamsMatchMethod() { + ClassName paramsClass = endpointRulesSpecUtils.parametersClassName(); + Map parameters = endpointBddModel.getParameters(); + + MethodSpec.Builder b = MethodSpec.methodBuilder("cacheParamsMatch") + .addModifiers(Modifier.PRIVATE, Modifier.STATIC) + .returns(boolean.class) + .addParameter(paramsClass, "a") + .addParameter(paramsClass, "b"); + + int listIndex = 0; + for (Map.Entry entry : cacheIndex.classifiedParameters().entrySet()) { + String paramName = entry.getKey(); + if (!parameters.containsKey(paramName)) { + throw new IllegalStateException( + "Endpoint parameter '" + paramName + "' was classified for the result cache but is not declared by " + + "the BDD model. Leaving it out of the cache key would let the provider return an endpoint " + + "resolved for a different value of it."); + } + String getter = endpointRulesSpecUtils.paramMethodName(paramName) + "()"; + if (entry.getValue() == EndpointCacheKeyClassification.REQUEST_LIST) { + addListParamCheck(b, getter, listIndex++); + } else { + addScalarParamCheck(b, getter, entry.getValue()); + } + } + + b.addStatement("return true"); + return b.build(); + } + + /** + * Emits the comparison for one non-list parameter, returning false on mismatch. + * + *

{@code BOOLEAN}, {@code CLIENT_STATIC_REF} and {@code OPERATION_STATIC} compare references only. For booleans + * that is complete, not just fast: autoboxing hands back the {@code Boolean.TRUE}/{@code Boolean.FALSE} singletons. + * For the other two the SDK hands the same reference to every request, and if it ever does not, the result is a + * miss and a re-resolution rather than a wrong endpoint. + * + *

The remaining classifications add an {@code equals} fallback so that an equal value arriving as a fresh + * reference still hits. + */ + private static void addScalarParamCheck(MethodSpec.Builder b, String getter, EndpointCacheKeyClassification cat) { + switch (cat) { + case BOOLEAN: + case CLIENT_STATIC_REF: + case OPERATION_STATIC: + b.addStatement("if (a.$L != b.$L) return false", getter, getter); + break; + default: + b.beginControlFlow("if (a.$L != b.$L)", getter, getter); + b.beginControlFlow("if (a.$L == null || !a.$L.equals(b.$L))", getter, getter, getter); + b.addStatement("return false"); + b.endControlFlow(); + b.endControlFlow(); + break; + } + } + + /** + * Emits the comparison for one {@code stringArray} parameter: identity, then null, then size, then the element + * walk. The size cap keeps the check bounded so a request carrying a large list cannot make the cache check itself + * a cost worth avoiding. + * + *

{@code idx} suffixes the local variable names so several list parameters can be compared in one method. + */ + private static void addListParamCheck(MethodSpec.Builder b, String getter, int idx) { + String listA = "listA" + idx; + String listB = "listB" + idx; + String i = "i" + idx; + String elementA = "elementA" + idx; + String elementB = "elementB" + idx; + TypeName listOfString = RuleRuntimeTypeMirror.LIST_OF_STRING.type(); + + b.addStatement("$T $L = a.$L", listOfString, listA, getter); + b.addStatement("$T $L = b.$L", listOfString, listB, getter); + b.beginControlFlow("if ($L != $L)", listA, listB); + b.addStatement("if ($L == null || $L == null) return false", listA, listB); + b.addStatement("if ($L.size() != $L.size()) return false", listA, listB); + b.addStatement("if ($L.size() > $L) return false", listA, EndpointProviderCacheIndex.MAX_LIST_COMPARISON_SIZE); + b.beginControlFlow("for (int $L = 0; $L < $L.size(); $L++)", i, i, listA, i); + b.addStatement("$T $L = $L.get($L)", String.class, elementA, listA, i); + b.addStatement("$T $L = $L.get($L)", String.class, elementB, listB, i); + b.addStatement("if ($L != $L && ($L == null || !$L.equals($L))) return false", + elementA, elementB, elementA, elementA, elementB); + b.endControlFlow(); + b.endControlFlow(); + } + private TypeSpec evaluatorClass() { TypeSpec.Builder builder = TypeSpec.classBuilder(evaluatorType) .addModifiers(Modifier.PRIVATE, Modifier.STATIC, Modifier.FINAL); @@ -386,6 +530,15 @@ private MethodSpec resolveEndpointMethod() { builder.addCode(validateRequiredParams()); + // Cache check. This sits after required-param validation so that invalid params fail the same way on a hit as + // on a miss. One volatile read into a local, so the entry cannot be replaced between the null check and the + // comparison. + builder.addComment("Single-entry result cache: reuse the last endpoint when the params still match."); + builder.addStatement("$T cached = this.cache", cacheEntryType); + builder.beginControlFlow("if (cached != null && cacheParamsMatch(endpointParams, cached.params))"); + builder.addStatement("return $T.completedFuture(cached.endpoint)", CompletableFuture.class); + builder.endControlFlow(); + builder.beginControlFlow("try"); // Allocate evaluator per call — lightweight (just fields, no maps), immediately young-gen collected. @@ -409,6 +562,9 @@ private MethodSpec resolveEndpointMethod() { CompletableFutureUtils.class, SdkClientException.class, "Rule engine did not reach an error or endpoint result") .endControlFlow(); + // Populate on success only. A rule error and a no-match both leave the previous entry in place, so a transient + // bad-params call cannot poison the cache and an error is never replayed from it. + builder.addStatement("this.cache = new $T(endpointParams, result)", cacheEntryType); builder.addStatement("return $T.completedFuture(result)", CompletableFuture.class); // Catch errors thrown from result methods diff --git a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/EndpointCacheKeyClassification.java b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/EndpointCacheKeyClassification.java new file mode 100644 index 000000000000..29a9f3a33570 --- /dev/null +++ b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/EndpointCacheKeyClassification.java @@ -0,0 +1,91 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://aws.amazon.com/apache2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + */ + +package software.amazon.awssdk.codegen.poet.rules2.bdd; + +/** + * Classifies one endpoint parameter to decide how the generated BDD endpoint provider compares it when checking its + * single-entry result cache. + * + *

Values are declared cheapest comparison first. {@code BddEndpointProviderSpec} emits the parameter checks in this + * order and returns on the first mismatch, so the parameters most likely to differ between two requests are also the + * ones reached last. + * + *

Why a wrong classification cannot return a wrong endpoint

+ * + *

Every classification compares references first and either stops there or falls back to {@code equals}. Comparing + * only references can report a mismatch for two equal values, which costs a re-resolution; it can never report a match + * for two different values, because equal references imply the same object. So over-classifying a parameter as stable + * costs hit rate, not correctness. + * + *

What does affect correctness is a parameter being left out of the comparison altogether. That is why + * {@link EndpointProviderCacheIndex} classifies every parameter the BDD model declares and + * {@code BddEndpointProviderSpec} fails codegen rather than skipping one it cannot classify. + */ +public enum EndpointCacheKeyClassification { + /** + * A {@code boolean} parameter. Compared with {@code ==} and no fallback, which is complete rather than merely fast: + * autoboxing and {@code Boolean.valueOf} both hand back the {@code Boolean.TRUE}/{@code Boolean.FALSE} singletons, + * so identity agrees with {@code equals} for every value a caller can produce short of the {@code Boolean} + * constructor deprecated in Java 9. + */ + BOOLEAN, + + /** + * A string parameter sourced entirely from client configuration, where the same reference is handed to every + * request: {@code AWS::Region} (interned by {@code Region.of}) and {@code clientContextParams} (read from the + * client's {@code AttributeMap}). Compared with {@code ==} only. + */ + CLIENT_STATIC_REF, + + /** + * A string parameter bound to a {@code staticContextParams} literal. {@code EndpointResolverUtilsSpec} emits string + * literals and hoists array values to {@code static final} fields, so the reference is fixed per operation. + * Compared with {@code ==} only. List-valued static params are classified {@link #REQUEST_LIST} instead, since they + * share the list comparison shape. + */ + OPERATION_STATIC, + + /** + * A string parameter that is logically fixed for the life of the client but whose reference stability depends on an + * implementation detail a customer can replace: {@code SDK::Endpoint}, stable only because + * {@code StaticClientEndpointProvider} computes the sanitized string once, and + * {@code AWS::Auth::AccountIdEndpointMode}, stable only because {@code AccountIdEndpointMode.endpointModeValue} + * returns an interned literal. A custom {@code ClientEndpointProvider} need not cache. Compared with {@code ==}, + * then {@code equals}. + */ + SEMI_STABLE, + + /** + * {@code AWS::Auth::AccountId}, read off the resolved identity. Stable while the credentials provider serves the + * same cached identity, and a fresh reference after every refresh. Compared with {@code ==}, then {@code equals}. + */ + IDENTITY_DERIVED, + + /** + * A string parameter bound to a request member ({@code contextParam}) or extracted from the request by JMESPath + * ({@code operationContextParams}). Generally a fresh reference per request; the identity check still pays for + * itself when one API call resolves the endpoint more than once from the same request object. Compared with + * {@code ==}, then {@code equals}. + */ + REQUEST_DYNAMIC, + + /** + * A {@code stringArray} parameter. Compared last, with a null-safe identity check, then size, then element-wise + * identity or {@code equals}. Lists longer than {@link EndpointProviderCacheIndex#MAX_LIST_COMPARISON_SIZE} report + * a miss without being walked, so the key check stays bounded no matter how large the request is. + */ + REQUEST_LIST +} diff --git a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/EndpointProviderCacheIndex.java b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/EndpointProviderCacheIndex.java new file mode 100644 index 000000000000..b4a581aa2198 --- /dev/null +++ b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/EndpointProviderCacheIndex.java @@ -0,0 +1,282 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://aws.amazon.com/apache2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + */ + +package software.amazon.awssdk.codegen.poet.rules2.bdd; + +import static software.amazon.awssdk.codegen.poet.rules2.bdd.EndpointCacheKeyClassification.BOOLEAN; +import static software.amazon.awssdk.codegen.poet.rules2.bdd.EndpointCacheKeyClassification.CLIENT_STATIC_REF; +import static software.amazon.awssdk.codegen.poet.rules2.bdd.EndpointCacheKeyClassification.IDENTITY_DERIVED; +import static software.amazon.awssdk.codegen.poet.rules2.bdd.EndpointCacheKeyClassification.OPERATION_STATIC; +import static software.amazon.awssdk.codegen.poet.rules2.bdd.EndpointCacheKeyClassification.REQUEST_DYNAMIC; +import static software.amazon.awssdk.codegen.poet.rules2.bdd.EndpointCacheKeyClassification.REQUEST_LIST; +import static software.amazon.awssdk.codegen.poet.rules2.bdd.EndpointCacheKeyClassification.SEMI_STABLE; + +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.Map; +import software.amazon.awssdk.codegen.model.intermediate.IntermediateModel; +import software.amazon.awssdk.codegen.model.intermediate.MemberModel; +import software.amazon.awssdk.codegen.model.intermediate.OperationModel; +import software.amazon.awssdk.codegen.model.rules.endpoints.BuiltInParameter; +import software.amazon.awssdk.codegen.model.rules.endpoints.ParameterModel; +import software.amazon.awssdk.codegen.model.service.ClientContextParam; +import software.amazon.awssdk.codegen.model.service.ContextParam; +import software.amazon.awssdk.codegen.model.service.StaticContextParam; +import software.amazon.awssdk.utils.CollectionUtils; + +/** + * Computes the {@link EndpointCacheKeyClassification} of every endpoint parameter, at codegen time, for + * {@code BddEndpointProviderSpec} to turn into the tiered {@code cacheParamsMatch} method inside + * {@code Default{Service}EndpointProvider}. + * + *

Parameter source

+ * + *

Parameters come from the BDD model, not the rule set, because the BDD is what the generated provider + * evaluates. The two agree for a service whose BDD was compiled from its own rule set, but nothing in codegen enforces + * that, and the codegen test models deliberately pair mismatched files. Reading the rule set here would silently drop + * every parameter the BDD declares and the rule set does not, and a parameter missing from the cache key is the one way + * this cache can hand back an endpoint resolved for different inputs. + * + *

Classification

+ * + *
    + *
  1. Seed each parameter from its own declaration: type first, then built-in, then + * {@code clientContextParams} membership.
    2. + *
  2. Scan every operation for binding sites ({@code contextParam}, {@code staticContextParams}, + * {@code operationContextParams}) and promote the parameter to the more dynamic classification. Most dynamic + * wins, so a parameter bound statically by one operation and from the request by another is compared the way the + * request-bound operation needs.
    3. + *
+ * + *

The returned map is ordered by classification, cheapest first, then by name within a classification. That ordering + * is the generated comparison order, so it is deterministic across builds. + */ +public final class EndpointProviderCacheIndex { + /** + * Lists longer than this report a cache miss without element-wise comparison, bounding the cost of the key check + * regardless of request size. Eight covers the list-valued endpoint parameters shipped today while keeping the + * worst-case check comfortably cheaper than a re-resolution. + */ + public static final int MAX_LIST_COMPARISON_SIZE = 8; + + private final IntermediateModel model; + + private EndpointProviderCacheIndex(IntermediateModel model) { + this.model = model; + } + + public static EndpointProviderCacheIndex of(IntermediateModel model) { + return new EndpointProviderCacheIndex(model); + } + + /** + * Returns the more dynamic of two classifications. Used with {@link Map#merge} to implement most-dynamic-wins. + */ + public static EndpointCacheKeyClassification moreExpensive(EndpointCacheKeyClassification existing, + EndpointCacheKeyClassification candidate) { + return candidate.ordinal() > existing.ordinal() ? candidate : existing; + } + + /** + * Returns an unmodifiable map from parameter name to classification, ordered cheapest comparison first. + */ + public Map classifiedParameters() { + Map parameters = model.getEndpointBddModel().getParameters(); + Map clientContextParams = model.getClientContextParams(); + + // A null value means "not decided from the declaration alone; let the binding sites decide". Map.merge treats a + // null value as absent, so the first binding site simply wins and later ones promote from there. + Map result = new LinkedHashMap<>(); + parameters.forEach((name, pm) -> result.put(name, initialClassification(name, pm, clientContextParams))); + + for (OperationModel op : model.getOperations().values()) { + promoteForContextParams(op, parameters, result); + promoteForStaticContextParams(op, parameters, result); + promoteForOperationContextParams(op, parameters, result); + } + + // No binding site named it, so we cannot say where its value comes from. Assume per-request. + result.replaceAll((name, category) -> category == null ? REQUEST_DYNAMIC : category); + + Map sorted = new LinkedHashMap<>(); + result.entrySet().stream() + .sorted((a, b) -> { + int cmp = Integer.compare(a.getValue().ordinal(), b.getValue().ordinal()); + return cmp != 0 ? cmp : a.getKey().compareTo(b.getKey()); + }) + .forEach(e -> sorted.put(e.getKey(), e.getValue())); + return Collections.unmodifiableMap(sorted); + } + + /** + * A {@code contextParam} binds the parameter to a member of the request, so its value arrives fresh per request. + */ + private static void promoteForContextParams(OperationModel op, + Map parameters, + Map result) { + if (op.getInputShape() == null) { + return; + } + for (MemberModel member : op.getInputShape().getMembers()) { + ContextParam cp = member.getContextParam(); + if (cp == null) { + continue; + } + String paramName = findParamName(parameters, cp.getName()); + if (paramName != null) { + result.merge(paramName, REQUEST_DYNAMIC, EndpointProviderCacheIndex::moreExpensive); + } + } + } + + /** + * A {@code staticContextParam} is a literal fixed at codegen time. List values still take the list comparison. + */ + private static void promoteForStaticContextParams(OperationModel op, + Map parameters, + Map result) { + Map statics = op.getStaticContextParams(); + if (CollectionUtils.isNullOrEmpty(statics)) { + return; + } + statics.forEach((paramName, scp) -> { + String canonicalName = findParamName(parameters, paramName); + if (canonicalName == null) { + return; + } + EndpointCacheKeyClassification category = + isList(parameters.get(canonicalName)) ? REQUEST_LIST : OPERATION_STATIC; + result.merge(canonicalName, category, EndpointProviderCacheIndex::moreExpensive); + }); + } + + /** + * An {@code operationContextParam} is a JMESPath expression evaluated over the request, producing a fresh value, + * and a fresh list when the parameter is a {@code stringArray}. + */ + private static void promoteForOperationContextParams(OperationModel op, + Map parameters, + Map result) { + if (CollectionUtils.isNullOrEmpty(op.getOperationContextParams())) { + return; + } + op.getOperationContextParams().forEach((paramName, ocp) -> { + String canonicalName = findParamName(parameters, paramName); + if (canonicalName == null) { + return; + } + EndpointCacheKeyClassification category = + isList(parameters.get(canonicalName)) ? REQUEST_LIST : REQUEST_DYNAMIC; + result.merge(canonicalName, category, EndpointProviderCacheIndex::moreExpensive); + }); + } + + /** + * Classifies a parameter from its declaration alone. Returns {@code null} when the declaration does not determine + * the classification and the operation binding sites should, which is the case for a plain string parameter that is + * neither a built-in nor a client context param: whether it is an {@code OPERATION_STATIC} literal or arrives from + * the request is visible only at the binding site. + */ + private static EndpointCacheKeyClassification initialClassification(String paramName, + ParameterModel pm, + Map clientContextParams) { + if (isBoolean(pm)) { + return BOOLEAN; + } + + if (isList(pm)) { + return REQUEST_LIST; + } + + // A built-in wins over a clientContextParams entry that happens to share the parameter's name. + BuiltInParameter builtIn = pm.getBuiltInEnum(); + if (builtIn != null) { + return classifyBuiltIn(builtIn); + } + + if (clientContextParams != null && isClientContextParam(paramName, clientContextParams)) { + return CLIENT_STATIC_REF; + } + + return null; + } + + private static EndpointCacheKeyClassification classifyBuiltIn(BuiltInParameter builtIn) { + switch (builtIn) { + case AWS_REGION: + return CLIENT_STATIC_REF; + case SDK_ENDPOINT: + case AWS_AUTH_ACCOUNT_ID_ENDPOINT_MODE: + return SEMI_STABLE; + case AWS_AUTH_ACCOUNT_ID: + return IDENTITY_DERIVED; + case AWS_USE_DUAL_STACK: + case AWS_USE_FIPS: + case AWS_S3_ACCELERATE: + case AWS_S3_CONTROL_USE_ARN_REGION: + case AWS_S3_DISABLE_MULTI_REGION_ACCESS_POINTS: + case AWS_S3_FORCE_PATH_STYLE: + case AWS_S3_USE_ARN_REGION: + case AWS_S3_USE_GLOBAL_ENDPOINT: + case AWS_STS_USE_GLOBAL_ENDPOINT: + // Every one of these is declared boolean in practice and is intercepted by the boolean check above. + // Reaching here means a model declared one as a string; they are all client-level config either way, + // so identity is the right comparison. + return CLIENT_STATIC_REF; + default: + // A built-in added to BuiltInParameter but not yet considered here. Compare with equals as a fallback + // rather than assuming a stable reference we have not verified. + return SEMI_STABLE; + } + } + + private static boolean isClientContextParam(String paramName, Map clientContextParams) { + if (clientContextParams.containsKey(paramName)) { + return true; + } + for (String key : clientContextParams.keySet()) { + if (key.equalsIgnoreCase(paramName)) { + return true; + } + } + return false; + } + + private static boolean isBoolean(ParameterModel pm) { + return "boolean".equalsIgnoreCase(pm.getType()); + } + + private static boolean isList(ParameterModel pm) { + return "stringarray".equalsIgnoreCase(pm.getType()); + } + + /** + * Resolves a parameter name as spelled at a binding site to the key used in the parameters map, which may differ in + * capitalisation. Returns {@code null} when the binding site names a parameter the model does not declare, which is + * legitimate: a service can bind a context param that its endpoint rules never read. + */ + private static String findParamName(Map parameters, String name) { + if (parameters.containsKey(name)) { + return name; + } + // Endpoint parameter names are unique case-insensitively. + for (String key : parameters.keySet()) { + if (key.equalsIgnoreCase(name)) { + return key; + } + } + return null; + } +} diff --git a/codegen/src/test/java/software/amazon/awssdk/codegen/poet/ClientTestModels.java b/codegen/src/test/java/software/amazon/awssdk/codegen/poet/ClientTestModels.java index 8137ad2213f5..7722d224e416 100644 --- a/codegen/src/test/java/software/amazon/awssdk/codegen/poet/ClientTestModels.java +++ b/codegen/src/test/java/software/amazon/awssdk/codegen/poet/ClientTestModels.java @@ -656,8 +656,11 @@ public static IntermediateModel queryServiceModelsWithBddEndpoints() { public static IntermediateModel queryServiceModelsWithBddEndpoints(boolean useS3ExpressSessionAuth) { File serviceModel = new File(ClientTestModels.class.getResource("client/c2j/query/service-2.json").getFile()); File waitersModel = new File(ClientTestModels.class.getResource("client/c2j/query/waiters-2.json").getFile()); + // The S3 rule set, not the default-regional one, because it declares the same 17 parameters as the S3 BDD. The + // generated params class comes from the rule set while the provider body comes from the BDD, so pairing the S3 + // BDD with a 4-parameter rule set would produce a provider referencing getters the params class does not have. File endpointRuleSetModel = - new File(ClientTestModels.class.getResource("client/c2j/query/endpoint-rule-set-default-regional.json").getFile()); + new File(ClientTestModels.class.getResource("client/c2j/s3-test/endpoint-rule-set.json").getFile()); File endpointTestsModel = new File(ClientTestModels.class.getResource("client/c2j/query/endpoint-tests.json").getFile()); File endpointBddModel = diff --git a/codegen/src/test/java/software/amazon/awssdk/codegen/poet/rules2/bdd/EndpointProviderCacheIndexTest.java b/codegen/src/test/java/software/amazon/awssdk/codegen/poet/rules2/bdd/EndpointProviderCacheIndexTest.java new file mode 100644 index 000000000000..8d1790cb76cf --- /dev/null +++ b/codegen/src/test/java/software/amazon/awssdk/codegen/poet/rules2/bdd/EndpointProviderCacheIndexTest.java @@ -0,0 +1,136 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://aws.amazon.com/apache2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + */ + +package software.amazon.awssdk.codegen.poet.rules2.bdd; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.entry; + +import java.util.ArrayList; +import java.util.Map; +import org.junit.jupiter.api.Test; +import software.amazon.awssdk.codegen.model.intermediate.IntermediateModel; +import software.amazon.awssdk.codegen.poet.ClientTestModels; + +/** + * Classification tests for the BDD endpoint provider result cache. + * + *

Classification is not observable from runtime behaviour: identity-only and identity-then-equals both invalidate + * when a value changes, and both hit when the reference is the same. The difference is only in how much work a hit + * costs. So the tier a parameter lands in has to be asserted here, or it is not covered at all. + */ +public class EndpointProviderCacheIndexTest { + + private static Map classify(IntermediateModel model) { + return EndpointProviderCacheIndex.of(model).classifiedParameters(); + } + + /** + * Every parameter the BDD declares must be classified. A parameter missing from this map is a parameter missing + * from the generated cache key, which is the one defect here that yields a wrong endpoint rather than a slow one. + */ + @Test + void everyBddParameterIsClassified() { + assertThat(classify(ClientTestModels.queryServiceModelsWithSimpleBddEndpoints())) + .containsOnlyKeys("Region", "UseDualStack", "UseFIPS", "Endpoint", + "stringContextParam", "staticStringParam", "operationContextParam", "arnList"); + } + + /** + * Pins the tier of each parameter shape. In particular {@code staticStringParam} must be {@code OPERATION_STATIC}: + * a parameter whose only binding site is a {@code staticContextParams} literal is fixed per operation, so it needs + * no {@code equals} fallback. Getting this wrong is invisible at runtime, so it is asserted rather than inferred. + */ + @Test + void parametersAreClassifiedByTheirBindingSite() { + assertThat(classify(ClientTestModels.queryServiceModelsWithSimpleBddEndpoints())) + .contains(entry("UseDualStack", EndpointCacheKeyClassification.BOOLEAN), + entry("UseFIPS", EndpointCacheKeyClassification.BOOLEAN), + entry("Region", EndpointCacheKeyClassification.CLIENT_STATIC_REF), + entry("stringContextParam", EndpointCacheKeyClassification.CLIENT_STATIC_REF), + entry("staticStringParam", EndpointCacheKeyClassification.OPERATION_STATIC), + entry("Endpoint", EndpointCacheKeyClassification.SEMI_STABLE), + entry("operationContextParam", EndpointCacheKeyClassification.REQUEST_DYNAMIC), + entry("arnList", EndpointCacheKeyClassification.REQUEST_LIST)); + } + + /** + * The generated comparison order is this map's iteration order, so it has to be deterministic and cheapest-first. + * Ordering the checks the other way round would still be correct but would pay for the expensive comparisons before + * the cheap ones had a chance to exit. + */ + @Test + void parametersAreOrderedCheapestComparisonFirst() { + Map classified = + classify(ClientTestModels.queryServiceModelsWithSimpleBddEndpoints()); + + assertThat(new ArrayList<>(classified.values())) + .isSortedAccordingTo((a, b) -> Integer.compare(a.ordinal(), b.ordinal())); + assertThat(classified.keySet()) + .containsExactly("UseDualStack", "UseFIPS", // BOOLEAN + "Region", "stringContextParam", // CLIENT_STATIC_REF + "staticStringParam", // OPERATION_STATIC + "Endpoint", // SEMI_STABLE + "operationContextParam", // REQUEST_DYNAMIC + "arnList"); // REQUEST_LIST + } + + /** + * A parameter no binding site names could come from anywhere, so it gets the conservative string classification + * rather than being assumed stable. The S3 BDD is paired with the query service model, which binds none of S3's + * request parameters, so this is the shape that model produces. + */ + @Test + void unboundStringParameterFallsBackToRequestDynamic() { + assertThat(classify(ClientTestModels.queryServiceModelsWithBddEndpoints())) + .contains(entry("Bucket", EndpointCacheKeyClassification.REQUEST_DYNAMIC), + entry("Key", EndpointCacheKeyClassification.REQUEST_DYNAMIC), + entry("CopySource", EndpointCacheKeyClassification.REQUEST_DYNAMIC), + entry("Prefix", EndpointCacheKeyClassification.REQUEST_DYNAMIC)); + } + + /** + * Built-ins are classified from the built-in rather than from a name collision with a client context param, and the + * two whose reference stability rests on an SDK implementation detail keep their {@code equals} fallback. + */ + @Test + void builtInsAreClassifiedFromTheBuiltIn() { + Map classified = + classify(ClientTestModels.queryServiceModelsWithBddEndpoints()); + + assertThat(classified) + .contains(entry("Region", EndpointCacheKeyClassification.CLIENT_STATIC_REF), + entry("Endpoint", EndpointCacheKeyClassification.SEMI_STABLE), + entry("UseFIPS", EndpointCacheKeyClassification.BOOLEAN), + entry("UseDualStack", EndpointCacheKeyClassification.BOOLEAN), + entry("Accelerate", EndpointCacheKeyClassification.BOOLEAN), + entry("UseArnRegion", EndpointCacheKeyClassification.BOOLEAN)); + } + + /** + * Classification is read off the BDD, not the rule set, because the BDD is what the generated provider evaluates. + * The complement model pairs a two-parameter BDD with the eight-parameter default-regional rule set, so reading the + * rule set here would silently add six parameters the provider cannot use — and, in the opposite pairing, silently + * drop parameters it does use. + */ + @Test + void classificationComesFromTheBddNotTheRuleSet() { + IntermediateModel model = ClientTestModels.queryServiceModelsWithComplementBddEndpoints(); + + assertThat(model.getEndpointRuleSetModel().getParameters()).hasSize(8); + assertThat(model.getEndpointBddModel().getParameters()).containsOnlyKeys("Endpoint", "Region"); + assertThat(classify(model)).containsOnlyKeys("Endpoint", "Region"); + } +} diff --git a/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/client/c2j/query/endpoint-bdd-default-regional.json b/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/client/c2j/query/endpoint-bdd-default-regional.json index 9f7c8fc3a7e4..c3c6d4fd961f 100644 --- a/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/client/c2j/query/endpoint-bdd-default-regional.json +++ b/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/client/c2j/query/endpoint-bdd-default-regional.json @@ -26,6 +26,26 @@ "required": false, "documentation": "Override the endpoint used to send this request", "type": "string" + }, + "stringContextParam": { + "required": false, + "documentation": "A client context parameter. Covers the CLIENT_STATIC_REF cache tier.", + "type": "string" + }, + "staticStringParam": { + "required": false, + "documentation": "Bound to a per-operation static literal. Covers the OPERATION_STATIC cache tier.", + "type": "string" + }, + "operationContextParam": { + "required": false, + "documentation": "Bound to a request member. Covers the REQUEST_DYNAMIC cache tier.", + "type": "string" + }, + "arnList": { + "required": false, + "documentation": "Extracted from the request by JMESPath. Covers the REQUEST_LIST cache tier.", + "type": "stringArray" } }, "conditions": [ diff --git a/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/client/c2j/query/endpoint-rule-set-default-regional.json b/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/client/c2j/query/endpoint-rule-set-default-regional.json index e0314767c0df..6d822ec9f08d 100644 --- a/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/client/c2j/query/endpoint-rule-set-default-regional.json +++ b/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/client/c2j/query/endpoint-rule-set-default-regional.json @@ -26,6 +26,26 @@ "required": false, "documentation": "Override the endpoint used to send this request", "type": "string" + }, + "stringContextParam": { + "required": false, + "documentation": "A client context parameter. Covers the CLIENT_STATIC_REF cache tier.", + "type": "string" + }, + "staticStringParam": { + "required": false, + "documentation": "Bound to a per-operation static literal. Covers the OPERATION_STATIC cache tier.", + "type": "string" + }, + "operationContextParam": { + "required": false, + "documentation": "Bound to a request member. Covers the REQUEST_DYNAMIC cache tier.", + "type": "string" + }, + "arnList": { + "required": false, + "documentation": "Extracted from the request by JMESPath. Covers the REQUEST_LIST cache tier.", + "type": "stringArray" } }, "rules": [ diff --git a/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules/endpoint-resolver-utils-with-endpointsbasedauth.java b/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules/endpoint-resolver-utils-with-endpointsbasedauth.java index ad3b2e674750..a98fee735480 100644 --- a/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules/endpoint-resolver-utils-with-endpointsbasedauth.java +++ b/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules/endpoint-resolver-utils-with-endpointsbasedauth.java @@ -191,7 +191,7 @@ private static String recordAccountIdEndpointMode(ExecutionAttributes executionA AccountIdEndpointMode mode = executionAttributes.getAttribute(AwsExecutionAttribute.AWS_AUTH_ACCOUNT_ID_ENDPOINT_MODE); BusinessMetricsUtils.resolveAccountIdEndpointModeMetric(mode).ifPresent( m -> executionAttributes.getAttribute(SdkInternalExecutionAttribute.BUSINESS_METRICS).addMetric(m)); - return mode.name().toLowerCase(); + return mode.endpointModeValue(); } public static void setMetricValues(Endpoint endpoint, ExecutionAttributes executionAttributes) { diff --git a/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules/endpoint-resolver-utils-with-stringarray.java b/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules/endpoint-resolver-utils-with-stringarray.java index aaed43624739..1d21848b0041 100644 --- a/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules/endpoint-resolver-utils-with-stringarray.java +++ b/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules/endpoint-resolver-utils-with-stringarray.java @@ -1,6 +1,7 @@ package software.amazon.awssdk.services.samplesvc.endpoints.internal; import java.util.Arrays; +import java.util.Collections; import java.util.List; import java.util.Optional; import software.amazon.awssdk.annotations.Generated; @@ -28,6 +29,10 @@ @Generated("software.amazon.awssdk:codegen") @SdkInternalApi public final class SampleSvcEndpointResolverUtils { + private static final List STATIC_LIST_EMPTY_STATIC_CONTEXT_OPERATION_STRING_ARRAY_PARAM = Collections.unmodifiableList(Arrays.asList()); + + private static final List STATIC_LIST_STATIC_CONTEXT_OPERATION_STRING_ARRAY_PARAM = Collections.unmodifiableList(Arrays.asList("staticValue1")); + private SampleSvcEndpointResolverUtils() { } @@ -57,12 +62,12 @@ private static void setStaticContextParams(SampleSvcEndpointParams.Builder param private static void emptyStaticContextOperationStaticContextParams( SampleSvcEndpointParams.Builder params) { - params.stringArrayParam(Arrays.asList()); + params.stringArrayParam(STATIC_LIST_EMPTY_STATIC_CONTEXT_OPERATION_STRING_ARRAY_PARAM); } private static void staticContextOperationStaticContextParams( SampleSvcEndpointParams.Builder params) { - params.stringArrayParam(Arrays.asList("staticValue1")); + params.stringArrayParam(STATIC_LIST_STATIC_CONTEXT_OPERATION_STRING_ARRAY_PARAM); } public static SelectedAuthScheme authSchemeWithEndpointSignerProperties( diff --git a/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules/endpoint-resolver-utils.java b/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules/endpoint-resolver-utils.java index 01c7ba43831f..7c723c51914c 100644 --- a/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules/endpoint-resolver-utils.java +++ b/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules/endpoint-resolver-utils.java @@ -198,7 +198,7 @@ private static String recordAccountIdEndpointMode(ExecutionAttributes executionA AccountIdEndpointMode mode = executionAttributes.getAttribute(AwsExecutionAttribute.AWS_AUTH_ACCOUNT_ID_ENDPOINT_MODE); BusinessMetricsUtils.resolveAccountIdEndpointModeMetric(mode).ifPresent( m -> executionAttributes.getAttribute(SdkInternalExecutionAttribute.BUSINESS_METRICS).addMetric(m)); - return mode.name().toLowerCase(); + return mode.endpointModeValue(); } public static void setMetricValues(Endpoint endpoint, ExecutionAttributes executionAttributes) { diff --git a/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules2/bdd/endpoint-provider-bdd-class.java b/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules2/bdd/endpoint-provider-bdd-class.java index 39e5da5ba9ff..14ed368284b0 100644 --- a/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules2/bdd/endpoint-provider-bdd-class.java +++ b/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules2/bdd/endpoint-provider-bdd-class.java @@ -1,5 +1,6 @@ package software.amazon.awssdk.services.query.endpoints.internal; +import java.util.List; import java.util.concurrent.CompletableFuture; import software.amazon.awssdk.annotations.Generated; import software.amazon.awssdk.annotations.SdkInternalApi; @@ -13,8 +14,15 @@ @Generated("software.amazon.awssdk:codegen") @SdkInternalApi public final class DefaultQueryEndpointProvider implements QueryEndpointProvider { + private volatile CacheEntry cache; + @Override public CompletableFuture resolveEndpoint(QueryEndpointParams endpointParams) { + // Single-entry result cache: reuse the last endpoint when the params still match. + CacheEntry cached = this.cache; + if (cached != null && cacheParamsMatch(endpointParams, cached.params)) { + return CompletableFuture.completedFuture(cached.endpoint); + } try { Evaluator evaluator = new Evaluator(); evaluator.params = endpointParams; @@ -23,6 +31,7 @@ public CompletableFuture resolveEndpoint(QueryEndpointParams endpointP if (result == null) { return CompletableFutureUtils.failedFuture(SdkClientException.create("Rule engine did not reach an error or endpoint result")); } + this.cache = new CacheEntry(endpointParams, result); return CompletableFuture.completedFuture(result); } catch (SdkClientException e) { String errorMsg = e.getMessage(); @@ -35,6 +44,37 @@ public CompletableFuture resolveEndpoint(QueryEndpointParams endpointP } } + private static boolean cacheParamsMatch(QueryEndpointParams a, QueryEndpointParams b) { + if (a.useDualStack() != b.useDualStack()) return false; + if (a.useFips() != b.useFips()) return false; + if (a.region() != b.region()) return false; + if (a.stringContextParam() != b.stringContextParam()) return false; + if (a.staticStringParam() != b.staticStringParam()) return false; + if (a.endpoint() != b.endpoint()) { + if (a.endpoint() == null || !a.endpoint().equals(b.endpoint())) { + return false; + } + } + if (a.operationContextParam() != b.operationContextParam()) { + if (a.operationContextParam() == null || !a.operationContextParam().equals(b.operationContextParam())) { + return false; + } + } + List listA0 = a.arnList(); + List listB0 = b.arnList(); + if (listA0 != listB0) { + if (listA0 == null || listB0 == null) return false; + if (listA0.size() != listB0.size()) return false; + if (listA0.size() > 8) return false; + for (int i0 = 0; i0 < listA0.size(); i0++) { + String elementA0 = listA0.get(i0); + String elementB0 = listB0.get(i0); + if (elementA0 != elementB0 && (elementA0 == null || !elementA0.equals(elementB0))) return false; + } + } + return true; + } + private static final class Evaluator { QueryEndpointParams params; @@ -195,4 +235,15 @@ private Endpoint result11() { throw SdkClientException.create("Invalid Configuration: Missing Region"); } } + + private static final class CacheEntry { + final QueryEndpointParams params; + + final Endpoint endpoint; + + CacheEntry(QueryEndpointParams params, Endpoint endpoint) { + this.params = params; + this.endpoint = endpoint; + } + } } diff --git a/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules2/bdd/endpoint-provider-bdd-s3-class.java b/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules2/bdd/endpoint-provider-bdd-s3-class.java index bb661bf8c9db..0da2bfa737e9 100644 --- a/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules2/bdd/endpoint-provider-bdd-s3-class.java +++ b/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules2/bdd/endpoint-provider-bdd-s3-class.java @@ -18,8 +18,15 @@ @Generated("software.amazon.awssdk:codegen") @SdkInternalApi public final class DefaultQueryEndpointProvider implements QueryEndpointProvider { + private volatile CacheEntry cache; + @Override public CompletableFuture resolveEndpoint(QueryEndpointParams endpointParams) { + // Single-entry result cache: reuse the last endpoint when the params still match. + CacheEntry cached = this.cache; + if (cached != null && cacheParamsMatch(endpointParams, cached.params)) { + return CompletableFuture.completedFuture(cached.endpoint); + } try { Evaluator evaluator = new Evaluator(); evaluator.params = endpointParams; @@ -28,6 +35,7 @@ public CompletableFuture resolveEndpoint(QueryEndpointParams endpointP if (result == null) { return CompletableFutureUtils.failedFuture(SdkClientException.create("Rule engine did not reach an error or endpoint result")); } + this.cache = new CacheEntry(endpointParams, result); return CompletableFuture.completedFuture(result); } catch (SdkClientException e) { String errorMsg = e.getMessage(); @@ -40,6 +48,47 @@ public CompletableFuture resolveEndpoint(QueryEndpointParams endpointP } } + private static boolean cacheParamsMatch(QueryEndpointParams a, QueryEndpointParams b) { + if (a.accelerate() != b.accelerate()) return false; + if (a.disableAccessPoints() != b.disableAccessPoints()) return false; + if (a.disableMultiRegionAccessPoints() != b.disableMultiRegionAccessPoints()) return false; + if (a.disableS3ExpressSessionAuth() != b.disableS3ExpressSessionAuth()) return false; + if (a.forcePathStyle() != b.forcePathStyle()) return false; + if (a.useArnRegion() != b.useArnRegion()) return false; + if (a.useDualStack() != b.useDualStack()) return false; + if (a.useFips() != b.useFips()) return false; + if (a.useGlobalEndpoint() != b.useGlobalEndpoint()) return false; + if (a.useObjectLambdaEndpoint() != b.useObjectLambdaEndpoint()) return false; + if (a.useS3ExpressControlEndpoint() != b.useS3ExpressControlEndpoint()) return false; + if (a.region() != b.region()) return false; + if (a.endpoint() != b.endpoint()) { + if (a.endpoint() == null || !a.endpoint().equals(b.endpoint())) { + return false; + } + } + if (a.bucket() != b.bucket()) { + if (a.bucket() == null || !a.bucket().equals(b.bucket())) { + return false; + } + } + if (a.copySource() != b.copySource()) { + if (a.copySource() == null || !a.copySource().equals(b.copySource())) { + return false; + } + } + if (a.key() != b.key()) { + if (a.key() == null || !a.key().equals(b.key())) { + return false; + } + } + if (a.prefix() != b.prefix()) { + if (a.prefix() == null || !a.prefix().equals(b.prefix())) { + return false; + } + } + return true; + } + private static final class Evaluator { QueryEndpointParams params; @@ -4099,4 +4148,15 @@ private Endpoint result114() { throw SdkClientException.create("A region must be set when sending requests to S3."); } } + + private static final class CacheEntry { + final QueryEndpointParams params; + + final Endpoint endpoint; + + CacheEntry(QueryEndpointParams params, Endpoint endpoint) { + this.params = params; + this.endpoint = endpoint; + } + } } diff --git a/core/aws-core/src/main/java/software/amazon/awssdk/awscore/endpoints/AccountIdEndpointMode.java b/core/aws-core/src/main/java/software/amazon/awssdk/awscore/endpoints/AccountIdEndpointMode.java index 7e6f0050dc4e..6d894e39252e 100644 --- a/core/aws-core/src/main/java/software/amazon/awssdk/awscore/endpoints/AccountIdEndpointMode.java +++ b/core/aws-core/src/main/java/software/amazon/awssdk/awscore/endpoints/AccountIdEndpointMode.java @@ -26,19 +26,25 @@ public enum AccountIdEndpointMode { /** * Default value that indicates account ID values will be used in endpoint rules if available. */ - PREFERRED, + PREFERRED("preferred"), /** * When mode is disabled, any resolved account ID will not be used in endpoint construction and rules that * reference them will be bypassed. */ - DISABLED, + DISABLED("disabled"), /** * Required mode would be used in scenarios where endpoint resolution should return an error if no account ID is * available. */ - REQUIRED; + REQUIRED("required"); + + private final String endpointModeValue; + + AccountIdEndpointMode(String endpointModeValue) { + this.endpointModeValue = endpointModeValue; + } /** * Returns the appropriate AccountIdEndpointMode value after parsing the parameter. @@ -59,4 +65,16 @@ public static AccountIdEndpointMode fromValue(String s) { throw new IllegalArgumentException("Unrecognized value for account id endpoint mode: " + s); } + + /** + * Returns the canonical lowercase string for this mode, as the endpoint rules engine expects to receive it in the + * {@code AWS::Auth::AccountIdEndpointMode} built-in. + *

+ * Unlike {@code name().toLowerCase()}, this returns the same interned {@link String} reference on every call rather + * than a fresh string per request. That removes an allocation from the request path and lets a generated endpoint + * provider compare the value by identity. + */ + public String endpointModeValue() { + return endpointModeValue; + } } diff --git a/core/aws-core/src/main/java/software/amazon/awssdk/awscore/endpoints/AwsEndpointProviderUtils.java b/core/aws-core/src/main/java/software/amazon/awssdk/awscore/endpoints/AwsEndpointProviderUtils.java index b2ee07acbf8b..a745cc7c75aa 100644 --- a/core/aws-core/src/main/java/software/amazon/awssdk/awscore/endpoints/AwsEndpointProviderUtils.java +++ b/core/aws-core/src/main/java/software/amazon/awssdk/awscore/endpoints/AwsEndpointProviderUtils.java @@ -15,8 +15,6 @@ package software.amazon.awssdk.awscore.endpoints; -import static software.amazon.awssdk.utils.FunctionalUtils.invokeSafely; - import java.net.URI; import software.amazon.awssdk.annotations.SdkProtectedApi; import software.amazon.awssdk.awscore.AwsExecutionAttribute; @@ -54,19 +52,19 @@ public static Boolean fipsEnabledBuiltIn(ExecutionAttributes executionAttributes } /** - * Returns the endpoint set on the client. Note that this strips off the query part of the URI because the endpoint - * rules library, e.g. {@code ParseURL} will return an exception if the URI it parses has query parameters. + * Returns the endpoint set on the client, sanitized for the rules engine. The rules engine (e.g. + * {@code ParseURL}) rejects URIs with query parameters, so we strip the query and user-info components. + *

+ * Delegates to {@link ClientEndpointProvider#sanitizedEndpointString()}, which returns a cached reference on + * {@link software.amazon.awssdk.core.internal.StaticClientEndpointProvider}, enabling identity ({@code ==}) + * comparison inside the endpoint-provider result cache. */ public static String endpointBuiltIn(ExecutionAttributes executionAttributes) { if (endpointIsOverridden(executionAttributes)) { executionAttributes.getOptionalAttribute(SdkInternalExecutionAttribute.BUSINESS_METRICS).ifPresent( metric -> metric.addMetric(BusinessMetricFeatureId.ENDPOINT_OVERRIDE.value())); - return invokeSafely(() -> { - URI endpointOverride = executionAttributes.getAttribute(SdkInternalExecutionAttribute.CLIENT_ENDPOINT_PROVIDER) - .clientEndpoint(); - return new URI(endpointOverride.getScheme(), null, endpointOverride.getHost(), endpointOverride.getPort(), - endpointOverride.getPath(), null, endpointOverride.getFragment()).toString(); - }); + return executionAttributes.getAttribute(SdkInternalExecutionAttribute.CLIENT_ENDPOINT_PROVIDER) + .sanitizedEndpointString(); } return null; } diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/ClientEndpointProvider.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/ClientEndpointProvider.java index 500dd446af4d..4a4443237271 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/ClientEndpointProvider.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/ClientEndpointProvider.java @@ -48,6 +48,21 @@ static ClientEndpointProvider create(URI uri, boolean isEndpointOverridden) { */ URI clientEndpoint(); + /** + * Returns the sanitized endpoint string suitable for passing to the endpoint rules engine as the + * {@code SDK::Endpoint} built-in. The default implementation strips query and user-info components on every call; + * implementations backed by a static URI (see {@link #create(URI, boolean)}) override this to return a cached + * reference, enabling identity ({@code ==}) comparisons in the endpoint-provider result cache. + *

+ * Returns {@code null} if the endpoint is not overridden. + */ + default String sanitizedEndpointString() { + if (!isEndpointOverridden()) { + return null; + } + return StaticClientEndpointProvider.sanitizeEndpoint(clientEndpoint()); + } + /** * Returns true if this endpoint was specified as an override by the customer, or false if it was determined * automatically by the SDK. diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/StaticClientEndpointProvider.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/StaticClientEndpointProvider.java index 40a9cd38f2c3..019a89514810 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/StaticClientEndpointProvider.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/StaticClientEndpointProvider.java @@ -18,6 +18,7 @@ import java.net.URI; import software.amazon.awssdk.annotations.SdkInternalApi; import software.amazon.awssdk.core.ClientEndpointProvider; +import software.amazon.awssdk.utils.FunctionalUtils; import software.amazon.awssdk.utils.ToString; import software.amazon.awssdk.utils.Validate; @@ -31,10 +32,49 @@ public class StaticClientEndpointProvider implements ClientEndpointProvider { private final URI clientEndpoint; private final boolean isEndpointOverridden; + /** + * A sanitized form of {@link #clientEndpoint} with the query and user-info components stripped, formatted as a + * string. This is the value that endpoint rules receive via the {@code SDK::Endpoint} built-in. Computed once at + * construction so that every call to {@code endpointBuiltIn()} returns the same {@link String} reference, enabling + * identity ({@code ==}) comparison inside the endpoint-provider cache key check. + *

+ * {@code null} when {@link #isEndpointOverridden} is {@code false}. + */ + private final String sanitizedEndpointString; + public StaticClientEndpointProvider(URI clientEndpoint, boolean isEndpointOverridden) { this.clientEndpoint = Validate.paramNotNull(clientEndpoint, "clientEndpoint"); this.isEndpointOverridden = isEndpointOverridden; Validate.paramNotNull(clientEndpoint.getScheme(), "The URI scheme of endpointOverride"); + this.sanitizedEndpointString = isEndpointOverridden ? sanitizeEndpoint(clientEndpoint) : null; + } + + /** + * Strips the query and user-info components from the given endpoint URI and returns the result as a string. + * This matches the transformation performed by the rules engine's {@code ParseURL} function, which rejects + * URIs with query parameters. + *

+ * This is the single definition of that transformation: {@link ClientEndpointProvider#sanitizedEndpointString()} + * delegates here so that a provider which recomputes the value per call and one which caches it at construction + * cannot drift apart. If they drifted, the value used as an endpoint cache key would no longer be the value the + * rules engine actually resolved against. + */ + public static String sanitizeEndpoint(URI endpoint) { + return FunctionalUtils.invokeSafely( + () -> new URI(endpoint.getScheme(), null, endpoint.getHost(), endpoint.getPort(), + endpoint.getPath(), null, endpoint.getFragment()).toString()); + } + + /** + * {@inheritDoc} + *

+ * Returns the same {@link String} reference on every call, because the value is computed once at construction. + * That lets a generated endpoint provider settle its {@code SDK::Endpoint} cache-key check with an identity + * ({@code ==}) comparison instead of falling through to {@code equals}. + */ + @Override + public String sanitizedEndpointString() { + return sanitizedEndpointString; } @Override diff --git a/test/codegen-generated-classes-test/src/main/resources/codegen-resources/bddendpoints/endpoint-bdd-1.json b/test/codegen-generated-classes-test/src/main/resources/codegen-resources/bddendpoints/endpoint-bdd-1.json index 157f1b1237d4..4a2dd64848c1 100644 --- a/test/codegen-generated-classes-test/src/main/resources/codegen-resources/bddendpoints/endpoint-bdd-1.json +++ b/test/codegen-generated-classes-test/src/main/resources/codegen-resources/bddendpoints/endpoint-bdd-1.json @@ -26,6 +26,38 @@ "required": false, "documentation": "Override the endpoint used to send this request", "type": "string" + }, + "AccountId": { + "builtIn": "AWS::Auth::AccountId", + "required": false, + "documentation": "The AWS account ID, read off the resolved identity. Declared to exercise the IDENTITY_DERIVED cache tier; the BDD graph does not read it.", + "type": "string" + }, + "AccountIdEndpointMode": { + "builtIn": "AWS::Auth::AccountIdEndpointMode", + "required": false, + "documentation": "Whether the account ID may be used in the endpoint. Declared to exercise the SEMI_STABLE cache tier; the BDD graph does not read it.", + "type": "string" + }, + "clientStringParam": { + "required": false, + "documentation": "A client context parameter. Declared to exercise the CLIENT_STATIC_REF cache tier; the BDD graph does not read it.", + "type": "string" + }, + "staticStringParam": { + "required": false, + "documentation": "A parameter bound to a per-operation static literal. Declared to exercise the OPERATION_STATIC cache tier; the BDD graph does not read it.", + "type": "string" + }, + "requestStringParam": { + "required": false, + "documentation": "A parameter bound to a request member. Declared to exercise the REQUEST_DYNAMIC cache tier; the BDD graph does not read it.", + "type": "string" + }, + "resourceArnList": { + "required": false, + "documentation": "A list extracted from the request by JMESPath. Declared to exercise the REQUEST_LIST cache tier; the BDD graph does not read it.", + "type": "stringArray" } }, "conditions": [ diff --git a/test/codegen-generated-classes-test/src/main/resources/codegen-resources/bddendpoints/endpoint-rule-set.json b/test/codegen-generated-classes-test/src/main/resources/codegen-resources/bddendpoints/endpoint-rule-set.json index c87dbf336b6d..11f51c22fffa 100644 --- a/test/codegen-generated-classes-test/src/main/resources/codegen-resources/bddendpoints/endpoint-rule-set.json +++ b/test/codegen-generated-classes-test/src/main/resources/codegen-resources/bddendpoints/endpoint-rule-set.json @@ -26,6 +26,38 @@ "required": false, "documentation": "Override the endpoint used to send this request", "type": "string" + }, + "AccountId": { + "builtIn": "AWS::Auth::AccountId", + "required": false, + "documentation": "The AWS account ID, read off the resolved identity. Declared to exercise the IDENTITY_DERIVED cache tier; the BDD graph does not read it.", + "type": "string" + }, + "AccountIdEndpointMode": { + "builtIn": "AWS::Auth::AccountIdEndpointMode", + "required": false, + "documentation": "Whether the account ID may be used in the endpoint. Declared to exercise the SEMI_STABLE cache tier; the BDD graph does not read it.", + "type": "string" + }, + "clientStringParam": { + "required": false, + "documentation": "A client context parameter. Declared to exercise the CLIENT_STATIC_REF cache tier; the BDD graph does not read it.", + "type": "string" + }, + "staticStringParam": { + "required": false, + "documentation": "A parameter bound to a per-operation static literal. Declared to exercise the OPERATION_STATIC cache tier; the BDD graph does not read it.", + "type": "string" + }, + "requestStringParam": { + "required": false, + "documentation": "A parameter bound to a request member. Declared to exercise the REQUEST_DYNAMIC cache tier; the BDD graph does not read it.", + "type": "string" + }, + "resourceArnList": { + "required": false, + "documentation": "A list extracted from the request by JMESPath. Declared to exercise the REQUEST_LIST cache tier; the BDD graph does not read it.", + "type": "stringArray" } }, "rules": [ diff --git a/test/codegen-generated-classes-test/src/main/resources/codegen-resources/bddendpoints/service-2.json b/test/codegen-generated-classes-test/src/main/resources/codegen-resources/bddendpoints/service-2.json index c8ef99738ea8..a77daf8c70ce 100644 --- a/test/codegen-generated-classes-test/src/main/resources/codegen-resources/bddendpoints/service-2.json +++ b/test/codegen-generated-classes-test/src/main/resources/codegen-resources/bddendpoints/service-2.json @@ -15,6 +15,12 @@ "aws.auth#sigv4" ] }, + "clientContextParams": { + "clientStringParam": { + "documentation": "A client-level string context parameter.", + "type": "string" + } + }, "operations": { "TestOperation": { "name": "TestOperation", @@ -28,6 +34,55 @@ "output": { "shape": "TestOperationResponse" } + }, + "OperationWithStaticParam": { + "name": "OperationWithStaticParam", + "http": { + "method": "POST", + "requestUri": "/" + }, + "staticContextParams": { + "staticStringParam": { + "value": "static-value" + } + }, + "input": { + "shape": "TestOperationRequest" + }, + "output": { + "shape": "TestOperationResponse" + } + }, + "OperationWithContextParam": { + "name": "OperationWithContextParam", + "http": { + "method": "POST", + "requestUri": "/" + }, + "input": { + "shape": "ContextParamInput" + }, + "output": { + "shape": "TestOperationResponse" + } + }, + "OperationWithListContextParam": { + "name": "OperationWithListContextParam", + "http": { + "method": "POST", + "requestUri": "/" + }, + "operationContextParams": { + "resourceArnList": { + "path": "Items[*].Arn" + } + }, + "input": { + "shape": "ListContextParamInput" + }, + "output": { + "shape": "TestOperationResponse" + } } }, "shapes": { @@ -47,6 +102,39 @@ } } }, + "ContextParamInput": { + "type": "structure", + "members": { + "RequestMember": { + "shape": "String", + "contextParam": { + "name": "requestStringParam" + } + } + } + }, + "ListContextParamInput": { + "type": "structure", + "members": { + "Items": { + "shape": "ItemList" + } + } + }, + "ItemList": { + "type": "list", + "member": { + "shape": "Item" + } + }, + "Item": { + "type": "structure", + "members": { + "Arn": { + "shape": "String" + } + } + }, "String": { "type": "string" } diff --git a/test/codegen-generated-classes-test/src/test/java/software/amazon/awssdk/services/bddendpoints/BddEndpointProviderCacheTest.java b/test/codegen-generated-classes-test/src/test/java/software/amazon/awssdk/services/bddendpoints/BddEndpointProviderCacheTest.java new file mode 100644 index 000000000000..b5a64682ca24 --- /dev/null +++ b/test/codegen-generated-classes-test/src/test/java/software/amazon/awssdk/services/bddendpoints/BddEndpointProviderCacheTest.java @@ -0,0 +1,412 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://aws.amazon.com/apache2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + */ + +package software.amazon.awssdk.services.bddendpoints; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import java.util.concurrent.Callable; +import java.util.concurrent.CompletionException; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.function.Consumer; +import java.util.stream.Collectors; +import java.util.stream.IntStream; +import org.junit.jupiter.api.Test; +import software.amazon.awssdk.endpoints.Endpoint; +import software.amazon.awssdk.regions.Region; +import software.amazon.awssdk.services.bddendpoints.endpoints.BddEndpointsEndpointParams; +import software.amazon.awssdk.services.bddendpoints.endpoints.BddEndpointsEndpointProvider; + +/** + * Behavioural tests for the single-entry result cache generated into the BDD endpoint provider. + * + *

The suite is organised around the one invariant that matters: a cache hit must be indistinguishable from a fresh + * resolution. That splits into two obligations. + * + *

    + *
  1. No stale hit. Changing any single parameter must not return the endpoint resolved for the previous value. + * Every parameter the model declares gets its own test, because a parameter accidentally left out of the + * generated key check is the one defect here that produces a wrong endpoint rather than a slow one.
    2. + *
  2. Hits where they are due. Equal params must actually reuse the cached instance, otherwise the change is + * cost without benefit. Asserted with {@code isSameAs}, which is the only externally visible evidence that the + * cache was consulted.
    3. + *
+ * + *

The model declares parameters its BDD graph never reads, so that every classification tier is represented. Those + * parameters cannot change the resolved URL, which makes them the more interesting cases to test: a stale hit is + * detectable only by instance identity, not by comparing hosts. + */ +class BddEndpointProviderCacheTest { + private static final Region REGION = Region.US_EAST_1; + private static final Region OTHER_REGION = Region.US_WEST_2; + + /** + * Returns params that resolve successfully, with every optional parameter left unset. + */ + private static BddEndpointsEndpointParams.Builder baseBuilder() { + return BddEndpointsEndpointParams.builder() + .region(REGION) + .useDualStack(false) + .useFips(false); + } + + private static BddEndpointsEndpointParams params(Consumer customizer) { + BddEndpointsEndpointParams.Builder builder = baseBuilder(); + customizer.accept(builder); + return builder.build(); + } + + private static BddEndpointsEndpointProvider provider() { + return BddEndpointsEndpointProvider.defaultProvider(); + } + + private static Endpoint resolve(BddEndpointsEndpointProvider provider, BddEndpointsEndpointParams params) { + return provider.resolveEndpoint(params).join(); + } + + /** + * Resolves {@code first}, then {@code second}, and asserts the second call did not reuse the first result. + * + *

Instance identity rather than URL comparison, so this works for the parameters that do not influence the + * resolved URL. Those are exactly the parameters where a missing key check would go unnoticed. + */ + private static void assertInvalidates(BddEndpointsEndpointParams first, BddEndpointsEndpointParams second) { + BddEndpointsEndpointProvider provider = provider(); + Endpoint firstEndpoint = resolve(provider, first); + Endpoint secondEndpoint = resolve(provider, second); + assertThat(secondEndpoint).isNotSameAs(firstEndpoint); + } + + private static void assertHits(BddEndpointsEndpointParams first, BddEndpointsEndpointParams second) { + BddEndpointsEndpointProvider provider = provider(); + Endpoint firstEndpoint = resolve(provider, first); + Endpoint secondEndpoint = resolve(provider, second); + assertThat(secondEndpoint).isSameAs(firstEndpoint); + } + + // ---- hits ---- + + @Test + void sameParamsInstance_reusesCachedEndpoint() { + BddEndpointsEndpointParams p = params(b -> { + }); + assertHits(p, p); + } + + @Test + void distinctButEqualParams_reusesCachedEndpoint() { + assertHits(params(b -> { + }), params(b -> { + })); + } + + @Test + void cacheIsPerProviderInstance() { + BddEndpointsEndpointParams p = params(b -> { + }); + Endpoint fromFirstProvider = resolve(provider(), p); + Endpoint fromSecondProvider = resolve(provider(), p); + assertThat(fromSecondProvider).isNotSameAs(fromFirstProvider); + assertThat(fromSecondProvider.endpointUrl().host()).isEqualTo(fromFirstProvider.endpointUrl().host()); + } + + /** + * The cached endpoint must be the one the params call for, not merely some previously resolved endpoint. Alternating + * between two parameter sets in a loop would pass even if the cache returned the wrong entry, so each round asserts + * the host as well. + */ + @Test + void alternatingParams_eachResolutionMatchesItsOwnParams() { + BddEndpointsEndpointProvider provider = provider(); + BddEndpointsEndpointParams plain = params(b -> { + }); + BddEndpointsEndpointParams fips = params(b -> b.useFips(true)); + + String plainHost = resolve(provider, plain).endpointUrl().host(); + String fipsHost = resolve(provider, fips).endpointUrl().host(); + assertThat(plainHost).doesNotContain("fips"); + assertThat(fipsHost).contains("fips"); + + for (int i = 0; i < 4; i++) { + assertThat(resolve(provider, plain).endpointUrl().host()).isEqualTo(plainHost); + assertThat(resolve(provider, fips).endpointUrl().host()).isEqualTo(fipsHost); + } + } + + // ---- no stale hit, one test per parameter ---- + + @Test + void booleanTier_useFipsChange_invalidates() { + assertInvalidates(params(b -> { + }), params(b -> b.useFips(true))); + } + + @Test + void booleanTier_useDualStackChange_invalidates() { + assertInvalidates(params(b -> { + }), params(b -> b.useDualStack(true))); + } + + @Test + void clientStaticRefTier_regionChange_invalidates() { + assertInvalidates(params(b -> { + }), params(b -> b.region(OTHER_REGION))); + } + + @Test + void clientStaticRefTier_clientStringParamChange_invalidates() { + assertInvalidates(params(b -> b.clientStringParam("first")), + params(b -> b.clientStringParam("second"))); + } + + @Test + void operationStaticTier_staticStringParamChange_invalidates() { + assertInvalidates(params(b -> b.staticStringParam("first")), + params(b -> b.staticStringParam("second"))); + } + + @Test + void semiStableTier_endpointOverrideChange_invalidates() { + assertInvalidates(params(b -> b.endpoint("https://first.example.com")), + params(b -> b.endpoint("https://second.example.com"))); + } + + @Test + void semiStableTier_accountIdEndpointModeChange_invalidates() { + assertInvalidates(params(b -> b.accountIdEndpointMode("preferred")), + params(b -> b.accountIdEndpointMode("disabled"))); + } + + @Test + void identityDerivedTier_accountIdChange_invalidates() { + assertInvalidates(params(b -> b.accountId("111111111111")), + params(b -> b.accountId("222222222222"))); + } + + @Test + void requestDynamicTier_requestStringParamChange_invalidates() { + assertInvalidates(params(b -> b.requestStringParam("first")), + params(b -> b.requestStringParam("second"))); + } + + @Test + void requestListTier_elementChange_invalidates() { + assertInvalidates(params(b -> b.resourceArnList(Arrays.asList("a", "b"))), + params(b -> b.resourceArnList(Arrays.asList("a", "c")))); + } + + @Test + void requestListTier_lengthChange_invalidates() { + assertInvalidates(params(b -> b.resourceArnList(Arrays.asList("a", "b"))), + params(b -> b.resourceArnList(Collections.singletonList("a")))); + } + + @Test + void requestListTier_orderChange_invalidates() { + assertInvalidates(params(b -> b.resourceArnList(Arrays.asList("a", "b"))), + params(b -> b.resourceArnList(Arrays.asList("b", "a")))); + } + + // ---- transitions to and from unset ---- + + @Test + void settingAPreviouslyUnsetParam_invalidates() { + assertInvalidates(params(b -> { + }), params(b -> b.requestStringParam("now-set"))); + } + + @Test + void clearingAPreviouslySetParam_invalidates() { + assertInvalidates(params(b -> b.requestStringParam("was-set")), params(b -> { + })); + } + + @Test + void settingAPreviouslyUnsetList_invalidates() { + assertInvalidates(params(b -> { + }), params(b -> b.resourceArnList(Collections.singletonList("a")))); + } + + @Test + void clearingAPreviouslySetList_invalidates() { + assertInvalidates(params(b -> b.resourceArnList(Collections.singletonList("a"))), params(b -> { + })); + } + + @Test + void emptyListAndUnsetList_areDistinguished() { + assertInvalidates(params(b -> b.resourceArnList(Collections.emptyList())), params(b -> { + })); + } + + // ---- equals fallback ---- + + /** + * The tiers with an {@code equals} fallback must hit on an equal value arriving as a fresh reference. Without the + * fallback, a request-derived string would miss on every call and the cache would never pay off for the services + * that need it most. + */ + @Test + void equalsFallbackTiers_equalValueDifferentReference_hits() { + String value = "shared-value"; + String copy = new String(value); + assertThat(value).isNotSameAs(copy); + + assertHits(params(b -> b.requestStringParam(value)), params(b -> b.requestStringParam(copy))); + assertHits(params(b -> b.accountId(value)), params(b -> b.accountId(copy))); + + String url = "https://override.example.com"; + assertHits(params(b -> b.endpoint(url)), params(b -> b.endpoint(new String(url)))); + } + + @Test + void requestList_equalContentsDifferentListInstance_hits() { + assertHits(params(b -> b.resourceArnList(new ArrayList<>(Arrays.asList("a", "b")))), + params(b -> b.resourceArnList(new ArrayList<>(Arrays.asList("a", "b"))))); + } + + /** + * Element comparison also falls back to {@code equals}, so equal strings held by different references still hit. + */ + @Test + void requestList_equalElementsDifferentReferences_hits() { + assertHits(params(b -> b.resourceArnList(Collections.singletonList("element"))), + params(b -> b.resourceArnList(Collections.singletonList(new String("element"))))); + } + + // ---- list size cap ---- + + /** + * At the cap the element walk still runs, so equal lists hit. + */ + @Test + void requestList_atSizeCap_stillHits() { + assertHits(params(b -> b.resourceArnList(listOfSize(8))), params(b -> b.resourceArnList(listOfSize(8)))); + } + + /** + * Past the cap the check bails out and reports a miss without walking the elements, which keeps the key check + * bounded. Equal lists therefore stop hitting; that is a deliberate cost ceiling, not a defect, and it is pinned + * here so that changing the cap is a conscious decision. + */ + @Test + void requestList_pastSizeCap_alwaysMisses() { + assertInvalidates(params(b -> b.resourceArnList(listOfSize(9))), + params(b -> b.resourceArnList(listOfSize(9)))); + } + + /** + * Even a miss must still resolve correctly, so an oversized list is not a functional break. + */ + @Test + void requestList_pastSizeCap_stillResolvesCorrectly() { + Endpoint endpoint = resolve(provider(), params(b -> b.resourceArnList(listOfSize(50)))); + assertThat(endpoint.endpointUrl().host()).isEqualTo("connect.us-east-1.amazonaws.com"); + } + + private static List listOfSize(int size) { + return IntStream.range(0, size).mapToObj(i -> "element-" + i).collect(Collectors.toList()); + } + + // ---- failures are never cached ---- + + /** + * A rule error must not be stored, and must not evict a good entry. Replaying a cached failure would turn one bad + * call into a permanently broken client. + */ + @Test + void ruleError_isNotCached_andLeavesEarlierEntryIntact() { + BddEndpointsEndpointProvider provider = provider(); + BddEndpointsEndpointParams good = params(b -> { + }); + Endpoint first = resolve(provider, good); + + // FIPS combined with an endpoint override is an error in this rule set. + BddEndpointsEndpointParams bad = params(b -> b.useFips(true).endpoint("https://override.example.com")); + assertThatThrownBy(() -> resolve(provider, bad)).isInstanceOf(CompletionException.class); + + assertThat(resolve(provider, good)).isSameAs(first); + // Still an error the second time, rather than a replayed success. + assertThatThrownBy(() -> resolve(provider, bad)).isInstanceOf(CompletionException.class); + } + + @Test + void missingRegion_isNotCached() { + BddEndpointsEndpointProvider provider = provider(); + BddEndpointsEndpointParams noRegion = BddEndpointsEndpointParams.builder() + .useDualStack(false) + .useFips(false) + .build(); + assertThatThrownBy(() -> resolve(provider, noRegion)).isInstanceOf(CompletionException.class); + assertThatThrownBy(() -> resolve(provider, noRegion)).isInstanceOf(CompletionException.class); + + assertThat(resolve(provider, params(b -> { + })).endpointUrl().host()).isEqualTo("connect.us-east-1.amazonaws.com"); + } + + // ---- concurrency ---- + + /** + * Concurrent resolution of two distinct parameter sets against one provider. The cache field is written without any + * lock, so threads race to overwrite it; every thread must still receive the endpoint its own params call for. A + * torn or misattributed entry shows up here as a host mismatch. + */ + @Test + void concurrentResolution_neverReturnsAnotherThreadsEndpoint() throws Exception { + BddEndpointsEndpointProvider provider = provider(); + BddEndpointsEndpointParams plain = params(b -> { + }); + BddEndpointsEndpointParams dualStack = params(b -> b.useDualStack(true)); + String plainHost = resolve(provider(), plain).endpointUrl().host(); + String dualStackHost = resolve(provider(), dualStack).endpointUrl().host(); + assertThat(plainHost).isNotEqualTo(dualStackHost); + + int threads = 16; + int iterations = 500; + ExecutorService executor = Executors.newFixedThreadPool(threads); + try { + CountDownLatch start = new CountDownLatch(1); + List> tasks = new ArrayList<>(); + for (int t = 0; t < threads; t++) { + boolean useDualStack = t % 2 == 0; + BddEndpointsEndpointParams params = useDualStack ? dualStack : plain; + String expectedHost = useDualStack ? dualStackHost : plainHost; + tasks.add(() -> { + start.await(); + for (int i = 0; i < iterations; i++) { + assertThat(resolve(provider, params).endpointUrl().host()).isEqualTo(expectedHost); + } + return null; + }); + } + List> futures = tasks.stream().map(executor::submit).collect(Collectors.toList()); + start.countDown(); + for (Future future : futures) { + future.get(60, TimeUnit.SECONDS); + } + } finally { + executor.shutdownNow(); + } + } +} From 6bed1c985a639395c4279e1b90abcca0a74e0156 Mon Sep 17 00:00:00 2001 From: Alex Woods Date: Wed, 26 Aug 2026 18:19:23 -0700 Subject: [PATCH 2/8] refactor(endpoints): Simplify the BDD cache key to one comparison form Replace the seven-tier cacheParamsMatch with a uniform Objects.equals chain, ordered into three coarse groups: booleans, then strings whose reference the SDK keeps stable, then everything else, each group in the model's declaration order. Benchmarking says the tiers were not earning their complexity. Against this form they bought nothing on the hit path, which is the only path a cache exists to improve, and about 0.2 ns on the miss shape that motivates ordering at all - ahead of a ~1400 ns resolution. They cost a classification pass over every operation, a seven-value enum, and three different emitted code shapes. Full data, including why hashing the params would be worse than comparing them and why the comparison stays a private static method in the provider rather than moving onto the params class, is in .kiro/reference/endpoint_cache_key_benchmark.md. Objects.equals is what makes one emitter sufficient: it tries identity before equals, so a parameter whose reference is stable settles on the identity check and one that arrives fresh falls through and still matches. That was the tiers' main trick, available for free. Ordering survives because it is nearly free to derive - a parameter's group follows from its declared type plus whether it is AWS::Region or a client context param, with no analysis of the service's operations - and it is worth 20 ns on a miss against a late-declared boolean. It cannot affect correctness, since the chain compares every parameter before returning true. List parameters route through a generated cacheListsMatch helper instead of Objects.equals, keeping every term in the chain a single boolean expression and keeping the comparison bounded. List.equals is unbounded, and resolution is typically indifferent to list length, so an unbounded key check can cost more than the resolution it avoids and turn the cache into a pessimisation for that request shape. Above the cap the provider reports a miss and resolves, which is what it would have done anyway. The helper is only emitted when the model declares a stringArray. Deletes EndpointCacheKeyClassification, EndpointProviderCacheIndex and EndpointProviderCacheIndexTest. Testing: - The classification unit test is replaced by two assertions on the generated source, which is a stronger place to make them: that the key compares every parameter the BDD declares, and that the three-group ordering holds. The first is the invariant that matters - a parameter missing from the key returns an endpoint resolved for a different value of it - and it now covers all three BDD test models. - Mutation-checked both levels. Dropping a parameter that exists only in the runtime model fails exactly one of the 30 BddEndpointProviderCacheTest cases, the one that names it; dropping parameters present in the codegen models fails the completeness assertion, the ordering assertion and both golden files. - codegen 707 pass, codegen-generated-classes-test 3677 pass, checkstyle clean. --- .../rules2/bdd/BddEndpointProviderSpec.java | 204 +++++++++---- .../bdd/EndpointCacheKeyClassification.java | 91 ------ .../bdd/EndpointProviderCacheIndex.java | 282 ------------------ .../bdd/BddEndpointProviderSpecTest.java | 94 ++++++ .../bdd/EndpointProviderCacheIndexTest.java | 136 --------- .../bdd/endpoint-provider-bdd-class.java | 46 ++- .../bdd/endpoint-provider-bdd-s3-class.java | 56 ++-- 7 files changed, 271 insertions(+), 638 deletions(-) delete mode 100644 codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/EndpointCacheKeyClassification.java delete mode 100644 codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/EndpointProviderCacheIndex.java delete mode 100644 codegen/src/test/java/software/amazon/awssdk/codegen/poet/rules2/bdd/EndpointProviderCacheIndexTest.java diff --git a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpec.java b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpec.java index 5a5e3d287a5e..4f598484a08f 100644 --- a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpec.java +++ b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpec.java @@ -30,6 +30,7 @@ import java.util.List; import java.util.Locale; import java.util.Map; +import java.util.Objects; import java.util.concurrent.CompletableFuture; import javax.lang.model.element.Modifier; import software.amazon.awssdk.annotations.SdkInternalApi; @@ -41,6 +42,7 @@ import software.amazon.awssdk.codegen.model.rules.endpoints.ConditionModel; import software.amazon.awssdk.codegen.model.rules.endpoints.ParameterModel; import software.amazon.awssdk.codegen.model.rules.endpoints.RuleModel; +import software.amazon.awssdk.codegen.model.service.ClientContextParam; import software.amazon.awssdk.codegen.model.service.EndpointBddModel; import software.amazon.awssdk.codegen.poet.ClassSpec; import software.amazon.awssdk.codegen.poet.PoetUtils; @@ -80,6 +82,12 @@ public class BddEndpointProviderSpec implements ClassSpec { */ private static final int NO_MATCH_RESULT = 100_000_000; + /** + * A {@code stringArray} cache key parameter longer than this reports a miss without comparing elements, so that the + * cost of a cache check stays bounded. Eight covers the list-valued endpoint parameters shipped today. + */ + private static final int MAX_LIST_COMPARISON_SIZE = 8; + private final IntermediateModel intermediateModel; private final EndpointBddModel endpointBddModel; private final EndpointRulesSpecUtils endpointRulesSpecUtils; @@ -90,7 +98,6 @@ public class BddEndpointProviderSpec implements ClassSpec { private final ClassName cacheEntryType; private final List bddNodes; private final List conditionTypes; - private final EndpointProviderCacheIndex cacheIndex; public BddEndpointProviderSpec(IntermediateModel intermediateModel) { this.intermediateModel = intermediateModel; @@ -104,7 +111,6 @@ public BddEndpointProviderSpec(IntermediateModel intermediateModel) { this.cacheEntryType = className().nestedClass("CacheEntry"); this.bddNodes = endpointBddModel.getDecodedNodes(); this.conditionTypes = analyzeConditions(); - this.cacheIndex = EndpointProviderCacheIndex.of(intermediateModel); } @Override @@ -119,6 +125,9 @@ public TypeSpec poetSpec() { builder.addType(cacheEntryClass()); builder.addMethod(resolveEndpointMethod()); builder.addMethod(cacheParamsMatchMethod()); + if (hasListParam()) { + builder.addMethod(cacheListsMatchMethod()); + } return builder.build(); } @@ -165,11 +174,22 @@ private TypeSpec cacheEntryClass() { * Generates {@code cacheParamsMatch(a, b)}: true when the two parameter objects are interchangeable as far as * endpoint resolution is concerned. * - *

Every parameter the BDD model declares is compared. Parameters are ordered by - * {@link EndpointCacheKeyClassification}, cheapest comparison first, and the method returns on the first mismatch. + *

One uniform {@link Objects#equals} term per parameter, joined with {@code &&} so the chain short-circuits on + * the first mismatch. {@code Objects.equals} tries identity before {@code equals}, which is what makes a single + * emitter sufficient: a parameter whose reference the SDK keeps stable settles on the identity check, and one that + * arrives as a fresh reference falls through to {@code equals} and still matches. * - *

A parameter that {@link EndpointProviderCacheIndex} classified but that the model no longer declares would be - * a hole in the key, so that combination fails codegen instead of generating a comparison that quietly skips it. + *

Parameter order comes from {@link #cacheKeyParameterOrder()}. Order is the only thing that varies between + * parameters, and it only affects how quickly a mismatch is found. + * + *

List-valued parameters go through the generated {@code cacheListsMatch} helper rather than + * {@code Objects.equals}, so that every term in the chain stays a single boolean expression and so that the + * comparison stays bounded. See {@link #cacheListsMatchMethod()}. + * + *

An earlier version of this generated a seven-tier comparison, with the tier of each parameter computed from a + * pass over the service's operations, and a different code shape per tier. Benchmarking showed the tiers bought + * nothing over this form on the hit path and only ~0.2 ns on the miss path; see + * {@code .kiro/reference/endpoint_cache_key_benchmark.md}. */ private MethodSpec cacheParamsMatchMethod() { ClassName paramsClass = endpointRulesSpecUtils.parametersClassName(); @@ -181,83 +201,137 @@ private MethodSpec cacheParamsMatchMethod() { .addParameter(paramsClass, "a") .addParameter(paramsClass, "b"); - int listIndex = 0; - for (Map.Entry entry : cacheIndex.classifiedParameters().entrySet()) { - String paramName = entry.getKey(); - if (!parameters.containsKey(paramName)) { - throw new IllegalStateException( - "Endpoint parameter '" + paramName + "' was classified for the result cache but is not declared by " - + "the BDD model. Leaving it out of the cache key would let the provider return an endpoint " - + "resolved for a different value of it."); - } + CodeBlock.Builder chain = CodeBlock.builder().add("return "); + boolean first = true; + for (String paramName : cacheKeyParameterOrder()) { String getter = endpointRulesSpecUtils.paramMethodName(paramName) + "()"; - if (entry.getValue() == EndpointCacheKeyClassification.REQUEST_LIST) { - addListParamCheck(b, getter, listIndex++); + if (!first) { + chain.add("\n && "); + } + if (isListParam(parameters.get(paramName))) { + chain.add("cacheListsMatch(a.$L, b.$L)", getter, getter); } else { - addScalarParamCheck(b, getter, entry.getValue()); + chain.add("$T.equals(a.$L, b.$L)", Objects.class, getter, getter); } + first = false; } - - b.addStatement("return true"); + if (first) { + // A rule set with no parameters at all resolves to the same endpoint every time. + chain.add("true"); + } + b.addStatement(chain.build()); return b.build(); } /** - * Emits the comparison for one non-list parameter, returning false on mismatch. + * Returns the parameter names in the order the generated cache key compares them: booleans, then strings whose + * reference the SDK keeps stable across requests, then everything else. Each group keeps the model's declaration + * order, so the result is deterministic across builds. * - *

{@code BOOLEAN}, {@code CLIENT_STATIC_REF} and {@code OPERATION_STATIC} compare references only. For booleans - * that is complete, not just fast: autoboxing hands back the {@code Boolean.TRUE}/{@code Boolean.FALSE} singletons. - * For the other two the SDK hands the same reference to every request, and if it ever does not, the result is a - * miss and a re-resolution rather than a wrong endpoint. + *

Ordering exists only to reach a mismatch sooner. It cannot change the outcome, because the chain compares + * every parameter before returning true. Booleans come first because they can never fall through to a real + * {@code equals}; reference-stable strings come next because they normally settle on the identity check; and the + * request-derived values that may have to compare characters come last. * - *

The remaining classifications add an {@code equals} fallback so that an equal value arriving as a fresh - * reference still hits. + *

The group of a parameter follows from its own declaration - its declared type, plus whether it is + * {@code AWS::Region} or a client context parameter - so this needs no analysis of the service's operations. */ - private static void addScalarParamCheck(MethodSpec.Builder b, String getter, EndpointCacheKeyClassification cat) { - switch (cat) { - case BOOLEAN: - case CLIENT_STATIC_REF: - case OPERATION_STATIC: - b.addStatement("if (a.$L != b.$L) return false", getter, getter); - break; - default: - b.beginControlFlow("if (a.$L != b.$L)", getter, getter); - b.beginControlFlow("if (a.$L == null || !a.$L.equals(b.$L))", getter, getter, getter); - b.addStatement("return false"); - b.endControlFlow(); - b.endControlFlow(); - break; + private List cacheKeyParameterOrder() { + Map parameters = endpointBddModel.getParameters(); + Map clientContextParams = intermediateModel.getClientContextParams(); + + List booleans = new ArrayList<>(); + List stableStrings = new ArrayList<>(); + List rest = new ArrayList<>(); + + parameters.forEach((name, model) -> { + if (isBooleanParam(model)) { + booleans.add(name); + } else if (isReferenceStable(name, model, clientContextParams)) { + stableStrings.add(name); + } else { + rest.add(name); + } + }); + + List order = new ArrayList<>(parameters.size()); + order.addAll(booleans); + order.addAll(stableStrings); + order.addAll(rest); + return order; + } + + /** + * Returns true for a string parameter the SDK hands to every request as the same reference: {@code AWS::Region}, + * which {@code Region.of} interns, and {@code clientContextParams}, which are read from the client's + * {@code AttributeMap}. + * + *

Only used to order the comparison. If one of these ever stops being reference-stable, the + * {@code Objects.equals} term still compares it correctly; the check simply costs an extra call. + */ + private static boolean isReferenceStable(String paramName, + ParameterModel model, + Map clientContextParams) { + if (model.getBuiltInEnum() == BuiltInParameter.AWS_REGION) { + return true; + } + if (clientContextParams == null) { + return false; + } + if (clientContextParams.containsKey(paramName)) { + return true; } + // Endpoint parameter names are unique case-insensitively, so a case-insensitive match is the same parameter. + for (String key : clientContextParams.keySet()) { + if (key.equalsIgnoreCase(paramName)) { + return true; + } + } + return false; } /** - * Emits the comparison for one {@code stringArray} parameter: identity, then null, then size, then the element - * walk. The size cap keeps the check bounded so a request carrying a large list cannot make the cache check itself - * a cost worth avoiding. + * Generates the {@code cacheListsMatch} helper, emitted only when the model declares a {@code stringArray} + * parameter. * - *

{@code idx} suffixes the local variable names so several list parameters can be compared in one method. + *

{@code Objects.equals} would be correct here, but {@code List.equals} is unbounded: a request carrying a large + * list would walk every element on every cache check. Since resolution itself is typically indifferent to list + * length, an unbounded key check can cost more than the resolution it avoids, turning the cache into a + * pessimisation for that request shape. Refusing to match above + * {@value #MAX_LIST_COMPARISON_SIZE} elements keeps the check bounded; the consequence is that a service handling + * longer lists simply misses, and pays resolution, which is what it would have paid anyway. */ - private static void addListParamCheck(MethodSpec.Builder b, String getter, int idx) { - String listA = "listA" + idx; - String listB = "listB" + idx; - String i = "i" + idx; - String elementA = "elementA" + idx; - String elementB = "elementB" + idx; + private MethodSpec cacheListsMatchMethod() { TypeName listOfString = RuleRuntimeTypeMirror.LIST_OF_STRING.type(); - - b.addStatement("$T $L = a.$L", listOfString, listA, getter); - b.addStatement("$T $L = b.$L", listOfString, listB, getter); - b.beginControlFlow("if ($L != $L)", listA, listB); - b.addStatement("if ($L == null || $L == null) return false", listA, listB); - b.addStatement("if ($L.size() != $L.size()) return false", listA, listB); - b.addStatement("if ($L.size() > $L) return false", listA, EndpointProviderCacheIndex.MAX_LIST_COMPARISON_SIZE); - b.beginControlFlow("for (int $L = 0; $L < $L.size(); $L++)", i, i, listA, i); - b.addStatement("$T $L = $L.get($L)", String.class, elementA, listA, i); - b.addStatement("$T $L = $L.get($L)", String.class, elementB, listB, i); - b.addStatement("if ($L != $L && ($L == null || !$L.equals($L))) return false", - elementA, elementB, elementA, elementA, elementB); - b.endControlFlow(); - b.endControlFlow(); + return MethodSpec.methodBuilder("cacheListsMatch") + .addModifiers(Modifier.PRIVATE, Modifier.STATIC) + .returns(boolean.class) + .addParameter(listOfString, "a") + .addParameter(listOfString, "b") + .addStatement("if (a == b) return true") + .addStatement("if (a == null || b == null) return false") + .addStatement("int size = a.size()") + .addStatement("if (size != b.size()) return false") + .addComment("Bounded so that a long list cannot make the cache check cost more than " + + "resolving.") + .addStatement("if (size > $L) return false", MAX_LIST_COMPARISON_SIZE) + .beginControlFlow("for (int i = 0; i < size; i++)") + .addStatement("if (!$T.equals(a.get(i), b.get(i))) return false", Objects.class) + .endControlFlow() + .addStatement("return true") + .build(); + } + + private boolean hasListParam() { + return endpointBddModel.getParameters().values().stream().anyMatch(BddEndpointProviderSpec::isListParam); + } + + private static boolean isBooleanParam(ParameterModel model) { + return "boolean".equalsIgnoreCase(model.getType()); + } + + private static boolean isListParam(ParameterModel model) { + return "stringarray".equalsIgnoreCase(model.getType()); } private TypeSpec evaluatorClass() { diff --git a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/EndpointCacheKeyClassification.java b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/EndpointCacheKeyClassification.java deleted file mode 100644 index 29a9f3a33570..000000000000 --- a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/EndpointCacheKeyClassification.java +++ /dev/null @@ -1,91 +0,0 @@ -/* - * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. - * - * Licensed under the Apache License, Version 2.0 (the "License"). - * You may not use this file except in compliance with the License. - * A copy of the License is located at - * - * http://aws.amazon.com/apache2.0 - * - * or in the "license" file accompanying this file. This file is distributed - * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either - * express or implied. See the License for the specific language governing - * permissions and limitations under the License. - */ - -package software.amazon.awssdk.codegen.poet.rules2.bdd; - -/** - * Classifies one endpoint parameter to decide how the generated BDD endpoint provider compares it when checking its - * single-entry result cache. - * - *

Values are declared cheapest comparison first. {@code BddEndpointProviderSpec} emits the parameter checks in this - * order and returns on the first mismatch, so the parameters most likely to differ between two requests are also the - * ones reached last. - * - *

Why a wrong classification cannot return a wrong endpoint

- * - *

Every classification compares references first and either stops there or falls back to {@code equals}. Comparing - * only references can report a mismatch for two equal values, which costs a re-resolution; it can never report a match - * for two different values, because equal references imply the same object. So over-classifying a parameter as stable - * costs hit rate, not correctness. - * - *

What does affect correctness is a parameter being left out of the comparison altogether. That is why - * {@link EndpointProviderCacheIndex} classifies every parameter the BDD model declares and - * {@code BddEndpointProviderSpec} fails codegen rather than skipping one it cannot classify. - */ -public enum EndpointCacheKeyClassification { - /** - * A {@code boolean} parameter. Compared with {@code ==} and no fallback, which is complete rather than merely fast: - * autoboxing and {@code Boolean.valueOf} both hand back the {@code Boolean.TRUE}/{@code Boolean.FALSE} singletons, - * so identity agrees with {@code equals} for every value a caller can produce short of the {@code Boolean} - * constructor deprecated in Java 9. - */ - BOOLEAN, - - /** - * A string parameter sourced entirely from client configuration, where the same reference is handed to every - * request: {@code AWS::Region} (interned by {@code Region.of}) and {@code clientContextParams} (read from the - * client's {@code AttributeMap}). Compared with {@code ==} only. - */ - CLIENT_STATIC_REF, - - /** - * A string parameter bound to a {@code staticContextParams} literal. {@code EndpointResolverUtilsSpec} emits string - * literals and hoists array values to {@code static final} fields, so the reference is fixed per operation. - * Compared with {@code ==} only. List-valued static params are classified {@link #REQUEST_LIST} instead, since they - * share the list comparison shape. - */ - OPERATION_STATIC, - - /** - * A string parameter that is logically fixed for the life of the client but whose reference stability depends on an - * implementation detail a customer can replace: {@code SDK::Endpoint}, stable only because - * {@code StaticClientEndpointProvider} computes the sanitized string once, and - * {@code AWS::Auth::AccountIdEndpointMode}, stable only because {@code AccountIdEndpointMode.endpointModeValue} - * returns an interned literal. A custom {@code ClientEndpointProvider} need not cache. Compared with {@code ==}, - * then {@code equals}. - */ - SEMI_STABLE, - - /** - * {@code AWS::Auth::AccountId}, read off the resolved identity. Stable while the credentials provider serves the - * same cached identity, and a fresh reference after every refresh. Compared with {@code ==}, then {@code equals}. - */ - IDENTITY_DERIVED, - - /** - * A string parameter bound to a request member ({@code contextParam}) or extracted from the request by JMESPath - * ({@code operationContextParams}). Generally a fresh reference per request; the identity check still pays for - * itself when one API call resolves the endpoint more than once from the same request object. Compared with - * {@code ==}, then {@code equals}. - */ - REQUEST_DYNAMIC, - - /** - * A {@code stringArray} parameter. Compared last, with a null-safe identity check, then size, then element-wise - * identity or {@code equals}. Lists longer than {@link EndpointProviderCacheIndex#MAX_LIST_COMPARISON_SIZE} report - * a miss without being walked, so the key check stays bounded no matter how large the request is. - */ - REQUEST_LIST -} diff --git a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/EndpointProviderCacheIndex.java b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/EndpointProviderCacheIndex.java deleted file mode 100644 index b4a581aa2198..000000000000 --- a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/EndpointProviderCacheIndex.java +++ /dev/null @@ -1,282 +0,0 @@ -/* - * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. - * - * Licensed under the Apache License, Version 2.0 (the "License"). - * You may not use this file except in compliance with the License. - * A copy of the License is located at - * - * http://aws.amazon.com/apache2.0 - * - * or in the "license" file accompanying this file. This file is distributed - * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either - * express or implied. See the License for the specific language governing - * permissions and limitations under the License. - */ - -package software.amazon.awssdk.codegen.poet.rules2.bdd; - -import static software.amazon.awssdk.codegen.poet.rules2.bdd.EndpointCacheKeyClassification.BOOLEAN; -import static software.amazon.awssdk.codegen.poet.rules2.bdd.EndpointCacheKeyClassification.CLIENT_STATIC_REF; -import static software.amazon.awssdk.codegen.poet.rules2.bdd.EndpointCacheKeyClassification.IDENTITY_DERIVED; -import static software.amazon.awssdk.codegen.poet.rules2.bdd.EndpointCacheKeyClassification.OPERATION_STATIC; -import static software.amazon.awssdk.codegen.poet.rules2.bdd.EndpointCacheKeyClassification.REQUEST_DYNAMIC; -import static software.amazon.awssdk.codegen.poet.rules2.bdd.EndpointCacheKeyClassification.REQUEST_LIST; -import static software.amazon.awssdk.codegen.poet.rules2.bdd.EndpointCacheKeyClassification.SEMI_STABLE; - -import java.util.Collections; -import java.util.LinkedHashMap; -import java.util.Map; -import software.amazon.awssdk.codegen.model.intermediate.IntermediateModel; -import software.amazon.awssdk.codegen.model.intermediate.MemberModel; -import software.amazon.awssdk.codegen.model.intermediate.OperationModel; -import software.amazon.awssdk.codegen.model.rules.endpoints.BuiltInParameter; -import software.amazon.awssdk.codegen.model.rules.endpoints.ParameterModel; -import software.amazon.awssdk.codegen.model.service.ClientContextParam; -import software.amazon.awssdk.codegen.model.service.ContextParam; -import software.amazon.awssdk.codegen.model.service.StaticContextParam; -import software.amazon.awssdk.utils.CollectionUtils; - -/** - * Computes the {@link EndpointCacheKeyClassification} of every endpoint parameter, at codegen time, for - * {@code BddEndpointProviderSpec} to turn into the tiered {@code cacheParamsMatch} method inside - * {@code Default{Service}EndpointProvider}. - * - *

Parameter source

- * - *

Parameters come from the BDD model, not the rule set, because the BDD is what the generated provider - * evaluates. The two agree for a service whose BDD was compiled from its own rule set, but nothing in codegen enforces - * that, and the codegen test models deliberately pair mismatched files. Reading the rule set here would silently drop - * every parameter the BDD declares and the rule set does not, and a parameter missing from the cache key is the one way - * this cache can hand back an endpoint resolved for different inputs. - * - *

Classification

- * - *
    - *
  1. Seed each parameter from its own declaration: type first, then built-in, then - * {@code clientContextParams} membership.
    2. - *
  2. Scan every operation for binding sites ({@code contextParam}, {@code staticContextParams}, - * {@code operationContextParams}) and promote the parameter to the more dynamic classification. Most dynamic - * wins, so a parameter bound statically by one operation and from the request by another is compared the way the - * request-bound operation needs.
    3. - *
- * - *

The returned map is ordered by classification, cheapest first, then by name within a classification. That ordering - * is the generated comparison order, so it is deterministic across builds. - */ -public final class EndpointProviderCacheIndex { - /** - * Lists longer than this report a cache miss without element-wise comparison, bounding the cost of the key check - * regardless of request size. Eight covers the list-valued endpoint parameters shipped today while keeping the - * worst-case check comfortably cheaper than a re-resolution. - */ - public static final int MAX_LIST_COMPARISON_SIZE = 8; - - private final IntermediateModel model; - - private EndpointProviderCacheIndex(IntermediateModel model) { - this.model = model; - } - - public static EndpointProviderCacheIndex of(IntermediateModel model) { - return new EndpointProviderCacheIndex(model); - } - - /** - * Returns the more dynamic of two classifications. Used with {@link Map#merge} to implement most-dynamic-wins. - */ - public static EndpointCacheKeyClassification moreExpensive(EndpointCacheKeyClassification existing, - EndpointCacheKeyClassification candidate) { - return candidate.ordinal() > existing.ordinal() ? candidate : existing; - } - - /** - * Returns an unmodifiable map from parameter name to classification, ordered cheapest comparison first. - */ - public Map classifiedParameters() { - Map parameters = model.getEndpointBddModel().getParameters(); - Map clientContextParams = model.getClientContextParams(); - - // A null value means "not decided from the declaration alone; let the binding sites decide". Map.merge treats a - // null value as absent, so the first binding site simply wins and later ones promote from there. - Map result = new LinkedHashMap<>(); - parameters.forEach((name, pm) -> result.put(name, initialClassification(name, pm, clientContextParams))); - - for (OperationModel op : model.getOperations().values()) { - promoteForContextParams(op, parameters, result); - promoteForStaticContextParams(op, parameters, result); - promoteForOperationContextParams(op, parameters, result); - } - - // No binding site named it, so we cannot say where its value comes from. Assume per-request. - result.replaceAll((name, category) -> category == null ? REQUEST_DYNAMIC : category); - - Map sorted = new LinkedHashMap<>(); - result.entrySet().stream() - .sorted((a, b) -> { - int cmp = Integer.compare(a.getValue().ordinal(), b.getValue().ordinal()); - return cmp != 0 ? cmp : a.getKey().compareTo(b.getKey()); - }) - .forEach(e -> sorted.put(e.getKey(), e.getValue())); - return Collections.unmodifiableMap(sorted); - } - - /** - * A {@code contextParam} binds the parameter to a member of the request, so its value arrives fresh per request. - */ - private static void promoteForContextParams(OperationModel op, - Map parameters, - Map result) { - if (op.getInputShape() == null) { - return; - } - for (MemberModel member : op.getInputShape().getMembers()) { - ContextParam cp = member.getContextParam(); - if (cp == null) { - continue; - } - String paramName = findParamName(parameters, cp.getName()); - if (paramName != null) { - result.merge(paramName, REQUEST_DYNAMIC, EndpointProviderCacheIndex::moreExpensive); - } - } - } - - /** - * A {@code staticContextParam} is a literal fixed at codegen time. List values still take the list comparison. - */ - private static void promoteForStaticContextParams(OperationModel op, - Map parameters, - Map result) { - Map statics = op.getStaticContextParams(); - if (CollectionUtils.isNullOrEmpty(statics)) { - return; - } - statics.forEach((paramName, scp) -> { - String canonicalName = findParamName(parameters, paramName); - if (canonicalName == null) { - return; - } - EndpointCacheKeyClassification category = - isList(parameters.get(canonicalName)) ? REQUEST_LIST : OPERATION_STATIC; - result.merge(canonicalName, category, EndpointProviderCacheIndex::moreExpensive); - }); - } - - /** - * An {@code operationContextParam} is a JMESPath expression evaluated over the request, producing a fresh value, - * and a fresh list when the parameter is a {@code stringArray}. - */ - private static void promoteForOperationContextParams(OperationModel op, - Map parameters, - Map result) { - if (CollectionUtils.isNullOrEmpty(op.getOperationContextParams())) { - return; - } - op.getOperationContextParams().forEach((paramName, ocp) -> { - String canonicalName = findParamName(parameters, paramName); - if (canonicalName == null) { - return; - } - EndpointCacheKeyClassification category = - isList(parameters.get(canonicalName)) ? REQUEST_LIST : REQUEST_DYNAMIC; - result.merge(canonicalName, category, EndpointProviderCacheIndex::moreExpensive); - }); - } - - /** - * Classifies a parameter from its declaration alone. Returns {@code null} when the declaration does not determine - * the classification and the operation binding sites should, which is the case for a plain string parameter that is - * neither a built-in nor a client context param: whether it is an {@code OPERATION_STATIC} literal or arrives from - * the request is visible only at the binding site. - */ - private static EndpointCacheKeyClassification initialClassification(String paramName, - ParameterModel pm, - Map clientContextParams) { - if (isBoolean(pm)) { - return BOOLEAN; - } - - if (isList(pm)) { - return REQUEST_LIST; - } - - // A built-in wins over a clientContextParams entry that happens to share the parameter's name. - BuiltInParameter builtIn = pm.getBuiltInEnum(); - if (builtIn != null) { - return classifyBuiltIn(builtIn); - } - - if (clientContextParams != null && isClientContextParam(paramName, clientContextParams)) { - return CLIENT_STATIC_REF; - } - - return null; - } - - private static EndpointCacheKeyClassification classifyBuiltIn(BuiltInParameter builtIn) { - switch (builtIn) { - case AWS_REGION: - return CLIENT_STATIC_REF; - case SDK_ENDPOINT: - case AWS_AUTH_ACCOUNT_ID_ENDPOINT_MODE: - return SEMI_STABLE; - case AWS_AUTH_ACCOUNT_ID: - return IDENTITY_DERIVED; - case AWS_USE_DUAL_STACK: - case AWS_USE_FIPS: - case AWS_S3_ACCELERATE: - case AWS_S3_CONTROL_USE_ARN_REGION: - case AWS_S3_DISABLE_MULTI_REGION_ACCESS_POINTS: - case AWS_S3_FORCE_PATH_STYLE: - case AWS_S3_USE_ARN_REGION: - case AWS_S3_USE_GLOBAL_ENDPOINT: - case AWS_STS_USE_GLOBAL_ENDPOINT: - // Every one of these is declared boolean in practice and is intercepted by the boolean check above. - // Reaching here means a model declared one as a string; they are all client-level config either way, - // so identity is the right comparison. - return CLIENT_STATIC_REF; - default: - // A built-in added to BuiltInParameter but not yet considered here. Compare with equals as a fallback - // rather than assuming a stable reference we have not verified. - return SEMI_STABLE; - } - } - - private static boolean isClientContextParam(String paramName, Map clientContextParams) { - if (clientContextParams.containsKey(paramName)) { - return true; - } - for (String key : clientContextParams.keySet()) { - if (key.equalsIgnoreCase(paramName)) { - return true; - } - } - return false; - } - - private static boolean isBoolean(ParameterModel pm) { - return "boolean".equalsIgnoreCase(pm.getType()); - } - - private static boolean isList(ParameterModel pm) { - return "stringarray".equalsIgnoreCase(pm.getType()); - } - - /** - * Resolves a parameter name as spelled at a binding site to the key used in the parameters map, which may differ in - * capitalisation. Returns {@code null} when the binding site names a parameter the model does not declare, which is - * legitimate: a service can bind a context param that its endpoint rules never read. - */ - private static String findParamName(Map parameters, String name) { - if (parameters.containsKey(name)) { - return name; - } - // Endpoint parameter names are unique case-insensitively. - for (String key : parameters.keySet()) { - if (key.equalsIgnoreCase(name)) { - return key; - } - } - return null; - } -} diff --git a/codegen/src/test/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpecTest.java b/codegen/src/test/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpecTest.java index 32e819a3f791..bec5e285fbee 100644 --- a/codegen/src/test/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpecTest.java +++ b/codegen/src/test/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpecTest.java @@ -19,9 +19,16 @@ import static org.assertj.core.api.Assertions.assertThatThrownBy; import static software.amazon.awssdk.codegen.poet.PoetMatchers.generatesTo; +import java.util.ArrayList; +import java.util.List; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import java.util.stream.Collectors; import org.hamcrest.MatcherAssert; import org.junit.jupiter.api.Test; +import software.amazon.awssdk.codegen.model.intermediate.IntermediateModel; import software.amazon.awssdk.codegen.poet.ClientTestModels; +import software.amazon.awssdk.codegen.poet.rules.EndpointRulesSpecUtils; public class BddEndpointProviderSpecTest { @@ -146,6 +153,93 @@ void complementEdge_generatesNodeNWithSwappedBranches() { assertThat(generated).contains("Endpoint nodeN1()"); } + /** + * The one invariant the result cache depends on: every parameter the BDD declares must appear in the generated key. + * A parameter left out is not a slow cache, it is a cache that returns an endpoint resolved for a different value of + * that parameter, and nothing else in the test suite would catch it. + * + *

Asserted against the generated source rather than against an intermediate model, so it holds regardless of how + * the comparison is built. + */ + @Test + void cacheKeyComparesEveryDeclaredParameter() { + assertCacheKeyIsComplete(ClientTestModels.queryServiceModelsWithSimpleBddEndpoints()); + assertCacheKeyIsComplete(ClientTestModels.queryServiceModelsWithBddEndpoints()); + assertCacheKeyIsComplete(ClientTestModels.queryServiceModelsWithComplementBddEndpoints()); + } + + private static void assertCacheKeyIsComplete(IntermediateModel model) { + EndpointRulesSpecUtils utils = new EndpointRulesSpecUtils(model); + List expected = model.getEndpointBddModel().getParameters().keySet().stream() + .map(utils::paramMethodName) + .collect(Collectors.toList()); + + assertThat(cacheKeyGetterOrder(new BddEndpointProviderSpec(model))) + .as("every parameter the BDD declares must be part of the cache key") + .containsExactlyInAnyOrderElementsOf(expected); + } + + /** + * The generated key compares booleans first, then the strings whose reference the SDK keeps stable, then everything + * else. Ordering cannot change the result - the chain compares every parameter before returning true - it only + * decides how quickly a mismatch is found, so this is a performance property rather than a correctness one. It is + * pinned because the ordering is the entire reason the grouping exists; if it silently degraded to declaration + * order the code would still be correct and the benefit would be gone. + */ + @Test + void cacheKeyOrdersBooleansThenStableStringsThenTheRest() { + List order = cacheKeyGetterOrder( + new BddEndpointProviderSpec(ClientTestModels.queryServiceModelsWithSimpleBddEndpoints())); + + assertThat(order).containsExactly("useDualStack", "useFips", // booleans + "region", "stringContextParam", // reference-stable strings + "endpoint", "staticStringParam", // everything else, declaration order + "operationContextParam", "arnList"); + } + + /** + * A list parameter needs a bounded comparison, so it routes through the emitted helper rather than + * {@code Objects.equals}, whose {@code List.equals} would walk every element however long the list is. + */ + @Test + void listParametersUseTheBoundedHelper() { + String generated = new BddEndpointProviderSpec( + ClientTestModels.queryServiceModelsWithSimpleBddEndpoints()).poetSpec().toString(); + + assertThat(generated).contains("cacheListsMatch(a.arnList(), b.arnList())"); + assertThat(generated).contains("if (size > 8) return false"); + } + + /** + * The helper is only useful when the model has a list parameter, and the S3 BDD has none. + */ + @Test + void listHelperIsOmittedWhenNoListParameterExists() { + String generated = new BddEndpointProviderSpec( + ClientTestModels.queryServiceModelsWithBddEndpoints()).poetSpec().toString(); + + assertThat(generated).doesNotContain("cacheListsMatch"); + } + + /** + * Returns the parameter getters referenced by the generated {@code cacheParamsMatch}, in the order they are + * compared. + */ + private static List cacheKeyGetterOrder(BddEndpointProviderSpec spec) { + String generated = spec.poetSpec().toString(); + int start = generated.indexOf("boolean cacheParamsMatch("); + assertThat(start).as("generated provider must contain cacheParamsMatch").isNotNegative(); + int end = generated.indexOf(";", start); + String body = generated.substring(start, end); + + List getters = new ArrayList<>(); + Matcher matcher = Pattern.compile("\\ba\\.(\\w+)\\(\\)").matcher(body); + while (matcher.find()) { + getters.add(matcher.group(1)); + } + return getters; + } + /** * {@code DynamicEndpointAuthSchemeFactory} is S3-specific, so a dynamically resolved auth scheme name in any other * service must fail codegen rather than emitting code that cannot compile. diff --git a/codegen/src/test/java/software/amazon/awssdk/codegen/poet/rules2/bdd/EndpointProviderCacheIndexTest.java b/codegen/src/test/java/software/amazon/awssdk/codegen/poet/rules2/bdd/EndpointProviderCacheIndexTest.java deleted file mode 100644 index 8d1790cb76cf..000000000000 --- a/codegen/src/test/java/software/amazon/awssdk/codegen/poet/rules2/bdd/EndpointProviderCacheIndexTest.java +++ /dev/null @@ -1,136 +0,0 @@ -/* - * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. - * - * Licensed under the Apache License, Version 2.0 (the "License"). - * You may not use this file except in compliance with the License. - * A copy of the License is located at - * - * http://aws.amazon.com/apache2.0 - * - * or in the "license" file accompanying this file. This file is distributed - * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either - * express or implied. See the License for the specific language governing - * permissions and limitations under the License. - */ - -package software.amazon.awssdk.codegen.poet.rules2.bdd; - -import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.entry; - -import java.util.ArrayList; -import java.util.Map; -import org.junit.jupiter.api.Test; -import software.amazon.awssdk.codegen.model.intermediate.IntermediateModel; -import software.amazon.awssdk.codegen.poet.ClientTestModels; - -/** - * Classification tests for the BDD endpoint provider result cache. - * - *

Classification is not observable from runtime behaviour: identity-only and identity-then-equals both invalidate - * when a value changes, and both hit when the reference is the same. The difference is only in how much work a hit - * costs. So the tier a parameter lands in has to be asserted here, or it is not covered at all. - */ -public class EndpointProviderCacheIndexTest { - - private static Map classify(IntermediateModel model) { - return EndpointProviderCacheIndex.of(model).classifiedParameters(); - } - - /** - * Every parameter the BDD declares must be classified. A parameter missing from this map is a parameter missing - * from the generated cache key, which is the one defect here that yields a wrong endpoint rather than a slow one. - */ - @Test - void everyBddParameterIsClassified() { - assertThat(classify(ClientTestModels.queryServiceModelsWithSimpleBddEndpoints())) - .containsOnlyKeys("Region", "UseDualStack", "UseFIPS", "Endpoint", - "stringContextParam", "staticStringParam", "operationContextParam", "arnList"); - } - - /** - * Pins the tier of each parameter shape. In particular {@code staticStringParam} must be {@code OPERATION_STATIC}: - * a parameter whose only binding site is a {@code staticContextParams} literal is fixed per operation, so it needs - * no {@code equals} fallback. Getting this wrong is invisible at runtime, so it is asserted rather than inferred. - */ - @Test - void parametersAreClassifiedByTheirBindingSite() { - assertThat(classify(ClientTestModels.queryServiceModelsWithSimpleBddEndpoints())) - .contains(entry("UseDualStack", EndpointCacheKeyClassification.BOOLEAN), - entry("UseFIPS", EndpointCacheKeyClassification.BOOLEAN), - entry("Region", EndpointCacheKeyClassification.CLIENT_STATIC_REF), - entry("stringContextParam", EndpointCacheKeyClassification.CLIENT_STATIC_REF), - entry("staticStringParam", EndpointCacheKeyClassification.OPERATION_STATIC), - entry("Endpoint", EndpointCacheKeyClassification.SEMI_STABLE), - entry("operationContextParam", EndpointCacheKeyClassification.REQUEST_DYNAMIC), - entry("arnList", EndpointCacheKeyClassification.REQUEST_LIST)); - } - - /** - * The generated comparison order is this map's iteration order, so it has to be deterministic and cheapest-first. - * Ordering the checks the other way round would still be correct but would pay for the expensive comparisons before - * the cheap ones had a chance to exit. - */ - @Test - void parametersAreOrderedCheapestComparisonFirst() { - Map classified = - classify(ClientTestModels.queryServiceModelsWithSimpleBddEndpoints()); - - assertThat(new ArrayList<>(classified.values())) - .isSortedAccordingTo((a, b) -> Integer.compare(a.ordinal(), b.ordinal())); - assertThat(classified.keySet()) - .containsExactly("UseDualStack", "UseFIPS", // BOOLEAN - "Region", "stringContextParam", // CLIENT_STATIC_REF - "staticStringParam", // OPERATION_STATIC - "Endpoint", // SEMI_STABLE - "operationContextParam", // REQUEST_DYNAMIC - "arnList"); // REQUEST_LIST - } - - /** - * A parameter no binding site names could come from anywhere, so it gets the conservative string classification - * rather than being assumed stable. The S3 BDD is paired with the query service model, which binds none of S3's - * request parameters, so this is the shape that model produces. - */ - @Test - void unboundStringParameterFallsBackToRequestDynamic() { - assertThat(classify(ClientTestModels.queryServiceModelsWithBddEndpoints())) - .contains(entry("Bucket", EndpointCacheKeyClassification.REQUEST_DYNAMIC), - entry("Key", EndpointCacheKeyClassification.REQUEST_DYNAMIC), - entry("CopySource", EndpointCacheKeyClassification.REQUEST_DYNAMIC), - entry("Prefix", EndpointCacheKeyClassification.REQUEST_DYNAMIC)); - } - - /** - * Built-ins are classified from the built-in rather than from a name collision with a client context param, and the - * two whose reference stability rests on an SDK implementation detail keep their {@code equals} fallback. - */ - @Test - void builtInsAreClassifiedFromTheBuiltIn() { - Map classified = - classify(ClientTestModels.queryServiceModelsWithBddEndpoints()); - - assertThat(classified) - .contains(entry("Region", EndpointCacheKeyClassification.CLIENT_STATIC_REF), - entry("Endpoint", EndpointCacheKeyClassification.SEMI_STABLE), - entry("UseFIPS", EndpointCacheKeyClassification.BOOLEAN), - entry("UseDualStack", EndpointCacheKeyClassification.BOOLEAN), - entry("Accelerate", EndpointCacheKeyClassification.BOOLEAN), - entry("UseArnRegion", EndpointCacheKeyClassification.BOOLEAN)); - } - - /** - * Classification is read off the BDD, not the rule set, because the BDD is what the generated provider evaluates. - * The complement model pairs a two-parameter BDD with the eight-parameter default-regional rule set, so reading the - * rule set here would silently add six parameters the provider cannot use — and, in the opposite pairing, silently - * drop parameters it does use. - */ - @Test - void classificationComesFromTheBddNotTheRuleSet() { - IntermediateModel model = ClientTestModels.queryServiceModelsWithComplementBddEndpoints(); - - assertThat(model.getEndpointRuleSetModel().getParameters()).hasSize(8); - assertThat(model.getEndpointBddModel().getParameters()).containsOnlyKeys("Endpoint", "Region"); - assertThat(classify(model)).containsOnlyKeys("Endpoint", "Region"); - } -} diff --git a/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules2/bdd/endpoint-provider-bdd-class.java b/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules2/bdd/endpoint-provider-bdd-class.java index 14ed368284b0..d2ac2377e5ae 100644 --- a/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules2/bdd/endpoint-provider-bdd-class.java +++ b/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules2/bdd/endpoint-provider-bdd-class.java @@ -1,6 +1,7 @@ package software.amazon.awssdk.services.query.endpoints.internal; import java.util.List; +import java.util.Objects; import java.util.concurrent.CompletableFuture; import software.amazon.awssdk.annotations.Generated; import software.amazon.awssdk.annotations.SdkInternalApi; @@ -45,32 +46,25 @@ public CompletableFuture resolveEndpoint(QueryEndpointParams endpointP } private static boolean cacheParamsMatch(QueryEndpointParams a, QueryEndpointParams b) { - if (a.useDualStack() != b.useDualStack()) return false; - if (a.useFips() != b.useFips()) return false; - if (a.region() != b.region()) return false; - if (a.stringContextParam() != b.stringContextParam()) return false; - if (a.staticStringParam() != b.staticStringParam()) return false; - if (a.endpoint() != b.endpoint()) { - if (a.endpoint() == null || !a.endpoint().equals(b.endpoint())) { - return false; - } - } - if (a.operationContextParam() != b.operationContextParam()) { - if (a.operationContextParam() == null || !a.operationContextParam().equals(b.operationContextParam())) { - return false; - } - } - List listA0 = a.arnList(); - List listB0 = b.arnList(); - if (listA0 != listB0) { - if (listA0 == null || listB0 == null) return false; - if (listA0.size() != listB0.size()) return false; - if (listA0.size() > 8) return false; - for (int i0 = 0; i0 < listA0.size(); i0++) { - String elementA0 = listA0.get(i0); - String elementB0 = listB0.get(i0); - if (elementA0 != elementB0 && (elementA0 == null || !elementA0.equals(elementB0))) return false; - } + return Objects.equals(a.useDualStack(), b.useDualStack()) + && Objects.equals(a.useFips(), b.useFips()) + && Objects.equals(a.region(), b.region()) + && Objects.equals(a.stringContextParam(), b.stringContextParam()) + && Objects.equals(a.endpoint(), b.endpoint()) + && Objects.equals(a.staticStringParam(), b.staticStringParam()) + && Objects.equals(a.operationContextParam(), b.operationContextParam()) + && cacheListsMatch(a.arnList(), b.arnList()); + } + + private static boolean cacheListsMatch(List a, List b) { + if (a == b) return true; + if (a == null || b == null) return false; + int size = a.size(); + if (size != b.size()) return false; + // Bounded so that a long list cannot make the cache check cost more than resolving. + if (size > 8) return false; + for (int i = 0; i < size; i++) { + if (!Objects.equals(a.get(i), b.get(i))) return false; } return true; } diff --git a/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules2/bdd/endpoint-provider-bdd-s3-class.java b/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules2/bdd/endpoint-provider-bdd-s3-class.java index 0da2bfa737e9..b5961bc1b718 100644 --- a/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules2/bdd/endpoint-provider-bdd-s3-class.java +++ b/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules2/bdd/endpoint-provider-bdd-s3-class.java @@ -1,6 +1,7 @@ package software.amazon.awssdk.services.query.endpoints.internal; import java.util.Arrays; +import java.util.Objects; import java.util.concurrent.CompletableFuture; import software.amazon.awssdk.annotations.Generated; import software.amazon.awssdk.annotations.SdkInternalApi; @@ -49,44 +50,23 @@ public CompletableFuture resolveEndpoint(QueryEndpointParams endpointP } private static boolean cacheParamsMatch(QueryEndpointParams a, QueryEndpointParams b) { - if (a.accelerate() != b.accelerate()) return false; - if (a.disableAccessPoints() != b.disableAccessPoints()) return false; - if (a.disableMultiRegionAccessPoints() != b.disableMultiRegionAccessPoints()) return false; - if (a.disableS3ExpressSessionAuth() != b.disableS3ExpressSessionAuth()) return false; - if (a.forcePathStyle() != b.forcePathStyle()) return false; - if (a.useArnRegion() != b.useArnRegion()) return false; - if (a.useDualStack() != b.useDualStack()) return false; - if (a.useFips() != b.useFips()) return false; - if (a.useGlobalEndpoint() != b.useGlobalEndpoint()) return false; - if (a.useObjectLambdaEndpoint() != b.useObjectLambdaEndpoint()) return false; - if (a.useS3ExpressControlEndpoint() != b.useS3ExpressControlEndpoint()) return false; - if (a.region() != b.region()) return false; - if (a.endpoint() != b.endpoint()) { - if (a.endpoint() == null || !a.endpoint().equals(b.endpoint())) { - return false; - } - } - if (a.bucket() != b.bucket()) { - if (a.bucket() == null || !a.bucket().equals(b.bucket())) { - return false; - } - } - if (a.copySource() != b.copySource()) { - if (a.copySource() == null || !a.copySource().equals(b.copySource())) { - return false; - } - } - if (a.key() != b.key()) { - if (a.key() == null || !a.key().equals(b.key())) { - return false; - } - } - if (a.prefix() != b.prefix()) { - if (a.prefix() == null || !a.prefix().equals(b.prefix())) { - return false; - } - } - return true; + return Objects.equals(a.useFips(), b.useFips()) + && Objects.equals(a.useDualStack(), b.useDualStack()) + && Objects.equals(a.forcePathStyle(), b.forcePathStyle()) + && Objects.equals(a.accelerate(), b.accelerate()) + && Objects.equals(a.useGlobalEndpoint(), b.useGlobalEndpoint()) + && Objects.equals(a.useObjectLambdaEndpoint(), b.useObjectLambdaEndpoint()) + && Objects.equals(a.disableAccessPoints(), b.disableAccessPoints()) + && Objects.equals(a.disableMultiRegionAccessPoints(), b.disableMultiRegionAccessPoints()) + && Objects.equals(a.useArnRegion(), b.useArnRegion()) + && Objects.equals(a.useS3ExpressControlEndpoint(), b.useS3ExpressControlEndpoint()) + && Objects.equals(a.disableS3ExpressSessionAuth(), b.disableS3ExpressSessionAuth()) + && Objects.equals(a.region(), b.region()) + && Objects.equals(a.bucket(), b.bucket()) + && Objects.equals(a.endpoint(), b.endpoint()) + && Objects.equals(a.key(), b.key()) + && Objects.equals(a.prefix(), b.prefix()) + && Objects.equals(a.copySource(), b.copySource()); } private static final class Evaluator { From 8c40a76a205cde564a6a9215d9108c47d68659da Mon Sep 17 00:00:00 2001 From: Alex Woods Date: Thu, 27 Aug 2026 09:58:21 -0700 Subject: [PATCH 3/8] perf(endpoints): Key the BDD cache on what the BDD actually reads Two changes to the generated cache key, both driven by the per-service measurements in .kiro/reference/endpoint_cache_service_shapes.md. 1. Exclude parameters no condition and no result reads. A parameter nothing reads cannot change the resolved endpoint, so comparing it can only turn a hit into a miss that resolves to the endpoint already cached. S3 is why this matters. It declares Key, Prefix and CopySource, reads none of them, and binds Key as a contextParam - so Key changes on essentially every object request. With Key in the key, S3's cache misses on almost every GetObject and pays 6.4 ns per request for nothing. Dropping the three unread parameters takes the key from 17 comparisons to 14, makes a hit 41% cheaper on fresh references, and converts the dominant miss into a hit. 2. Compare only element 0 of a stringArray read only at index 0. When every read of a list is getAttr(list, "[0]"), nothing past the first element reaches the endpoint, so the rest cannot change the answer. DynamoDB is why this matters. It reads ResourceArnList only through getAttr(ResourceArnList, "[0]"), and comparing a freshly built three-element ARN list measured 15.5 ns against a 28 ns regional resolution - over half the cost the cache exists to avoid, on a latency path that matters. Comparing element 0 makes it O(1). This also makes an absent list and an empty one the same key, which is correct rather than a concession: the runtime's listAccess returns null for both, so both take the same branch during resolution. It is strictly more permissive than comparing whole lists, so it can only turn misses into hits. Detection is conservative in the safe direction. BddParameterReferences walks the conditions and results with the same parser the generator uses, and any read that is not an index-0 access - isSet, a template interpolation, a non-zero index, passing the list to a function - marks the parameter as needing a full comparison. Erring that way costs comparison work; erring the other way would drop something from the key that can change the endpoint. Testing: - BddParameterReferences is not tested directly. Both behaviours are asserted on the generated source and on runtime behaviour, because those are what can be wrong in a way that matters; a unit test over the usage map would restate the implementation. - The codegen tests assert the key covers every referenced parameter for all three BDD models, omits the unreferenced ones, routes a whole-list parameter and an index-0-only parameter to their respective helpers, and that the provider really does read only element 0 - so the comparison and the thing it depends on cannot drift apart. - The runtime suite grows to 38 tests: changing element 0 invalidates while changing a later element, shortening the list, or going far past the size cap all hit; an unread parameter never invalidates; and the whole-list parameter keeps the previous element-wise coverage. - Mutation-checked three ways. Misclassifying whole-list reads as index-0-only fails the 4 whole-list tests; treating every parameter as unreferenced fails 22; never detecting index-0-only fails the 4 first-element tests. The suite discriminates in both directions. - Both test models needed their extra parameters wired into the node graph. They had been declared but unreferenced, so they would now be correctly excluded and the tests covering them would assert nothing. Each new condition is a node whose branches share a successor, which makes it genuinely evaluated without changing what any request resolves to. codegen 709 pass, codegen-generated-classes-test 3685 pass, checkstyle clean. --- .../rules2/bdd/BddEndpointProviderSpec.java | 73 +++++++- .../rules2/bdd/BddParameterReferences.java | 174 ++++++++++++++++++ .../bdd/BddEndpointProviderSpecTest.java | 107 ++++++++--- .../query/endpoint-bdd-default-regional.json | 68 ++++++- .../endpoint-rule-set-default-regional.json | 20 +- .../bdd/endpoint-provider-bdd-class.java | 50 ++++- .../bdd/endpoint-provider-bdd-s3-class.java | 5 +- .../bddendpoints/endpoint-bdd-1.json | 88 ++++++++- .../bddendpoints/endpoint-rule-set.json | 24 ++- .../bddendpoints/service-2.json | 3 + .../BddEndpointProviderCacheTest.java | 110 +++++++++-- 11 files changed, 631 insertions(+), 91 deletions(-) create mode 100644 codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddParameterReferences.java diff --git a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpec.java b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpec.java index 4f598484a08f..55107aaa002b 100644 --- a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpec.java +++ b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpec.java @@ -98,6 +98,7 @@ public class BddEndpointProviderSpec implements ClassSpec { private final ClassName cacheEntryType; private final List bddNodes; private final List conditionTypes; + private final Map paramUsage; public BddEndpointProviderSpec(IntermediateModel intermediateModel) { this.intermediateModel = intermediateModel; @@ -111,6 +112,7 @@ public BddEndpointProviderSpec(IntermediateModel intermediateModel) { this.cacheEntryType = className().nestedClass("CacheEntry"); this.bddNodes = endpointBddModel.getDecodedNodes(); this.conditionTypes = analyzeConditions(); + this.paramUsage = BddParameterReferences.analyze(endpointBddModel); } @Override @@ -125,9 +127,12 @@ public TypeSpec poetSpec() { builder.addType(cacheEntryClass()); builder.addMethod(resolveEndpointMethod()); builder.addMethod(cacheParamsMatchMethod()); - if (hasListParam()) { + if (needsFullListHelper()) { builder.addMethod(cacheListsMatchMethod()); } + if (needsFirstElementHelper()) { + builder.addMethod(cacheFirstElementsMatchMethod()); + } return builder.build(); } @@ -208,15 +213,18 @@ private MethodSpec cacheParamsMatchMethod() { if (!first) { chain.add("\n && "); } - if (isListParam(parameters.get(paramName))) { - chain.add("cacheListsMatch(a.$L, b.$L)", getter, getter); - } else { + if (!isListParam(parameters.get(paramName))) { chain.add("$T.equals(a.$L, b.$L)", Objects.class, getter, getter); + } else if (paramUsage.get(paramName) == BddParameterReferences.Usage.FIRST_ELEMENT_ONLY) { + chain.add("cacheFirstElementsMatch(a.$L, b.$L)", getter, getter); + } else { + chain.add("cacheListsMatch(a.$L, b.$L)", getter, getter); } first = false; } if (first) { - // A rule set with no parameters at all resolves to the same endpoint every time. + // Either the model declares no parameters, or it reads none of them. Both mean one endpoint for every + // request, so any two parameter objects are interchangeable. chain.add("true"); } b.addStatement(chain.build()); @@ -245,6 +253,10 @@ private List cacheKeyParameterOrder() { List rest = new ArrayList<>(); parameters.forEach((name, model) -> { + if (paramUsage.get(name) == BddParameterReferences.Usage.UNREFERENCED) { + // Nothing reads it, so it cannot change the endpoint and must not force a miss. + return; + } if (isBooleanParam(model)) { booleans.add(name); } else if (isReferenceStable(name, model, clientContextParams)) { @@ -254,7 +266,7 @@ private List cacheKeyParameterOrder() { } }); - List order = new ArrayList<>(parameters.size()); + List order = new ArrayList<>(booleans.size() + stableStrings.size() + rest.size()); order.addAll(booleans); order.addAll(stableStrings); order.addAll(rest); @@ -322,8 +334,53 @@ private MethodSpec cacheListsMatchMethod() { .build(); } - private boolean hasListParam() { - return endpointBddModel.getParameters().values().stream().anyMatch(BddEndpointProviderSpec::isListParam); + /** + * Generates the {@code cacheFirstElementsMatch} helper, emitted only when a {@code stringArray} parameter is read + * exclusively as {@code param[0]}. + * + *

When only the first element can reach the endpoint, comparing the rest is work that cannot change the answer. + * DynamoDB is why this exists: it reads {@code ResourceArnList} only through + * {@code getAttr(ResourceArnList, "[0]")}, and comparing a freshly built three-element ARN list measured at 15.5 ns + * against a 28 ns regional resolution - over half the cost the cache is meant to avoid. + * + *

Absent and empty both yield null here, matching the runtime's {@code listAccess}, which returns null for a null + * list and for an index past the end. So the two are interchangeable, exactly as they are during resolution. + */ + private MethodSpec cacheFirstElementsMatchMethod() { + TypeName listOfString = RuleRuntimeTypeMirror.LIST_OF_STRING.type(); + return MethodSpec.methodBuilder("cacheFirstElementsMatch") + .addModifiers(Modifier.PRIVATE, Modifier.STATIC) + .returns(boolean.class) + .addParameter(listOfString, "a") + .addParameter(listOfString, "b") + .addStatement("if (a == b) return true") + .addComment("Only element 0 reaches the endpoint; absent and empty are both null to the " + + "rules engine.") + .addStatement("$T firstA = a == null || a.isEmpty() ? null : a.get(0)", String.class) + .addStatement("$T firstB = b == null || b.isEmpty() ? null : b.get(0)", String.class) + .addStatement("return $T.equals(firstA, firstB)", Objects.class) + .build(); + } + + /** + * True when some list parameter in the cache key needs a whole-list comparison. + */ + private boolean needsFullListHelper() { + return listParamsInKeyWithUsage(BddParameterReferences.Usage.FULL); + } + + /** + * True when some list parameter in the cache key is read only at index 0. + */ + private boolean needsFirstElementHelper() { + return listParamsInKeyWithUsage(BddParameterReferences.Usage.FIRST_ELEMENT_ONLY); + } + + private boolean listParamsInKeyWithUsage(BddParameterReferences.Usage usage) { + Map parameters = endpointBddModel.getParameters(); + return cacheKeyParameterOrder().stream() + .filter(name -> isListParam(parameters.get(name))) + .anyMatch(name -> paramUsage.get(name) == usage); } private static boolean isBooleanParam(ParameterModel model) { diff --git a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddParameterReferences.java b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddParameterReferences.java new file mode 100644 index 000000000000..08a6dc4c79f2 --- /dev/null +++ b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddParameterReferences.java @@ -0,0 +1,174 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://aws.amazon.com/apache2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + */ + +package software.amazon.awssdk.codegen.poet.rules2.bdd; + +import java.util.Collections; +import java.util.HashSet; +import java.util.LinkedHashMap; +import java.util.Locale; +import java.util.Map; +import java.util.Set; +import software.amazon.awssdk.codegen.model.rules.endpoints.ConditionModel; +import software.amazon.awssdk.codegen.model.rules.endpoints.ParameterModel; +import software.amazon.awssdk.codegen.model.rules.endpoints.RuleModel; +import software.amazon.awssdk.codegen.model.service.EndpointBddModel; +import software.amazon.awssdk.codegen.poet.rules2.ExpressionParser; +import software.amazon.awssdk.codegen.poet.rules2.IndexedAccessExpression; +import software.amazon.awssdk.codegen.poet.rules2.MemberAccessExpression; +import software.amazon.awssdk.codegen.poet.rules2.RuleExpression; +import software.amazon.awssdk.codegen.poet.rules2.VariableReferenceExpression; +import software.amazon.awssdk.codegen.poet.rules2.WalkRuleExpressionVisitor; + +/** + * Works out how each endpoint parameter is used by a BDD, so that the generated result cache only compares what can + * actually change the resolved endpoint. + * + *

Analysis runs over the BDD's conditions and results using the same parser the generator uses, so the answer + * reflects what the generated provider reads rather than what the model happens to declare. + * + *

Two things fall out of it: + * + *

+ */ +final class BddParameterReferences { + + /** + * How much of a parameter's value can influence the resolved endpoint. + */ + enum Usage { + /** No condition and no result reads it. It cannot be part of the cache key. */ + UNREFERENCED, + + /** + * A {@code stringArray} read only as {@code param[0]}. Comparing the first element is sufficient. + * + *

This also makes a null list and an empty list interchangeable, which is correct: the runtime's + * {@code listAccess} returns null for both, so both take the same branch. It is strictly more permissive than + * comparing whole lists, so it can only turn misses into hits. + */ + FIRST_ELEMENT_ONLY, + + /** Read in a way that can depend on the entire value. */ + FULL + } + + private BddParameterReferences() { + } + + /** + * Returns the usage of every parameter the model declares, in declaration order. + */ + static Map analyze(EndpointBddModel model) { + Collector collector = new Collector(); + + for (ConditionModel condition : model.getConditions()) { + // Wrapped the same way BddEndpointProviderSpec wraps a condition before generating it, so the parse - and + // therefore the set of references - is identical to the one the emitted code is built from. + RuleModel synthetic = new RuleModel(); + synthetic.setType("error"); + synthetic.setError("synthetic"); + synthetic.setConditions(Collections.singletonList(condition)); + ExpressionParser.parseRuleSetExpression(synthetic).accept(collector); + } + for (RuleModel result : model.getResults()) { + ExpressionParser.parseRuleSetExpression(result).accept(collector); + } + + Map usage = new LinkedHashMap<>(); + model.getParameters().forEach((name, parameter) -> usage.put(name, usageOf(name, parameter, collector))); + return Collections.unmodifiableMap(usage); + } + + private static Usage usageOf(String name, ParameterModel parameter, Collector collector) { + if (!collector.referenced.contains(name)) { + return Usage.UNREFERENCED; + } + if (collector.wholeValue.contains(name)) { + return Usage.FULL; + } + // Only lists benefit, and only lists can be read element-wise. Anything else that somehow reached here is + // compared in full rather than guessed at. + return isList(parameter) ? Usage.FIRST_ELEMENT_ONLY : Usage.FULL; + } + + private static boolean isList(ParameterModel parameter) { + return "stringarray".equals(parameter.getType().toLowerCase(Locale.ENGLISH)); + } + + /** + * Collects, for every name the expressions reference, whether any reference needs more than the first element. + * + *

Erring towards {@link Usage#FULL} is the safe direction: it costs comparison work, whereas erring the other + * way would drop something from the cache key that can change the endpoint. + */ + private static final class Collector extends WalkRuleExpressionVisitor { + private final Set referenced = new HashSet<>(); + private final Set wholeValue = new HashSet<>(); + + @Override + public Void visitIndexedAccessExpression(IndexedAccessExpression e) { + String subject = firstElementSubject(e); + if (subject != null) { + referenced.add(subject); + // Deliberately not descending. Descending would reach the variable reference underneath and record it + // as a whole-value read, which is the thing this case exists to avoid. + return null; + } + return super.visitIndexedAccessExpression(e); + } + + @Override + public Void visitVariableReferenceExpression(VariableReferenceExpression e) { + referenced.add(e.variableName()); + wholeValue.add(e.variableName()); + return null; + } + + /** + * Returns the parameter name when this is an index-0 read of a plain parameter, otherwise null. + * + *

Both spellings reach here: {@code "list[0]"} inside a template parses to an indexed access straight over + * the variable, while {@code getAttr(list, "[0]")} wraps it in a direct-index member access first. + */ + private static String firstElementSubject(IndexedAccessExpression e) { + if (e.index() != 0) { + return null; + } + RuleExpression source = e.source(); + if (source instanceof MemberAccessExpression) { + MemberAccessExpression member = (MemberAccessExpression) source; + if (!member.directIndex()) { + return null; + } + source = member.source(); + } + if (source instanceof VariableReferenceExpression) { + return ((VariableReferenceExpression) source).variableName(); + } + return null; + } + } +} diff --git a/codegen/src/test/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpecTest.java b/codegen/src/test/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpecTest.java index bec5e285fbee..e57950d63f8b 100644 --- a/codegen/src/test/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpecTest.java +++ b/codegen/src/test/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpecTest.java @@ -23,12 +23,9 @@ import java.util.List; import java.util.regex.Matcher; import java.util.regex.Pattern; -import java.util.stream.Collectors; import org.hamcrest.MatcherAssert; import org.junit.jupiter.api.Test; -import software.amazon.awssdk.codegen.model.intermediate.IntermediateModel; import software.amazon.awssdk.codegen.poet.ClientTestModels; -import software.amazon.awssdk.codegen.poet.rules.EndpointRulesSpecUtils; public class BddEndpointProviderSpecTest { @@ -154,29 +151,57 @@ void complementEdge_generatesNodeNWithSwappedBranches() { } /** - * The one invariant the result cache depends on: every parameter the BDD declares must appear in the generated key. - * A parameter left out is not a slow cache, it is a cache that returns an endpoint resolved for a different value of - * that parameter, and nothing else in the test suite would catch it. + * The invariant the result cache depends on: every parameter the BDD reads must appear in the generated + * key. A parameter left out is not a slow cache, it is a cache that returns an endpoint resolved for a different + * value of that parameter, and nothing else in the test suite would catch it. * - *

Asserted against the generated source rather than against an intermediate model, so it holds regardless of how - * the comparison is built. + *

Asserted against the generated source rather than an intermediate model, so it holds regardless of how the + * comparison is built. */ @Test - void cacheKeyComparesEveryDeclaredParameter() { - assertCacheKeyIsComplete(ClientTestModels.queryServiceModelsWithSimpleBddEndpoints()); - assertCacheKeyIsComplete(ClientTestModels.queryServiceModelsWithBddEndpoints()); - assertCacheKeyIsComplete(ClientTestModels.queryServiceModelsWithComplementBddEndpoints()); + void cacheKeyComparesEveryReferencedParameter() { + // The simple BDD declares 10 parameters and reads 9; unusedParam is the one it does not read. + assertThat(cacheKeyGetterOrder( + new BddEndpointProviderSpec(ClientTestModels.queryServiceModelsWithSimpleBddEndpoints()))) + .containsExactlyInAnyOrder("useDualStack", "useFips", "region", "stringContextParam", "endpoint", + "staticStringParam", "operationContextParam", "arnList", + "customEndpointArray"); + + // The S3 BDD declares 17 and reads 14; Key, Prefix and CopySource are vestigial declarations. + assertThat(cacheKeyGetterOrder( + new BddEndpointProviderSpec(ClientTestModels.queryServiceModelsWithBddEndpoints()))) + .containsExactlyInAnyOrder("useFips", "useDualStack", "forcePathStyle", "accelerate", "useGlobalEndpoint", + "useObjectLambdaEndpoint", "disableAccessPoints", + "disableMultiRegionAccessPoints", "useArnRegion", + "useS3ExpressControlEndpoint", "disableS3ExpressSessionAuth", "region", + "bucket", "endpoint"); + + // The complement BDD declares and reads exactly two. + assertThat(cacheKeyGetterOrder( + new BddEndpointProviderSpec(ClientTestModels.queryServiceModelsWithComplementBddEndpoints()))) + .containsExactlyInAnyOrder("region", "endpoint"); } - private static void assertCacheKeyIsComplete(IntermediateModel model) { - EndpointRulesSpecUtils utils = new EndpointRulesSpecUtils(model); - List expected = model.getEndpointBddModel().getParameters().keySet().stream() - .map(utils::paramMethodName) - .collect(Collectors.toList()); - - assertThat(cacheKeyGetterOrder(new BddEndpointProviderSpec(model))) - .as("every parameter the BDD declares must be part of the cache key") - .containsExactlyInAnyOrderElementsOf(expected); + /** + * A parameter no condition and no result reads cannot change the resolved endpoint, so comparing it could only turn + * hits into misses that resolve to the endpoint already cached. + * + *

This is what makes the cache worth having for S3, whose rule set declares {@code Key}, {@code Prefix} and + * {@code CopySource} and reads none of them. {@code Key} changes on essentially every object request, so including + * it would mean the cache almost never hits. + */ + @Test + void cacheKeyOmitsParametersTheBddNeverReads() { + assertThat(cacheKeyGetterOrder( + new BddEndpointProviderSpec(ClientTestModels.queryServiceModelsWithSimpleBddEndpoints()))) + .as("a parameter nothing reads must not force a cache miss") + .doesNotContain("unusedParam"); + + assertThat(cacheKeyGetterOrder( + new BddEndpointProviderSpec(ClientTestModels.queryServiceModelsWithBddEndpoints()))) + .as("S3 declares Key, Prefix and CopySource but reads none of them") + .doesNotContain("key", "prefix", "copySource") + .contains("bucket"); } /** @@ -191,34 +216,56 @@ void cacheKeyOrdersBooleansThenStableStringsThenTheRest() { List order = cacheKeyGetterOrder( new BddEndpointProviderSpec(ClientTestModels.queryServiceModelsWithSimpleBddEndpoints())); - assertThat(order).containsExactly("useDualStack", "useFips", // booleans - "region", "stringContextParam", // reference-stable strings - "endpoint", "staticStringParam", // everything else, declaration order - "operationContextParam", "arnList"); + assertThat(order).containsExactly("useDualStack", "useFips", // booleans + "region", "stringContextParam", // reference-stable strings + "endpoint", "staticStringParam", // everything else, declaration order + "operationContextParam", "arnList", "customEndpointArray"); } /** - * A list parameter needs a bounded comparison, so it routes through the emitted helper rather than + * A list read as a whole needs a bounded comparison, so it routes through the emitted helper rather than * {@code Objects.equals}, whose {@code List.equals} would walk every element however long the list is. */ @Test - void listParametersUseTheBoundedHelper() { + void listReadAsAWholeUsesTheBoundedHelper() { String generated = new BddEndpointProviderSpec( ClientTestModels.queryServiceModelsWithSimpleBddEndpoints()).poetSpec().toString(); - assertThat(generated).contains("cacheListsMatch(a.arnList(), b.arnList())"); + assertThat(generated).contains("cacheListsMatch(a.customEndpointArray(), b.customEndpointArray())"); assertThat(generated).contains("if (size > 8) return false"); } /** - * The helper is only useful when the model has a list parameter, and the S3 BDD has none. + * When the only read of a list is its first element, the rest of the list cannot reach the endpoint, so the key + * compares element 0 alone. This is the DynamoDB shape: it reads {@code ResourceArnList} only through + * {@code getAttr(ResourceArnList, "[0]")}, and comparing the whole list costs more than half of a regional + * resolution. + * + *

Also asserts the generated provider really does read only element 0, so the two halves cannot drift apart: + * were a second, whole-list read to appear, this comparison would silently become wrong. + */ + @Test + void listReadOnlyAtIndexZeroComparesOnlyTheFirstElement() { + String generated = new BddEndpointProviderSpec( + ClientTestModels.queryServiceModelsWithSimpleBddEndpoints()).poetSpec().toString(); + + assertThat(generated).contains("cacheFirstElementsMatch(a.arnList(), b.arnList())"); + assertThat(generated).doesNotContain("cacheListsMatch(a.arnList()"); + assertThat(generated) + .as("the first-element comparison is only valid while the provider reads nothing but element 0") + .contains("RulesFunctions.listAccess(params.arnList(), 0)"); + } + + /** + * Neither helper is worth emitting when nothing needs it, and the S3 BDD keeps no list parameter in its key. */ @Test - void listHelperIsOmittedWhenNoListParameterExists() { + void listHelpersAreOmittedWhenNoListParameterIsInTheKey() { String generated = new BddEndpointProviderSpec( ClientTestModels.queryServiceModelsWithBddEndpoints()).poetSpec().toString(); assertThat(generated).doesNotContain("cacheListsMatch"); + assertThat(generated).doesNotContain("cacheFirstElementsMatch"); } /** diff --git a/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/client/c2j/query/endpoint-bdd-default-regional.json b/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/client/c2j/query/endpoint-bdd-default-regional.json index c3c6d4fd961f..dc45976fc131 100644 --- a/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/client/c2j/query/endpoint-bdd-default-regional.json +++ b/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/client/c2j/query/endpoint-bdd-default-regional.json @@ -29,23 +29,33 @@ }, "stringContextParam": { "required": false, - "documentation": "A client context parameter. Covers the CLIENT_STATIC_REF cache tier.", + "documentation": "A client context parameter, so the cache key compares it early as reference-stable.", "type": "string" }, "staticStringParam": { "required": false, - "documentation": "Bound to a per-operation static literal. Covers the OPERATION_STATIC cache tier.", + "documentation": "Bound to a per-operation static literal.", "type": "string" }, "operationContextParam": { "required": false, - "documentation": "Bound to a request member. Covers the REQUEST_DYNAMIC cache tier.", + "documentation": "Bound to a request member, so it can change per request.", "type": "string" }, "arnList": { "required": false, - "documentation": "Extracted from the request by JMESPath. Covers the REQUEST_LIST cache tier.", + "documentation": "Read only as arnList[0], so the cache key compares just the first element.", "type": "stringArray" + }, + "customEndpointArray": { + "required": false, + "documentation": "Read as a whole, so the cache key compares every element.", + "type": "stringArray" + }, + "unusedParam": { + "required": false, + "documentation": "Declared but read by no condition and no result, so the cache key must leave it out.", + "type": "string" } }, "conditions": [ @@ -136,6 +146,48 @@ ] } ] + }, + { + "fn": "isSet", + "argv": [ + { + "ref": "stringContextParam" + } + ] + }, + { + "fn": "isSet", + "argv": [ + { + "ref": "staticStringParam" + } + ] + }, + { + "fn": "isSet", + "argv": [ + { + "ref": "operationContextParam" + } + ] + }, + { + "fn": "isSet", + "argv": [ + { + "ref": "customEndpointArray" + } + ] + }, + { + "fn": "getAttr", + "argv": [ + { + "ref": "arnList" + }, + "[0]" + ], + "assign": "FirstArn" } ], "results": [ @@ -226,7 +278,7 @@ "type": "error" } ], - "root": 15, - "nodeCount": 15, - "nodes": "/////wAAAAH/////AAAABAX14QIF9eEDAAAAAgX14QEAAAACAAAABgX14QQF9eEFAAAABQAAAAQF9eEFAAAABwX14QYF9eEHAAAABQAAAAYF9eEIAAAABAAAAAUAAAAHAAAAAwAAAAgF9eEMAAAABgX14QkF9eEKAAAABAAAAAoF9eELAAAAAwAAAAsF9eEMAAAAAgAAAAkAAAAMAAAAAQAAAA0F9eEMAAAAAAAAAAMAAAAO" -} \ No newline at end of file + "root": 16, + "nodeCount": 20, + "nodes": "/////wAAAAH/////AAAABAX14QIF9eEDAAAAAgX14QEAAAACAAAABgX14QQF9eEFAAAABQAAAAQF9eEFAAAABwX14QYF9eEHAAAABQAAAAYF9eEIAAAABAAAAAUAAAAHAAAAAwAAAAgF9eEMAAAABgX14QkF9eEKAAAABAAAAAoF9eELAAAAAwAAAAsF9eEMAAAAAgAAAAkAAAAMAAAAAQAAAA0F9eEMAAAAAAAAAAMAAAAOAAAACAAAABEAAAARAAAACQAAABIAAAASAAAACgAAABMAAAATAAAACwAAABQAAAAUAAAADAAAAA8AAAAP" +} diff --git a/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/client/c2j/query/endpoint-rule-set-default-regional.json b/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/client/c2j/query/endpoint-rule-set-default-regional.json index 6d822ec9f08d..6c3b34e6e15f 100644 --- a/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/client/c2j/query/endpoint-rule-set-default-regional.json +++ b/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/client/c2j/query/endpoint-rule-set-default-regional.json @@ -29,23 +29,33 @@ }, "stringContextParam": { "required": false, - "documentation": "A client context parameter. Covers the CLIENT_STATIC_REF cache tier.", + "documentation": "A client context parameter, so the cache key compares it early as reference-stable.", "type": "string" }, "staticStringParam": { "required": false, - "documentation": "Bound to a per-operation static literal. Covers the OPERATION_STATIC cache tier.", + "documentation": "Bound to a per-operation static literal.", "type": "string" }, "operationContextParam": { "required": false, - "documentation": "Bound to a request member. Covers the REQUEST_DYNAMIC cache tier.", + "documentation": "Bound to a request member, so it can change per request.", "type": "string" }, "arnList": { "required": false, - "documentation": "Extracted from the request by JMESPath. Covers the REQUEST_LIST cache tier.", + "documentation": "Read only as arnList[0], so the cache key compares just the first element.", "type": "stringArray" + }, + "customEndpointArray": { + "required": false, + "documentation": "Read as a whole, so the cache key compares every element.", + "type": "stringArray" + }, + "unusedParam": { + "required": false, + "documentation": "Declared but read by no condition and no result, so the cache key must leave it out.", + "type": "string" } }, "rules": [ @@ -356,4 +366,4 @@ "type": "error" } ] -} \ No newline at end of file +} diff --git a/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules2/bdd/endpoint-provider-bdd-class.java b/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules2/bdd/endpoint-provider-bdd-class.java index d2ac2377e5ae..c243299f159d 100644 --- a/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules2/bdd/endpoint-provider-bdd-class.java +++ b/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules2/bdd/endpoint-provider-bdd-class.java @@ -28,7 +28,7 @@ public CompletableFuture resolveEndpoint(QueryEndpointParams endpointP Evaluator evaluator = new Evaluator(); evaluator.params = endpointParams; evaluator.region = endpointParams.region() == null ? null : endpointParams.region().id(); - Endpoint result = evaluator.nodeP14(); + Endpoint result = evaluator.nodeP15(); if (result == null) { return CompletableFutureUtils.failedFuture(SdkClientException.create("Rule engine did not reach an error or endpoint result")); } @@ -53,7 +53,8 @@ private static boolean cacheParamsMatch(QueryEndpointParams a, QueryEndpointPara && Objects.equals(a.endpoint(), b.endpoint()) && Objects.equals(a.staticStringParam(), b.staticStringParam()) && Objects.equals(a.operationContextParam(), b.operationContextParam()) - && cacheListsMatch(a.arnList(), b.arnList()); + && cacheFirstElementsMatch(a.arnList(), b.arnList()) + && cacheListsMatch(a.customEndpointArray(), b.customEndpointArray()); } private static boolean cacheListsMatch(List a, List b) { @@ -69,6 +70,14 @@ private static boolean cacheListsMatch(List a, List b) { return true; } + private static boolean cacheFirstElementsMatch(List a, List b) { + if (a == b) return true; + // Only element 0 reaches the endpoint; absent and empty are both null to the rules engine. + String firstA = a == null || a.isEmpty() ? null : a.get(0); + String firstB = b == null || b.isEmpty() ? null : b.get(0); + return Objects.equals(firstA, firstB); + } + private static final class Evaluator { QueryEndpointParams params; @@ -76,6 +85,8 @@ private static final class Evaluator { RulePartition partitionResult; + String firstArn; + private Endpoint nodeP0() { return null; } @@ -164,6 +175,36 @@ private Endpoint nodeP14() { : nodeP13(); } + private Endpoint nodeP15() { + return params.stringContextParam() != null + ? nodeP16() + : nodeP16(); + } + + private Endpoint nodeP16() { + return params.staticStringParam() != null + ? nodeP17() + : nodeP17(); + } + + private Endpoint nodeP17() { + return params.operationContextParam() != null + ? nodeP18() + : nodeP18(); + } + + private Endpoint nodeP18() { + return params.customEndpointArray() != null + ? nodeP19() + : nodeP19(); + } + + private Endpoint nodeP19() { + return cond12() + ? nodeP14() + : nodeP14(); + } + private boolean cond3() { partitionResult = RulesFunctions.awsPartition(region); return partitionResult != null; @@ -181,6 +222,11 @@ private boolean cond7() { return ("aws-us-gov".equals(partitionResult.name())); } + private boolean cond12() { + firstArn = RulesFunctions.listAccess(params.arnList(), 0); + return firstArn != null; + } + private Endpoint result0() { throw SdkClientException.create("Invalid Configuration: FIPS and custom endpoint are not supported"); } diff --git a/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules2/bdd/endpoint-provider-bdd-s3-class.java b/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules2/bdd/endpoint-provider-bdd-s3-class.java index b5961bc1b718..7b55c6c57497 100644 --- a/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules2/bdd/endpoint-provider-bdd-s3-class.java +++ b/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules2/bdd/endpoint-provider-bdd-s3-class.java @@ -63,10 +63,7 @@ private static boolean cacheParamsMatch(QueryEndpointParams a, QueryEndpointPara && Objects.equals(a.disableS3ExpressSessionAuth(), b.disableS3ExpressSessionAuth()) && Objects.equals(a.region(), b.region()) && Objects.equals(a.bucket(), b.bucket()) - && Objects.equals(a.endpoint(), b.endpoint()) - && Objects.equals(a.key(), b.key()) - && Objects.equals(a.prefix(), b.prefix()) - && Objects.equals(a.copySource(), b.copySource()); + && Objects.equals(a.endpoint(), b.endpoint()); } private static final class Evaluator { diff --git a/test/codegen-generated-classes-test/src/main/resources/codegen-resources/bddendpoints/endpoint-bdd-1.json b/test/codegen-generated-classes-test/src/main/resources/codegen-resources/bddendpoints/endpoint-bdd-1.json index 4a2dd64848c1..3fc4fe05d154 100644 --- a/test/codegen-generated-classes-test/src/main/resources/codegen-resources/bddendpoints/endpoint-bdd-1.json +++ b/test/codegen-generated-classes-test/src/main/resources/codegen-resources/bddendpoints/endpoint-bdd-1.json @@ -30,33 +30,43 @@ "AccountId": { "builtIn": "AWS::Auth::AccountId", "required": false, - "documentation": "The AWS account ID, read off the resolved identity. Declared to exercise the IDENTITY_DERIVED cache tier; the BDD graph does not read it.", + "documentation": "The AWS account ID, read off the resolved identity.", "type": "string" }, "AccountIdEndpointMode": { "builtIn": "AWS::Auth::AccountIdEndpointMode", "required": false, - "documentation": "Whether the account ID may be used in the endpoint. Declared to exercise the SEMI_STABLE cache tier; the BDD graph does not read it.", + "documentation": "Whether the account ID may be used in the endpoint.", "type": "string" }, "clientStringParam": { "required": false, - "documentation": "A client context parameter. Declared to exercise the CLIENT_STATIC_REF cache tier; the BDD graph does not read it.", + "documentation": "A client context parameter, so the cache key compares it early as reference-stable.", "type": "string" }, "staticStringParam": { "required": false, - "documentation": "A parameter bound to a per-operation static literal. Declared to exercise the OPERATION_STATIC cache tier; the BDD graph does not read it.", + "documentation": "Bound to a per-operation static literal.", "type": "string" }, "requestStringParam": { "required": false, - "documentation": "A parameter bound to a request member. Declared to exercise the REQUEST_DYNAMIC cache tier; the BDD graph does not read it.", + "documentation": "Bound to a request member, so it can change per request.", "type": "string" }, "resourceArnList": { "required": false, - "documentation": "A list extracted from the request by JMESPath. Declared to exercise the REQUEST_LIST cache tier; the BDD graph does not read it.", + "documentation": "Read only as resourceArnList[0], so the cache key compares just the first element. This is the DynamoDB shape.", + "type": "stringArray" + }, + "unusedStringParam": { + "required": false, + "documentation": "Declared but read by no condition and no result, so the cache key must leave it out.", + "type": "string" + }, + "wholeArnList": { + "required": false, + "documentation": "Read as a whole, so the cache key compares every element.", "type": "stringArray" } }, @@ -148,6 +158,64 @@ }, "aws-us-gov" ] + }, + { + "fn": "isSet", + "argv": [ + { + "ref": "AccountId" + } + ] + }, + { + "fn": "isSet", + "argv": [ + { + "ref": "AccountIdEndpointMode" + } + ] + }, + { + "fn": "isSet", + "argv": [ + { + "ref": "clientStringParam" + } + ] + }, + { + "fn": "isSet", + "argv": [ + { + "ref": "staticStringParam" + } + ] + }, + { + "fn": "isSet", + "argv": [ + { + "ref": "requestStringParam" + } + ] + }, + { + "fn": "getAttr", + "argv": [ + { + "ref": "resourceArnList" + }, + "[0]" + ], + "assign": "FirstArn" + }, + { + "fn": "isSet", + "argv": [ + { + "ref": "wholeArnList" + } + ] } ], "results": [ @@ -238,7 +306,7 @@ "type": "error" } ], - "root": 2, - "nodeCount": 14, - "nodes": "/////wAAAAH/////AAAAAAAAAA0AAAADAAAAAQAAAAQF9eEMAAAAAgAAAAUF9eEMAAAAAwAAAAgAAAAGAAAABAAAAAcF9eELAAAABQX14QkF9eEKAAAABAAAAAsAAAAJAAAABgAAAAoF9eEIAAAABwX14QYF9eEHAAAABQAAAAwF9eEFAAAABgX14QQF9eEFAAAAAwX14QEAAAAOAAAABAX14QIF9eED" -} \ No newline at end of file + "root": 21, + "nodeCount": 21, + "nodes": "/////wAAAAH/////AAAAAAAAAA0AAAADAAAAAQAAAAQF9eEMAAAAAgAAAAUF9eEMAAAAAwAAAAgAAAAGAAAABAAAAAcF9eELAAAABQX14QkF9eEKAAAABAAAAAsAAAAJAAAABgAAAAoF9eEIAAAABwX14QYF9eEHAAAABQAAAAwF9eEFAAAABgX14QQF9eEFAAAAAwX14QEAAAAOAAAABAX14QIF9eEDAAAACAAAABAAAAAQAAAACQAAABEAAAARAAAACgAAABIAAAASAAAACwAAABMAAAATAAAADAAAABQAAAAUAAAADQAAAAIAAAACAAAADgAAAA8AAAAP" +} diff --git a/test/codegen-generated-classes-test/src/main/resources/codegen-resources/bddendpoints/endpoint-rule-set.json b/test/codegen-generated-classes-test/src/main/resources/codegen-resources/bddendpoints/endpoint-rule-set.json index 11f51c22fffa..74d707c19c06 100644 --- a/test/codegen-generated-classes-test/src/main/resources/codegen-resources/bddendpoints/endpoint-rule-set.json +++ b/test/codegen-generated-classes-test/src/main/resources/codegen-resources/bddendpoints/endpoint-rule-set.json @@ -30,33 +30,43 @@ "AccountId": { "builtIn": "AWS::Auth::AccountId", "required": false, - "documentation": "The AWS account ID, read off the resolved identity. Declared to exercise the IDENTITY_DERIVED cache tier; the BDD graph does not read it.", + "documentation": "The AWS account ID, read off the resolved identity.", "type": "string" }, "AccountIdEndpointMode": { "builtIn": "AWS::Auth::AccountIdEndpointMode", "required": false, - "documentation": "Whether the account ID may be used in the endpoint. Declared to exercise the SEMI_STABLE cache tier; the BDD graph does not read it.", + "documentation": "Whether the account ID may be used in the endpoint.", "type": "string" }, "clientStringParam": { "required": false, - "documentation": "A client context parameter. Declared to exercise the CLIENT_STATIC_REF cache tier; the BDD graph does not read it.", + "documentation": "A client context parameter, so the cache key compares it early as reference-stable.", "type": "string" }, "staticStringParam": { "required": false, - "documentation": "A parameter bound to a per-operation static literal. Declared to exercise the OPERATION_STATIC cache tier; the BDD graph does not read it.", + "documentation": "Bound to a per-operation static literal.", "type": "string" }, "requestStringParam": { "required": false, - "documentation": "A parameter bound to a request member. Declared to exercise the REQUEST_DYNAMIC cache tier; the BDD graph does not read it.", + "documentation": "Bound to a request member, so it can change per request.", "type": "string" }, "resourceArnList": { "required": false, - "documentation": "A list extracted from the request by JMESPath. Declared to exercise the REQUEST_LIST cache tier; the BDD graph does not read it.", + "documentation": "Read only as resourceArnList[0], so the cache key compares just the first element. This is the DynamoDB shape.", + "type": "stringArray" + }, + "unusedStringParam": { + "required": false, + "documentation": "Declared but read by no condition and no result, so the cache key must leave it out.", + "type": "string" + }, + "wholeArnList": { + "required": false, + "documentation": "Read as a whole, so the cache key compares every element.", "type": "stringArray" } }, @@ -368,4 +378,4 @@ "type": "error" } ] -} \ No newline at end of file +} diff --git a/test/codegen-generated-classes-test/src/main/resources/codegen-resources/bddendpoints/service-2.json b/test/codegen-generated-classes-test/src/main/resources/codegen-resources/bddendpoints/service-2.json index a77daf8c70ce..0b1408801fca 100644 --- a/test/codegen-generated-classes-test/src/main/resources/codegen-resources/bddendpoints/service-2.json +++ b/test/codegen-generated-classes-test/src/main/resources/codegen-resources/bddendpoints/service-2.json @@ -75,6 +75,9 @@ "operationContextParams": { "resourceArnList": { "path": "Items[*].Arn" + }, + "wholeArnList": { + "path": "Items[*].Arn" } }, "input": { diff --git a/test/codegen-generated-classes-test/src/test/java/software/amazon/awssdk/services/bddendpoints/BddEndpointProviderCacheTest.java b/test/codegen-generated-classes-test/src/test/java/software/amazon/awssdk/services/bddendpoints/BddEndpointProviderCacheTest.java index b5a64682ca24..37b4670cccfc 100644 --- a/test/codegen-generated-classes-test/src/test/java/software/amazon/awssdk/services/bddendpoints/BddEndpointProviderCacheTest.java +++ b/test/codegen-generated-classes-test/src/test/java/software/amazon/awssdk/services/bddendpoints/BddEndpointProviderCacheTest.java @@ -210,24 +210,100 @@ void requestDynamicTier_requestStringParamChange_invalidates() { params(b -> b.requestStringParam("second"))); } + // ---- lists read as a whole: wholeArnList, reached via isSet, so every element is part of the key ---- + @Test - void requestListTier_elementChange_invalidates() { - assertInvalidates(params(b -> b.resourceArnList(Arrays.asList("a", "b"))), - params(b -> b.resourceArnList(Arrays.asList("a", "c")))); + void wholeList_elementChange_invalidates() { + assertInvalidates(params(b -> b.wholeArnList(Arrays.asList("a", "b"))), + params(b -> b.wholeArnList(Arrays.asList("a", "c")))); + } + + @Test + void wholeList_lengthChange_invalidates() { + assertInvalidates(params(b -> b.wholeArnList(Arrays.asList("a", "b"))), + params(b -> b.wholeArnList(Collections.singletonList("a")))); } @Test - void requestListTier_lengthChange_invalidates() { + void wholeList_orderChange_invalidates() { + assertInvalidates(params(b -> b.wholeArnList(Arrays.asList("a", "b"))), + params(b -> b.wholeArnList(Arrays.asList("b", "a")))); + } + + // ---- lists read only at index 0: resourceArnList, reached via getAttr(list, "[0]") ---- + // + // The BDD's only read of this list is its first element, so nothing past element 0 can reach the endpoint. The key + // therefore compares element 0 alone, which turns changes the endpoint cannot see into hits rather than misses. + // This is the DynamoDB shape, where comparing the whole list costs more than half a regional resolution. + + @Test + void firstElementList_firstElementChange_invalidates() { assertInvalidates(params(b -> b.resourceArnList(Arrays.asList("a", "b"))), - params(b -> b.resourceArnList(Collections.singletonList("a")))); + params(b -> b.resourceArnList(Arrays.asList("z", "b")))); } @Test - void requestListTier_orderChange_invalidates() { + void firstElementList_laterElementChange_isAHit() { + assertHits(params(b -> b.resourceArnList(Arrays.asList("a", "b"))), + params(b -> b.resourceArnList(Arrays.asList("a", "c")))); + } + + @Test + void firstElementList_lengthChangeKeepingFirstElement_isAHit() { + assertHits(params(b -> b.resourceArnList(Arrays.asList("a", "b", "c"))), + params(b -> b.resourceArnList(Collections.singletonList("a")))); + } + + @Test + void firstElementList_orderChangeMovingFirstElement_invalidates() { assertInvalidates(params(b -> b.resourceArnList(Arrays.asList("a", "b"))), params(b -> b.resourceArnList(Arrays.asList("b", "a")))); } + /** + * The rules engine's {@code listAccess} returns null for a null list and for an index past the end, so an absent + * list and an empty one take the same branch and must be treated as the same key. + */ + @Test + void firstElementList_emptyAndUnset_areTheSameKey() { + assertHits(params(b -> b.resourceArnList(Collections.emptyList())), params(b -> { + })); + } + + /** + * No size cap applies when only the first element is compared, so a list far past the cap still hits. That is the + * point: the comparison is O(1) rather than bounded-but-linear. + */ + @Test + void firstElementList_farPastTheSizeCap_stillHits() { + List long1 = new ArrayList<>(listOfSize(500)); + List long2 = new ArrayList<>(listOfSize(500)); + long2.set(499, "different-tail"); + assertHits(params(b -> b.resourceArnList(long1)), params(b -> b.resourceArnList(long2))); + } + + // ---- parameters the BDD never reads are not part of the key ---- + + /** + * {@code unusedStringParam} is declared by the model and read by no condition and no result, so it cannot change the + * resolved endpoint and must not evict the cached one. + * + *

This is the behaviour that makes the cache worth having for S3, whose rule set declares {@code Key}, + * {@code Prefix} and {@code CopySource} and reads none of them. {@code Key} changes on essentially every object + * request, so treating it as part of the key would mean the cache almost never hits. + */ + @Test + void parameterTheBddNeverReads_doesNotInvalidate() { + assertHits(params(b -> b.unusedStringParam("first")), + params(b -> b.unusedStringParam("second"))); + } + + @Test + void parameterTheBddNeverReads_settingItDoesNotInvalidate() { + assertHits(params(b -> { + }), params(b -> b.unusedStringParam("now-set"))); + } + // ---- transitions to and from unset ---- @Test @@ -245,18 +321,18 @@ void clearingAPreviouslySetParam_invalidates() { @Test void settingAPreviouslyUnsetList_invalidates() { assertInvalidates(params(b -> { - }), params(b -> b.resourceArnList(Collections.singletonList("a")))); + }), params(b -> b.wholeArnList(Collections.singletonList("a")))); } @Test void clearingAPreviouslySetList_invalidates() { - assertInvalidates(params(b -> b.resourceArnList(Collections.singletonList("a"))), params(b -> { + assertInvalidates(params(b -> b.wholeArnList(Collections.singletonList("a"))), params(b -> { })); } @Test void emptyListAndUnsetList_areDistinguished() { - assertInvalidates(params(b -> b.resourceArnList(Collections.emptyList())), params(b -> { + assertInvalidates(params(b -> b.wholeArnList(Collections.emptyList())), params(b -> { })); } @@ -282,8 +358,8 @@ void equalsFallbackTiers_equalValueDifferentReference_hits() { @Test void requestList_equalContentsDifferentListInstance_hits() { - assertHits(params(b -> b.resourceArnList(new ArrayList<>(Arrays.asList("a", "b")))), - params(b -> b.resourceArnList(new ArrayList<>(Arrays.asList("a", "b"))))); + assertHits(params(b -> b.wholeArnList(new ArrayList<>(Arrays.asList("a", "b")))), + params(b -> b.wholeArnList(new ArrayList<>(Arrays.asList("a", "b"))))); } /** @@ -291,8 +367,8 @@ void requestList_equalContentsDifferentListInstance_hits() { */ @Test void requestList_equalElementsDifferentReferences_hits() { - assertHits(params(b -> b.resourceArnList(Collections.singletonList("element"))), - params(b -> b.resourceArnList(Collections.singletonList(new String("element"))))); + assertHits(params(b -> b.wholeArnList(Collections.singletonList("element"))), + params(b -> b.wholeArnList(Collections.singletonList(new String("element"))))); } // ---- list size cap ---- @@ -302,7 +378,7 @@ void requestList_equalElementsDifferentReferences_hits() { */ @Test void requestList_atSizeCap_stillHits() { - assertHits(params(b -> b.resourceArnList(listOfSize(8))), params(b -> b.resourceArnList(listOfSize(8)))); + assertHits(params(b -> b.wholeArnList(listOfSize(8))), params(b -> b.wholeArnList(listOfSize(8)))); } /** @@ -312,8 +388,8 @@ void requestList_atSizeCap_stillHits() { */ @Test void requestList_pastSizeCap_alwaysMisses() { - assertInvalidates(params(b -> b.resourceArnList(listOfSize(9))), - params(b -> b.resourceArnList(listOfSize(9)))); + assertInvalidates(params(b -> b.wholeArnList(listOfSize(9))), + params(b -> b.wholeArnList(listOfSize(9)))); } /** @@ -321,7 +397,7 @@ void requestList_pastSizeCap_alwaysMisses() { */ @Test void requestList_pastSizeCap_stillResolvesCorrectly() { - Endpoint endpoint = resolve(provider(), params(b -> b.resourceArnList(listOfSize(50)))); + Endpoint endpoint = resolve(provider(), params(b -> b.wholeArnList(listOfSize(50)))); assertThat(endpoint.endpointUrl().host()).isEqualTo("connect.us-east-1.amazonaws.com"); } From 2b43c5c68f54be4deeecb456d5f201ef308a969f Mon Sep 17 00:00:00 2001 From: Alex Woods Date: Thu, 27 Aug 2026 10:52:10 -0700 Subject: [PATCH 4/8] fix(endpoints): Let an isSet guard keep the first-element list comparison The rules language requires a null check before an indexed access, so a model that reads list[0] always reads isSet(list) as well. Counting that null check as a whole-value read meant the first-element comparison never fired on a real model: DynamoDB's ResourceArnList, the case it was added for, was still compared element by element. isSet observes only whether the parameter is present, so on its own it no longer disqualifies the parameter. Verified against the real DynamoDB BDD: the analysis now classifies ResourceArnList as FIRST_ELEMENT_ONLY and the other eight parameters as FULL. Presence does have to stay in the cache key, though, and that is a change from the previous commit. Because isSet tells an absent list apart from an empty one, the generated comparison now checks presence as well as element 0, rather than treating both as null: if (a == b) return true; if (a == null || b == null) return false; String firstA = a.isEmpty() ? null : a.get(0); String firstB = b.isEmpty() ? null : b.get(0); return Objects.equals(firstA, firstB); Collapsing absent and empty is sound only when the BDD's branches for the two converge. DynamoDB's do - traced through its graph, cond20 false and cond21 false both land on nodeP27, and likewise nodeP53 for the second occurrence - so it would have been correct there. It is a property of the graph rather than of the parameter, so relying on it would mean a future model could quietly invalidate the comparison. One extra reference check buys independence from that, and the comparison stays O(1). Test model changes: - Both BDD fixtures gained the isSet guard ahead of their index-0 access, so they match the shape a real model produces. Without it the fixtures were testing a case that cannot occur. - The whole-list parameters were previously read only via isSet, which now correctly qualifies them for the first-element comparison and left the whole-list path uncovered. Each now also reads index 1, the smallest realistic change that makes the rest of the list matter. - firstElementList_emptyAndUnset_areTheSameKey becomes ..._areDistinguished, matching the new semantics. - New codegen assertion that a list read past the head is compared in full, since the boundary between the two helpers is a correctness line rather than a preference. - Mutation-checked the new logic: treating isSet as a whole-value read - exactly the reported regression - fails three runtime tests, the codegen assertion, and the golden file. codegen 710 pass, codegen-generated-classes-test 3685 pass, checkstyle clean. --- .../rules2/bdd/BddEndpointProviderSpec.java | 24 ++++++----- .../rules2/bdd/BddParameterReferences.java | 38 ++++++++++++++++-- .../bdd/BddEndpointProviderSpecTest.java | 40 ++++++++++++++----- .../query/endpoint-bdd-default-regional.json | 24 +++++++++-- .../bdd/endpoint-provider-bdd-class.java | 29 ++++++++++++-- .../bddendpoints/endpoint-bdd-1.json | 24 +++++++++-- .../BddEndpointProviderCacheTest.java | 12 ++++-- 7 files changed, 154 insertions(+), 37 deletions(-) diff --git a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpec.java b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpec.java index 55107aaa002b..7ce4ac13bb82 100644 --- a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpec.java +++ b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpec.java @@ -338,13 +338,16 @@ private MethodSpec cacheListsMatchMethod() { * Generates the {@code cacheFirstElementsMatch} helper, emitted only when a {@code stringArray} parameter is read * exclusively as {@code param[0]}. * - *

When only the first element can reach the endpoint, comparing the rest is work that cannot change the answer. - * DynamoDB is why this exists: it reads {@code ResourceArnList} only through - * {@code getAttr(ResourceArnList, "[0]")}, and comparing a freshly built three-element ARN list measured at 15.5 ns - * against a 28 ns regional resolution - over half the cost the cache is meant to avoid. + *

When the rules can only see whether the list is present and what its first element is, comparing the rest is + * work that cannot change the answer. DynamoDB is why this exists: it reads {@code ResourceArnList} only through + * {@code isSet} and {@code getAttr(ResourceArnList, "[0]")}, and comparing a freshly built three-element ARN list + * measured at 15.5 ns against a 28 ns regional resolution - over half the cost the cache is meant to avoid. This + * comparison is O(1) instead. * - *

Absent and empty both yield null here, matching the runtime's {@code listAccess}, which returns null for a null - * list and for an index past the end. So the two are interchangeable, exactly as they are during resolution. + *

Presence is compared as well as the first element, because {@code isSet} tells an absent list apart from an + * empty one even though {@code listAccess} yields null for both. Collapsing them would be sound only for a BDD whose + * branches for absent and empty converge - true of DynamoDB's, but a property of the graph rather than of the + * parameter, and not worth depending on for the one extra reference comparison it would save. */ private MethodSpec cacheFirstElementsMatchMethod() { TypeName listOfString = RuleRuntimeTypeMirror.LIST_OF_STRING.type(); @@ -354,10 +357,11 @@ private MethodSpec cacheFirstElementsMatchMethod() { .addParameter(listOfString, "a") .addParameter(listOfString, "b") .addStatement("if (a == b) return true") - .addComment("Only element 0 reaches the endpoint; absent and empty are both null to the " - + "rules engine.") - .addStatement("$T firstA = a == null || a.isEmpty() ? null : a.get(0)", String.class) - .addStatement("$T firstB = b == null || b.isEmpty() ? null : b.get(0)", String.class) + .addComment("isSet can tell an absent list from an empty one, so presence is part of the key.") + .addStatement("if (a == null || b == null) return false") + .addComment("Nothing past element 0 can reach the endpoint.") + .addStatement("$T firstA = a.isEmpty() ? null : a.get(0)", String.class) + .addStatement("$T firstB = b.isEmpty() ? null : b.get(0)", String.class) .addStatement("return $T.equals(firstA, firstB)", Objects.class) .build(); } diff --git a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddParameterReferences.java b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddParameterReferences.java index 08a6dc4c79f2..aeb9f5743445 100644 --- a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddParameterReferences.java +++ b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddParameterReferences.java @@ -26,6 +26,7 @@ import software.amazon.awssdk.codegen.model.rules.endpoints.RuleModel; import software.amazon.awssdk.codegen.model.service.EndpointBddModel; import software.amazon.awssdk.codegen.poet.rules2.ExpressionParser; +import software.amazon.awssdk.codegen.poet.rules2.FunctionCallExpression; import software.amazon.awssdk.codegen.poet.rules2.IndexedAccessExpression; import software.amazon.awssdk.codegen.poet.rules2.MemberAccessExpression; import software.amazon.awssdk.codegen.poet.rules2.RuleExpression; @@ -63,11 +64,18 @@ enum Usage { UNREFERENCED, /** - * A {@code stringArray} read only as {@code param[0]}. Comparing the first element is sufficient. + * A {@code stringArray} the rules can only observe through {@code isSet(param)} and {@code param[0]}. Whether + * the list is present, plus its first element, is therefore the whole of what can reach the endpoint, and + * nothing past element 0 can change the answer. * - *

This also makes a null list and an empty list interchangeable, which is correct: the runtime's - * {@code listAccess} returns null for both, so both take the same branch. It is strictly more permissive than - * comparing whole lists, so it can only turn misses into hits. + *

{@code isSet} has to be allowed here, not just tolerated: the rules language requires a null check before + * an indexed access, so every real model that reads {@code param[0]} also reads {@code isSet(param)}. Treating + * the null check as a whole-value read would mean this case never fired. + * + *

Because {@code isSet} distinguishes an absent list from an empty one, the generated comparison keeps a + * null check alongside the first-element check. Collapsing the two would only be sound for a BDD whose branches + * for absent and empty converge, which is a property of the graph rather than of the parameter, so it is not + * assumed here. */ FIRST_ELEMENT_ONLY, @@ -140,6 +148,28 @@ public Void visitIndexedAccessExpression(IndexedAccessExpression e) { return super.visitIndexedAccessExpression(e); } + /** + * {@code isSet(param)} observes only whether the parameter is present, so on its own it does not force a + * whole-value comparison. + * + *

This matters because the rules language requires a null check before an indexed access: a model that reads + * {@code list[0]} always reads {@code isSet(list)} too. Counting the null check as a whole-value read would stop + * {@link Usage#FIRST_ELEMENT_ONLY} from ever applying to a real model. + * + *

Presence still has to be part of the cache key, which the generated comparison handles. + */ + @Override + public Void visitFunctionCallExpression(FunctionCallExpression e) { + if ("isSet".equals(e.name()) && e.arguments().size() == 1) { + RuleExpression argument = e.arguments().get(0); + if (argument instanceof VariableReferenceExpression) { + referenced.add(((VariableReferenceExpression) argument).variableName()); + return null; + } + } + return super.visitFunctionCallExpression(e); + } + @Override public Void visitVariableReferenceExpression(VariableReferenceExpression e) { referenced.add(e.variableName()); diff --git a/codegen/src/test/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpecTest.java b/codegen/src/test/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpecTest.java index e57950d63f8b..be4f05085c7c 100644 --- a/codegen/src/test/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpecTest.java +++ b/codegen/src/test/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpecTest.java @@ -236,24 +236,46 @@ void listReadAsAWholeUsesTheBoundedHelper() { } /** - * When the only read of a list is its first element, the rest of the list cannot reach the endpoint, so the key - * compares element 0 alone. This is the DynamoDB shape: it reads {@code ResourceArnList} only through - * {@code getAttr(ResourceArnList, "[0]")}, and comparing the whole list costs more than half of a regional - * resolution. + * When the rules can only see whether a list is present and what its first element is, the rest of the list cannot + * reach the endpoint, so the key compares presence and element 0. This is the DynamoDB shape: it reads + * {@code ResourceArnList} through {@code isSet} and {@code getAttr(ResourceArnList, "[0]")} and nothing else, and + * comparing the whole list costs more than half of a regional resolution. * - *

Also asserts the generated provider really does read only element 0, so the two halves cannot drift apart: - * were a second, whole-list read to appear, this comparison would silently become wrong. + *

The {@code isSet} guard is not incidental. The rules language requires a null check before an indexed access, + * so every real model that reads {@code list[0]} also reads {@code isSet(list)}; if the null check disqualified the + * parameter this case would never fire in production. + * + *

Also asserts the generated provider really does read only element 0, so the two halves cannot drift apart: were + * a read past the head to appear, this comparison would silently become wrong. */ @Test - void listReadOnlyAtIndexZeroComparesOnlyTheFirstElement() { + void listReadOnlyAtIndexZeroComparesPresenceAndTheFirstElement() { String generated = new BddEndpointProviderSpec( ClientTestModels.queryServiceModelsWithSimpleBddEndpoints()).poetSpec().toString(); assertThat(generated).contains("cacheFirstElementsMatch(a.arnList(), b.arnList())"); assertThat(generated).doesNotContain("cacheListsMatch(a.arnList()"); assertThat(generated) - .as("the first-element comparison is only valid while the provider reads nothing but element 0") - .contains("RulesFunctions.listAccess(params.arnList(), 0)"); + .as("the first-element comparison is only valid while the provider reads nothing past element 0") + .contains("RulesFunctions.listAccess(params.arnList(), 0)") + .doesNotContain("RulesFunctions.listAccess(params.arnList(), 1)"); + assertThat(generated) + .as("isSet distinguishes an absent list from an empty one, so presence stays part of the key") + .contains("if (a == null || b == null) return false"); + } + + /** + * A read past the head disqualifies the first-element comparison, because elements beyond the first can then reach + * the endpoint. Pinned because the difference between the two helpers is a correctness boundary, not a preference. + */ + @Test + void listReadPastTheHeadIsComparedInFull() { + String generated = new BddEndpointProviderSpec( + ClientTestModels.queryServiceModelsWithSimpleBddEndpoints()).poetSpec().toString(); + + assertThat(generated).contains("cacheListsMatch(a.customEndpointArray(), b.customEndpointArray())"); + assertThat(generated).doesNotContain("cacheFirstElementsMatch(a.customEndpointArray()"); + assertThat(generated).contains("RulesFunctions.listAccess(params.customEndpointArray(), 1)"); } /** diff --git a/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/client/c2j/query/endpoint-bdd-default-regional.json b/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/client/c2j/query/endpoint-bdd-default-regional.json index dc45976fc131..a05255ae456e 100644 --- a/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/client/c2j/query/endpoint-bdd-default-regional.json +++ b/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/client/c2j/query/endpoint-bdd-default-regional.json @@ -188,6 +188,24 @@ "[0]" ], "assign": "FirstArn" + }, + { + "fn": "isSet", + "argv": [ + { + "ref": "arnList" + } + ] + }, + { + "fn": "getAttr", + "argv": [ + { + "ref": "customEndpointArray" + }, + "[1]" + ], + "assign": "SecondEndpoint" } ], "results": [ @@ -278,7 +296,7 @@ "type": "error" } ], - "root": 16, - "nodeCount": 20, - "nodes": "/////wAAAAH/////AAAABAX14QIF9eEDAAAAAgX14QEAAAACAAAABgX14QQF9eEFAAAABQAAAAQF9eEFAAAABwX14QYF9eEHAAAABQAAAAYF9eEIAAAABAAAAAUAAAAHAAAAAwAAAAgF9eEMAAAABgX14QkF9eEKAAAABAAAAAoF9eELAAAAAwAAAAsF9eEMAAAAAgAAAAkAAAAMAAAAAQAAAA0F9eEMAAAAAAAAAAMAAAAOAAAACAAAABEAAAARAAAACQAAABIAAAASAAAACgAAABMAAAATAAAACwAAABQAAAAUAAAADAAAAA8AAAAP" + "root": 22, + "nodeCount": 22, + "nodes": "/////wAAAAH/////AAAABAX14QIF9eEDAAAAAgX14QEAAAACAAAABgX14QQF9eEFAAAABQAAAAQF9eEFAAAABwX14QYF9eEHAAAABQAAAAYF9eEIAAAABAAAAAUAAAAHAAAAAwAAAAgF9eEMAAAABgX14QkF9eEKAAAABAAAAAoF9eELAAAAAwAAAAsF9eEMAAAAAgAAAAkAAAAMAAAAAQAAAA0F9eEMAAAAAAAAAAMAAAAOAAAACAAAABEAAAARAAAACQAAABIAAAASAAAACgAAABMAAAATAAAACwAAABQAAAAUAAAADAAAAA8AAAAPAAAADQAAABAAAAAQAAAADgAAABUAAAAV" } diff --git a/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules2/bdd/endpoint-provider-bdd-class.java b/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules2/bdd/endpoint-provider-bdd-class.java index c243299f159d..32365c942d44 100644 --- a/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules2/bdd/endpoint-provider-bdd-class.java +++ b/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules2/bdd/endpoint-provider-bdd-class.java @@ -28,7 +28,7 @@ public CompletableFuture resolveEndpoint(QueryEndpointParams endpointP Evaluator evaluator = new Evaluator(); evaluator.params = endpointParams; evaluator.region = endpointParams.region() == null ? null : endpointParams.region().id(); - Endpoint result = evaluator.nodeP15(); + Endpoint result = evaluator.nodeP21(); if (result == null) { return CompletableFutureUtils.failedFuture(SdkClientException.create("Rule engine did not reach an error or endpoint result")); } @@ -72,9 +72,11 @@ private static boolean cacheListsMatch(List a, List b) { private static boolean cacheFirstElementsMatch(List a, List b) { if (a == b) return true; - // Only element 0 reaches the endpoint; absent and empty are both null to the rules engine. - String firstA = a == null || a.isEmpty() ? null : a.get(0); - String firstB = b == null || b.isEmpty() ? null : b.get(0); + // isSet can tell an absent list from an empty one, so presence is part of the key. + if (a == null || b == null) return false; + // Nothing past element 0 can reach the endpoint. + String firstA = a.isEmpty() ? null : a.get(0); + String firstB = b.isEmpty() ? null : b.get(0); return Objects.equals(firstA, firstB); } @@ -87,6 +89,8 @@ private static final class Evaluator { String firstArn; + String secondEndpoint; + private Endpoint nodeP0() { return null; } @@ -205,6 +209,18 @@ private Endpoint nodeP19() { : nodeP14(); } + private Endpoint nodeP20() { + return params.arnList() != null + ? nodeP15() + : nodeP15(); + } + + private Endpoint nodeP21() { + return cond14() + ? nodeP20() + : nodeP20(); + } + private boolean cond3() { partitionResult = RulesFunctions.awsPartition(region); return partitionResult != null; @@ -227,6 +243,11 @@ private boolean cond12() { return firstArn != null; } + private boolean cond14() { + secondEndpoint = RulesFunctions.listAccess(params.customEndpointArray(), 1); + return secondEndpoint != null; + } + private Endpoint result0() { throw SdkClientException.create("Invalid Configuration: FIPS and custom endpoint are not supported"); } diff --git a/test/codegen-generated-classes-test/src/main/resources/codegen-resources/bddendpoints/endpoint-bdd-1.json b/test/codegen-generated-classes-test/src/main/resources/codegen-resources/bddendpoints/endpoint-bdd-1.json index 3fc4fe05d154..66e220ed535f 100644 --- a/test/codegen-generated-classes-test/src/main/resources/codegen-resources/bddendpoints/endpoint-bdd-1.json +++ b/test/codegen-generated-classes-test/src/main/resources/codegen-resources/bddendpoints/endpoint-bdd-1.json @@ -216,6 +216,24 @@ "ref": "wholeArnList" } ] + }, + { + "fn": "isSet", + "argv": [ + { + "ref": "resourceArnList" + } + ] + }, + { + "fn": "getAttr", + "argv": [ + { + "ref": "wholeArnList" + }, + "[1]" + ], + "assign": "SecondArn" } ], "results": [ @@ -306,7 +324,7 @@ "type": "error" } ], - "root": 21, - "nodeCount": 21, - "nodes": "/////wAAAAH/////AAAAAAAAAA0AAAADAAAAAQAAAAQF9eEMAAAAAgAAAAUF9eEMAAAAAwAAAAgAAAAGAAAABAAAAAcF9eELAAAABQX14QkF9eEKAAAABAAAAAsAAAAJAAAABgAAAAoF9eEIAAAABwX14QYF9eEHAAAABQAAAAwF9eEFAAAABgX14QQF9eEFAAAAAwX14QEAAAAOAAAABAX14QIF9eEDAAAACAAAABAAAAAQAAAACQAAABEAAAARAAAACgAAABIAAAASAAAACwAAABMAAAATAAAADAAAABQAAAAUAAAADQAAAAIAAAACAAAADgAAAA8AAAAP" + "root": 23, + "nodeCount": 23, + "nodes": "/////wAAAAH/////AAAAAAAAAA0AAAADAAAAAQAAAAQF9eEMAAAAAgAAAAUF9eEMAAAAAwAAAAgAAAAGAAAABAAAAAcF9eELAAAABQX14QkF9eEKAAAABAAAAAsAAAAJAAAABgAAAAoF9eEIAAAABwX14QYF9eEHAAAABQAAAAwF9eEFAAAABgX14QQF9eEFAAAAAwX14QEAAAAOAAAABAX14QIF9eEDAAAACAAAABAAAAAQAAAACQAAABEAAAARAAAACgAAABIAAAASAAAACwAAABMAAAATAAAADAAAABQAAAAUAAAADQAAAAIAAAACAAAADgAAAA8AAAAPAAAADwAAABUAAAAVAAAAEAAAABYAAAAW" } diff --git a/test/codegen-generated-classes-test/src/test/java/software/amazon/awssdk/services/bddendpoints/BddEndpointProviderCacheTest.java b/test/codegen-generated-classes-test/src/test/java/software/amazon/awssdk/services/bddendpoints/BddEndpointProviderCacheTest.java index 37b4670cccfc..cc326d4db103 100644 --- a/test/codegen-generated-classes-test/src/test/java/software/amazon/awssdk/services/bddendpoints/BddEndpointProviderCacheTest.java +++ b/test/codegen-generated-classes-test/src/test/java/software/amazon/awssdk/services/bddendpoints/BddEndpointProviderCacheTest.java @@ -261,12 +261,16 @@ void firstElementList_orderChangeMovingFirstElement_invalidates() { } /** - * The rules engine's {@code listAccess} returns null for a null list and for an index past the end, so an absent - * list and an empty one take the same branch and must be treated as the same key. + * {@code isSet} can tell an absent list from an empty one, so presence stays part of the key even though + * {@code listAccess} yields null for both. + * + *

Collapsing the two would be sound only for a BDD whose branches for absent and empty converge. DynamoDB's do - + * both reach the same node - but that is a property of the graph rather than of the parameter, so the generated + * comparison does not assume it. The cost is one extra reference check and a miss in this case. */ @Test - void firstElementList_emptyAndUnset_areTheSameKey() { - assertHits(params(b -> b.resourceArnList(Collections.emptyList())), params(b -> { + void firstElementList_emptyAndUnset_areDistinguished() { + assertInvalidates(params(b -> b.resourceArnList(Collections.emptyList())), params(b -> { })); } From daa17ba63767c986fe2ef9bbee0e2fe627167f9a Mon Sep 17 00:00:00 2001 From: Alex Woods Date: Thu, 27 Aug 2026 13:08:18 -0700 Subject: [PATCH 5/8] refactor(endpoints): Drop the public sanitizeEndpoint helper It was public only because of a package boundary: the transformation lived on StaticClientEndpointProvider in core.internal, and the caller - ClientEndpointProvider's default method - sits in core, so nothing weaker than public could reach it. That is a poor reason for a public member, even on an @SdkInternalApi class. The transformation now lives in the interface default, which is the implementation every provider that does not override it already uses, and StaticClientEndpointProvider's constructor calls ClientEndpointProvider.super.sanitizedEndpointString() to compute the value it caches. One definition, as before, with nothing public added. The class is now final. That is what makes the constructor call provably safe: the super call is non-virtual, but the default it invokes reads clientEndpoint() and isEndpointOverridden(), and a subclass overriding either could have observed partial construction. Nothing subclasses it and it is @SdkInternalApi, so sealing it costs nothing and removes the hazard rather than documenting it. Verified the two implementations still agree: for a matrix of endpoints covering query parameters, user info, explicit ports, fragments and plain hosts, the caching implementation and the interface default produce the same string, the not-overridden case still yields null, and the caching one still returns an identical reference across calls. sdk-core 1505 + 624 pass, aws-core 317, codegen 710, codegen-generated-classes-test 3685, checkstyle clean. --- .../awssdk/core/ClientEndpointProvider.java | 19 +++++++++--- .../StaticClientEndpointProvider.java | 31 +++++-------------- 2 files changed, 22 insertions(+), 28 deletions(-) diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/ClientEndpointProvider.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/ClientEndpointProvider.java index 4a4443237271..f4630a48709a 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/ClientEndpointProvider.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/ClientEndpointProvider.java @@ -19,6 +19,7 @@ import software.amazon.awssdk.annotations.SdkProtectedApi; import software.amazon.awssdk.core.internal.StaticClientEndpointProvider; import software.amazon.awssdk.endpoints.EndpointProvider; +import software.amazon.awssdk.utils.FunctionalUtils; /** * Client endpoint providers are responsible for resolving client-level endpoints. {@link EndpointProvider}s are @@ -49,10 +50,15 @@ static ClientEndpointProvider create(URI uri, boolean isEndpointOverridden) { URI clientEndpoint(); /** - * Returns the sanitized endpoint string suitable for passing to the endpoint rules engine as the - * {@code SDK::Endpoint} built-in. The default implementation strips query and user-info components on every call; - * implementations backed by a static URI (see {@link #create(URI, boolean)}) override this to return a cached - * reference, enabling identity ({@code ==}) comparisons in the endpoint-provider result cache. + * Returns the endpoint string to pass to the endpoint rules engine as the {@code SDK::Endpoint} built-in, with the + * query and user-info components stripped because the rules engine's {@code ParseURL} rejects a URI carrying query + * parameters. + *

+ * This is the single definition of that transformation. {@link #create(URI, boolean)} returns an implementation that + * calls it once and caches the result, which removes a URI construction and its string conversion from every + * request; an implementation that does not override it recomputes per call. Both must produce the same string, + * because it is what the rules engine resolves against and what a generated endpoint provider uses as part of its + * cache key. *

* Returns {@code null} if the endpoint is not overridden. */ @@ -60,7 +66,10 @@ default String sanitizedEndpointString() { if (!isEndpointOverridden()) { return null; } - return StaticClientEndpointProvider.sanitizeEndpoint(clientEndpoint()); + URI endpoint = clientEndpoint(); + return FunctionalUtils.invokeSafely( + () -> new URI(endpoint.getScheme(), null, endpoint.getHost(), endpoint.getPort(), + endpoint.getPath(), null, endpoint.getFragment()).toString()); } /** diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/StaticClientEndpointProvider.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/StaticClientEndpointProvider.java index 019a89514810..2760511c4545 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/StaticClientEndpointProvider.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/StaticClientEndpointProvider.java @@ -18,7 +18,6 @@ import java.net.URI; import software.amazon.awssdk.annotations.SdkInternalApi; import software.amazon.awssdk.core.ClientEndpointProvider; -import software.amazon.awssdk.utils.FunctionalUtils; import software.amazon.awssdk.utils.ToString; import software.amazon.awssdk.utils.Validate; @@ -28,15 +27,14 @@ * @see ClientEndpointProvider#create(URI, boolean) */ @SdkInternalApi -public class StaticClientEndpointProvider implements ClientEndpointProvider { +public final class StaticClientEndpointProvider implements ClientEndpointProvider { private final URI clientEndpoint; private final boolean isEndpointOverridden; /** * A sanitized form of {@link #clientEndpoint} with the query and user-info components stripped, formatted as a * string. This is the value that endpoint rules receive via the {@code SDK::Endpoint} built-in. Computed once at - * construction so that every call to {@code endpointBuiltIn()} returns the same {@link String} reference, enabling - * identity ({@code ==}) comparison inside the endpoint-provider cache key check. + * construction so that every call to {@code endpointBuiltIn()} returns the same {@link String} reference. *

* {@code null} when {@link #isEndpointOverridden} is {@code false}. */ @@ -46,31 +44,18 @@ public StaticClientEndpointProvider(URI clientEndpoint, boolean isEndpointOverri this.clientEndpoint = Validate.paramNotNull(clientEndpoint, "clientEndpoint"); this.isEndpointOverridden = isEndpointOverridden; Validate.paramNotNull(clientEndpoint.getScheme(), "The URI scheme of endpointOverride"); - this.sanitizedEndpointString = isEndpointOverridden ? sanitizeEndpoint(clientEndpoint) : null; - } - - /** - * Strips the query and user-info components from the given endpoint URI and returns the result as a string. - * This matches the transformation performed by the rules engine's {@code ParseURL} function, which rejects - * URIs with query parameters. - *

- * This is the single definition of that transformation: {@link ClientEndpointProvider#sanitizedEndpointString()} - * delegates here so that a provider which recomputes the value per call and one which caches it at construction - * cannot drift apart. If they drifted, the value used as an endpoint cache key would no longer be the value the - * rules engine actually resolved against. - */ - public static String sanitizeEndpoint(URI endpoint) { - return FunctionalUtils.invokeSafely( - () -> new URI(endpoint.getScheme(), null, endpoint.getHost(), endpoint.getPort(), - endpoint.getPath(), null, endpoint.getFragment()).toString()); + // Calls the interface's implementation rather than repeating the transformation here, so the cached value cannot + // drift from what a provider that does not override the method produces. Safe from a constructor: this is a + // non-virtual call, the two accessors it reads are assigned above, and the class is final so neither can be + // overridden to observe partial construction. + this.sanitizedEndpointString = ClientEndpointProvider.super.sanitizedEndpointString(); } /** * {@inheritDoc} *

* Returns the same {@link String} reference on every call, because the value is computed once at construction. - * That lets a generated endpoint provider settle its {@code SDK::Endpoint} cache-key check with an identity - * ({@code ==}) comparison instead of falling through to {@code equals}. + * Avoids additional allocations and expensive URI creation per request. */ @Override public String sanitizedEndpointString() { From 05a1962a4d7dbe13a561d637efc8a8fd00d61daa Mon Sep 17 00:00:00 2001 From: Alex Woods Date: Thu, 27 Aug 2026 13:13:04 -0700 Subject: [PATCH 6/8] Cleanups --- .../feature-AWSSDKforJavav2-e54c03a.json | 6 ---- .../rules/EndpointParamsKnowledgeIndex.java | 4 +-- .../poet/rules/EndpointResolverUtilsSpec.java | 11 ++----- .../rules2/bdd/BddEndpointProviderSpec.java | 31 ++++--------------- .../rules2/bdd/BddParameterReferences.java | 23 ++------------ .../endpoints/AccountIdEndpointMode.java | 3 +- .../endpoints/AwsEndpointProviderUtils.java | 4 +-- .../awssdk/core/ClientEndpointProvider.java | 13 ++------ 8 files changed, 17 insertions(+), 78 deletions(-) delete mode 100644 .changes/next-release/feature-AWSSDKforJavav2-e54c03a.json diff --git a/.changes/next-release/feature-AWSSDKforJavav2-e54c03a.json b/.changes/next-release/feature-AWSSDKforJavav2-e54c03a.json deleted file mode 100644 index 1d94afc6e63a..000000000000 --- a/.changes/next-release/feature-AWSSDKforJavav2-e54c03a.json +++ /dev/null @@ -1,6 +0,0 @@ -{ - "type": "feature", - "category": "AWS SDK for Java v2", - "contributor": "", - "description": "Remove two per-request allocations from endpoint parameter construction. A client configured with an endpoint override no longer rebuilds and re-stringifies the sanitized override URI on every request; the value is now computed once when the client endpoint is set. Operations that declare a `staticContextParams` array value now pass a shared immutable list instead of constructing an equal list per request." -} diff --git a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules/EndpointParamsKnowledgeIndex.java b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules/EndpointParamsKnowledgeIndex.java index cecd08f2d2c5..454d1824b1fc 100644 --- a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules/EndpointParamsKnowledgeIndex.java +++ b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules/EndpointParamsKnowledgeIndex.java @@ -192,8 +192,8 @@ public MethodSpec recordAccountIdEndpointModeMethod() { BusinessMetricsUtils.class, SdkInternalExecutionAttribute.class); // Use endpointModeValue() rather than name().toLowerCase() so that the returned String is an interned - // compile-time literal. This makes the reference stable across calls, enabling identity (==) comparison - // in the endpoint-provider result cache key check. + // compile-time literal. This makes the reference stable across calls and reducing allocations + // and expensive URI.create calls. builder.addStatement("return mode.endpointModeValue()"); return builder.build(); diff --git a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules/EndpointResolverUtilsSpec.java b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules/EndpointResolverUtilsSpec.java index 6ca5cd0b80e6..4152c83a069c 100644 --- a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules/EndpointResolverUtilsSpec.java +++ b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules/EndpointResolverUtilsSpec.java @@ -347,8 +347,6 @@ private MethodSpec addStaticContextParamsMethod(OperationModel opModel) { b.addStatement("params.$N($L)", setterName, ((JrsBoolean) value).booleanValue()); break; case START_ARRAY: - // Reference the hoisted static final field instead of constructing a new list. - // This guarantees reference stability for the endpoint-provider cache key check. String fieldName = staticListFieldName(opModel, n); b.addStatement("params.$N($N)", setterName, fieldName); break; @@ -368,7 +366,7 @@ private String staticContextParamsMethodName(OperationModel opModel) { * Generates the name of the {@code static final List} field holding the static array value of * {@code paramName} for {@code opModel}. * - *

Format: {@code STATIC_LIST_{OPERATION}_{PARAM}}, both parts in screaming snake case. + *

Format: {@code STATIC_LIST_{OPERATION}_{PARAM}} */ private static String staticListFieldName(OperationModel opModel, String paramName) { return "STATIC_LIST_" + screamCase(opModel.getOperationName()) + "_" + screamCase(paramName); @@ -383,12 +381,7 @@ private static String screamCase(String word) { /** * Generates a {@code private static final List} field for every {@code staticContextParams} entry whose * value is an array, so that {@code setStaticContextParams} hands the same list reference to the endpoint params - * builder on every call rather than constructing an equal list each time. - * - *

Reference stability is what lets a generated endpoint provider settle a list-valued cache-key check with an - * identity comparison instead of walking the elements. It also removes a per-request list allocation on the - * request path for every operation that declares a static array parameter, which stands on its own regardless of - * whether the provider caches. + * builder on every call rather than constructing a new list each time. */ private void addStaticListFields(TypeSpec.Builder classBuilder) { ParameterizedTypeName listOfString = ParameterizedTypeName.get(List.class, String.class); diff --git a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpec.java b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpec.java index 7ce4ac13bb82..396f43714315 100644 --- a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpec.java +++ b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpec.java @@ -187,14 +187,8 @@ private TypeSpec cacheEntryClass() { *

Parameter order comes from {@link #cacheKeyParameterOrder()}. Order is the only thing that varies between * parameters, and it only affects how quickly a mismatch is found. * - *

List-valued parameters go through the generated {@code cacheListsMatch} helper rather than - * {@code Objects.equals}, so that every term in the chain stays a single boolean expression and so that the - * comparison stays bounded. See {@link #cacheListsMatchMethod()}. - * - *

An earlier version of this generated a seven-tier comparison, with the tier of each parameter computed from a - * pass over the service's operations, and a different code shape per tier. Benchmarking showed the tiers bought - * nothing over this form on the hit path and only ~0.2 ns on the miss path; see - * {@code .kiro/reference/endpoint_cache_key_benchmark.md}. + *

List-valued parameters are special cased to avoid slow comparisons for large lists. + * See {@link #cacheListsMatchMethod()}. */ private MethodSpec cacheParamsMatchMethod() { ClassName paramsClass = endpointRulesSpecUtils.parametersClassName(); @@ -240,9 +234,6 @@ private MethodSpec cacheParamsMatchMethod() { * every parameter before returning true. Booleans come first because they can never fall through to a real * {@code equals}; reference-stable strings come next because they normally settle on the identity check; and the * request-derived values that may have to compare characters come last. - * - *

The group of a parameter follows from its own declaration - its declared type, plus whether it is - * {@code AWS::Region} or a client context parameter - so this needs no analysis of the service's operations. */ private List cacheKeyParameterOrder() { Map parameters = endpointBddModel.getParameters(); @@ -307,11 +298,9 @@ private static boolean isReferenceStable(String paramName, * parameter. * *

{@code Objects.equals} would be correct here, but {@code List.equals} is unbounded: a request carrying a large - * list would walk every element on every cache check. Since resolution itself is typically indifferent to list - * length, an unbounded key check can cost more than the resolution it avoids, turning the cache into a - * pessimisation for that request shape. Refusing to match above - * {@value #MAX_LIST_COMPARISON_SIZE} elements keeps the check bounded; the consequence is that a service handling - * longer lists simply misses, and pays resolution, which is what it would have paid anyway. + * list would walk every element on every cache check whichi can be longer than resolution itself. + * The consequence is that a service handling longer lists simply misses, and pays resolution, + * which is what it would have paid anyway. */ private MethodSpec cacheListsMatchMethod() { TypeName listOfString = RuleRuntimeTypeMirror.LIST_OF_STRING.type(); @@ -339,15 +328,7 @@ private MethodSpec cacheListsMatchMethod() { * exclusively as {@code param[0]}. * *

When the rules can only see whether the list is present and what its first element is, comparing the rest is - * work that cannot change the answer. DynamoDB is why this exists: it reads {@code ResourceArnList} only through - * {@code isSet} and {@code getAttr(ResourceArnList, "[0]")}, and comparing a freshly built three-element ARN list - * measured at 15.5 ns against a 28 ns regional resolution - over half the cost the cache is meant to avoid. This - * comparison is O(1) instead. - * - *

Presence is compared as well as the first element, because {@code isSet} tells an absent list apart from an - * empty one even though {@code listAccess} yields null for both. Collapsing them would be sound only for a BDD whose - * branches for absent and empty converge - true of DynamoDB's, but a property of the graph rather than of the - * parameter, and not worth depending on for the one extra reference comparison it would save. + * work that cannot change the answer. */ private MethodSpec cacheFirstElementsMatchMethod() { TypeName listOfString = RuleRuntimeTypeMirror.LIST_OF_STRING.type(); diff --git a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddParameterReferences.java b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddParameterReferences.java index aeb9f5743445..1718d47a5b07 100644 --- a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddParameterReferences.java +++ b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddParameterReferences.java @@ -48,10 +48,8 @@ * it declares {@code Key}, {@code Prefix} and {@code CopySource}, no rule reads any of them, and {@code Key} * changes on essentially every object request - so including it means the cache almost never hits. *

  • {@link Usage#FIRST_ELEMENT_ONLY} - a {@code stringArray} whose every use is an index-0 access. Only the first - * element can reach the endpoint, so the generated comparison looks at that element instead of walking the list. - * DynamoDB is the motivating case: it reads {@code ResourceArnList} only as - * {@code getAttr(ResourceArnList, "[0]")}, and comparing the whole list costs over half of a regional - * resolution.
    • + * element can reach the endpoint, so the generated comparison is optimized by comparing only that element instead + * of walking the list. * */ final class BddParameterReferences { @@ -67,15 +65,6 @@ enum Usage { * A {@code stringArray} the rules can only observe through {@code isSet(param)} and {@code param[0]}. Whether * the list is present, plus its first element, is therefore the whole of what can reach the endpoint, and * nothing past element 0 can change the answer. - * - *

    {@code isSet} has to be allowed here, not just tolerated: the rules language requires a null check before - * an indexed access, so every real model that reads {@code param[0]} also reads {@code isSet(param)}. Treating - * the null check as a whole-value read would mean this case never fired. - * - *

    Because {@code isSet} distinguishes an absent list from an empty one, the generated comparison keeps a - * null check alongside the first-element check. Collapsing the two would only be sound for a BDD whose branches - * for absent and empty converge, which is a property of the graph rather than of the parameter, so it is not - * assumed here. */ FIRST_ELEMENT_ONLY, @@ -93,8 +82,6 @@ static Map analyze(EndpointBddModel model) { Collector collector = new Collector(); for (ConditionModel condition : model.getConditions()) { - // Wrapped the same way BddEndpointProviderSpec wraps a condition before generating it, so the parse - and - // therefore the set of references - is identical to the one the emitted code is built from. RuleModel synthetic = new RuleModel(); synthetic.setType("error"); synthetic.setError("synthetic"); @@ -151,12 +138,6 @@ public Void visitIndexedAccessExpression(IndexedAccessExpression e) { /** * {@code isSet(param)} observes only whether the parameter is present, so on its own it does not force a * whole-value comparison. - * - *

    This matters because the rules language requires a null check before an indexed access: a model that reads - * {@code list[0]} always reads {@code isSet(list)} too. Counting the null check as a whole-value read would stop - * {@link Usage#FIRST_ELEMENT_ONLY} from ever applying to a real model. - * - *

    Presence still has to be part of the cache key, which the generated comparison handles. */ @Override public Void visitFunctionCallExpression(FunctionCallExpression e) { diff --git a/core/aws-core/src/main/java/software/amazon/awssdk/awscore/endpoints/AccountIdEndpointMode.java b/core/aws-core/src/main/java/software/amazon/awssdk/awscore/endpoints/AccountIdEndpointMode.java index 6d894e39252e..8ee0d7867186 100644 --- a/core/aws-core/src/main/java/software/amazon/awssdk/awscore/endpoints/AccountIdEndpointMode.java +++ b/core/aws-core/src/main/java/software/amazon/awssdk/awscore/endpoints/AccountIdEndpointMode.java @@ -71,8 +71,7 @@ public static AccountIdEndpointMode fromValue(String s) { * {@code AWS::Auth::AccountIdEndpointMode} built-in. *

    * Unlike {@code name().toLowerCase()}, this returns the same interned {@link String} reference on every call rather - * than a fresh string per request. That removes an allocation from the request path and lets a generated endpoint - * provider compare the value by identity. + * than a fresh string per request removing an allocation from the request path. */ public String endpointModeValue() { return endpointModeValue; diff --git a/core/aws-core/src/main/java/software/amazon/awssdk/awscore/endpoints/AwsEndpointProviderUtils.java b/core/aws-core/src/main/java/software/amazon/awssdk/awscore/endpoints/AwsEndpointProviderUtils.java index a745cc7c75aa..63323fe6be58 100644 --- a/core/aws-core/src/main/java/software/amazon/awssdk/awscore/endpoints/AwsEndpointProviderUtils.java +++ b/core/aws-core/src/main/java/software/amazon/awssdk/awscore/endpoints/AwsEndpointProviderUtils.java @@ -56,8 +56,8 @@ public static Boolean fipsEnabledBuiltIn(ExecutionAttributes executionAttributes * {@code ParseURL}) rejects URIs with query parameters, so we strip the query and user-info components. *

    * Delegates to {@link ClientEndpointProvider#sanitizedEndpointString()}, which returns a cached reference on - * {@link software.amazon.awssdk.core.internal.StaticClientEndpointProvider}, enabling identity ({@code ==}) - * comparison inside the endpoint-provider result cache. + * {@link software.amazon.awssdk.core.internal.StaticClientEndpointProvider} reducing allocations and expensive + * URI.create calls. */ public static String endpointBuiltIn(ExecutionAttributes executionAttributes) { if (endpointIsOverridden(executionAttributes)) { diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/ClientEndpointProvider.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/ClientEndpointProvider.java index f4630a48709a..572830b0c152 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/ClientEndpointProvider.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/ClientEndpointProvider.java @@ -50,17 +50,8 @@ static ClientEndpointProvider create(URI uri, boolean isEndpointOverridden) { URI clientEndpoint(); /** - * Returns the endpoint string to pass to the endpoint rules engine as the {@code SDK::Endpoint} built-in, with the - * query and user-info components stripped because the rules engine's {@code ParseURL} rejects a URI carrying query - * parameters. - *

    - * This is the single definition of that transformation. {@link #create(URI, boolean)} returns an implementation that - * calls it once and caches the result, which removes a URI construction and its string conversion from every - * request; an implementation that does not override it recomputes per call. Both must produce the same string, - * because it is what the rules engine resolves against and what a generated endpoint provider uses as part of its - * cache key. - *

    - * Returns {@code null} if the endpoint is not overridden. + * Returns the client endpoint as a string with the query and user-info components stripped, as the rules engine's + * {@code ParseURL} rejects a URI carrying query parameters. Returns {@code null} if the endpoint is not overridden. */ default String sanitizedEndpointString() { if (!isEndpointOverridden()) { From 5f4c7dfd4357576534794c82d5a88d6d0bdd8b85 Mon Sep 17 00:00:00 2001 From: Alex Woods Date: Fri, 28 Aug 2026 08:57:43 -0700 Subject: [PATCH 7/8] Additional cleanups/fixes from review pass --- .../rules/EndpointParamsKnowledgeIndex.java | 5 +- .../poet/rules/EndpointResolverUtilsSpec.java | 11 +++- .../rules2/bdd/BddEndpointProviderSpec.java | 41 ++++++++++---- .../rules2/bdd/BddParameterReferences.java | 9 +--- .../awssdk/codegen/poet/ClientTestModels.java | 2 +- .../bdd/BddEndpointProviderSpecTest.java | 9 +++- ...point-resolver-utils-with-stringarray.java | 2 +- .../bdd/endpoint-provider-bdd-class.java | 2 +- .../endpoints/AccountIdEndpointMode.java | 9 ++-- .../endpoints/AwsEndpointProviderUtils.java | 2 +- .../StaticClientEndpointProvider.java | 34 ++++++++---- .../BddEndpointProviderCacheTest.java | 54 ++++++++++++------- 12 files changed, 122 insertions(+), 58 deletions(-) diff --git a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules/EndpointParamsKnowledgeIndex.java b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules/EndpointParamsKnowledgeIndex.java index 454d1824b1fc..4f1914000833 100644 --- a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules/EndpointParamsKnowledgeIndex.java +++ b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules/EndpointParamsKnowledgeIndex.java @@ -192,8 +192,9 @@ public MethodSpec recordAccountIdEndpointModeMethod() { BusinessMetricsUtils.class, SdkInternalExecutionAttribute.class); // Use endpointModeValue() rather than name().toLowerCase() so that the returned String is an interned - // compile-time literal. This makes the reference stable across calls and reducing allocations - // and expensive URI.create calls. + // compile-time literal. That keeps the reference stable across calls and removes a per-request allocation. + // It also avoids name().toLowerCase()'s dependence on the default locale, which mangles the value under a + // Turkish locale. builder.addStatement("return mode.endpointModeValue()"); return builder.build(); diff --git a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules/EndpointResolverUtilsSpec.java b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules/EndpointResolverUtilsSpec.java index 4152c83a069c..cd49ec926a5e 100644 --- a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules/EndpointResolverUtilsSpec.java +++ b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules/EndpointResolverUtilsSpec.java @@ -396,10 +396,17 @@ private void addStaticListFields(TypeSpec.Builder classBuilder) { if (value.asToken() != JsonToken.START_ARRAY) { return; } - CodeBlock arrayCode = endpointRulesSpecUtils.treeNodeToLiteral((JrsArray) value); + JrsArray arrayValue = (JrsArray) value; + CodeBlock initializer; + if (arrayValue.size() == 0) { + initializer = CodeBlock.of("$T.emptyList()", Collections.class); + } else { + initializer = CodeBlock.of("$T.unmodifiableList($L)", Collections.class, + endpointRulesSpecUtils.treeNodeToLiteral(arrayValue)); + } FieldSpec field = FieldSpec.builder(listOfString, staticListFieldName(opModel, paramName), Modifier.PRIVATE, Modifier.STATIC, Modifier.FINAL) - .initializer("$T.unmodifiableList($L)", Collections.class, arrayCode) + .initializer(initializer) .build(); classBuilder.addField(field); }); diff --git a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpec.java b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpec.java index 396f43714315..614498045d0d 100644 --- a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpec.java +++ b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpec.java @@ -83,10 +83,18 @@ public class BddEndpointProviderSpec implements ClassSpec { private static final int NO_MATCH_RESULT = 100_000_000; /** - * A {@code stringArray} cache key parameter longer than this reports a miss without comparing elements, so that the - * cost of a cache check stays bounded. Eight covers the list-valued endpoint parameters shipped today. + * A {@code stringArray} cache key parameter longer than this reports a miss without comparing elements, so the cost + * of a cache check cannot grow with the size of the caller's list. + * + *

    The cap is a cost bound, not a coverage target. Above it the provider resolves, which is what it would have + * done anyway, so the value trades hit rate against a bounded worst case and can never affect correctness. Four is + * arbitrary but deliberately conservative. + * + *

    No shipped service needs this path today. DynamoDB's {@code ResourceArnList} is the only {@code stringArray} + * any shipped rule set declares, and it is read only at index 0, so it is compared by + * {@link #cacheFirstElementsMatchMethod()} with no cap at all. */ - private static final int MAX_LIST_COMPARISON_SIZE = 8; + private static final int MAX_LIST_COMPARISON_SIZE = 4; private final IntermediateModel intermediateModel; private final EndpointBddModel endpointBddModel; @@ -159,6 +167,18 @@ private FieldSpec cacheField() { /** * Generates the immutable {@code CacheEntry} holding one {@code (params, endpoint)} snapshot. + * + *

    The entry keeps the caller's params object rather than copying it, so it relies on that object and any + * collections it holds being effectively immutable after {@code build()}. Generated params do not copy list values + * ({@code this.resourceArnList = builder.resourceArnList;}), so the assumption is load-bearing rather than + * enforced. + * + *

    It holds on every SDK path, because request objects are immutable and the params for a request are built and + * discarded within the call. It is an assumption only for a caller that invokes + * {@code EndpointProvider#resolveEndpoint} directly, retains a list it passed in, and mutates it afterwards: the + * mutation reaches the stored key, and a later call carrying the post-mutation contents can then hit an entry that + * was resolved for the pre-mutation contents. Snapshotting list parameters here would close that off, at the cost + * of an allocation on every miss. */ private TypeSpec cacheEntryClass() { ClassName paramsClass = endpointRulesSpecUtils.parametersClassName(); @@ -281,9 +301,6 @@ private static boolean isReferenceStable(String paramName, if (clientContextParams == null) { return false; } - if (clientContextParams.containsKey(paramName)) { - return true; - } // Endpoint parameter names are unique case-insensitively, so a case-insensitive match is the same parameter. for (String key : clientContextParams.keySet()) { if (key.equalsIgnoreCase(paramName)) { @@ -298,9 +315,9 @@ private static boolean isReferenceStable(String paramName, * parameter. * *

    {@code Objects.equals} would be correct here, but {@code List.equals} is unbounded: a request carrying a large - * list would walk every element on every cache check whichi can be longer than resolution itself. - * The consequence is that a service handling longer lists simply misses, and pays resolution, - * which is what it would have paid anyway. + * list would walk every element on every cache check, which can cost more than the resolution the cache exists to + * avoid. The comparison is therefore capped at {@value #MAX_LIST_COMPARISON_SIZE} elements. A service handling + * longer lists simply misses and pays resolution, which is what it would have paid anyway. */ private MethodSpec cacheListsMatchMethod() { TypeName listOfString = RuleRuntimeTypeMirror.LIST_OF_STRING.type(); @@ -372,7 +389,11 @@ private static boolean isBooleanParam(ParameterModel model) { return "boolean".equalsIgnoreCase(model.getType()); } - private static boolean isListParam(ParameterModel model) { + /** + * Shared with {@link BddParameterReferences}, which must agree with this class on what a list is: the usage it + * derives selects which comparison helper gets emitted for the parameter. + */ + static boolean isListParam(ParameterModel model) { return "stringarray".equalsIgnoreCase(model.getType()); } diff --git a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddParameterReferences.java b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddParameterReferences.java index 1718d47a5b07..04439e4a718b 100644 --- a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddParameterReferences.java +++ b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddParameterReferences.java @@ -18,7 +18,6 @@ import java.util.Collections; import java.util.HashSet; import java.util.LinkedHashMap; -import java.util.Locale; import java.util.Map; import java.util.Set; import software.amazon.awssdk.codegen.model.rules.endpoints.ConditionModel; @@ -49,7 +48,7 @@ * changes on essentially every object request - so including it means the cache almost never hits. *

  • {@link Usage#FIRST_ELEMENT_ONLY} - a {@code stringArray} whose every use is an index-0 access. Only the first * element can reach the endpoint, so the generated comparison is optimized by comparing only that element instead - * of walking the list. + * of walking the list.
    • * */ final class BddParameterReferences { @@ -106,11 +105,7 @@ private static Usage usageOf(String name, ParameterModel parameter, Collector co } // Only lists benefit, and only lists can be read element-wise. Anything else that somehow reached here is // compared in full rather than guessed at. - return isList(parameter) ? Usage.FIRST_ELEMENT_ONLY : Usage.FULL; - } - - private static boolean isList(ParameterModel parameter) { - return "stringarray".equals(parameter.getType().toLowerCase(Locale.ENGLISH)); + return BddEndpointProviderSpec.isListParam(parameter) ? Usage.FIRST_ELEMENT_ONLY : Usage.FULL; } /** diff --git a/codegen/src/test/java/software/amazon/awssdk/codegen/poet/ClientTestModels.java b/codegen/src/test/java/software/amazon/awssdk/codegen/poet/ClientTestModels.java index 7722d224e416..d59eb4cf701a 100644 --- a/codegen/src/test/java/software/amazon/awssdk/codegen/poet/ClientTestModels.java +++ b/codegen/src/test/java/software/amazon/awssdk/codegen/poet/ClientTestModels.java @@ -632,7 +632,7 @@ public static IntermediateModel batchManagerModels() { return new IntermediateModelBuilder(models).build(); } - + public static IntermediateModel presignedUrlExtensionModels() { File serviceModel = new File(ClientTestModels.class.getResource("client/c2j/presignedurl/service-2.json").getFile()); File customizationModel = new File(ClientTestModels.class.getResource("client/c2j/presignedurl/customization.config").getFile()); diff --git a/codegen/src/test/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpecTest.java b/codegen/src/test/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpecTest.java index be4f05085c7c..2ebf862b2461 100644 --- a/codegen/src/test/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpecTest.java +++ b/codegen/src/test/java/software/amazon/awssdk/codegen/poet/rules2/bdd/BddEndpointProviderSpecTest.java @@ -29,6 +29,13 @@ public class BddEndpointProviderSpecTest { + /** + * The trailing nodes of {@code endpoint-bdd-default-regional.json} were appended by hand with {@code high == low}, + * so that each parameter is read by a condition without altering any resolved endpoint. That is the one shape a + * reduced BDD can never contain, which is why it shows up in the golden file as degenerate {@code nodeP} methods + * whose branches are identical. A future peephole pass that collapses {@code high == low} nodes would silently make + * those parameters unreferenced and void the cache-key coverage the tests below rely on. + */ @Test void endpointProviderClass_simpleBdd_generatesExpectedCode() { BddEndpointProviderSpec spec = new BddEndpointProviderSpec( @@ -232,7 +239,7 @@ void listReadAsAWholeUsesTheBoundedHelper() { ClientTestModels.queryServiceModelsWithSimpleBddEndpoints()).poetSpec().toString(); assertThat(generated).contains("cacheListsMatch(a.customEndpointArray(), b.customEndpointArray())"); - assertThat(generated).contains("if (size > 8) return false"); + assertThat(generated).contains("if (size > 4) return false"); } /** diff --git a/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules/endpoint-resolver-utils-with-stringarray.java b/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules/endpoint-resolver-utils-with-stringarray.java index 1d21848b0041..a3ff124832a3 100644 --- a/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules/endpoint-resolver-utils-with-stringarray.java +++ b/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules/endpoint-resolver-utils-with-stringarray.java @@ -29,7 +29,7 @@ @Generated("software.amazon.awssdk:codegen") @SdkInternalApi public final class SampleSvcEndpointResolverUtils { - private static final List STATIC_LIST_EMPTY_STATIC_CONTEXT_OPERATION_STRING_ARRAY_PARAM = Collections.unmodifiableList(Arrays.asList()); + private static final List STATIC_LIST_EMPTY_STATIC_CONTEXT_OPERATION_STRING_ARRAY_PARAM = Collections.emptyList(); private static final List STATIC_LIST_STATIC_CONTEXT_OPERATION_STRING_ARRAY_PARAM = Collections.unmodifiableList(Arrays.asList("staticValue1")); diff --git a/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules2/bdd/endpoint-provider-bdd-class.java b/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules2/bdd/endpoint-provider-bdd-class.java index 32365c942d44..c0566ec44f4d 100644 --- a/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules2/bdd/endpoint-provider-bdd-class.java +++ b/codegen/src/test/resources/software/amazon/awssdk/codegen/poet/rules2/bdd/endpoint-provider-bdd-class.java @@ -63,7 +63,7 @@ private static boolean cacheListsMatch(List a, List b) { int size = a.size(); if (size != b.size()) return false; // Bounded so that a long list cannot make the cache check cost more than resolving. - if (size > 8) return false; + if (size > 4) return false; for (int i = 0; i < size; i++) { if (!Objects.equals(a.get(i), b.get(i))) return false; } diff --git a/core/aws-core/src/main/java/software/amazon/awssdk/awscore/endpoints/AccountIdEndpointMode.java b/core/aws-core/src/main/java/software/amazon/awssdk/awscore/endpoints/AccountIdEndpointMode.java index 8ee0d7867186..eadbf7ea3172 100644 --- a/core/aws-core/src/main/java/software/amazon/awssdk/awscore/endpoints/AccountIdEndpointMode.java +++ b/core/aws-core/src/main/java/software/amazon/awssdk/awscore/endpoints/AccountIdEndpointMode.java @@ -58,7 +58,9 @@ public static AccountIdEndpointMode fromValue(String s) { } for (AccountIdEndpointMode value : values()) { - if (value.name().equalsIgnoreCase(s)) { + // Matched against endpointModeValue rather than name() so that the wire form has a single definition. + // Behaviour is unchanged: the two differ only in case, and the comparison is case-insensitive. + if (value.endpointModeValue.equalsIgnoreCase(s)) { return value; } } @@ -70,8 +72,9 @@ public static AccountIdEndpointMode fromValue(String s) { * Returns the canonical lowercase string for this mode, as the endpoint rules engine expects to receive it in the * {@code AWS::Auth::AccountIdEndpointMode} built-in. *

    - * Unlike {@code name().toLowerCase()}, this returns the same interned {@link String} reference on every call rather - * than a fresh string per request removing an allocation from the request path. + * Unlike {@code name().toLowerCase()}, this returns the same interned {@link String} reference on every call instead + * of a fresh string per request, which removes an allocation from the request path. It is also independent of the + * default locale, which {@code name().toLowerCase()} is not. */ public String endpointModeValue() { return endpointModeValue; diff --git a/core/aws-core/src/main/java/software/amazon/awssdk/awscore/endpoints/AwsEndpointProviderUtils.java b/core/aws-core/src/main/java/software/amazon/awssdk/awscore/endpoints/AwsEndpointProviderUtils.java index 63323fe6be58..56ac41f02c69 100644 --- a/core/aws-core/src/main/java/software/amazon/awssdk/awscore/endpoints/AwsEndpointProviderUtils.java +++ b/core/aws-core/src/main/java/software/amazon/awssdk/awscore/endpoints/AwsEndpointProviderUtils.java @@ -52,7 +52,7 @@ public static Boolean fipsEnabledBuiltIn(ExecutionAttributes executionAttributes } /** - * Returns the endpoint set on the client, sanitized for the rules engine. The rules engine (e.g. + * Returns the endpoint set on the client, sanitized for the rules engine. The rules engine (e.g. * {@code ParseURL}) rejects URIs with query parameters, so we strip the query and user-info components. *

    * Delegates to {@link ClientEndpointProvider#sanitizedEndpointString()}, which returns a cached reference on diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/StaticClientEndpointProvider.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/StaticClientEndpointProvider.java index 2760511c4545..43be721c9110 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/StaticClientEndpointProvider.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/StaticClientEndpointProvider.java @@ -18,6 +18,7 @@ import java.net.URI; import software.amazon.awssdk.annotations.SdkInternalApi; import software.amazon.awssdk.core.ClientEndpointProvider; +import software.amazon.awssdk.utils.FunctionalUtils; import software.amazon.awssdk.utils.ToString; import software.amazon.awssdk.utils.Validate; @@ -27,13 +28,13 @@ * @see ClientEndpointProvider#create(URI, boolean) */ @SdkInternalApi -public final class StaticClientEndpointProvider implements ClientEndpointProvider { +public class StaticClientEndpointProvider implements ClientEndpointProvider { private final URI clientEndpoint; private final boolean isEndpointOverridden; /** * A sanitized form of {@link #clientEndpoint} with the query and user-info components stripped, formatted as a - * string. This is the value that endpoint rules receive via the {@code SDK::Endpoint} built-in. Computed once at + * string. This is the value that endpoint rules receive via the {@code SDK::Endpoint} built-in. Computed once at * construction so that every call to {@code endpointBuiltIn()} returns the same {@link String} reference. *

    * {@code null} when {@link #isEndpointOverridden} is {@code false}. @@ -44,24 +45,37 @@ public StaticClientEndpointProvider(URI clientEndpoint, boolean isEndpointOverri this.clientEndpoint = Validate.paramNotNull(clientEndpoint, "clientEndpoint"); this.isEndpointOverridden = isEndpointOverridden; Validate.paramNotNull(clientEndpoint.getScheme(), "The URI scheme of endpointOverride"); - // Calls the interface's implementation rather than repeating the transformation here, so the cached value cannot - // drift from what a provider that does not override the method produces. Safe from a constructor: this is a - // non-virtual call, the two accessors it reads are assigned above, and the class is final so neither can be - // overridden to observe partial construction. - this.sanitizedEndpointString = ClientEndpointProvider.super.sanitizedEndpointString(); + this.sanitizedEndpointString = isEndpointOverridden ? sanitize(clientEndpoint) : null; } /** * {@inheritDoc} *

    - * Returns the same {@link String} reference on every call, because the value is computed once at construction. - * Avoids additional allocations and expensive URI creation per request. + * Returns the same {@link String} reference on every call, because the value is computed once at construction. That + * removes a URI construction and its string conversion from every request that resolves an endpoint against an + * overridden client endpoint. + *

    + * {@code final} so that the cached value cannot be shadowed by a subclass whose overridden accessors the constructor + * did not see. */ @Override - public String sanitizedEndpointString() { + public final String sanitizedEndpointString() { return sanitizedEndpointString; } + /** + * Repeats {@link ClientEndpointProvider#sanitizedEndpointString()}'s transformation over the constructor's argument, + * rather than calling that default from the constructor, so no virtual method runs before this class is fully + * initialised. The two must agree, and + * {@code ClientEndpointProviderTest.sanitizedEndpointString_cachedFormMatchesRecomputedForm} is what holds them + * together. + */ + private static String sanitize(URI endpoint) { + return FunctionalUtils.invokeSafely( + () -> new URI(endpoint.getScheme(), null, endpoint.getHost(), endpoint.getPort(), + endpoint.getPath(), null, endpoint.getFragment()).toString()); + } + @Override public URI clientEndpoint() { return this.clientEndpoint; diff --git a/test/codegen-generated-classes-test/src/test/java/software/amazon/awssdk/services/bddendpoints/BddEndpointProviderCacheTest.java b/test/codegen-generated-classes-test/src/test/java/software/amazon/awssdk/services/bddendpoints/BddEndpointProviderCacheTest.java index cc326d4db103..45887d41bc15 100644 --- a/test/codegen-generated-classes-test/src/test/java/software/amazon/awssdk/services/bddendpoints/BddEndpointProviderCacheTest.java +++ b/test/codegen-generated-classes-test/src/test/java/software/amazon/awssdk/services/bddendpoints/BddEndpointProviderCacheTest.java @@ -53,14 +53,25 @@ * cache was consulted. * * - *

    The model declares parameters its BDD graph never reads, so that every classification tier is represented. Those - * parameters cannot change the resolved URL, which makes them the more interesting cases to test: a stale hit is + *

    The model over-declares parameters on purpose, to cover every {@code BddParameterReferences.Usage} value and every + * parameter kind: boolean, string, {@code stringArray}, built-in, client context, static context and request context. + * Several of them cannot change the resolved URL, which makes them the more interesting cases to test: a stale hit is * detectable only by instance identity, not by comparing hosts. + * + *

    The trailing nodes of {@code bddendpoints/endpoint-bdd-1.json} were appended by hand with {@code high == low}, so + * that each parameter is read by a condition without altering any resolved endpoint. A future peephole pass that + * collapses {@code high == low} nodes would silently make those parameters unreferenced and void the coverage above. */ class BddEndpointProviderCacheTest { private static final Region REGION = Region.US_EAST_1; private static final Region OTHER_REGION = Region.US_WEST_2; + /** + * Mirrors {@code BddEndpointProviderSpec.MAX_LIST_COMPARISON_SIZE}. Not importable from here, so the cap-related + * tests below derive their sizes from this one constant. + */ + private static final int LIST_SIZE_CAP = 4; + /** * Returns params that resolve successfully, with every optional parameter left unset. */ @@ -90,6 +101,10 @@ private static Endpoint resolve(BddEndpointsEndpointProvider provider, BddEndpoi * *

    Instance identity rather than URL comparison, so this works for the parameters that do not influence the * resolved URL. Those are exactly the parameters where a missing key check would go unnoticed. + * + *

    Identity is a valid miss signal only while every resolution constructs a new {@link Endpoint}. If codegen ever + * hoists constant endpoint results to {@code static final}, this helper and {@link #cacheIsPerProviderInstance()} + * start failing for that reason rather than because the cache regressed. */ private static void assertInvalidates(BddEndpointsEndpointParams first, BddEndpointsEndpointParams second) { BddEndpointsEndpointProvider provider = provider(); @@ -157,55 +172,55 @@ void alternatingParams_eachResolutionMatchesItsOwnParams() { // ---- no stale hit, one test per parameter ---- @Test - void booleanTier_useFipsChange_invalidates() { + void useFipsChange_invalidates() { assertInvalidates(params(b -> { }), params(b -> b.useFips(true))); } @Test - void booleanTier_useDualStackChange_invalidates() { + void useDualStackChange_invalidates() { assertInvalidates(params(b -> { }), params(b -> b.useDualStack(true))); } @Test - void clientStaticRefTier_regionChange_invalidates() { + void regionChange_invalidates() { assertInvalidates(params(b -> { }), params(b -> b.region(OTHER_REGION))); } @Test - void clientStaticRefTier_clientStringParamChange_invalidates() { + void clientStringParamChange_invalidates() { assertInvalidates(params(b -> b.clientStringParam("first")), params(b -> b.clientStringParam("second"))); } @Test - void operationStaticTier_staticStringParamChange_invalidates() { + void staticStringParamChange_invalidates() { assertInvalidates(params(b -> b.staticStringParam("first")), params(b -> b.staticStringParam("second"))); } @Test - void semiStableTier_endpointOverrideChange_invalidates() { + void endpointOverrideChange_invalidates() { assertInvalidates(params(b -> b.endpoint("https://first.example.com")), params(b -> b.endpoint("https://second.example.com"))); } @Test - void semiStableTier_accountIdEndpointModeChange_invalidates() { + void accountIdEndpointModeChange_invalidates() { assertInvalidates(params(b -> b.accountIdEndpointMode("preferred")), params(b -> b.accountIdEndpointMode("disabled"))); } @Test - void identityDerivedTier_accountIdChange_invalidates() { + void accountIdChange_invalidates() { assertInvalidates(params(b -> b.accountId("111111111111")), params(b -> b.accountId("222222222222"))); } @Test - void requestDynamicTier_requestStringParamChange_invalidates() { + void requestStringParamChange_invalidates() { assertInvalidates(params(b -> b.requestStringParam("first")), params(b -> b.requestStringParam("second"))); } @@ -251,7 +266,7 @@ void firstElementList_laterElementChange_isAHit() { @Test void firstElementList_lengthChangeKeepingFirstElement_isAHit() { assertHits(params(b -> b.resourceArnList(Arrays.asList("a", "b", "c"))), - params(b -> b.resourceArnList(Collections.singletonList("a")))); + params(b -> b.resourceArnList(Collections.singletonList("a")))); } @Test @@ -343,12 +358,12 @@ void emptyListAndUnsetList_areDistinguished() { // ---- equals fallback ---- /** - * The tiers with an {@code equals} fallback must hit on an equal value arriving as a fresh reference. Without the - * fallback, a request-derived string would miss on every call and the cache would never pay off for the services - * that need it most. + * A parameter compared with {@code Objects.equals} must hit on an equal value arriving as a fresh reference. Without + * the {@code equals} fallback, a request-derived string would miss on every call and the cache would never pay off + * for the services that need it most. */ @Test - void equalsFallbackTiers_equalValueDifferentReference_hits() { + void equalValueDifferentReference_hits() { String value = "shared-value"; String copy = new String(value); assertThat(value).isNotSameAs(copy); @@ -382,7 +397,8 @@ void requestList_equalElementsDifferentReferences_hits() { */ @Test void requestList_atSizeCap_stillHits() { - assertHits(params(b -> b.wholeArnList(listOfSize(8))), params(b -> b.wholeArnList(listOfSize(8)))); + assertHits(params(b -> b.wholeArnList(listOfSize(LIST_SIZE_CAP))), + params(b -> b.wholeArnList(listOfSize(LIST_SIZE_CAP)))); } /** @@ -392,8 +408,8 @@ void requestList_atSizeCap_stillHits() { */ @Test void requestList_pastSizeCap_alwaysMisses() { - assertInvalidates(params(b -> b.wholeArnList(listOfSize(9))), - params(b -> b.wholeArnList(listOfSize(9)))); + assertInvalidates(params(b -> b.wholeArnList(listOfSize(LIST_SIZE_CAP + 1))), + params(b -> b.wholeArnList(listOfSize(LIST_SIZE_CAP + 1)))); } /** From 1676b02f902107886ba08a96f3eee8872c15c16d Mon Sep 17 00:00:00 2001 From: Alex Woods Date: Fri, 28 Aug 2026 12:54:17 -0700 Subject: [PATCH 8/8] Add tests (missed adding in earlier commit) --- .../endpoints/AccountIdEndpointModeTest.java | 99 ++++++++++++++++ .../core/ClientEndpointProviderTest.java | 107 ++++++++++++++++++ 2 files changed, 206 insertions(+) create mode 100644 core/aws-core/src/test/java/software/amazon/awssdk/awscore/endpoints/AccountIdEndpointModeTest.java create mode 100644 core/sdk-core/src/test/java/software/amazon/awssdk/core/ClientEndpointProviderTest.java diff --git a/core/aws-core/src/test/java/software/amazon/awssdk/awscore/endpoints/AccountIdEndpointModeTest.java b/core/aws-core/src/test/java/software/amazon/awssdk/awscore/endpoints/AccountIdEndpointModeTest.java new file mode 100644 index 000000000000..a946feb57cd1 --- /dev/null +++ b/core/aws-core/src/test/java/software/amazon/awssdk/awscore/endpoints/AccountIdEndpointModeTest.java @@ -0,0 +1,99 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://aws.amazon.com/apache2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + */ + +package software.amazon.awssdk.awscore.endpoints; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.util.Locale; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EnumSource; + +/** + * {@link AccountIdEndpointMode#endpointModeValue()} is what generated endpoint resolvers pass to the rules engine as the + * {@code AWS::Auth::AccountIdEndpointMode} built-in, so these tests pin the wire form itself rather than any caller. + */ +class AccountIdEndpointModeTest { + private final Locale defaultLocale = Locale.getDefault(); + + @AfterEach + void restoreLocale() { + Locale.setDefault(defaultLocale); + } + + @Test + void endpointModeValue_returnsTheLowercaseWireForm() { + assertThat(AccountIdEndpointMode.PREFERRED.endpointModeValue()).isEqualTo("preferred"); + assertThat(AccountIdEndpointMode.DISABLED.endpointModeValue()).isEqualTo("disabled"); + assertThat(AccountIdEndpointMode.REQUIRED.endpointModeValue()).isEqualTo("required"); + } + + /** + * The value is a compile-time literal, so the same reference comes back every time. That is what removes the + * per-request allocation the generated resolver used to pay for {@code name().toLowerCase()}. + */ + @ParameterizedTest + @EnumSource(AccountIdEndpointMode.class) + void endpointModeValue_returnsTheSameReferenceEachCall(AccountIdEndpointMode mode) { + assertThat(mode.endpointModeValue()).isSameAs(mode.endpointModeValue()); + } + + /** + * {@code fromValue} and {@code endpointModeValue} must describe the same mapping in both directions, otherwise a + * value the SDK emits is one it cannot read back. + */ + @ParameterizedTest + @EnumSource(AccountIdEndpointMode.class) + void fromValue_roundTripsEndpointModeValue(AccountIdEndpointMode mode) { + assertThat(AccountIdEndpointMode.fromValue(mode.endpointModeValue())).isSameAs(mode); + } + + /** + * The predecessor of this method was {@code name().toLowerCase()}, which uses the default locale. Under a Turkish + * locale that produces a dotless i (U+0131), so {@code DISABLED} became {@code dısabled} and was handed to the rules + * engine in that form. + */ + @ParameterizedTest + @EnumSource(AccountIdEndpointMode.class) + void endpointModeValue_isIndependentOfTheDefaultLocale(AccountIdEndpointMode mode) { + String underDefaultLocale = mode.endpointModeValue(); + + Locale.setDefault(new Locale("tr", "TR")); + + assertThat(mode.endpointModeValue()).isEqualTo(underDefaultLocale); + assertThat(mode.endpointModeValue()).doesNotContain("\u0131"); + } + + @Test + void fromValue_isCaseInsensitive() { + assertThat(AccountIdEndpointMode.fromValue("PREFERRED")).isSameAs(AccountIdEndpointMode.PREFERRED); + assertThat(AccountIdEndpointMode.fromValue("Disabled")).isSameAs(AccountIdEndpointMode.DISABLED); + } + + @Test + void fromValue_nullReturnsNull() { + assertThat(AccountIdEndpointMode.fromValue(null)).isNull(); + } + + @Test + void fromValue_unrecognizedThrows() { + assertThatThrownBy(() -> AccountIdEndpointMode.fromValue("nonsense")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("nonsense"); + } +} diff --git a/core/sdk-core/src/test/java/software/amazon/awssdk/core/ClientEndpointProviderTest.java b/core/sdk-core/src/test/java/software/amazon/awssdk/core/ClientEndpointProviderTest.java new file mode 100644 index 000000000000..43846d815357 --- /dev/null +++ b/core/sdk-core/src/test/java/software/amazon/awssdk/core/ClientEndpointProviderTest.java @@ -0,0 +1,107 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://aws.amazon.com/apache2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + */ + +package software.amazon.awssdk.core; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.net.URI; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; + +/** + * Covers {@link ClientEndpointProvider#sanitizedEndpointString()}'s default implementation, which is the value generated + * endpoint resolvers receive as the {@code SDK::Endpoint} built-in. + * + *

    Every provider the SDK itself installs is a {@code StaticClientEndpointProvider}, which overrides the method with a + * value computed once at construction. The default therefore runs only for an external implementor, and it exists to be + * the single definition of the transformation that the overriding implementation caches. Both halves are asserted here: + * what the transformation does, and that the two forms agree. + */ +class ClientEndpointProviderTest { + /** + * A provider that leaves {@code sanitizedEndpointString()} to the interface, which is what an external implementor + * gets and what no SDK code path produces. + */ + private static ClientEndpointProvider defaultImplementation(URI uri, boolean isEndpointOverridden) { + return new ClientEndpointProvider() { + @Override + public URI clientEndpoint() { + return uri; + } + + @Override + public boolean isEndpointOverridden() { + return isEndpointOverridden; + } + }; + } + + @ParameterizedTest + @CsvSource({ + // Query parameters are rejected by the rules engine's ParseURL, which is why they are stripped. + "https://example.com/path?foo=bar, https://example.com/path", + "https://example.com?foo=bar, https://example.com", + "https://example.com/path?foo=bar&baz=qux, https://example.com/path", + // User-info is stripped for the same reason: it is not part of what the rules engine resolves against. + "https://user:pass@example.com/path, https://example.com/path", + "https://user@example.com, https://example.com", + "https://user:pass@example.com/path?foo=bar,https://example.com/path", + // Everything else survives untouched. + "https://example.com:8443/path, https://example.com:8443/path", + "http://example.com/path, http://example.com/path", + "https://example.com/path#frag, https://example.com/path#frag", + "https://example.com/a/b/c, https://example.com/a/b/c" + }) + void sanitizedEndpointString_stripsQueryAndUserInfo(String input, String expected) { + assertThat(defaultImplementation(URI.create(input), true).sanitizedEndpointString()).isEqualTo(expected); + } + + @Test + void sanitizedEndpointString_returnsNullWhenNotOverridden() { + URI uri = URI.create("https://example.com/path?foo=bar"); + + assertThat(defaultImplementation(uri, false).sanitizedEndpointString()).isNull(); + assertThat(ClientEndpointProvider.create(uri, false).sanitizedEndpointString()).isNull(); + } + + /** + * The claim that makes it safe for {@code StaticClientEndpointProvider} to compute this once at construction: the + * cached value and the recomputed one cannot disagree, because there is one definition of the transformation. + */ + @ParameterizedTest + @CsvSource({ + "https://example.com/path?foo=bar", + "https://user:pass@example.com/path?foo=bar", + "https://example.com:8443/path", + "https://example.com/path#frag", + "http://example.com" + }) + void sanitizedEndpointString_cachedFormMatchesRecomputedForm(String input) { + URI uri = URI.create(input); + + assertThat(ClientEndpointProvider.create(uri, true).sanitizedEndpointString()) + .isEqualTo(defaultImplementation(uri, true).sanitizedEndpointString()); + } + + @Test + void sanitizedEndpointString_overridingImplementationReturnsAStableReference() { + ClientEndpointProvider provider = ClientEndpointProvider.forEndpointOverride( + URI.create("https://example.com/path?foo=bar")); + + assertThat(provider.sanitizedEndpointString()).isSameAs(provider.sanitizedEndpointString()); + } +}