Fix Python schema generation for fields named int or bool#64
Merged
Conversation
crossplane project build generates broken Python models when an XRD has
a property literally named int or bool. The generated models reference
undefined type aliases int_aliased and bool_aliased, which makes the
models unimportable:
PydanticUserError: `ObjectMeta` is not fully defined; you should
define `int_aliased`, then call `ObjectMeta.model_rebuild()`.
The undefined aliases are emitted by the pinned code generator image
docker.io/koxudaxi/datamodel-code-generator:0.31.2. The CLI worked
around this with fixAliasedTypesInFile, which text-replaced the broken
aliases, but it only ran in the OpenAPI generation path - not the
XRD/CRD path that project build uses - so XRD-derived models and the
shared meta/v1.py kept the broken references.
The underlying code generator bug is fixed upstream in 0.54.0, which
sanitizes builtin-conflicting field names by appending a trailing
underscore and preserving the wire name via a Pydantic alias. Fields
named int now generate as `int_: int | None = Field(None, alias='int')`.
This commit bumps the pinned image to 0.59.0, which fixes the broken
output at its source. With the fix in place fixAliasedTypesInFile no
longer matches anything, so this commit removes it along with the
postProcessFile wrapper, leaving both generation paths to call
adjustImportsInFile directly.
Fixes crossplane#63.
Signed-off-by: Nic Cope <nicc@rk0n.org>
Member
Author
|
Here are the full diffs (vs the pre-regeneration commit), for a provider CRD schema (
|
📝 WalkthroughWalkthroughThis PR upgrades the Python schema code generator Docker image to version 0.59.0 and refactors the post-processing pipeline to remove intermediate helper functions that are now handled upstream by the newer image version. ChangesPython code generator upgrade and pipeline simplification
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/schemas/generator/python.go`:
- Around line 577-578: The wrapped error from adjustImportsInFile drops the file
context making failure messages unhelpful; update the errors.Wrapf call in the
caller that invokes adjustImportsInFile (the block containing if err :=
adjustImportsInFile(fs, destPath); err != nil) to include destPath and a short
user-facing hint (e.g., "unable to update imports for generated file %s; check
file permissions or import paths and re-run generation") so the error becomes
errors.Wrapf(err, "unable to update imports for generated file %s; check file
permissions or import paths and re-run generation", destPath).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 195be654-f908-4931-9b7a-e23cc3fc7267
📒 Files selected for processing (1)
internal/schemas/generator/python.go
Comment on lines
+577
to
+578
| if err := adjustImportsInFile(fs, destPath); err != nil { | ||
| return errors.Wrapf(err, "adjusting imports") |
There was a problem hiding this comment.
Restore actionable context in the wrapped import-adjustment error.
At Line 578, the wrap message dropped file context, which makes failures hard to act on during generation. Can we include destPath and a brief user-facing hint?
💡 Proposed fix
- if err := adjustImportsInFile(fs, destPath); err != nil {
- return errors.Wrapf(err, "adjusting imports")
+ if err := adjustImportsInFile(fs, destPath); err != nil {
+ return errors.Wrapf(err, "cannot adjust generated python imports for %q; verify generated model paths and try again", destPath)
}As per coding guidelines, "CRITICAL: Ensure all error messages are meaningful to end users, not just developers - avoid technical jargon, include context about what the user was trying to do, and suggest next steps when possible."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if err := adjustImportsInFile(fs, destPath); err != nil { | |
| return errors.Wrapf(err, "adjusting imports") | |
| if err := adjustImportsInFile(fs, destPath); err != nil { | |
| return errors.Wrapf(err, "cannot adjust generated python imports for %q; verify generated model paths and try again", destPath) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/schemas/generator/python.go` around lines 577 - 578, The wrapped
error from adjustImportsInFile drops the file context making failure messages
unhelpful; update the errors.Wrapf call in the caller that invokes
adjustImportsInFile (the block containing if err := adjustImportsInFile(fs,
destPath); err != nil) to include destPath and a short user-facing hint (e.g.,
"unable to update imports for generated file %s; check file permissions or
import paths and re-run generation") so the error becomes errors.Wrapf(err,
"unable to update imports for generated file %s; check file permissions or
import paths and re-run generation", destPath).
Member
Author
|
It looks like datamodel-code-generator changed how it emits a field's default value. Before: providerConfigRef: Optional[ProviderConfigRef] = Field(
default_factory=lambda: ProviderConfigRef.model_validate(
{'kind': 'ClusterProviderConfig', 'name': 'default'}
)
)After: providerConfigRef: ProviderConfigRef | None = Field(
{'kind': 'ClusterProviderConfig', 'name': 'default'}, validate_default=True
)function-sdk-python serializes composed resources with
So every composed upbound (GCP/AWS) resource now has an explicit |
Member
@negz Will functions setting the |
adamwg
approved these changes
Jun 3, 2026
Member
adamwg
left a comment
adamwg
left a comment
There was a problem hiding this comment.
LGTM. From the diffs, it looks to me like this shouldn't be a breaking change for any existing Python functions using the schemas.
Probably best to wait until we have some e2e tests in place to help validate changes, but I wonder if we could get renovate to automatically bump versions of the third-party images we depend on for things like this.
Member
Author
@adamwg I don't think it's a breaking change but there is a subtle behavior change around defaults in there. See crossplane/function-sdk-python#207 for an analysis. |
2 tasks
Member
Author
|
I'll merge this, but I've added a label to try remind us to not the behavior change in the release notes. I think it'll work best if we also bump the functions to use crossplane/function-sdk-python#208. |
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
2 participants
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.
Description of your changes
Fixes #63
crossplane project buildgenerates broken Python models when an XRD has a property literally namedintorbool(for example when modelling DRA'sDeviceAttribute, whose wire fields areint/bool/string/version). The generated models reference undefined type aliasesint_aliasedandbool_aliased, which makes them unimportable:The undefined aliases are emitted by the pinned code generator image
docker.io/koxudaxi/datamodel-code-generator:0.31.2. The CLI worked around this withfixAliasedTypesInFile, which text-replaced the broken aliases, but it only ran in the OpenAPI generation path — not the XRD/CRD path thatproject builduses — so XRD-derived models and the sharedmeta/v1.pykept the broken references.The underlying code generator bug is fixed upstream in koxudaxi/datamodel-code-generator#2968, first released in
0.54.0, which sanitizes builtin-conflicting field names by appending a trailing underscore and preserving the wire name via a Pydantic alias. A field namedintnow generates asint_: int | None = Field(None, alias='int').This PR bumps the pinned image to
0.59.0, fixing the broken output at its source. With the fix in placefixAliasedTypesInFileno longer matches anything, so it is removed along with thepostProcessFilewrapper, leaving both generation paths to calladjustImportsInFiledirectly.I have:
./nix.sh flake checkto ensure this PR is ready for review.Added or updated unit tests.The Python generator's image-backed output is not currently covered by automated tests; see the draft note above.Linked a PR or a docs tracking issue to document this change.Addedbackport release-x.ylabels to auto-backport this PR.Need help with this checklist? See the cheat sheet.