-
Notifications
You must be signed in to change notification settings - Fork 37
Forbid converting objects with profiles #7504
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe pull request modifies the convert object type mutation to add validation for profile-based objects. The changes replace a generic ValueError with a ValidationError during fields_mapping validation and introduce a new runtime check to prevent converting objects whose source_schema attributes originate from profiles. The implementation adds appropriate imports for ValidationError and BaseAttribute. A corresponding unit test is added to verify the error handling when attempting to convert profile-based objects, ensuring the mutation properly rejects such conversions with a specific error message. Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
CodSpeed Performance ReportMerging #7504 will not alter performanceComparing Summary
|
3e3f479 to
eab9580
Compare
eab9580 to
650a2c6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/tests/unit/graphql/mutations/test_object_conversion.py (1)
18-82: Consider more robust error assertions.The test correctly validates the error scenario, but the error assertion at lines 78-82 uses string matching which could be fragile if the error message format changes.
Consider asserting on the error type or structure more explicitly:
assert result.errors assert len(result.errors) == 1 error_message = str(result.errors[0]) assert "is from a profile" in error_message assert "converting objects that use profiles is not yet supported" in error_messageThis makes the test more resilient to minor message wording changes while still validating the essential error content.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/infrahub/graphql/mutations/convert_object_type.py(2 hunks)backend/tests/unit/graphql/mutations/test_object_conversion.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/mutations/test_object_conversion.py
backend/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/tests/unit/graphql/mutations/test_object_conversion.pybackend/infrahub/graphql/mutations/convert_object_type.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/mutations/test_object_conversion.pybackend/infrahub/graphql/mutations/convert_object_type.py
🧬 Code graph analysis (2)
backend/tests/unit/graphql/mutations/test_object_conversion.py (6)
backend/infrahub/core/convert_object_type/object_conversion.py (1)
convert_object_type(146-201)backend/tests/helpers/graphql.py (1)
graphql(26-61)backend/infrahub/graphql/initialization.py (1)
prepare_graphql_params(79-125)backend/tests/conftest.py (1)
schemas_conversion(1264-1367)backend/infrahub/core/schema/manager.py (1)
register_schema(205-212)backend/infrahub/core/branch/models.py (1)
update_schema_hash(124-132)
backend/infrahub/graphql/mutations/convert_object_type.py (4)
backend/infrahub/exceptions.py (1)
ValidationError(319-341)backend/infrahub/core/manager.py (6)
get_one(899-913)get_one(917-931)get_one(935-949)get_one(953-967)get_one(971-985)get_one(988-1051)backend/infrahub/core/schema/basenode_schema.py (1)
attribute_names(379-380)backend/tests/functional/profiles/test_profiles.py (1)
is_from_profile(27-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
backend/infrahub/graphql/mutations/convert_object_type.py (2)
13-13: LGTM: Appropriate imports added.The
ValidationErrorimport and the TYPE_CHECKING import forBaseAttributeare both correctly added to support the new validation logic and type hints.Also applies to: 17-17
54-56: Good improvement: Using ValidationError for user-facing validation.Replacing
ValueErrorwithValidationErrorprovides better error handling for GraphQL API consumers with the appropriate HTTP 422 status code.backend/tests/unit/graphql/mutations/test_object_conversion.py (2)
1-16: LGTM: Imports are well-organized.The imports follow best practices with TYPE_CHECKING guards for type-only imports.
85-100: LGTM: Mutation definition is clear and reusable.The GraphQL mutation is well-defined as a constant, making the test more readable and maintainable.
| node_to_convert = await NodeManager.get_one( | ||
| id=str(data.node_id), db=graphql_context.db, branch=graphql_context.branch | ||
| ) | ||
| for attribute_name in source_schema.attribute_names: | ||
| attribute: BaseAttribute = getattr(node_to_convert, attribute_name) | ||
| if attribute.is_from_profile: | ||
| raise ValidationError( | ||
| input_value=f"The attribute '{attribute_name}' is from a profile, converting objects that use profiles is not yet supported." | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Remove duplicate node loading and consider relationships.
The node was already loaded at lines 45-47, so the second load at lines 61-63 is redundant and inefficient.
Additionally, the validation only checks attributes but not relationships. If relationships can also originate from profiles, they should be validated too.
Apply this diff to remove the duplicate loading and use the existing node_to_convert:
- node_to_convert = await NodeManager.get_one(
- id=str(data.node_id), db=graphql_context.db, branch=graphql_context.branch
- )
for attribute_name in source_schema.attribute_names:
attribute: BaseAttribute = getattr(node_to_convert, attribute_name)
if attribute.is_from_profile:To verify if relationships also need validation, run:
🏁 Script executed:
#!/bin/bash
# Description: Check if relationships have source_profile_id or is_from_profile properties
# Search for is_from_profile or source_profile_id in relationship-related classes
rg -nP --type=py -C5 'class.*Relationship.*:|def is_from_profile|source_profile_id' backend/infrahub/core/relationship/Length of output: 10323
---
🏁 Script executed:
```shell
#!/bin/bash
# Description: Get full context of the file to verify if node is loaded twice
# Show lines 40-75 of the file to see both potential load sites
sed -n '40,75p' backend/infrahub/graphql/mutations/convert_object_type.py
Length of output: 1902
Remove duplicate node loading.
The node is already loaded at lines 45-47 and used to get the source schema at line 50. The second load at lines 62-64 is redundant and should be removed to use the existing node_to_convert variable.
- node_to_convert = await NodeManager.get_one(
- id=str(data.node_id), db=graphql_context.db, branch=graphql_context.branch
- )
for attribute_name in source_schema.attribute_names:Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/infrahub/graphql/mutations/convert_object_type.py around lines 61 to
69, the code calls NodeManager.get_one a second time to load node_to_convert
even though the same node was already loaded at lines 45-47 and used to obtain
source_schema at line 50; remove the duplicate NodeManager.get_one call and use
the existing node_to_convert variable already in scope when iterating
source_schema.attribute_names, preserving the is_from_profile check and error
raise as-is.
Converting objects that use profiles is currently not supported, this change highlights this and provides a more user friendly error.
Summary by CodeRabbit
Bug Fixes