fix: add NonNegativeInt scalar and validate GraphQL pagination args #7610
Conversation
WalkthroughA new GraphQL scalar NonNegativeInt was added in backend/infrahub/graphql/types/scalars.py. Several query field arguments and pagination-like parameters (limit, offset, log_limit, log_offset) were changed from Int to NonNegativeInt across backend/infrahub/graphql/{queries,manager}. Unit tests and test queries were updated to use or assert NonNegativeInt behavior. A changelog entry documenting the scalar and validation changes was added. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (3)backend/tests/**/*📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
backend/**/*📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.py📄 CodeRabbit inference engine (.github/instructions/python-docstring.instructions.md)
Files:
🔇 Additional comments (1)
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: 2
🧹 Nitpick comments (2)
backend/infrahub/graphql/queries/branch.py (1)
42-45: Consider removing redundant offset validation.Since NonNegativeInt now validates that offset cannot be negative at the GraphQL layer, the runtime check at Line 44 is redundant. However, the limit >= 1 validation at Line 42 is still needed since NonNegativeInt allows 0.
Apply this diff to remove the redundant validation:
if isinstance(limit, int) and limit < 1: raise ValidationError("limit must be >= 1") -if isinstance(offset, int) and offset < 0: - raise ValidationError("offset must be >= 0")backend/infrahub/graphql/types/scalars.py (1)
19-26: Improve error handling for type conversion.Line 23 uses
int(value)which can raise aValueErrorif the value cannot be converted to an integer. While GraphQL's type system should typically prevent this, providing a clearer error message improves the user experience when invalid values are provided.Apply this diff to add better error handling:
@staticmethod def _validate(value): if value is None: return None - value = int(value) + try: + value = int(value) + except (ValueError, TypeError): + raise GraphQLError("Value must be a non-negative integer") if value < 0: raise GraphQLError("Value must be a non-negative integer") return value
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
backend/infrahub/graphql/manager.py(2 hunks)backend/infrahub/graphql/queries/account.py(2 hunks)backend/infrahub/graphql/queries/branch.py(2 hunks)backend/infrahub/graphql/queries/diff/tree.py(2 hunks)backend/infrahub/graphql/queries/event.py(2 hunks)backend/infrahub/graphql/queries/relationship.py(2 hunks)backend/infrahub/graphql/queries/resource_manager.py(2 hunks)backend/infrahub/graphql/queries/search.py(2 hunks)backend/infrahub/graphql/queries/task.py(2 hunks)backend/infrahub/graphql/types/scalars.py(1 hunks)backend/tests/unit/graphql/test_query.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
backend/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/infrahub/graphql/queries/resource_manager.pybackend/infrahub/graphql/queries/event.pybackend/infrahub/graphql/manager.pybackend/infrahub/graphql/types/scalars.pybackend/infrahub/graphql/queries/branch.pybackend/infrahub/graphql/queries/task.pybackend/tests/unit/graphql/test_query.pybackend/infrahub/graphql/queries/relationship.pybackend/infrahub/graphql/queries/account.pybackend/infrahub/graphql/queries/search.pybackend/infrahub/graphql/queries/diff/tree.py
**/*.py
📄 CodeRabbit inference engine (.github/instructions/python-docstring.instructions.md)
**/*.py: Use triple double quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include a brief one-line description at the top of each docstring
Add a detailed description section when additional context is needed
Document function/method parameters under an Args/Parameters section without typing information
Include a Returns section describing the return value
Include a Raises section listing possible exceptions
Provide an Examples section demonstrating usage when helpfulUse ruff and mypy to validate and lint Python files
**/*.py: Use type hints for all Python function parameters and return values
Prefer asynchronous code in Python when feasible
Define asynchronous Python functions withasync def
Useawaitfor asynchronous calls in Python
Use Pydantic models instead of standard dataclasses
Use ruff and mypy for linting and type checking
**/*.py: Use type hints for all Python function parameters and return values
Use async/await whenever possible in Python code
Define asynchronous functions withasync def
Await asynchronous calls withawait
Use Pydantic models instead of standard dataclasses for data modeling
Use triple quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include docstring sections when applicable: one-line summary, optional details, Args (without types), Returns, Raises, Examples
Validate and lint Python with ruff and mypy
Files:
backend/infrahub/graphql/queries/resource_manager.pybackend/infrahub/graphql/queries/event.pybackend/infrahub/graphql/manager.pybackend/infrahub/graphql/types/scalars.pybackend/infrahub/graphql/queries/branch.pybackend/infrahub/graphql/queries/task.pybackend/tests/unit/graphql/test_query.pybackend/infrahub/graphql/queries/relationship.pybackend/infrahub/graphql/queries/account.pybackend/infrahub/graphql/queries/search.pybackend/infrahub/graphql/queries/diff/tree.py
backend/tests/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place backend tests in
backend/tests/
Files:
backend/tests/unit/graphql/test_query.py
🧬 Code graph analysis (10)
backend/infrahub/graphql/queries/resource_manager.py (1)
backend/infrahub/graphql/types/scalars.py (1)
NonNegativeInt(4-26)
backend/infrahub/graphql/queries/event.py (1)
backend/infrahub/graphql/types/scalars.py (1)
NonNegativeInt(4-26)
backend/infrahub/graphql/manager.py (1)
backend/infrahub/graphql/types/scalars.py (1)
NonNegativeInt(4-26)
backend/infrahub/graphql/queries/branch.py (1)
backend/infrahub/graphql/types/scalars.py (1)
NonNegativeInt(4-26)
backend/infrahub/graphql/queries/task.py (1)
backend/infrahub/graphql/types/scalars.py (1)
NonNegativeInt(4-26)
backend/tests/unit/graphql/test_query.py (5)
backend/tests/benchmark/test_graphql_query.py (1)
default_branch(36-40)backend/tests/conftest.py (1)
register_core_models_schema(234-235)backend/infrahub/core/branch/models.py (1)
update_schema_hash(126-134)backend/infrahub/graphql/initialization.py (1)
prepare_graphql_params(79-125)backend/tests/helpers/graphql.py (1)
graphql(26-61)
backend/infrahub/graphql/queries/relationship.py (1)
backend/infrahub/graphql/types/scalars.py (1)
NonNegativeInt(4-26)
backend/infrahub/graphql/queries/account.py (1)
backend/infrahub/graphql/types/scalars.py (1)
NonNegativeInt(4-26)
backend/infrahub/graphql/queries/search.py (1)
backend/infrahub/graphql/types/scalars.py (1)
NonNegativeInt(4-26)
backend/infrahub/graphql/queries/diff/tree.py (1)
backend/infrahub/graphql/types/scalars.py (1)
NonNegativeInt(4-26)
🔇 Additional comments (20)
backend/infrahub/graphql/queries/event.py (2)
14-14: LGTM: NonNegativeInt import added correctly.The import is appropriately placed and required for the field type updates below.
100-101: LGTM: Pagination arguments updated to NonNegativeInt.The limit and offset fields now correctly use NonNegativeInt, which will reject negative values at the GraphQL layer before they reach the resolver or Neo4j.
backend/infrahub/graphql/queries/diff/tree.py (2)
21-21: LGTM: NonNegativeInt import added correctly.The import is appropriately placed and supports the pagination field updates.
591-592: LGTM: DiffTreeQuery pagination arguments updated to NonNegativeInt.The limit and offset fields now use NonNegativeInt, providing input validation at the GraphQL layer to prevent negative pagination values.
backend/infrahub/graphql/manager.py (2)
26-26: LGTM: NonNegativeInt import added correctly.The import is appropriately placed and required for the filter generation updates.
917-917: LGTM: Auto-generated filters updated to use NonNegativeInt.This change applies NonNegativeInt validation to all auto-generated offset and limit filters across the GraphQL schema, ensuring consistent pagination validation.
backend/infrahub/graphql/queries/task.py (2)
13-13: LGTM: NonNegativeInt import added correctly.The import is appropriately placed and supports the pagination field updates.
110-111: LGTM: All Task pagination arguments updated to NonNegativeInt.The limit, offset, log_limit, and log_offset fields now use NonNegativeInt, providing consistent non-negative validation across all pagination-related arguments.
Also applies to: 118-119
backend/infrahub/graphql/queries/account.py (2)
11-11: LGTM: NonNegativeInt import added correctly.The import is appropriately placed and required for the pagination field updates.
67-68: LGTM: AccountToken pagination arguments updated to NonNegativeInt.The limit and offset fields now use NonNegativeInt, preventing negative pagination values at the GraphQL layer.
backend/infrahub/graphql/queries/branch.py (2)
11-11: LGTM: NonNegativeInt import added correctly.The import is appropriately placed and supports the pagination field updates.
61-62: LGTM: InfrahubBranchQueryList pagination arguments updated to NonNegativeInt.The offset and limit fields now use NonNegativeInt, providing input validation at the GraphQL layer.
backend/tests/unit/graphql/test_query.py (2)
88-111: LGTM: Test correctly verifies negative limit rejection.The test properly validates that NonNegativeInt rejects negative limit values at the GraphQL layer, with appropriate error message and structure assertions.
113-136: LGTM: Test correctly verifies negative offset rejection.The test properly validates that NonNegativeInt rejects negative offset values at the GraphQL layer, with appropriate error message and structure assertions. Together with the negative limit test, this provides good coverage of the NonNegativeInt validation behavior.
backend/infrahub/graphql/queries/search.py (2)
13-13: LGTM: NonNegativeInt import added correctly.The import is appropriately placed and supports the pagination field update.
151-151: LGTM: InfrahubSearchAnywhere limit argument updated to NonNegativeInt.The limit field now uses NonNegativeInt, preventing negative values at the GraphQL layer, consistent with pagination validation improvements throughout the codebase.
backend/infrahub/graphql/queries/relationship.py (2)
10-10: LGTM!The import of
NonNegativeIntis correctly placed and follows the module's import structure.
80-81: LGTM!The replacement of
IntwithNonNegativeIntforlimitandoffsetparameters correctly enforces non-negative pagination values at the GraphQL layer, preventing invalid values from reaching the database query.backend/infrahub/graphql/queries/resource_manager.py (2)
21-22: LGTM!The import of
NonNegativeIntis correctly placed at the appropriate location in the imports section.
345-346: LGTM!The replacement of
IntwithNonNegativeIntforlimitandoffsetparameters correctly enforces non-negative pagination values, providing consistent validation across all resource pool allocation queries.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
backend/infrahub/graphql/manager.py(2 hunks)backend/infrahub/graphql/queries/branch.py(2 hunks)backend/infrahub/graphql/queries/diff/tree.py(2 hunks)backend/infrahub/graphql/queries/event.py(2 hunks)backend/infrahub/graphql/queries/resource_manager.py(3 hunks)backend/infrahub/graphql/queries/search.py(2 hunks)backend/infrahub/graphql/queries/task.py(2 hunks)backend/infrahub/graphql/types/scalars.py(1 hunks)backend/tests/unit/graphql/test_query.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/infrahub/graphql/queries/event.py
- backend/infrahub/graphql/queries/branch.py
- backend/infrahub/graphql/manager.py
🧰 Additional context used
📓 Path-based instructions (3)
backend/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/infrahub/graphql/queries/search.pybackend/infrahub/graphql/types/scalars.pybackend/infrahub/graphql/queries/resource_manager.pybackend/tests/unit/graphql/test_query.pybackend/infrahub/graphql/queries/diff/tree.pybackend/infrahub/graphql/queries/task.py
**/*.py
📄 CodeRabbit inference engine (.github/instructions/python-docstring.instructions.md)
**/*.py: Use triple double quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include a brief one-line description at the top of each docstring
Add a detailed description section when additional context is needed
Document function/method parameters under an Args/Parameters section without typing information
Include a Returns section describing the return value
Include a Raises section listing possible exceptions
Provide an Examples section demonstrating usage when helpfulUse ruff and mypy to validate and lint Python files
**/*.py: Use type hints for all Python function parameters and return values
Prefer asynchronous code in Python when feasible
Define asynchronous Python functions withasync def
Useawaitfor asynchronous calls in Python
Use Pydantic models instead of standard dataclasses
Use ruff and mypy for linting and type checking
**/*.py: Use type hints for all Python function parameters and return values
Use async/await whenever possible in Python code
Define asynchronous functions withasync def
Await asynchronous calls withawait
Use Pydantic models instead of standard dataclasses for data modeling
Use triple quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include docstring sections when applicable: one-line summary, optional details, Args (without types), Returns, Raises, Examples
Validate and lint Python with ruff and mypy
Files:
backend/infrahub/graphql/queries/search.pybackend/infrahub/graphql/types/scalars.pybackend/infrahub/graphql/queries/resource_manager.pybackend/tests/unit/graphql/test_query.pybackend/infrahub/graphql/queries/diff/tree.pybackend/infrahub/graphql/queries/task.py
backend/tests/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place backend tests in
backend/tests/
Files:
backend/tests/unit/graphql/test_query.py
🧬 Code graph analysis (6)
backend/infrahub/graphql/queries/search.py (1)
backend/infrahub/graphql/types/scalars.py (1)
NonNegativeInt(7-83)
backend/infrahub/graphql/types/scalars.py (1)
backend/infrahub/types.py (1)
Any(312-319)
backend/infrahub/graphql/queries/resource_manager.py (1)
backend/infrahub/graphql/types/scalars.py (1)
NonNegativeInt(7-83)
backend/tests/unit/graphql/test_query.py (4)
backend/tests/conftest.py (1)
register_core_models_schema(234-235)backend/infrahub/core/branch/models.py (1)
update_schema_hash(126-134)backend/infrahub/graphql/initialization.py (1)
prepare_graphql_params(79-125)backend/tests/helpers/graphql.py (1)
graphql(26-61)
backend/infrahub/graphql/queries/diff/tree.py (1)
backend/infrahub/graphql/types/scalars.py (1)
NonNegativeInt(7-83)
backend/infrahub/graphql/queries/task.py (1)
backend/infrahub/graphql/types/scalars.py (1)
NonNegativeInt(7-83)
🔇 Additional comments (5)
backend/infrahub/graphql/queries/search.py (1)
12-12: LGTM! Clean adoption of NonNegativeInt for pagination.The import and field type change correctly apply the new scalar to the
limitparameter, ensuring negative values are rejected at the GraphQL layer.Also applies to: 150-150
backend/infrahub/graphql/queries/diff/tree.py (1)
20-20: LGTM! Consistent application of NonNegativeInt.The import and field type changes for
limitandoffsetcorrectly enforce non-negative validation for diff tree pagination.Also applies to: 590-591
backend/tests/unit/graphql/test_query.py (1)
88-135: LGTM! Comprehensive validation tests.Both tests properly verify that negative
limitandoffsetvalues are rejected at the GraphQL layer with clear error messages. The async test structure follows coding guidelines and provides good coverage for the NonNegativeInt scalar behavior.backend/infrahub/graphql/queries/task.py (1)
9-9: LGTM! Complete pagination parameter validation.The import and field type changes consistently apply NonNegativeInt to all pagination-related parameters (
limit,offset,log_limit,log_offset), ensuring comprehensive validation for task queries.Also applies to: 109-110, 117-118
backend/infrahub/graphql/queries/resource_manager.py (1)
5-5: LGTM! Resource pool pagination secured.The import updates and field type changes for
limitandoffsetcorrectly apply NonNegativeInt validation to resource pool allocation queries.Also applies to: 19-19, 344-345
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
backend/tests/unit/graphql/test_query.py (2)
88-185: Consider parametrizing the test cases.The four new test functions follow nearly identical patterns and differ only in the parameter tested (limit/offset) and input value (-1/""). Parametrizing these tests would reduce duplication and improve maintainability.
Example parametrized approach:
@pytest.mark.parametrize( "param_name,param_value", [ ("limit", -1), ("offset", -1), ("limit", '""'), ("offset", '""'), ], ) async def test_builtin_tag_rejects_invalid_pagination_args( db: InfrahubDatabase, default_branch: Branch, register_core_models_schema, param_name: str, param_value: int | str, ) -> None: """Test that BuiltinTag query rejects invalid pagination arguments.""" default_branch.update_schema_hash() gql_params = await prepare_graphql_params(db=db, branch=default_branch) result = await graphql( schema=gql_params.schema, source=f""" query {{ BuiltinTag({param_name}: {param_value}) {{ count }} }} """, context_value=gql_params.context, ) assert result.data is None assert result.errors assert "non-negative integer" in result.errors[0].message assert result.errors[0].path is None
138-159: Clarify test naming for invalid type cases.The test names
test_builtin_tag_rejects_unknown_limitandtest_builtin_tag_rejects_unknown_offsettest empty string inputs, which are actually testing invalid type inputs (string instead of int), not "unknown" values. Consider more descriptive names liketest_builtin_tag_rejects_invalid_type_limitortest_builtin_tag_rejects_non_integer_limit.Also applies to: 162-184
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/infrahub/graphql/types/scalars.py(1 hunks)backend/tests/unit/graphql/test_query.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
backend/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/infrahub/graphql/types/scalars.pybackend/tests/unit/graphql/test_query.py
**/*.py
📄 CodeRabbit inference engine (.github/instructions/python-docstring.instructions.md)
**/*.py: Use triple double quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include a brief one-line description at the top of each docstring
Add a detailed description section when additional context is needed
Document function/method parameters under an Args/Parameters section without typing information
Include a Returns section describing the return value
Include a Raises section listing possible exceptions
Provide an Examples section demonstrating usage when helpfulUse ruff and mypy to validate and lint Python files
**/*.py: Use type hints for all Python function parameters and return values
Prefer asynchronous code in Python when feasible
Define asynchronous Python functions withasync def
Useawaitfor asynchronous calls in Python
Use Pydantic models instead of standard dataclasses
Use ruff and mypy for linting and type checking
**/*.py: Use type hints for all Python function parameters and return values
Use async/await whenever possible in Python code
Define asynchronous functions withasync def
Await asynchronous calls withawait
Use Pydantic models instead of standard dataclasses for data modeling
Use triple quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include docstring sections when applicable: one-line summary, optional details, Args (without types), Returns, Raises, Examples
Validate and lint Python with ruff and mypy
Files:
backend/infrahub/graphql/types/scalars.pybackend/tests/unit/graphql/test_query.py
backend/tests/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place backend tests in
backend/tests/
Files:
backend/tests/unit/graphql/test_query.py
🧬 Code graph analysis (1)
backend/tests/unit/graphql/test_query.py (5)
backend/tests/benchmark/test_graphql_query.py (1)
default_branch(36-40)backend/tests/conftest.py (1)
register_core_models_schema(234-235)backend/infrahub/core/branch/models.py (1)
update_schema_hash(126-134)backend/infrahub/graphql/initialization.py (1)
prepare_graphql_params(79-125)backend/tests/helpers/graphql.py (1)
graphql(26-61)
🔇 Additional comments (1)
backend/infrahub/graphql/types/scalars.py (1)
7-86: LGTM! Well-implemented NonNegativeInt scalar.The implementation correctly validates non-negative integers with proper error handling:
- None values are handled appropriately
- Type conversion errors (ValueError/TypeError) are caught and converted to GraphQLError
- Negative values are rejected with clear error messages
- parse_literal correctly restricts inputs to IntValueNode
- All methods have proper type hints and Google-style docstrings
The scalar provides robust validation for pagination arguments and will fail fast at the GraphQL layer before reaching downstream code.
| @pytest.mark.asyncio | ||
| async def test_builtin_tag_rejects_negative_limit( | ||
| db: InfrahubDatabase, | ||
| default_branch: Branch, | ||
| register_core_models_schema, | ||
| ) -> None: | ||
| default_branch.update_schema_hash() | ||
| gql_params = await prepare_graphql_params(db=db, branch=default_branch) | ||
|
|
||
| result = await graphql( | ||
| schema=gql_params.schema, | ||
| source=""" | ||
| query { | ||
| BuiltinTag(limit: -1) { count } | ||
| } | ||
| """, | ||
| context_value=gql_params.context, | ||
| ) | ||
|
|
||
| assert result.data is None | ||
| assert result.errors | ||
| assert "non-negative integer" in result.errors[0].message | ||
| assert result.errors[0].path is None | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add docstrings to all new test functions.
All four new test functions lack docstrings. Per coding guidelines, all Python functions require Google-style docstrings with at least a brief one-line description.
Apply this diff to add a docstring:
@pytest.mark.asyncio
async def test_builtin_tag_rejects_negative_limit(
db: InfrahubDatabase,
default_branch: Branch,
register_core_models_schema,
) -> None:
+ """Test that BuiltinTag query rejects negative limit values."""
default_branch.update_schema_hash()Apply similar docstrings to the other three test functions (lines 113-136, 137-160, 162-185).
As per coding guidelines.
📝 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.
| @pytest.mark.asyncio | |
| async def test_builtin_tag_rejects_negative_limit( | |
| db: InfrahubDatabase, | |
| default_branch: Branch, | |
| register_core_models_schema, | |
| ) -> None: | |
| default_branch.update_schema_hash() | |
| gql_params = await prepare_graphql_params(db=db, branch=default_branch) | |
| result = await graphql( | |
| schema=gql_params.schema, | |
| source=""" | |
| query { | |
| BuiltinTag(limit: -1) { count } | |
| } | |
| """, | |
| context_value=gql_params.context, | |
| ) | |
| assert result.data is None | |
| assert result.errors | |
| assert "non-negative integer" in result.errors[0].message | |
| assert result.errors[0].path is None | |
| @pytest.mark.asyncio | |
| async def test_builtin_tag_rejects_negative_limit( | |
| db: InfrahubDatabase, | |
| default_branch: Branch, | |
| register_core_models_schema, | |
| ) -> None: | |
| """Test that BuiltinTag query rejects negative limit values.""" | |
| default_branch.update_schema_hash() | |
| gql_params = await prepare_graphql_params(db=db, branch=default_branch) | |
| result = await graphql( | |
| schema=gql_params.schema, | |
| source=""" | |
| query { | |
| BuiltinTag(limit: -1) { count } | |
| } | |
| """, | |
| context_value=gql_params.context, | |
| ) | |
| assert result.data is None | |
| assert result.errors | |
| assert "non-negative integer" in result.errors[0].message | |
| assert result.errors[0].path is None |
🤖 Prompt for AI Agents
In backend/tests/unit/graphql/test_query.py around lines 88 to 111 (and
similarly for the other three new test functions at 113-136, 137-160, and
162-185), the test functions are missing Google-style docstrings; add a one-line
Google-style docstring (brief description in triple quotes) immediately after
each async def line for each of the four test functions to satisfy the coding
guidelines (e.g., """Test that BuiltinTag rejects negative limit values."""),
keeping the docstring concise and following Google-style formatting.
| assert "Unable to find the CoreGraphQLQuery" in str(exc.value) | ||
|
|
||
|
|
||
| @pytest.mark.asyncio |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify the necessity of @pytest.mark.asyncio decorator.
The four new test functions use @pytest.mark.asyncio but existing async tests in this file (lines 60, 80) don't use this decorator. If pytest-asyncio is configured with asyncio_mode = auto in pytest configuration, this decorator is redundant.
Run this script to check the pytest configuration:
Also applies to: 113-113, 137-137, 162-162
🏁 Script executed:
#!/bin/bash
# Check pytest configuration for asyncio_mode setting
fd -e toml -e ini -e cfg . | xargs grep -l "asyncio_mode"Length of output: 14
Now I need to check the actual asyncio_mode value in the pyproject.toml file:
Let me check the actual pyproject.toml content directly:
🏁 Script executed:
#!/bin/bash
# Check the pytest configuration in pyproject.toml for asyncio_mode
cat pyproject.toml | grep -A 20 "\[tool.pytest.ini_options\]"Length of output: 712
Now let me verify the actual test functions to confirm the decorator inconsistency:
---
🏁 Script executed:
```shell
#!/bin/bash
# Check the test functions at the specified lines to confirm decorator usage
sed -n '55,90p; 75,85p; 85,165p' backend/tests/unit/graphql/test_query.py | head -100
Length of output: 3203
🏁 Script executed:
#!/bin/bash
# Get lines around the existing tests and new tests
rg -n "async def test_" backend/tests/unit/graphql/test_query.py -B 1 | head -50Length of output: 580
Remove redundant @pytest.mark.asyncio decorators to match file style.
With asyncio_mode = "auto" in pyproject.toml, the @pytest.mark.asyncio decorator is redundant. Existing async tests in this file (lines 60, 80) omit it; the new tests at lines 88, 113, 137, and 162 should do the same for consistency.
🤖 Prompt for AI Agents
In backend/tests/unit/graphql/test_query.py around line 88 (and similarly at the
new async tests on lines 113, 137 and 162), remove the redundant
@pytest.mark.asyncio decorators so the async tests rely on the project-wide
asyncio_mode = "auto" setting and match the existing file style; simply delete
the decorator lines above those test functions and leave the async def test...
functions unchanged.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/tests/unit/graphql/test_query.py (1)
88-111: DRY the four tests via parametrization (limit/offset × values).Reduce duplication and keep future additions simple by parametrizing arg/value pairs and reusing a tiny helper to build the query. Optional, but keeps this suite compact.
-async def test_builtin_tag_rejects_negative_limit(...): ... -async def test_builtin_tag_rejects_negative_offset(...): ... -async def test_builtin_tag_rejects_unknown_limit(...): ... -async def test_builtin_tag_rejects_unknown_offset(...): ... +import pytest + +@pytest.mark.parametrize( + "arg,value_literal", + [("limit", "-1"), ("offset", "-1")], +) +async def test_builtin_tag_rejects_negative_pagination_arg( + db: InfrahubDatabase, default_branch: Branch, register_core_models_schema, arg: str, value_literal: str +) -> None: + """Test BuiltinTag rejects negative pagination values.""" + default_branch.update_schema_hash() + gql_params = await prepare_graphql_params(db=db, branch=default_branch) + query = f""" + query {{ + BuiltinTag({arg}: {value_literal}) {{ count }} + }} + """ + result = await graphql(schema=gql_params.schema, source=query, context_value=gql_params.context) + assert result.data is None + assert result.errors and any("non-negative integer" in e.message for e in result.errors) + assert all(e.path is None for e in result.errors) + +@pytest.mark.parametrize( + "arg,value_literal", + [("limit", '""'), ("offset", '""')], +) +async def test_builtin_tag_rejects_non_numeric_pagination_arg( + db: InfrahubDatabase, default_branch: Branch, register_core_models_schema, arg: str, value_literal: str +) -> None: + """Test BuiltinTag rejects non-numeric pagination values.""" + default_branch.update_schema_hash() + gql_params = await prepare_graphql_params(db=db, branch=default_branch) + query = f""" + query {{ + BuiltinTag({arg}: {value_literal}) {{ count }} + }} + """ + result = await graphql(schema=gql_params.schema, source=query, context_value=gql_params.context) + assert result.data is None + assert result.errors and any("non-negative integer" in e.message for e in result.errors) + assert all(e.path is None for e in result.errors)Optional: consider using the test helper (backend/tests/helpers/graphql.py) for richer error logs when failures occur. Based on learnings.
Also applies to: 113-136, 137-160, 162-185
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/tests/unit/graphql/test_query.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
backend/tests/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place backend tests in
backend/tests/
Files:
backend/tests/unit/graphql/test_query.py
backend/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/tests/unit/graphql/test_query.py
**/*.py
📄 CodeRabbit inference engine (.github/instructions/python-docstring.instructions.md)
**/*.py: Use triple double quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include a brief one-line description at the top of each docstring
Add a detailed description section when additional context is needed
Document function/method parameters under an Args/Parameters section without typing information
Include a Returns section describing the return value
Include a Raises section listing possible exceptions
Provide an Examples section demonstrating usage when helpfulUse ruff and mypy to validate and lint Python files
**/*.py: Use type hints for all Python function parameters and return values
Prefer asynchronous code in Python when feasible
Define asynchronous Python functions withasync def
Useawaitfor asynchronous calls in Python
Use Pydantic models instead of standard dataclasses
Use ruff and mypy for linting and type checking
**/*.py: Use type hints for all Python function parameters and return values
Use async/await whenever possible in Python code
Define asynchronous functions withasync def
Await asynchronous calls withawait
Use Pydantic models instead of standard dataclasses for data modeling
Use triple quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include docstring sections when applicable: one-line summary, optional details, Args (without types), Returns, Raises, Examples
Validate and lint Python with ruff and mypy
Files:
backend/tests/unit/graphql/test_query.py
🧠 Learnings (2)
📚 Learning: 2025-10-09T13:23:41.482Z
Learnt from: CR
Repo: opsmill/infrahub PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T13:23:41.482Z
Learning: Applies to **/*.py : Use async/await whenever possible in Python code
Applied to files:
backend/tests/unit/graphql/test_query.py
📚 Learning: 2025-10-07T11:28:41.769Z
Learnt from: CR
Repo: opsmill/infrahub PR: 0
File: .github/instructions/python.instructions.md:0-0
Timestamp: 2025-10-07T11:28:41.769Z
Learning: Applies to **/*.py : Prefer asynchronous code in Python when feasible
Applied to files:
backend/tests/unit/graphql/test_query.py
🧬 Code graph analysis (1)
backend/tests/unit/graphql/test_query.py (4)
backend/tests/conftest.py (1)
register_core_models_schema(234-235)backend/infrahub/core/branch/models.py (1)
update_schema_hash(126-134)backend/infrahub/graphql/initialization.py (1)
prepare_graphql_params(79-125)backend/tests/helpers/graphql.py (1)
graphql(26-61)
🔇 Additional comments (4)
backend/tests/unit/graphql/test_query.py (4)
88-111: Negative limit validation test — LGTMCovers scalar coercion failure path cleanly; async style and docstring align with guidelines.
113-136: Negative offset validation test — LGTMGood assertion set (data None, error present, path None).
137-160: Empty-string limit rejection — LGTMSolid coverage for non-numeric literal; keeps error message check resilient via substring.
162-185: Empty-string offset rejection — LGTMConsistent with the other cases; clear and focused.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/tests/unit/graphql/test_query.py (2)
88-185: Consider parametrizing tests to reduce duplication.All four test functions follow an identical pattern. Consider using
pytest.mark.parametrizeto consolidate them into a single parameterized test function, improving maintainability.Example refactoring:
@pytest.mark.parametrize("param_name,param_value", [ ("limit", -1), ("offset", -1), ("limit", '""'), ("offset", '""'), ]) async def test_builtin_tag_rejects_invalid_pagination_args( db: InfrahubDatabase, default_branch: Branch, register_core_models_schema, param_name: str, param_value: int | str, ) -> None: """Test that BuiltinTag query rejects invalid pagination arguments.""" default_branch.update_schema_hash() gql_params = await prepare_graphql_params(db=db, branch=default_branch) result = await graphql( schema=gql_params.schema, source=f""" query {{ BuiltinTag({param_name}: {param_value}) {{ count }} }} """, context_value=gql_params.context, ) assert result.data is None assert result.errors assert "non-negative integer" in result.errors[0].message assert result.errors[0].path is None
88-185: Consider adding complementary positive test cases.The test suite currently only verifies that invalid values are rejected. Consider adding test cases that verify valid non-negative integers (e.g.,
0,10,100) are accepted for bothlimitandoffsetto ensure the scalar doesn't over-restrict valid inputs.Example:
async def test_builtin_tag_accepts_valid_pagination_args( db: InfrahubDatabase, default_branch: Branch, register_core_models_schema, ) -> None: """Test that BuiltinTag query accepts valid non-negative pagination arguments.""" default_branch.update_schema_hash() gql_params = await prepare_graphql_params(db=db, branch=default_branch) result = await graphql( schema=gql_params.schema, source=""" query { BuiltinTag(limit: 10, offset: 0) { count } } """, context_value=gql_params.context, ) assert result.errors is None assert result.data is not None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/infrahub/graphql/types/scalars.py(1 hunks)backend/tests/unit/graphql/test_query.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/infrahub/graphql/types/scalars.py
🧰 Additional context used
📓 Path-based instructions (3)
backend/tests/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place backend tests in
backend/tests/
Files:
backend/tests/unit/graphql/test_query.py
backend/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/tests/unit/graphql/test_query.py
**/*.py
📄 CodeRabbit inference engine (.github/instructions/python-docstring.instructions.md)
**/*.py: Use triple double quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include a brief one-line description at the top of each docstring
Add a detailed description section when additional context is needed
Document function/method parameters under an Args/Parameters section without typing information
Include a Returns section describing the return value
Include a Raises section listing possible exceptions
Provide an Examples section demonstrating usage when helpfulUse ruff and mypy to validate and lint Python files
**/*.py: Use type hints for all Python function parameters and return values
Prefer asynchronous code in Python when feasible
Define asynchronous Python functions withasync def
Useawaitfor asynchronous calls in Python
Use Pydantic models instead of standard dataclasses
Use ruff and mypy for linting and type checking
**/*.py: Use type hints for all Python function parameters and return values
Use async/await whenever possible in Python code
Define asynchronous functions withasync def
Await asynchronous calls withawait
Use Pydantic models instead of standard dataclasses for data modeling
Use triple quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include docstring sections when applicable: one-line summary, optional details, Args (without types), Returns, Raises, Examples
Validate and lint Python with ruff and mypy
Files:
backend/tests/unit/graphql/test_query.py
🧠 Learnings (2)
📚 Learning: 2025-10-09T13:23:41.482Z
Learnt from: CR
Repo: opsmill/infrahub PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T13:23:41.482Z
Learning: Applies to **/*.py : Use async/await whenever possible in Python code
Applied to files:
backend/tests/unit/graphql/test_query.py
📚 Learning: 2025-10-07T11:28:41.769Z
Learnt from: CR
Repo: opsmill/infrahub PR: 0
File: .github/instructions/python.instructions.md:0-0
Timestamp: 2025-10-07T11:28:41.769Z
Learning: Applies to **/*.py : Prefer asynchronous code in Python when feasible
Applied to files:
backend/tests/unit/graphql/test_query.py
🧬 Code graph analysis (1)
backend/tests/unit/graphql/test_query.py (5)
backend/tests/benchmark/test_graphql_query.py (1)
default_branch(36-40)backend/tests/conftest.py (1)
register_core_models_schema(234-235)backend/infrahub/core/branch/models.py (1)
update_schema_hash(126-134)backend/infrahub/graphql/initialization.py (1)
prepare_graphql_params(79-125)backend/tests/helpers/graphql.py (1)
graphql(26-61)
🔇 Additional comments (4)
backend/tests/unit/graphql/test_query.py (4)
88-111: LGTM! Docstring added and test logic is sound.The test correctly validates that negative limit values are rejected by the NonNegativeInt scalar. The docstring has been added per the previous review feedback, and all assertions appropriately verify validation failure.
113-136: LGTM! Consistent validation for offset parameter.The test correctly validates that negative offset values are rejected, mirroring the limit validation logic. The docstring follows guidelines.
138-161: LGTM! Non-integer validation works correctly.The test properly validates that non-integer values (empty string) are rejected for the limit parameter with appropriate error messaging.
163-185: LGTM! Completes the validation test suite.The test correctly validates that non-integer values are rejected for the offset parameter, completing coverage of the four validation scenarios.
ogenstad
left a comment
ogenstad
left a comment
There was a problem hiding this comment.
Thank you for picking this up! I added a small comment about the exceptions being raised. Also we use towncrier when applicable to indicate what we've changed.
Can you create a towncrier newsfragment and include with this PR?
It's done by running towncrier create and when prompted for the issue you indicate the issue ID of the bug this is targeting and then select "fixed" as the type of change. Then just write what it was that was updated.
| from typing import Any | ||
|
|
||
| from graphene import Scalar | ||
| from graphql import GraphQLError, language |
There was a problem hiding this comment.
Raising a GraphQLError in this case will hide some context of the query, while the change isn't significant if you instead had:
from infrahub.exceptions import ValidationErrorAnd raised a ValidationError some additional context with regards to where the error occurred would be provided as feedback to the user.
There was a problem hiding this comment.
Thanks @ogenstad for the feedback!
I chose GraphQLError to stay consistent with Graphene’s internal error handling (e.g. graphene.Int()) allowing the executor to automatically attach locations and AST nodes and ensuring a GraphQL-compliant error structure.
My hesitation was with ValidationError is that it would surface a domain-specific exception into the Graphene layer, which I wanted to avoid.
That said, I am happy to switch if you prefer — I will follow your lead here.
There was a problem hiding this comment.
The reasoning in this case is around what the GraphQL response would look like, if we look at a query that looks like this:
query MyQuery {
BuiltinTag(limit: -1, name__value: "red") {
edges {
node {
name {
value
}
}
}
}
}If we raise a GraphQLError the response will be:
{
"data": null,
"errors": [
{
"message": "Value must be a non-negative integer"
}
]
}If we instead raise a Validation error the output would be:
{
"data": null,
"errors": [
{
"message": "Expected value of type 'NonNegativeInt', found -1; Value must be a non-negative integer",
"locations": [
{
"line": 2,
"column": 21
}
]
}
]
}Which while perhaps not pretty is more helpful as there's at least a chance for the user to track down where the error is happening within the provided query. It's also more in line with what Graphene would return if we have a type violation of the builtin types such as for a query like this:
query MyQuery {
BuiltinTag(name__value: -1) {
edges {
node {
name {
value
}
}
}
}
}Where the response would be:
{
"data": null,
"errors": [
{
"message": "String cannot represent a non string value: -1",
"locations": [
{
"line": 2,
"column": 27
}
]
}
]
}There was a problem hiding this comment.
Thanks @ogenstad, it makes sense. Changed it to ValidationError and added changelog with towncrier accordingly.
CodSpeed Performance ReportMerging #7610 will not alter performanceComparing Summary
|
2 participants
Fixing #7474 with new scalar class called NonNegativeInt()
Summary by CodeRabbit
-
- Pagination arguments (limit, offset and related paging params) now enforce non-negative integers across GraphQL endpoints, improving validation and error clarity.
-
- Introduced a NonNegativeInt scalar to standardize non-negative integer validation for GraphQL inputs.
-
- Added unit tests verifying negative or invalid pagination inputs are rejected with "non-negative integer" validation errors.
by CodeRabbitImprovements
New Features
Tests
Tested