Conversation
…umentation Document lessons learned from the enhanced search feature where missing value_lower on AttributeValueIndexed caused silent data integrity issues. - Add value_lower property and write path registry to database-schema.md - Add Neo4j gotchas section (NOT NULL, MERGE semantics, index bypass) - Add MERGE vs CREATE semantics to query-pattern.md - New guide: writing-migrations.md (IN TRANSACTIONS, idempotency) - New guide: adding-node-properties.md (checklist for new properties) - Update backend AGENTS.md with references to new guides Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
WalkthroughThis pull request adds comprehensive backend documentation covering Neo4j database operations. Two new guide documents are introduced: one describing the process for writing graph migrations and another detailing steps for adding new properties to existing Neo4j nodes. The knowledge base is expanded with additional details on the database schema, including a new 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
dev/guides/backend/adding-node-properties.md (1)
24-32: Use repo-root file paths consistently in the write-path table.At Line 26-Line 32, shortened paths (
core/...) are inconsistent with Step 1 commands (backend/infrahub/...) and can mislead copy/paste navigation.Proposed doc-only path normalization
-| `BaseAttribute.to_db()` | `core/attribute.py` | Normal CRUD (create/update node attributes) | -| `NodeCreateFullBatchQuery` | `core/query/node.py` | Batch node creation | -| `Resource pool allocation` | `core/query/resource_manager.py` | -| `Graph migrations` | `core/migrations/graph/m0*.py` | -| `Schema migrations` | `core/migrations/schema/*.py` | -| `Shared migration queries` | `core/migrations/query/*.py` | +| `BaseAttribute.to_db()` | `backend/infrahub/core/attribute.py` | Normal CRUD (create/update node attributes) | +| `NodeCreateFullBatchQuery` | `backend/infrahub/core/query/node.py` | Batch node creation | +| `Resource pool allocation` | `backend/infrahub/core/query/resource_manager.py` | +| `Graph migrations` | `backend/infrahub/core/migrations/graph/m0*.py` | +| `Schema migrations` | `backend/infrahub/core/migrations/schema/*.py` | +| `Shared migration queries` | `backend/infrahub/core/migrations/query/*.py` |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev/guides/backend/adding-node-properties.md` around lines 24 - 32, The table uses shortened paths like `core/attribute.py → to_db()` and `core/query/node.py → NodeCreateFullBatchQuery`, which is inconsistent with Step 1's repo-root style (e.g., `backend/infrahub/...`); update each entry in the write-path table to the full repo-root paths matching the commands in Step 1 (for example replace `core/attribute.py` with the full path under `backend/infrahub/...` and similarly expand `core/query/resource_manager.py`, `core/migrations/graph/m0*.py`, `core/migrations/schema/*.py`, and `core/migrations/query/*.py`) so copy/paste navigation is accurate and consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dev/knowledge/backend/database-schema.md`:
- Around line 409-410: Replace the asymptotic claim "O(1)-ish" near the WHERE
example that uses the TEXT index on value_lower; instead update the sentence to
say the condition is "index-backed and avoids full scans" (or similar
non-asymptotic phrasing) so the doc accurately reflects Neo4j query planning for
the WHERE av.value_lower CONTAINS $search example.
---
Nitpick comments:
In `@dev/guides/backend/adding-node-properties.md`:
- Around line 24-32: The table uses shortened paths like `core/attribute.py →
to_db()` and `core/query/node.py → NodeCreateFullBatchQuery`, which is
inconsistent with Step 1's repo-root style (e.g., `backend/infrahub/...`);
update each entry in the write-path table to the full repo-root paths matching
the commands in Step 1 (for example replace `core/attribute.py` with the full
path under `backend/infrahub/...` and similarly expand
`core/query/resource_manager.py`, `core/migrations/graph/m0*.py`,
`core/migrations/schema/*.py`, and `core/migrations/query/*.py`) so copy/paste
navigation is accurate and consistent.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/AGENTS.mddev/guides/backend/adding-node-properties.mddev/guides/backend/writing-migrations.mddev/knowledge/backend/database-schema.mddev/knowledge/backend/query-pattern.md
Replace "O(1)-ish" with "index-backed, avoids full scans" for the TEXT index CONTAINS example, which more accurately reflects Neo4j query planning behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ajtmccarty
left a comment
There was a problem hiding this comment.
I think this could be helpful, but I am not really sure. I am not confident that I can determine what context an LLM needs or when it will use that context
There was a problem hiding this comment.
I think this is a good guide for adding a new required property that needs to be backfilled, but it should make it clear that it is just for that case
we have added optional properties in the past that don't require a migration or backfilling and we just have optional handling to check if the property is set or set it to a default value when getting it from the db
There was a problem hiding this comment.
I'd say this is specifically for writing graph migrations, which the test kind of says implicitly
we also include "database" migrations where we make changes using a mix of cypher and Python or just using existing Python code, in which case this document might not apply
| - Queries that filter on a property will silently exclude nodes that lack it | ||
| - TEXT and RANGE indexes only index nodes that **have** the indexed property — a node without the property is invisible to index-backed queries | ||
|
|
||
| **Mitigation:** Maintain a write path registry (see [AttributeValueIndexed Write Paths](#attributevalueindexed-write-paths)) and audit all paths when adding properties. |
There was a problem hiding this comment.
this seems likely to be forgotten and get out of date, but perhaps LLMs can help us keep it up-to-date
(Following feature MVP in #8448)
Document lessons learned from the enhanced search feature, where missing value_lower on AttributeValueIndexed caused silent data integrity issues.
Summary by CodeRabbit