fix(provider-generator): collapse unsupported nested collections to nearest Any wrapper#300
fix(provider-generator): collapse unsupported nested collections to nearest Any wrapper#300so0k wants to merge 2 commits into
Conversation
Mirrors of the existing StringMapMap for computed map(map(number)) and map(map(bool)) attributes, plus AnyMapMap which closes the wrapper-class set for arbitrarily deep collection nestings: any unsupported shape can now be collapsed to a nearest typed Any-wrapper (AnyListList, AnyMapList, AnyListMap or AnyMapMap) by the provider generator. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
c3b468e to
e773583
Compare
Actual fallback scenarios to plain untyped interpolation getter (it's not version skew!): Allowlist and collapse rule are hand-maintained invariants, not enforced ones
Both can drift within a single release: someone removes or renames a wrapper class in core and updates the list, a refactor breaks The guard is the difference between those mistakes shipping as compile-broken generated bindings in users' .gen folders — the exact issue #11 failure mode — versus a degraded-but-compiling untyped getter. The cost asymmetry favors keeping itThe fallback isn't a subsystem; it's a conditional routing to the emitter's pre-existing plain getter path (used today for tokenizable and assignable attributes). Deleting it saves a few lines. Keeping it converts an entire class of future bugs from "provider unusable, user's build breaks" into "one attribute reads as an untyped token, with a breadcrumb." The alternative failure modes are worseWithout the guard, an unresolved shape either emits a non-existent class (silent until the user's So the honest framing: it's cheap insurance against our own future mistakes and unknown future schema shapes — not against version skew, which only the consumer's compiler (and the lockstep versioning contract) can catch. If we judge that insurance not worth one conditional plus one test, removal is a two-line change |
| /** | ||
| * Terraform type: \`map(map(list(string)))\`. | ||
| * This nesting is deeper than the typed wrapper classes in the core cdktn package; it is exposed as \`AnyMapMap\` and values below that boundary are untyped tokens. | ||
| */ | ||
| public get mapMapListString() { | ||
| return this._mapMapListString; | ||
| } |
There was a problem hiding this comment.
collapsed AnyMapMap JSDocs indicate original terraform type
…earest Any wrapper Computed attributes whose type nests list/set/map deeper than the fixed set of wrapper classes hand-written in the core cdktn package (e.g. the awscc provider's map(map(list(string)))) composed stored-class names like "StringListMapMap" that don't exist, producing bindings that fail to compile: error TS2551: Property 'StringListMapMap' does not exist on type ... The generator now resolves the composed name against the set of classes that actually exist in core; when unsupported, it collapses to the nearest typed Any-wrapper that still encodes the outer two collection layers (map(map(list(string))) => AnyMapMap), preserving typed navigation down to where core's class coverage ends instead of skipping the attribute (the approach of the closed upstream hashicorp/terraform-cdk#3922) or degrading to a fully opaque token. Collapsed getters carry a JSDoc block stating the full Terraform type and the untyped boundary, so the shape stays visible in IntelliSense without a docs round-trip. Input/setter types are unaffected. If no stored class can be resolved at all (should be unreachable), the getter falls back to a plain untyped interpolation getter, guarded by a direct unit test and a debug-log breadcrumb. Fixes #11 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
e773583 to
6f65fe3
Compare
Review updateI re-checked the latest head ( Validation I ran on the reviewed implementation:
The AWSCC repro output now emits the expected collapsed wrapper for the problematic data source shape: private _objectConfiguration = new cdktn.AnyMapMap(this, "object_configuration");and the synthesized Terraform output uses the expected nested lookup expression: data.awscc_appintegrations_data_integration.data_integration.object_configuration["example_object"]["fields"]A couple of notes from the provider checks:
Given the validations and spot checks above, I do not think additional follow-up work is needed for this PR at this stage. Looks good to ship. |
this addresses #110 (comment) - but spot checks didn't show any provider ( |
| const SUPPORTED_STORED_CLASSES = new Set([ | ||
| "StringMap", | ||
| "NumberMap", | ||
| "BooleanMap", | ||
| "AnyMap", | ||
| "StringMapMap", | ||
| "NumberMapMap", | ||
| "BooleanMapMap", | ||
| "AnyMapMap", | ||
| "BooleanList", | ||
| "StringListList", | ||
| "NumberListList", | ||
| "BooleanListList", | ||
| "AnyListList", | ||
| "StringMapList", | ||
| "NumberMapList", | ||
| "BooleanMapList", | ||
| "AnyMapList", | ||
| "StringListMap", | ||
| "NumberListMap", | ||
| "BooleanListMap", | ||
| "AnyListMap", | ||
| ]); |
There was a problem hiding this comment.
Optionally we add a test which ensures this list matches ../../../../../../cdktn/src/complex-computed-list.ts
# packages/@cdktn/provider-generator/src/get/__tests__/generator/supported-stored-classes.test.ts
// Hand-written wrapper classes follow the naming convention
// <String|Number|Boolean|Any><Map|List>(<Map|List>)* (e.g. StringMap, AnyMapMap,
// StringListMap). Other exported classes in that file (ComplexComputedList,
// ComplexObject, and the abstract ComplexList/ComplexMap/MapList base classes) are
// structural helpers, not stored-class wrappers, and are intentionally excluded by
// this pattern rather than by name, so newly added wrapper classes are picked up
// automatically.
const WRAPPER_CLASS_NAME = /^(?:String|Number|Boolean|Any)(?:List|Map)+$/;
function getHandWrittenWrapperClasses(): string[] {
const source = readFileSync(
join(
__dirname,
"../../../../../../cdktn/src/complex-computed-list.ts",
),
"utf-8",
);
const classNames: string[] = [];
const classDeclaration = /^export (?:abstract )?class (\w+)/gm;
let match: RegExpExecArray | null;
while ((match = classDeclaration.exec(source))) {
classNames.push(match[1]);
}
return classNames.filter((name) => WRAPPER_CLASS_NAME.test(name));
}
test("every hand-written wrapper class is registered in SUPPORTED_STORED_CLASSES", () => {
const wrapperClasses = getHandWrittenWrapperClasses();
// Sanity-check the extraction itself so a change to complex-computed-list.ts that
// breaks the parser (rather than the coverage) fails loudly and specifically.
expect(wrapperClasses.length).toBeGreaterThan(0);
const missing = wrapperClasses.filter(
(name) => !SUPPORTED_STORED_CLASSES.has(name),
);
expect(missing).toEqual([]);
});
test("SUPPORTED_STORED_CLASSES has no entries that don't correspond to a real class", () => {
const wrapperClasses = new Set(getHandWrittenWrapperClasses());
const stale = [...SUPPORTED_STORED_CLASSES].filter(
(name) => !wrapperClasses.has(name),
);
expect(stale).toEqual([]);
});Cons:
- Regex-on-source parsing is format-coupled. It relies on prettier's stable export class Name single-line form. Acceptable: prettier enforces that shape repo-wide, the length > 0 sanity check catches parser rot in one direction, and a totally broken extraction would make all 21 entries look "stale" and fail the other direction too. A file rename/split in core fails with ENOENT rather than a descriptive message — loud, just less self-explanatory.
- A future wrapper with a divergent constructor signature would pass parity and only be caught if its shape appears in the fixture snapshot. Given all 21 current classes conform and new ones are written by mirroring, I'd not add machinery for this now — worth a line in the test's comment at most.
- Cross-package reach. The test reads another workspace's source file, which is slightly unusual coupling — but it's precisely the coupling the invariant is about, so encoding it explicitly is honest. The alternative (importing the built cdktn package and reflecting on exports) would be stronger (it would also catch a dropped index re-export) but requires the core build artifact to exist before provider-generator unit tests run; the source-read approach has no build-order dependency. Reasonable trade-off as-is.
Fixes #11
Problem
Running
cdktn getfor theawsccprovider generates bindings that fail to compile:The generator composes stored-class names recursively (
string→stringList→stringListMap→stringListMapMapformap(map(list(string)))) and emitsnew cdktn.StringListMapMap(...)for computed attributes — but the corecdktnpackage only hand-writes a fixed set of collection wrapper classes, so triple-nested combinations reference classes that don't exist. Upstream's attempt (hashicorp/terraform-cdk#3922, closed unmerged) skipped the attribute entirely.Fix: collapse to the nearest typed
Anywrapperfeat(lib)— addsNumberMapMap,BooleanMapMap(typed depth-2 shapes that were also broken; mirrors of the existingStringMapMap) andAnyMapMap.AnyMapMapcloses the set: any collection nesting of any depth now collapses to one ofAnyListList/AnyMapList/AnyListMap/AnyMapMap, which all exist. Additive JSII API.fix(provider-generator)—resolveStoredClassName()checks the composed name against the verified set of core classes; when unsupported it collapses to the nearestAnywrapper that still encodes the outer two collection layers. Theanyboundary sits exactly where core's class coverage ends, preserving typed navigation above it:Collapsed getters carry a JSDoc block so the full shape stays visible in IntelliSense (attribute getters previously had no JSDoc at all):
Supported shapes are untouched (
map(list(string))still emitscdktn.StringListMap, no JSDoc added — all pre-existing snapshots pass byte-for-byte), and input/setter types keep their full precision. If no stored class can be resolved at all — should be unreachable now — the getter falls back to a plain untyped interpolation getter, kept loud by a direct unit test on the branch plus a debug-log breadcrumb (attribute name + composed type) so a wild firing leaves evidence instead of a silent typing downgrade.Note on version alignment: bindings generated by this CLI reference the new core classes, so
cdktnandcdktn-climust be on the same release (the standard support contract — first troubleshooting step for post-upgrade tsc errors is aligning the two).Worked examples (all covered by the new fixture/snapshot test)
map(map(list(string)))(awscc repro)AnyMapMap(collapsed)list(map(map(string)))AnyMapList(collapsed)set(map(list(number)))AnyMapList(collapsed, set-wrapped)map(map(map(list(string))))AnyMapMap(collapsed)map(map(number))/map(map(bool))NumberMapMap/BooleanMapMap(newly typed)map(list(string))(control)StringListMap(unchanged)Validation
pnpm nx build cdktn(tsc + jsii): cleanpnpm nx test @cdktn/provider-generator: 18/18 suites, 84/84 tests, 89/89 snapshotspnpm nx test cdktn: 443/444 (the one failure is the pre-existingtoPlanSuccessfullymatcher test requiring a liveterraform planwith Docker; fails identically onmain)StringMapMaplookup tests for the three new classes🤖 Generated with Claude Code