IFC-1681: Fix variable nested data#9282
Conversation
There was a problem hiding this comment.
No issues found across 4 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would auto-approve. This PR fixes a targeted GraphQL variable substitution bug by introducing a dedicated scalar that forwards _variables to recursive calls and resolves VariableNode, swapped only in two pool mutation input fields, with comprehensive tests confirming correct behavior and no changes to core logic...
Re-trigger cubic
There was a problem hiding this comment.
looks like the tests only cover the string path of the new FixedGenericScalar. it would be good if the tests covered all the paths. I think it would require creating a new NodeSchema that inherits from BuiltinIPPrefix and includes attributes that cover all of the various paths from the new logic: string, bool, float, etc.
| """ | ||
|
|
||
| @staticmethod | ||
| def parse_literal(ast: ValueNode, _variables: dict[str, Any] | None = None) -> Any: # noqa: PLR0911 |
There was a problem hiding this comment.
if you set a variable to return and then only return it at the end, you can get ride of the PLR0911 ignore
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Shadow auto-approve: would auto-approve. This change fixes a GraphQL variable resolution bug in two specific mutation inputs with a focused new scalar and comprehensive tests, having no broader system impact or risk to critical logic.
Re-trigger cubic
ajtmccarty
left a comment
There was a problem hiding this comment.
should the new FixedGenericScalar class be used throughout our graphql logic in place of the GenericScalar that does not support variables? probably worth a follow-up issue to track it
Why
graphene.GenericScalar.parse_literaldoes not forward_variablesto its recursive calls when processing nestedObjectValueNodeorListValueNodeAST nodes, and does not handleVariableNodeat all. As a result, any$variablereference inside thedatasub-object ofIPPrefixPoolGetResourceorIPAddressPoolGetResourceis silently coerced tonullinstead of its supplied value.Closes #6850
What changed
FixedGenericScalarinbackend/infrahub/graphql/scalars.py— aGenericScalarsubclass whoseparse_literalcorrectly forwards_variablesto recursive calls and resolvesVariableNodeby looking up the variable name in the provided map.IPPrefixPoolGetResourceInput.dataandIPAddressPoolGetResourceInput.datainresource_manager.pyswitched fromGenericScalartoFixedGenericScalar.How to test
uv run pytest backend/tests/component/graphql/resource_manager/test_resource_manager_data_variables.py -v # Full regression uv run pytest backend/tests/component/graphql/resource_manager/ -vChecklist
changelog/6850.fixed.md)Summary by cubic
Fixes GraphQL variable substitution inside the data field for pool allocation mutations. Addresses IFC-1681 by ensuring nested variables resolve instead of becoming null.
FixedGenericScalarto replacegraphene.GenericScalarparsing, forwarding_variablesand resolvingVariableNode.IPPrefixPoolGetResourceInput.dataandIPAddressPoolGetResourceInput.datatoFixedGenericScalar, and updated the GraphQL schema to expose this scalar and reflect the input changes.FixedGenericScalarand the new input field types.InfrahubIPPrefixPoolGetResourceandInfrahubIPAddressPoolGetResource.FixedGenericScalar.parse_literal, covering nested lists/objects, variables, and numeric bounds.Written for commit 21310e0. Summary will update on new commits. Review in cubic