display labels aggregation bug #7023
Conversation
github-actions
bot
added
the
group/backend
WalkthroughAdds a unit test covering GenericSchema integration with display label field generation, a changelog entry documenting the display-label bug fix, and updates the tracked commit of the python_sdk submodule. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (3)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
changelog/7022.fixed.md (1)
1-1: Polish changelog fragment for clarity, consistency, and punctuationUse code formatting for the API field name, be explicit about “node schemas inheriting from the same generic,” and end the sentence with a period.
-Fix bug in display label rendering that prevented schemas from defining display labels with the same attribute names in different ways (`name` vs `name__value`, for example) +Fix `display_label` retrieval failing when node schemas inheriting from the same generic define the same attribute in both forms (`name` and `name__value`).backend/tests/unit/core/test_schema.py (1)
118-118: Add return type and a concise docstring per guidelinesPer the project’s Python guidelines, add a return type hint and a short Google-style docstring.
Apply this diff:
-async def test_node_schema_generate_fields_for_display_label_with_generic(default_branch: Branch): +async def test_node_schema_generate_fields_for_display_label_with_generic(default_branch: Branch) -> None: + """Validate merging of display label fields across generic inheritors. + + Args: + default_branch: Branch fixture used to scope a temporary SchemaBranch. + + Returns: + None + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/tests/unit/core/test_schema.py(3 hunks)changelog/7022.fixed.md(1 hunks)python_sdk(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{md,mdx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.{md,mdx}: Lint markdown files using markdownlint with.markdownlint.yamlconfiguration
Documentation content must be accurate, reflect the latest version of Infrahub, and have correct markdown formatting, spelling, and grammar
Files:
changelog/7022.fixed.md
backend/tests/**/*
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Place backend tests in
backend/tests/
Files:
backend/tests/unit/core/test_schema.py
backend/**/*
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/tests/unit/core/test_schema.py
**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.py: Use type hints for all function parameters and return values in Python files
Use async/await whenever possible in Python code
Useasync deffor asynchronous functions in Python
Useawaitfor asynchronous calls in Python
Use Pydantic models for dataclasses in Python
All Python functions must have docstrings
Always use triple quotes (""") for docstrings in Python
Follow Google-style docstring format in Python files
Include sections in Python docstrings when applicable: Brief one-line description, Detailed description, Args/Parameters (without typing), Returns, Raises, Examples
Use ruff and mypy for type checking and validations in Python files
Format all Python files usingpoetry run invoke format
Validate Python formatting and linting usingpoetry run invoke lint
Files:
backend/tests/unit/core/test_schema.py
⏰ 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). (10)
- GitHub Check: E2E-testing-version-upgrade / From 1.1.0
- GitHub Check: documentation
- GitHub Check: validate-generated-documentation
- GitHub Check: E2E-testing-invoke-demo-start
- GitHub Check: backend-docker-integration
- GitHub Check: backend-benchmark
- GitHub Check: E2E-testing-playwright
- GitHub Check: backend-tests-functional
- GitHub Check: backend-tests-unit
- GitHub Check: backend-tests-integration
🔇 Additional comments (5)
python_sdk (1)
1-1: ✅ Submodule bump verified
python_sdkis pinned to commit d5ac4abfb3fa7889a366b27a31c579a4b301d078 in the superproject tree.- Submodule HEAD at that commit shows the “handle None comparison in deep_merge_dict” change, which matches the fix from opsmill/infrahub-sdk-python#494.
- Working tree is clean with no local modifications in the submodule.
No further action required.
backend/tests/unit/core/test_schema.py (4)
7-7: Importing Branch for fixture typing is correctThe import is necessary for the
default_branch: Branchannotation and keeps mypy happy.
20-20: Importing GenericSchema is appropriateRequired for constructing the generic used in the new test; looks good.
148-149: Assertion matches the intended bugfix behaviorAsserting the union with normalized property access ({'name': {'value': None}}) ensures the deep-merge regression is caught. Good coverage for the None-vs-dict merge case.
145-147: Verifydefault_branchfixture isolation and consider localSchemaBranchThe test under
backend/tests/unit/core/test_schema.pycurrently mutates the global registry:registry.schema.register_schema(schema=schema_root, branch=default_branch.name) schema_branch = registry.schema.get_schema_branch(name=default_branch.name)Mutating
registry.schemacan leak state across tests. Please verify:
- Where and how the
default_branchfixture is implemented (e.g. inbackend/tests/conftest.pyor a plugin).- That it resets
registry.schema._branchesandregistry.schema._cache(or the entire manager) between test runs.If the fixture does not clear the registry, replace the global mutation with a local
SchemaBranchto isolate state:- registry.schema.register_schema(schema=schema_root, branch=default_branch.name) - schema_branch = registry.schema.get_schema_branch(name=default_branch.name) + schema_branch = SchemaBranch(cache={}, name=default_branch.name) + schema_branch.load_schema(schema=schema_root)This ensures the test only affects its own
SchemaBranchand avoids global side-effects.
CodSpeed Performance ReportMerging #7023 will improve performances by 21.8%Comparing Summary
Benchmarks breakdown
|
3 participants
fixes #7022
depends on opsmill/infrahub-sdk-python#494
fixed bug in
deep_merge_dict(used bygenerate_fields_for_display_label) that prevented merging a dictionary like{"name": None}with{"name": {"value": None}}Summary by CodeRabbit