Conversation
…ubmodule
Generated Python bindings for any provider that declares provider-defined
functions (Terraform >= 1.8) were unimportable:
File ".../imports/kubernetes/provider/__init__.py", line 40, in <module>
from .functions import (
ModuleNotFoundError: No module named 'imports.kubernetes.provider.functions'
Root cause is in jsii-pacmak's Python target. Cross-submodule type
references are rendered as relative imports computed by
`lib/targets/python/type-name.ts#relativeImportPath`, which decides "is the
target a child of me?" with a bare prefix test and no `.`-boundary check:
if (toPkg.startsWith(fromPkg)) return `.${toPkg.substring(fromPkg.length + 1)}`;
The provider class lives in the `provider` submodule and imports the
functions wrapper from a sibling submodule. With the folder named
`provider-functions`, that sibling's Python name is
`<provider>.provider_functions`, which string-prefixes
`<provider>.provider` - so pacmak treated it as a child and emitted
`from .functions import ...`, naming a module that is never written.
Renaming the emitted folder to `functions` keeps the two submodule names
prefix-disjoint, so pacmak emits the correct `from ..functions import ...`.
Only Python was affected: Go, Java and C# reference the sibling package by
its fully qualified name and never compute a relative path.
The regression test replays pacmak's own `relativeImportPath` over the
emitted layout and asserts the import it would write resolves to the
emitted functions submodule, rather than asserting the folder name.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
fc5f812 to
c923a26
Compare
| export * as ephemeralCachedSecret from './ephemeral-cached-secret/index'; | ||
| export * as provider from './provider/index'; | ||
| export * as providerFunctions from './provider-functions/index'; | ||
| export * as functions from './functions/index'; |
There was a problem hiding this comment.
small change, big impact.
Too bad my out-of-repo harnesses didn't really validate Python consumption of provider functions - the real issue is correctly being addressed now with raising the version ceiling of what we test of course
but if I build demo harnesses I should include JSII cross compiled library testing
https://github.com/sakul-learning/cdktn-provider-features-demo
This comment has been minimized.
This comment has been minimized.
Status after #395 / #398: no longer a blocker — the rename is now the risk#395 (jsii-pacmak 1.140.0) merged, #398 was rebased on it and went fully green, including What remains in this PR is the My own position is that the supported surface is Worth keeping, re-scoped
Suggest either re-scoping this PR to (1) + (2) and dropping the rename, or closing it and opening those two as small follow-ups. Happy with either. |
|
Agreed on all of it — closing in favour of the re-scoped follow-ups. You're right that the rename is the risk now rather than the fix. #395 landing makes it unnecessary, and "internal emit detail" was wrong of me: the jsii submodule name is a public FQN in every target, and renaming it as a non-major on packages whose versions track the upstream provider — with Split as suggested:
On the pacmak-replay test: agreed it would pass vacuously now, so it's dropped rather than carried over — #404 is the durable replacement. And you're right that the upstream |
…g the functions submodule (#403) ### Related issue Follow-up from the review on #400, point 1. #400 is closed; this is the part of it worth keeping, re-scoped and without the submodule rename. ### Description A provider that declares provider-defined functions emits them to `providers/<provider>/provider-functions/index.ts`. A resource or data source named `<provider>_provider_functions` sanitizes to the base name `provider_functions`, which `getFileName` maps to that **same** directory — so the resource silently overwrites the functions wrapper. This is a live gap on `main`, independent of #400's rename. `sanitizeClassOrNamespaceName` already reserves `function` (singular), `license`, `version` and the TypeScript keywords, and gives a resource named `provider` the `_resource` suffix for exactly this reason — but nothing covers `provider_functions`. #### What actually happens today Generating the new fixture (a provider with one function plus an `example_provider_functions` resource) against `main` produces: ``` providers/example/index.ts providers/example/lazy-index.ts providers/example/provider-functions/README.md providers/example/provider-functions/index.ts ``` `provider-functions/index.ts` contains `export class ProviderFunctions extends cdktn.TerraformResource` — the resource, not the functions wrapper. The provider's functions are gone, and `index.ts` reads: ```ts export * as providerFunctions from './provider-functions/index'; export * as providerFunctions from './provider-functions/index'; ``` The duplicate name is a TypeScript error, so the generated bindings do not compile. `lazy-index.ts` gets the same duplication. #### The fix Extend the existing `provider` special case to `provider_functions`, so the resource lands in `provider-functions-resource/` and both are exported under distinct names. Only `provider_functions` is reserved. `functions` is not a directory the generator emits, so a resource by that name does not collide today — and as noted in the #400 review, `provider_functions` is a far less likely resource name than `functions`, which is a point in favour of the current layout. ### Testing New fixture and test asserting the three things that break without the guard: the functions submodule holds the wrapper rather than the resource, the resource gets its own directory, and the provider index exports no duplicate names. Plus unit assertions on `sanitizeClassOrNamespaceName`. Confirmed to be a real regression guard — with the guard reverted, 3 of the 6 fail; with it, all pass. Full `@cdktn/provider-generator` suite: **23 suites / 112 tests / 101 snapshots pass**. `nx lint` clean. ### Checklist - [x] I have updated the PR title to match [CDKTN's style guide](https://github.com/open-constructs/cdk-terrain/blob/main/CONTRIBUTING.md#pull-requests-1) - [x] I have run the linter on my code locally - [x] I have performed a self-review of my code - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation if applicable — n/a - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective - [x] New and existing unit tests pass locally with my changes 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
### Related issue Follow-up from the review on #400, point 2. #400 is closed; this is the durable half of its regression coverage. ### Description `test/python/edge/test.ts` has been `describe.skip`'d since 3acb935 (**1 December 2022**), waiting on [aws/jsii#3866](aws/jsii#3866), with the stated intent to "re-add them when updating JSII". That fix shipped shortly after, and the JSII update has since landed (#395: jsii 6.0, jsii-pacmak 1.140.0) — but the skip was never lifted. Python is the only language whose edge test does not run. TypeScript, Go, Java and C# all do. That gap has a concrete cost: it is why #311's cross-language compile coverage never saw the broken Python provider import, which only surfaced once CI could reach Terraform >= 1.8 (#398) — and then via the `python-documentation` example rather than a test aimed at it. `main.py` calls `edge.provider.EdgeProvider(...)`, so `beforeAll`'s synth imports the generated `edge.provider` module — exactly the module whose `__init__.py` carried the unresolvable import. A regression there fails the suite loudly, at the import. ### Two stale assertions, and why they were wrong Re-enabling produced **18 passed / 2 failed**. The suite is substantially intact after three years dormant; both failures were expectations that had never once executed. **1. A typo.** The `numList` expectation read `...reqnum)}` — a stray closing paren — where synth produces `...reqnum}`. **2. A wrong assumption about `Fn.lookup`.** The map expectations assumed every `Fn.lookup(map, key, default)` renders as a Terraform `lookup()`. It does not. `Fn.lookup` (`packages/cdktn/src/terraform-functions.ts:41`) emits `lookup()` only when the default is **truthy**: ```ts static lookup(inputMap: any, key: string, defaultValue?: any) { if (defaultValue) return Fn._lookup(inputMap, key, [defaultValue]); return asAny(propertyAccess(inputMap, [key])); // -> renders inputMap[key] } ``` The fixture passes the same `Fn.lookup(...)` call for all six attributes, with different defaults: | attribute | default | truthy | renders as | | --- | --- | --- | --- | | `reqMap` | `false` | no | `${map_resource.map.reqMap.key1}` | | `optMap` | `"missing"` | yes | `${lookup(map_resource.map.optMap, "key1", "missing")}` | | `computedMap` | `0` | no | `${map_resource.map.computedMap.key1}` | So the deciding factor is the truthiness of the default, **not** whether the attribute is required, optional or computed. The corrected expectations match, and are now identical to `test/typescript/edge/test.ts`. Only the first failing assertion in each block is reported by jest, so the rest of the map block was corrected at the same time rather than surfacing them one CI run at a time. ### These pin current behaviour, not necessarily correct behaviour (#416) Worth flagging rather than burying: that truthiness guard looks like a bug. `Fn.lookup(map, "key", false)` reads as "return `false` if the key is missing", but the falsy default is discarded and the emitted expression becomes a bare property access — which errors on a missing key instead of returning the default. The guard should almost certainly be `defaultValue !== undefined`. Filed as #416. I have deliberately **not** changed it here. This PR is test coverage; altering `Fn.lookup` is a behavioural change to the construct library with its own blast radius, and it would be wrong to smuggle it in. If it is fixed, these expectations move with it — and at that point the edge tests will be the thing that proves the fix, which is rather the point of un-skipping them. Note this also means the equivalent TypeScript assertions were changed to match this behaviour in 09ac17b ("chore: fix assertions", Aug 2023) with no rationale recorded. ### Testing CI on this branch: **257 pass, 0 fail**, 1 skip (`windows_integration`, `if: false`). Both `python/edge` jobs pass, on Terraform 1.5.7 and 1.16.1. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
2 participants
Related issue
No filed issue — found by CI on #398, which raises the Terraform test ceiling to 1.16.1. #398 is deliberately on hold until this lands.
Description
Generated Python bindings for any provider declaring provider-defined functions are unimportable:
Root cause: a jsii-pacmak bug, triggered by our emitted layout
jsii-pacmak/lib/targets/python/type-name.ts#relativeImportPathdecides "is the target submodule a child of me?" with a bare string-prefix test and no.-boundary check:The provider class lives in the
providerjsii submodule and imports the functions wrapper from a sibling submodule. Namedprovider-functions, that sibling's Python name is<provider>.provider_functions— which string-prefixes<provider>.provider. pacmak therefore treats the sibling as a child and emitsfrom .functions import …, naming<provider>.provider.functions, a module that is never written.Renaming the emitted folder to
functionskeeps the two submodule names prefix-disjoint, so pacmak emits the correctfrom ..functions import ….Only Python is affected. Go (
.../edge/providerfunctions), Java (imports.edgeprovider.provider_functions) and C# (Providers/Edge/ProviderFunctions) reference the sibling by fully qualified name and never compute a relative path — verified in the generated edge-provider bindings.Already fixed upstream — in a pacmak we don't use yet
jsii-pacmak stopped emitting relative cross-submodule imports in 1.136.0, switching to absolute
LazyImportreferences.relativeImportPathstill exists in 1.140.0 but is dead code. Verified by inspecting each published tarball:relativeImportPathcall sitesLazyImportrefsThis is why prebuilt providers were never affected, despite being generated against a modern Terraform that emits
functions.cdktn-provider-time@14.0.1on PyPI ships both aprovider/and aprovider_functions/submodule — the exact prefix-colliding pair — yet itsprovider/__init__.pycontainsimport cdktn_provider_time.provider_functions as _provider_functions_1fc2c0c2, an absolute import, with no broken relative one. It was built with jsii 5.9.51 and a post-1.136 pacmak. Onlycdktn getrun from this repo's pinned 1.128.0 produces the broken output.So there are two ways to fix this: rename the folder (this PR), or upgrade jsii-pacmak past 1.136.0 — which #373 already proposes (
1.128.0→1.139.0+) as part of the jsii 6.0 migration.I went with the rename because it is small, independent, and unblocks #398 now, whereas #373 is a coordinated jsii/TypeScript/constructs upgrade. The rename also stays worthwhile after #373: the two submodule names become prefix-disjoint regardless of which pacmak is in use, so the layout stops depending on an upstream implementation detail. If you would rather wait for #373 and drop this, that is a reasonable call — the regression test would need rethinking, since it replays
relativeImportPathdeliberately.Why this layer
The real defect is upstream. Fixing it there means patching a bundled dependency that also ships inside
cdktn-cli. The generator owns the emitted layout and is the only thing that has to change: two emit sites plus a documented constant,PROVIDER_FUNCTIONS_FOLDER_NAME, carrying the "must not start withprovider" rationale at the point where someone would otherwise rename it back.The change is invisible to users — the functions are reached through the
provider.functionsgetter, and nothing inexamples/,test/ordocs/referenced the old submodule name.Worth reporting upstream to
aws/jsiiseparately; the fix there is a.-boundary check inrelativeImportPath.Why it was never caught
Terraform only emits a
functionssection interraform providers schema -jsonfrom 1.8 onward, and CI's ceiling was 1.6.5, so no CI job could produce a schema that reaches this codepath. That is exactly the gap #337 was filed about.test/python/edge/test.tsis alsodescribe.skip'd, which is why #311's cross-language compile coverage did not catch it either.Testing
New test in
packages/@cdktn/provider-generator/src/get/__tests__/generator/provider-functions.test.ts. Rather than asserting the folder name — a change-detector that would be re-broken by the same rename — it copies pacmak'srelativeImportPathverbatim, reads the provider→functions submodule pair back out of the generatedindex.ts, computes the import specifier pacmak would write, and asserts it resolves to the emitted functions submodule.Confirmed to be a real regression guard: with
PROVIDER_FUNCTIONS_FOLDER_NAMEreverted to"provider-functions", the suite fails 8 of 15; with the fix it passes.Also verified:
@cdktn/provider-generator: 22 suites / 107 tests / 101 snapshots pass (re-run after rebasing onto fix(provider-generator): satisfy module provider configuration aliases on get #383). 7 snapshots updated, all theproviderFunctions→functionsexport line.python/edge/functions/exists andprovider/__init__.pyhasfrom ..functions import …. An AST walk resolving every relative import in the generated Python: 22 checked, 0 unresolvable; the same walk pre-fix flags exactly one.cdktn getagainst the realhashicorp/time@0.14.1provider (which declares provider functions) withtargetLanguage: PYTHON— real schema → jsii → pacmak → Python. Produces aimports/time/functions/sibling package andfrom ..functions import TimeProviderFunctions; all 8 relative imports resolve.Not verified:
@examples/python-documentationitself againstkubernetes@~> 2.0, which needs a fullpnpm package+ example synth. Thetimerun exercises the identical path with a real functions-bearing schema, and the pre-fix symptom matched the CI traceback's file and line exactly. #398's CI is the definitive check — that example is what fails there today.Follow-ups, deliberately not here
test/python/edge/test.tsisdescribe.skip'd; un-skipping it is how this class of bug gets caught in future.<provider>_functions, which would collide on thefunctionsfolder.instance/instance_state); they don't cross-reference today, so the pacmak bug stays latent, but it is one reference away from biting again.Checklist
🤖 Generated with Claude Code