Render optional attributes with default values as mandatory#909
Render optional attributes with default values as mandatory#909
Conversation
WalkthroughThis pull request fixes a bug in the protocols generator where attributes with default values were incorrectly rendered as Optional in schema protocols. The Jinja2 attribute rendering filter has been updated to only append "Optional" when an attribute is marked optional and has no default value. A changelog entry documenting this fix and parametrized unit tests validating the corrected behavior across different optional and default_value combinations have been added. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## stable #909 +/- ##
==========================================
+ Coverage 72.91% 80.71% +7.80%
==========================================
Files 119 119
Lines 10336 10336
Branches 1551 1551
==========================================
+ Hits 7536 8343 +807
+ Misses 2287 1469 -818
- Partials 513 524 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 32 files with indirect coverage changes 🚀 New features to boost your workflow:
|
![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
🧹 Nitpick comments (1)
tests/unit/sdk/test_protocols_generator.py (1)
48-73: Consider adding falsy default-value cases to harden the matrix.The current cases cover
NoneandTrue; addingFalse,0, and""would explicitly guard theis Nonesentinel behavior.Suggested additions
RENDER_ATTRIBUTE_TEST_CASES = [ @@ RenderAttributeTestCase( name="optional-with-default", optional=True, default_value=True, expected="enabled: Boolean", ), + RenderAttributeTestCase( + name="optional-with-false-default", + optional=True, + default_value=False, + expected="enabled: Boolean", + ), + RenderAttributeTestCase( + name="optional-with-zero-default", + optional=True, + default_value=0, + expected="enabled: Boolean", + ), + RenderAttributeTestCase( + name="optional-with-empty-string-default", + optional=True, + default_value="", + expected="enabled: Boolean", + ), ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/sdk/test_protocols_generator.py` around lines 48 - 73, The test matrix RENDER_ATTRIBUTE_TEST_CASES in tests/unit/sdk/test_protocols_generator.py lacks falsy default_value cases; add additional RenderAttributeTestCase entries that use default_value=False, default_value=0, and default_value="" (with appropriate name fields like "optional-false-default", "optional-zero-default", "optional-emptystr-default") to ensure the rendering logic in the renderer (look at RenderAttributeTestCase usage and the optional vs default handling) treats non-None falsy values as explicit defaults and produces the expected types accordingly; update expected strings to match the behavior verified for those falsy defaults.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@changelog/894.fixed.md`:
- Line 1: Rewrite the single-line changelog to be clearer and grammatically
correct by stating the behavior precisely; replace the current sentence with
something like: "Generate protocols where optional attributes with a default
value are rendered as required (non-nullable) attributes," ensuring it mentions
"optional attributes with a default value" and that they become "required
(non-nullable)" in generated typing; update the line in changelog/894.fixed.md
accordingly.
---
Nitpick comments:
In `@tests/unit/sdk/test_protocols_generator.py`:
- Around line 48-73: The test matrix RENDER_ATTRIBUTE_TEST_CASES in
tests/unit/sdk/test_protocols_generator.py lacks falsy default_value cases; add
additional RenderAttributeTestCase entries that use default_value=False,
default_value=0, and default_value="" (with appropriate name fields like
"optional-false-default", "optional-zero-default", "optional-emptystr-default")
to ensure the rendering logic in the renderer (look at RenderAttributeTestCase
usage and the optional vs default handling) treats non-None falsy values as
explicit defaults and produces the expected types accordingly; update expected
strings to match the behavior verified for those falsy defaults.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7a7f378c-ccd0-4dd4-9cd0-dad65f7b9fea
📒 Files selected for processing (3)
changelog/894.fixed.mdinfrahub_sdk/protocols_generator/generator.pytests/unit/sdk/test_protocols_generator.py
| @@ -0,0 +1 @@ | |||
| Generate protocols where optional attributes with a default value renders attributes that are required, i.e. not nullable. | |||
There was a problem hiding this comment.
Tighten changelog wording for clarity and grammar.
Line 1 is grammatically awkward and a bit ambiguous. Consider a clearer statement focused on generated typing behavior.
Suggested wording
-Generate protocols where optional attributes with a default value renders attributes that are required, i.e. not nullable.
+Generate protocols so attributes with a default value are rendered as required (not nullable).📝 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.
| Generate protocols where optional attributes with a default value renders attributes that are required, i.e. not nullable. | |
| Generate protocols so attributes with a default value are rendered as required (not nullable). |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@changelog/894.fixed.md` at line 1, Rewrite the single-line changelog to be
clearer and grammatically correct by stating the behavior precisely; replace the
current sentence with something like: "Generate protocols where optional
attributes with a default value are rendered as required (non-nullable)
attributes," ensuring it mentions "optional attributes with a default value" and
that they become "required (non-nullable)" in generated typing; update the line
in changelog/894.fixed.md accordingly.
There was a problem hiding this comment.
this suggestion is easier to understand, but also leaves out the word "optional"
There was a problem hiding this comment.
@ajtmccarty, that's a fair point — "optional" is meaningful here since the fix specifically targets attributes marked as optional in the schema that also have a default value. Here's a revised suggestion that preserves it:
-Generate protocols where optional attributes with a default value renders attributes that are required, i.e. not nullable.
+Generate protocols so that optional attributes with a default value are rendered as required (not nullable).This keeps "optional attributes with a default value" intact while fixing the grammatical awkwardness of the original.
| @@ -0,0 +1 @@ | |||
| Generate protocols where optional attributes with a default value renders attributes that are required, i.e. not nullable. | |||
There was a problem hiding this comment.
this suggestion is easier to understand, but also leaves out the word "optional"
2 participants
Why
Currently when you create a schema in Infrahub with a node that has a mandatory attribute and that attribute has a default value, then Infrahub will update the schema definition and mark the attribute as optional. The intention is to change this behaviour, until that happens we want to update the protocol definitions that are generated out of such a schema so that the attributes appear to be required (a separate PR will be opened against the Infrahub repo to disallow changing such attributes to a
nullvalue.Closes #894.
What changed
Impact & rollout
At some later date we'll want to revert this in order to properly support schemas where the user wants these attributes to actually be optional (and nullable) even though they have a default_value set.
Summary by CodeRabbit