[Do not merge] Feature/master/endpoints bdd s3 ddb staging - #7339
Open
alextwoods wants to merge 10 commits into
Open
[Do not merge] Feature/master/endpoints bdd s3 ddb staging#7339alextwoods wants to merge 10 commits into
alextwoods wants to merge 10 commits into
Conversation
Add the foundational infrastructure for BDD (Binary Decision Diagram) endpoint resolution without the code generation visitors themselves: Model loading: - EndpointBddModel: parse BDD JSON (endpoint-bdd-1.json) with node graph, conditions, results, and parameters - C2jModels/IntermediateModel: wire EndpointBddModel into the codegen model pipeline - GenerationMojo: discover endpoint-bdd-1.json in codegen-resources Expression/type extensions: - ExpressionParser: support parseExpressionFrom for BDD result templates, negative indexed access, direct negative index syntax - Tokenizer: add directNegativeIndexedAccess and consumeDirectNegative support for BDD template expressions - RuleRuntimeTypeMirror: add LIST_OF_STRING, coalesce return type - EndpointUrlCodeEmitter: add urlFromExpression for BDD url emission - StringConcatExpression: add needsParenthesization flag Runtime additions (RulesFunctions.java.resource): - coalesce(T... args) / coalesce(String... args) - split(value, delimiter, limit) - ite(condition, ifTrue, ifFalse) - listAccess: support negative indices Supporting: - EndpointRulesSpecUtils: simplify rulesEngineFilesFromDirectory - EndpointProviderTestSpec: BDD test case loading support - EndpointProviderTestCase: add bddEndpointProvider field
* feat(codegen): Add BDD endpoint resolver code generation Implement the BDD endpoint resolver code generation visitors that transform a Binary Decision Diagram into a Java endpoint provider. Code generation visitors: - BddEndpointProviderSpec: main spec emitting the provider class with nodeP/nodeN method dispatch — each BDD node becomes a method returning Endpoint directly (null = no match, throw = error) - BddResultCodeGeneratorVisitor: traverses the BDD node graph and emits node methods with condition checks and edge traversal - ConditionFnCodeGeneratorVisitor: emits condition evaluations (isSet, booleanEquals, stringEquals, function calls, getAttr) - ResultFnCodeGeneratorVisitor: emits endpoint construction and error results from BDD result nodes - AssignTypeInferringVisitor: infers types for BDD register variables from the condition/result expressions - RegistryInfo: metadata for BDD registers (name, type, index) Wiring: - EndpointProviderTasks: when EndpointBddModel is present, generate BddEndpointProvider instead of rules-based provider The generated resolver uses direct method dispatch per BDD node, with simple conditions inlined as ternary expressions for branch-predictor-friendly code paths. * fix(codegen): Fix null Boolean handling in BDD condition visitor Boolean.FALSE != x treats null as true, which is incorrect per the endpoint rules spec (booleanEquals(null, true) should be false). Use Boolean.TRUE.equals(x) instead, which correctly returns false when x is null.
…7304) * perf(endpoints): Add peephole optimizations to BDD endpoint codegen Reduce allocations and method dispatch in BDD-generated endpoint resolvers. All codegen changes are scoped to the BDD path; the tree-based rules code generation is untouched. New BddPeepholeVisitor (BDD-only IR pass, runs before PrepareForCodegenVisitor) rewrites: - stringEquals(coalesce(substring(str, 0, N, false), ""), literal) -> str.startsWith(literal) - stringEquals(coalesce(substring(str, 0, N, true), ""), literal) -> str.endsWith(literal) - stringEquals(coalesce(substring(str, X, Y, rev), ""), literal) -> str.regionMatches(offset, literal, ...) for interior checks - stringEquals(a, b) -> a.equals(b), skipping RulesFunctions dispatch - coalesce(boolExpr, default) -> (expr != null ? expr : default) - ite(cond, a, b) -> (cond ? a : b) - isValidHostLabel(str, constBool) -> specialized runtime helper The BDD condition and result generators emit these synthetic forms as inline Java. Synthetic names are prefixed with __ so they cannot collide with endpoint rule standard library functions. RulesFunctions (shared generated runtime) additions: - awsPartition(region) gains a per-thread last-value cache. The same region resolves on nearly every call in steady state, so this avoids a HashMap lookup plus regex fallback each time. - isValidHostLabelSingle / isValidHostLabelMulti skip the allowDots branch when the flag is known at codegen time. On the S3 test BDD this eliminates all RulesFunctions.stringEquals, ite, coalesce, and isValidHostLabel dispatches, replacing them with 11 native String comparisons and 9 specialized host-label calls. Verified: 690 codegen tests (3 pre-existing EndpointRulesClientTestSpec failures also present on the base branch), rules2 golden files byte-identical to base, and all 32 bddendpoints endpoint test cases passing against the generated BDD resolver. * test(codegen): Update endpoint provider test fixtures for test case names EndpointProviderTestSpec was changed to pass each endpoint test case's documentation string to the EndpointProviderTestCase constructor, so generated test cases are named in test output instead of appearing as anonymous entries. The three affected golden files were not updated alongside that change, leaving EndpointRulesClientTestSpecTest failing on the base branch. Adds the expected name argument to the fixtures. Test-resource only — no production code change. * fix(endpoints): Make BDD peephole spec-faithful Addresses review findings on the BDD endpoint codegen peephole pass and the awsPartition memo. Two of the four are silent-wrong-endpoint or new-exception risks, so they are grouped as one verified changeset rather than split across commits that share hunks in the same files. Substring peephole diverged from spec substring semantics. The pass rewrote stringEquals(coalesce(substring(str, X, Y, rev), ""), literal) into inlined startsWith/endsWith/regionMatches, none of which reproduce substring's rejection of input containing a non-ASCII character. A bucket name with a non-ASCII character anywhere would take a different rule branch than the spec dictates. This fires today on the S3 BDD model: a name ending in --x-s3 would be routed to an S3 Express endpoint with sigv4-s3express instead of a regular S3 endpoint with SigV4. Replaced the three emitters with a single RulesFunctions.substringEquals helper that mirrors substring exactly. The positional comparison runs first and fails for almost every input, so the O(n) ASCII scan is only reached once the characters already match, which is where substring paid the same cost. The rewrite is skipped for an empty literal, where coalesce makes the spec comparison true even for null input. stringEquals lost null-safety for two nullable operands. The fallback emitted left.equals(right) whenever neither side was a string constant, throwing NullPointerException where the spec's stringEquals returns false. Reachable in the S3 BDD via stringEquals(region, bucketArn.region()) and two others. Removed the fallback; the pass now leaves plain comparisons to PrepareForCodegenVisitor, which already handles the constant cases identically and correctly keeps the null-safe RulesFunctions.stringEquals call otherwise. awsPartition ThreadLocal memo replaced with precomputed instances. RulesFunctions is copied into every service, so a static ThreadLocal holding a service-loaded RulePartition is the container classloader-leak pattern Tomcat warns about, and allocates per virtual thread with a near-zero hit rate. Benchmarking a volatile single-slot alternative showed it is worse than no cache at all once concurrent threads resolve different regions (122 ns/op vs 3.0 ns/op at 4 threads, from cache-line contention on the write). RulePartition is immutable and a pure function of its Partition, of which there are a handful loaded once, so the instances are now built up front in loadPartitionData and shared. That removes the allocation the memo existed to avoid rather than caching around it: 1.65-2.52 ns/op against a 2.93-3.50 ns/op baseline, flat across region count and thread count, with no per-call state. The now-redundant AWS_PARTITION LazyValue is gone. Removed dead peephole emitters from BddResultCodeGeneratorVisitor. All six were copy-pasted from ConditionFnCodeGeneratorVisitor but are unreachable: the BDD hoists computation into conditions, so results only consume assigned registers, and the visitor already rejects conditions and let-bindings outright. Dropping the pass from the result path leaves generated output byte-identical for all three BDD test models. Verified: 698 codegen tests, 393 S3 and 574 DynamoDB generated endpoint conformance tests, and a 1670-case differential test asserting substringEquals is indistinguishable from the spec composition across Latin-1, CJK and surrogate-pair inputs. Checkstyle and SpotBugs clean. * fix(endpoints): Address BDD peephole review findings Applies seven findings from the PR #7304 review. The two correctness ones are the same class as the bugs the previous commit fixed: an invariant asserted in a comment rather than enforced in code. isAlwaysNonNull elided a register null-check on an unenforced assumption (F1). It returned true for any __ite by name, while the comment claimed "a ternary between two string literals". simplifyIte only checks arity, RuleRuntimeTypeMirror types both branches as STRING, and the BDD model is produced by the endpoint compiler rather than by this repo, so a {"ref": ...} branch is legal input and can be null. For ite(cond, ref, "") the assign condition reported success with a null register, flipping a BDD edge and resolving an endpoint the spec does not permit. Both branches are now checked. The three literal-branch ite nodes in the S3 BDD keep the elided check, so the optimization is retained where it is provable. emitCoalesceBoolean evaluated its operand twice (F3). It emitted (expr != null ? expr : default), so a non-trivial operand such as a nested rules function ran twice per evaluation - a pessimization inside a performance change. Replaced with wrapper equality, which is exactly equivalent for both defaults, evaluates the subject once, and has no branch and no boxing: coalesce(x, false) becomes Boolean.TRUE.equals(x) and coalesce(x, true) becomes !Boolean.FALSE.equals(x). Removed the unreachable STRING_CONCAT receiver handling (F5). All three MethodCallExpression producers are in PrepareForCodegenVisitor and put either a string constant or a boolean-typed expression on the receiver, never a concatenation. It was needed by the left.equals(right) fallback removed in the previous commit. With it and a stray blank line gone, BddResultCodeGeneratorVisitor is byte-identical to the base branch and drops out of the PR diff entirely. awsPartition returned null where it previously threw (F6). Generated code null-checks the result, so an unloadable 'aws' partition silently made the condition false and took a different rule branch instead of failing. loadPartitionData now validates it and throws SdkClientException naming partitions.json. Restructuring for that also lets PartitionData hold all final fields, removing the non-final awsRulePartition that the previous commit had exposed to LazyValue's unsafe publication. Deduplicated host-label validation (F4). isValidHostLabelMulti was a character-for-character copy of the allowDots branch in isValidHostLabel, in a template copied into every service module, so the two could drift independently. Inverted the delegation: isValidHostLabel now dispatches to the two specialized helpers and there is one copy of each algorithm. Also dropped the redundant length guard in isValidHostLabelSingle, which isValidSingleLabel already applies. Moved RulesFunctionsSubstringEqualsTest to codegen-generated-classes-test (F8). RulesFunctions is a codegen template shared by every service, not a class S3 owns, so per docs/guidelines/testing-guidelines.md it belongs with "generated SDK common functionalities". It now sits beside EndpointUrlConformanceTest and RuleUrlTest, which cover other generated classes from the same template set, and gained coverage of the F4 delegation. Test names follow the methodToTest_when_expectedBehavior convention and the placement rationale is recorded in the class javadoc. Added the missing regression guards (F2). Three of the four rewrites had no assertions: only the substring peephole and the stringEquals null-safety decision were covered. The simple-BDD golden fixture contains none of these shapes and the S3 BDD was asserted only as non-null, so a change to the emitters or to PrepareForCodegenVisitor could silently restore the RulesFunctions dispatches with a green build. New ConditionFnCodeGeneratorVisitorTest covers the null-check elision decision and the single-evaluation coalesce form directly; BddEndpointProviderSpecTest now asserts the ite, coalesce and isValidHostLabel forms in the generated S3 BDD and that no dispatch remains. Confirmed the F1 tests are genuine guards: reverting isAlwaysNonNull to the name-only check fails two of them. Not changed, with reasons: the synthetic-name constants stay split across BddPeepholeVisitor and RuleRuntimeTypeMirror (F7), because moving them onto the visitor would make the rules2 package depend on rules2.bdd, which already depends on it, and because Checkstyle's DeclarationOrder forces SUBSTRING_EQUALS_FN to its current position. The fixture commit stays on this branch (F9) since relocating it means rewriting pushed history; it is called out in the PR description instead. Verified: 705 codegen tests; the 32 generated bddendpoints conformance tests, which exercise the BDD provider behaviourally and had not previously been run; 1671 substringEquals differential cases plus 65 sibling generated-class tests; 393 S3 and 574 DynamoDB endpoint conformance tests. rules2 golden fixtures remain byte-identical to feature/master/endpoints-bdd. Checkstyle and SpotBugs clean. * test(codegen): Add golden file for the S3 BDD provider The S3 BDD is the only test model that exercises all four peephole rewrites, the dynamic auth scheme name and the ite assign conditions, and it was covered only by assertThat(poetSpec()).isNotNull() plus targeted string assertions. Golden files are the established pattern for rules2 providers and make the effect of a codegen change reviewable as a diff, so the S3 BDD now has one. Replaces the isNotNull smoke test with generatesTo. Verified that codegen output is deterministic across separate JVM runs before adopting it, since a fixture is otherwise flaky, and that reverting an emitter makes the fixture test fail. Keeps the correctness invariants as explicit negative assertions rather than folding them into the fixture, consolidated into s3Bdd_neverEmitsSpecViolatingForms. Regenerating a golden file is a one-command operation and doing it without reading the diff is how a defect gets blessed into a fixture - this repo already has an instance of that. The assertions name what must never be emitted and why, so restoring a faster-but-spec-violating form means deleting a stated invariant rather than accepting a regenerated file. Confirmed both layers fail independently when an emitter regresses. Dropped the positive assertions the fixture now subsumes. literalBranchIteAssigns_keepElidedNullCheck stays, because it and ConditionFnCodeGeneratorVisitorTest only make sense as a pair. Fixture is 4102 lines, in the same raw JavaPoet form as the sibling endpoint-provider-bdd-class.java. PoetMatchers formats both sides and compares ignoring whitespace, so the fixture's own indentation is not significant. Verified: 703 codegen tests, Checkstyle and SpotBugs clean. Test-only change; no generated production code is affected.
…#7317) Endpoint's builder eagerly allocated two HashMaps (headers and attributes) for every endpoint, and handed them to the built Endpoint. Both escape into the result, so they cannot be optimized away by the JIT, and most endpoints need neither: headers are almost always absent and attributes are almost always a single AUTH_SCHEMES entry. Stage instead of allocate: - The header map is allocated only once a header is actually added. - The first attribute is held in two fields on the builder; a map is allocated only if a second distinct key arrives. - build() collapses whatever was staged into the smallest immutable map that fits: emptyMap, singletonMap, or an unmodifiable view. - Both maps are created with capacity 4 rather than the default 16, whose backing table was the largest single allocation on the path. No API change. The builder itself does not escape resolveEndpoint, so escape analysis already eliminates it; this removes the allocations that actually survive. Measured with JMH (allocation profiler, 2 forks, JDK 21, M-series), against an Endpoint-shaped holder built only from emptyMap/singletonMap as the theoretical floor: shape before after floor no attributes 120 B/op 24 B/op 24 B/op one attribute 232 B/op 64 B/op 64 B/op two attributes 264 B/op 232 B/op one attr + header 424 B/op 288 B/op The two shapes that dominate real endpoint rules now allocate exactly what a hand-written factory would. Time per construction drops 67% and 71% for those two shapes. Two-attribute construction costs ~3ns more because it stages an attribute and then promotes anyway; that path is S3 Express only and 3ns against a ~50us endpoint resolution. Behavior notes: - headers() and the attribute map are now unmodifiable. Mutating a map returned from a getter was never supported, and SdkHttpRequest already behaves this way. - Mutating a builder after build() no longer leaks into the Endpoint that was already built, for the no-attribute, single-attribute and header cases. Previously it leaked in every case. The two-or-more attribute case still shares state, unchanged from before.
…7322) * 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. * 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. * 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. * 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. * 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. * Cleanups * Additional cleanups/fixes from review pass * Add tests (missed adding in earlier commit) * Fix minor edge issues with split and loadPartitionData * Minor - rename method in AccoundIdEndpointMode to just value
…oints-bdd Master merged #7294 "Use code-generated endpoint rules", which renamed the codegen poet.rules2 package into poet.rules and deleted the legacy interpreted endpoint rules path. Resolve against the BDD work as follows: * EndpointProviderTasks: master collapsed the compiled/interpreted branch into a single unconditional path. Keep master's shape and re-add only the BDD branch, selecting the BDD provider when an endpoint BDD model is present. The two runtime-copy tasks the BDD path used, RulesEngineRuntimeLiteGenerator Task and RulesEngineRuntimeGeneratorTask2, were deleted upstream; they collapse into master's single RulesEngineRuntimeGeneratorTask, whose resource set is a superset of what the BDD provider needs. * Move poet/rules2/bdd to poet/rules/bdd (main, test and test resources) and rewrite poet.rules2 references to poet.rules. Git's rename detection carried the edits to the 28 renamed rules2 classes automatically, but the new BDD files were adds rather than renames and were left behind importing a package that no longer exists. * Re-sort imports in the moved BDD files. Once rules2.* becomes rules.*, the previously sorted import block no longer satisfies the CustomImportOrder checkstyle rule relative to the pre-existing poet.rules imports. Note that the RuleRuntimeTypeMirror listAccess return type fix and the RulesFunctions.java.resource additions now apply to the only remaining codegen path rather than just the BDD path, since master removed the interpreted one. Verified with mvn install -pl :codegen (checkstyle plus tests): all endpoint rules and BDD codegen tests pass, including both BDD golden files.
…ints-bdd-s3-ddb-staging Brings in the origin/master merge resolved on the feature branch, which moves the BDD codegen from poet.rules2 to poet.rules following upstream #7294. Merged cleanly; this branch's only delta is the S3 and DynamoDB BDD models. Note that S3 codegen does not yet compile on this branch, for a reason that predates this merge: BddResultCodeGeneratorVisitor emits a reference to software.amazon.awssdk.services.s3.endpoints.authscheme.DynamicEndpointAuth SchemeFactory, and that class was never carried into the BDD PR chain. It exists only on benchmark_only/endpoints-bdd-complete. The feature branch ships no service BDD models, so nothing exercised the path until this branch added the S3 model.
BddResultCodeGeneratorVisitor emits a reference to software.amazon.awssdk.services.s3.endpoints.authscheme.DynamicEndpointAuth SchemeFactory whenever an endpoint result carries an auth scheme whose name is resolved at runtime, but the class itself was never carried into the BDD PR chain. Port it in from cd0c5a3. Endpoint rulesets normally declare the auth scheme name as a string literal, so codegen can emit a direct call to the matching concrete builder. S3 is the exception: its BDD ruleset merges two otherwise identical results whose auth scheme names differ, lifting the difference into a runtime conditional, so the concrete type cannot be selected at codegen time. This factory collects the shared properties up front and defers type selection to create(name). The gap was invisible on this branch because it ships no service BDD models, so nothing exercised the path. It surfaces as an S3 compile failure as soon as a service BDD model is added. Ported faithfully; the only additions are the @NotThreadSafe annotation and documentation of the null-property and null-name behaviour. Testing: adds DynamicEndpointAuthSchemeFactoryTest covering both scheme names, property pass-through, fluent and order-independent setters, unset and explicitly null properties, reuse across calls, and fail-fast on unsupported and null names. Verified the tests are non-vacuous by mutating the class twice (swapping the two scheme names, and returning a default instead of throwing); each mutation failed the suite. Full mvn install -pl :s3 passes, 1804 tests, including checkstyle and spotbugs.
…ints-bdd-s3-ddb-staging Brings in DynamicEndpointAuthSchemeFactory, which the BDD endpoint codegen emits for S3 whenever an endpoint result carries an auth scheme whose name is resolved at runtime. This unblocks S3 codegen on this branch: the class was missing, so the generated DefaultS3EndpointProvider referenced a type that did not exist and S3 failed to compile. Verified on this branch, where the S3 and DynamoDB BDD models actually put both services on the BDD path (553 and 82 generated BDD node methods respectively, with 5 uses of the factory in the S3 provider): * mvn clean install -pl :s3,:dynamodb - BUILD SUCCESS, 1804 S3 and 61 DynamoDB unit tests, including checkstyle and spotbugs. * mvn verify -pl :s3,:dynamodb -P endpoint-tests - BUILD SUCCESS. S3 823 tests (393 provider, 430 client) and DynamoDB 1108 tests (548 provider, 534 client, 26 streams provider), no failures. All 393 S3 ruleset test cases run against the BDD provider with no skips. The 26 skipped S3 client tests are the pre-existing skipEndpointTests entries in the S3 customization.config, a file this branch does not modify.
alextwoods
added
changelog-not-required
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Branch for testing only.
Motivation and Context
Modifications
Testing
Screenshots (if appropriate)
Types of changes
Checklist
mvn installsucceedsscripts/new-changescript and following the instructions. Commit the new file created by the script in.changes/next-releasewith your changes.License