From ca2190d08c892a727d81c0ada2cd938158701f2a Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 07:47:04 +0000 Subject: [PATCH] docs: harvest review lessons from PRs reviewed 2026-07-31 to 2026-08-07 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A harvesting-review run over the week's review threads, restricted to the PRs the in-flight harvests (#10145, #10098) did not read: open and recently-merged PRs across develop, stable, and release-1.11. New rules and knowledge: - mutations.md: retry_db_transaction placement — wrap only rollback-able transaction scopes, skip under a caller-supplied transaction (#10121) - query-pattern.md: READ queries with insert_limit=False need their own LIMIT; auto-paginated reads need a total-order ORDER BY (#10132) - creating-migrations.md: fix data bugs at the migration layer, not in runtime save paths (#10105); per-item error collection (#10132); batch by the memory-bounding unit (#10132) - database-schema.md: generic kinds exist only as labels — n.kind never matches a generic; type concrete-only inputs as list[NodeSchema] (#9805) - events.md: changelog models mask secrets only at construction — post-hoc assignment leaks them into events (#10105) - async-tasks.md: InfrahubBatch is concurrent, not ordered (#10113) - testing.md + testing-python.md rules: wiring tests parse source with ast/inspect instead of instrumenting production code (#10121); poll-don't-sleep for async effects (#10133); branch-attributable removal assertions (#10132); pure-function extraction before skipping the cheap test tier (#10137) - checklist.md: new Settings fields must reach both compose entry points — one generated and CI-checked, one hand-maintained (#10122) - backend/AGENTS.md: new REST endpoints are ask-first — prefer existing GraphQL for SDK needs (#8594); route creating-migrations.md in Guides - pre-ci.md: invoke lint tasks run ruff check --diff, which exits 0 on unfixable violations — added the plain CI-parity ruff check (#10146) - development/grafana/AGENTS.md (new): only defined datasource variables, sweep drill-down links, regenerate the standalone compose (#10153) - docs/AGENTS.md: Excalidraw exports use an opaque white background (#9953) Strengthened in place: - creating-changelog-entries skill: removed the 'most maintenance still gets a fragment' push that contradicted the no-user-facing-effect boundary; added lint/typing config cleanups as a named example (#10146) Paid for by compressing testing.md's nested-dataclass example and query-pattern.md's duplicated accessor/return-properties sections. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01LYisRTn3sPygP55cxbVqzG --- .agents/commands/pre-ci.md | 12 ++-- .agents/rules/testing-python.md | 6 +- .../creating-changelog-entries/SKILL.md | 7 ++- backend/AGENTS.md | 4 ++ dev/guidelines/backend/checklist.md | 13 ++++ dev/guidelines/backend/testing.md | 62 +++++++------------ dev/guides/backend/creating-migrations.md | 18 ++++++ dev/knowledge/backend/async-tasks.md | 8 +++ dev/knowledge/backend/database-schema.md | 9 ++- dev/knowledge/backend/events.md | 8 +++ dev/knowledge/backend/mutations.md | 16 +++++ dev/knowledge/backend/query-pattern.md | 55 ++++------------ development/grafana/AGENTS.md | 12 ++++ docs/AGENTS.md | 4 +- 14 files changed, 143 insertions(+), 91 deletions(-) create mode 100644 development/grafana/AGENTS.md diff --git a/.agents/commands/pre-ci.md b/.agents/commands/pre-ci.md index 3af5d768a5e..d99cc72f373 100644 --- a/.agents/commands/pre-ci.md +++ b/.agents/commands/pre-ci.md @@ -51,11 +51,12 @@ Auto-fixes formatting and lint issues in TypeScript/TSX files. If Biome reports ## Phase 2 — Fast checks (parallel) -**IMPORTANT: Send ALL 3 commands below in a SINGLE message with 3 parallel Bash tool calls.** Do NOT run them one at a time. +**IMPORTANT: Send ALL 4 commands below in a SINGLE message with 4 parallel Bash tool calls.** Do NOT run them one at a time. -1. `uv run invoke main.lint` — If ruff reports issues, they were not auto-fixable — report them to the user. -2. `uv lock --check` — Ensures `uv.lock` matches `pyproject.toml`. If this fails, run `uv lock` and commit the updated lockfile. -3. `cd frontend/app && npm run codegen:graphql` — Regenerates `graphql-env.d.ts` and `graphql-cache.d.ts` from `schema/schema.graphql`. If the files change, they need to be staged and committed. +1. `uv run invoke main.lint` — If ruff reports issues, report them to the user. +2. `uv run ruff check . --exclude python_sdk` — The exact command CI's `python-lint` job runs. This is not redundant with `main.lint`: the invoke tasks run `ruff check --diff`, which exits 0 on violations that have no auto-fix (e.g. `BLE001`), so only the plain check proves CI will pass. +3. `uv lock --check` — Ensures `uv.lock` matches `pyproject.toml`. If this fails, run `uv lock` and commit the updated lockfile. +4. `cd frontend/app && npm run codegen:graphql` — Regenerates `graphql-env.d.ts` and `graphql-cache.d.ts` from `schema/schema.graphql`. If the files change, they need to be staged and committed. --- @@ -67,7 +68,7 @@ Auto-fixes formatting and lint issues in TypeScript/TSX files. If Biome reports **IMPORTANT: Send ALL 7 commands below in a SINGLE message with 7 parallel Bash tool calls.** Do NOT run them one at a time. -1. `uv run invoke backend.lint` — Run separately from main.lint to avoid `uv run invoke lint` which includes a `yamllint -s .` step that fails on vendored packages in `.venv`. If ruff reports issues, they were not auto-fixable — report them to the user. +1. `uv run invoke backend.lint` — Run separately from main.lint to avoid `uv run invoke lint` which includes a `yamllint -s .` step that fails on vendored packages in `.venv`. Its ruff step shares the `--diff` blind spot noted in Phase 2; the ty/mypy output is what this check adds. 2. `cd frontend/app && npx betterer` — Ensures no new TypeScript errors are introduced. The issue count must stay the same or decrease. If it increases, report the new issues to the user. 3. `uv run invoke docs.lint` — Report any errors. Note: some pre-existing errors in `docs/docs/` may exist — only flag errors in files the user has changed. 4. `uv run invoke backend.validate-generated` — Ensures generated schema and protocol files are up to date. If this fails, run `uv run invoke backend.generate` and report the regenerated files. @@ -95,6 +96,7 @@ Summarize results in a table: | Docs format | ... | | Frontend format/lint | ... | | Main Python lint | ... | +| Ruff (CI parity) | ... | | Lockfile sync | ... | | Frontend GraphQL types | ... | | Backend lint (ty/mypy) | ... | diff --git a/.agents/rules/testing-python.md b/.agents/rules/testing-python.md index fe47e8954b3..a864f430f9d 100644 --- a/.agents/rules/testing-python.md +++ b/.agents/rules/testing-python.md @@ -63,7 +63,11 @@ Skip tests that only exercise library behavior: plain `Enum` value/round-trip ch ## Pick the cheapest test tier -If the logic needs only in-memory inputs (a `SchemaBranch`, a dataclass, a pure function), write a unit test without DB fixtures — don't default to a component test because a neighbor uses one. Use the database or containers only when behavior genuinely depends on them. +If the logic needs only in-memory inputs (a `SchemaBranch`, a dataclass, a pure function), write a unit test without DB fixtures — don't default to a component test because a neighbor uses one. Use the database or containers only when behavior genuinely depends on them. When the changed logic seems to need the full integration fixture, first check whether it can be extracted as a pure function over directly-constructible data and unit-tested there. + +## Wiring tests parse source, never instrument it + +Never add a marker, attribute, or `type: ignore` to production code so a test can observe it. To assert wiring or a convention (the right decorator applied, with the right arguments), parse the module with `ast` + `inspect.getsource` — see `backend/tests/unit/workflows/test_flow_session_convention.py`. ## Test file placement diff --git a/.agents/skills/creating-changelog-entries/SKILL.md b/.agents/skills/creating-changelog-entries/SKILL.md index a6ef939091a..2d099ffa185 100644 --- a/.agents/skills/creating-changelog-entries/SKILL.md +++ b/.agents/skills/creating-changelog-entries/SKILL.md @@ -18,7 +18,7 @@ compatibility: Requires the project to use Towncrier for changelog management - You're asked to "add a changelog entry", "create a news fragment", or "run towncrier". - A PR checklist or CI flags a missing changelog entry. -When NOT to use: the project doesn't use Towncrier; pure internal refactors with no user-facing or maintenance impact; and changes the team has explicitly decided don't warrant an entry. (Most internal maintenance still gets a `housekeeping` fragment.) +When NOT to use: the project doesn't use Towncrier; pure internal refactors with no user-facing or maintenance impact; and changes the team has explicitly decided don't warrant an entry. (The boundary for internal maintenance is below — don't add a `housekeeping` fragment by default.) **Exception — unreleased features need no fragment.** A fix or follow-up to a feature that has not shipped in any release is not user-observable: the feature's own `added` fragment already covers @@ -26,8 +26,9 @@ everything a user will ever see, and a `fixed` entry for something never release **`housekeeping` is not a catch-all.** It covers internal work a user could still notice — a dependency bump, a build or tooling change. A change with no user-facing effect at all (an internal -type annotation, a behavior-preserving refactor, cleanup of internal docs or spec scaffolding) gets -no fragment. When it's unclear whether a change is user-facing, ask instead of adding one by default. +type annotation, a behavior-preserving refactor, a lint or type-checker config cleanup that touches +no source code, cleanup of internal docs or spec scaffolding) gets no fragment. When it's unclear +whether a change is user-facing, ask instead of adding one by default. ## Quick Reference diff --git a/backend/AGENTS.md b/backend/AGENTS.md index 93277d8f6ef..6da14a0f8a1 100644 --- a/backend/AGENTS.md +++ b/backend/AGENTS.md @@ -80,6 +80,9 @@ See `dev/knowledge/backend/testing.md` for detailed testing infrastructure docum - New database indexes - Core schema definition changes - New GraphQL mutations/queries +- New REST endpoints — serve SDK/client needs through existing GraphQL queries first; every REST + endpoint is a second public contract that ripples into `schema/openapi.json` and the generated + frontend REST types ### Never Do @@ -125,6 +128,7 @@ Each entry says *when* to load it — open the doc before working in that area. - `dev/guides/backend/creating-events.md` - Creating new events - `dev/guides/backend/creating-async-tasks.md` - Creating async tasks - `dev/guides/backend/creating-messages.md` - Creating message bus messages +- `dev/guides/backend/creating-migrations.md` - Graph migrations (batching, idempotency, error handling); read before writing a migration or fixing data a migration got wrong ### ADRs (Why we decided) diff --git a/dev/guidelines/backend/checklist.md b/dev/guidelines/backend/checklist.md index e382078a39f..7bb3ec4bf81 100644 --- a/dev/guidelines/backend/checklist.md +++ b/dev/guidelines/backend/checklist.md @@ -57,6 +57,19 @@ Plan the access control requirements: - **Object-level permissions** - should users only access/modify their own resources? - **Branch permissions** - are branch-specific restrictions needed? +## Configuration + +### Does this feature add a setting to `infrahub.config.Settings`? + +A new setting must be reachable from every Compose entry point, and the two files are maintained +differently: + +- The root `docker-compose.yml` env block is **generated** from the `Settings` classes and + CI-validated — regenerate it with `uv run invoke release.validate-dockercomposeenv` +- `development/docker-compose.yml` forwards each `INFRAHUB_*` variable through a **hand-maintained** + anchor that nothing generates or validates — add the mapping yourself, or the setting is + documented but untunable in the dev stack + ## Error Handling & User Experience ### How will users be informed when the feature fails? diff --git a/dev/guidelines/backend/testing.md b/dev/guidelines/backend/testing.md index a9d9740f1dd..0a1dffa8f8c 100644 --- a/dev/guidelines/backend/testing.md +++ b/dev/guidelines/backend/testing.md @@ -201,42 +201,8 @@ def test_my_function(test_case: MyFunctionTestCase) -> None: ### Complex Test Cases -For tests with complex inputs or expected outputs, the dataclass can contain nested objects: - -```python -from dataclasses import dataclass - -from infrahub.core.schema import NodeSchema, SchemaRoot - - -@dataclass -class SchemaValidationTestCase: - name: str - """Descriptive name for the test scenario.""" - - schema: SchemaRoot - """The schema to validate.""" - - expected_errors: list[str] - """List of expected validation error messages.""" - - -SCHEMA_VALIDATION_TEST_CASES: list[SchemaValidationTestCase] = [ - SchemaValidationTestCase( - name="missing_required_field", - schema=SchemaRoot( - nodes=[ - NodeSchema( - namespace="Test", - name="Device", - attributes=[], - ) - ] - ), - expected_errors=["Node TestDevice requires at least one attribute"], - ), -] -``` +Case fields can hold nested objects (a `SchemaRoot`, a list of expected error messages) — the +structure above scales unchanged; keep constructing them with keyword arguments. ### When to Use This Pattern @@ -282,9 +248,20 @@ If you find yourself wanting to mock: 2. **Move up the test pyramid** - A component test requiring extensive mocking to simulate an end-to-end flow is often better written as an integration or functional test 3. **Question the test scope** - If testing requires mocking half the system, the unit under test may be too large -### Don't shape a mutation to serve its own test +### Don't shape production code to serve its own test -A method that performs a side effect (write/delete) should not also return data whose only purpose is to let a test assert it ran. If you need to verify the effect happened, assert against the actual state it changed — e.g. read the fake cache/store the method wrote to — rather than trusting a return value added for that purpose. A method either acts or returns something; bending that rule just for a test is a smell, not a shortcut. +Production code must not change shape just so a test can observe it — no return value added to a +mutating method, no marker attribute stamped on a wrapper, no `type: ignore` absorbed to make a +hook attachable. If you need to verify a side effect happened, assert against the actual state it +changed — e.g. read the fake cache/store the method wrote to — rather than trusting a return value +added for that purpose. + +To verify *wiring* — the right decorator applied, a convention held across a module — parse the +source instead of instrumenting it: an `ast.parse(inspect.getsource(module))` test reads decorator +names and arguments off the tree with zero production hooks. +`backend/tests/unit/workflows/test_flow_session_convention.py` is the house example. Behavioral +coverage (does the decorator retry?) belongs on the decorator's own tests; the wiring test only +proves it is attached. ```python # ❌ Bad - return value exists only so the test can assert on it @@ -346,6 +323,14 @@ not an ambient fact; treat it like any other injected collaborator (see Use monotonic time for durations. Wall-clock time (`datetime.now`) is for timestamps that get stored or displayed, and it can jump backwards. +### Waiting on async effects: poll, don't sleep + +In integration-tier tests the clock cannot be injected — a consumer or worker really does need +wall-clock time to act. Never guess that duration with a fixed `asyncio.sleep(n)` before +asserting: on a loaded CI runner the test flakes, and on a fast machine it wastes the time. Poll +the expected state in a small loop with a deadline, so the test waits exactly as long as the +outcome takes and fails with a timeout when it never arrives. + ## Exception Testing When testing that code raises an exception, use the `match` parameter of `pytest.raises` to validate the error message: @@ -394,6 +379,7 @@ The exact-match principle above is not limited to error messages — it applies - **Don't stop at non-emptiness when a specific result is expected.** `assert result` (or `assert len(result) > 0`) is fine for an existence-only contract, but it does not verify *which* result came back — assert the specific expected value when that is part of the behavior under test. And avoid checks that don't even establish non-emptiness: `assert result != frozenset()` is `True` for an empty `list`/`dict`, so it passes when nothing was returned. - **Assert a positive count where the number matters.** A test that only checks "no failures" can pass while measuring zero of the thing it claims to test — e.g. if a workflow/name string changes so nothing is counted. Assert that the expected count is `> 0` (or the exact number) so a silently-zero run fails. - **Make the scenario actually hold.** A "missing row" test must not create the row; a "no second object" test must prove the count is one. Verify the setup produces the state under test. +- **Make removal assertions branch-attributable.** A "data is gone" check must read on the branch that held the data, and assert the data resolved *before* the operation as well as after — a read on the wrong branch raises the same not-found either way, so the assertion passes whether or not the code ran. - **Denial tests must verify nothing changed.** When asserting an operation is rejected, also reload the target and assert its state is unchanged (or that no row was created/deleted). Asserting only that an error was returned does not prove the write was actually blocked. - **When a result is reachable via more than one code path, assert an intermediate signal too.** If "the lookup was never attempted" and "the lookup ran and found nothing" converge on the same final value (e.g. both produce an empty filter), asserting only that final value can't tell a working implementation from a regressed one that silently skipped the lookup. Also assert what was queried or which branch ran — a signal only the intended path produces. diff --git a/dev/guides/backend/creating-migrations.md b/dev/guides/backend/creating-migrations.md index f3fd0c8417d..1817503e838 100644 --- a/dev/guides/backend/creating-migrations.md +++ b/dev/guides/backend/creating-migrations.md @@ -16,6 +16,12 @@ Create a migration when you need to: Schema definition changes that can be resolved by normal schema migrations (renames, type changes) do not need a graph migration. +When a bug's root cause is a migration that failed to materialize or transform data (e.g. on an +inheritance change), fix the migration layer — do not compensate by lazily repairing state in +runtime paths like `Node.save()`, even when that is the smaller diff. A runtime repair leaves the +stored data wrong for every path that doesn't go through it, and the repair code itself runs after +validation hooks that the normal write path relies on. + ## Migration Types Choose the right base class in `backend/infrahub/core/migrations/shared.py`: @@ -111,6 +117,18 @@ CALL (n) { `CALL ... IN TRANSACTIONS` requires the `MATCH` to be outside the subquery — move all matching up front. +Size batches by the unit that actually bounds transaction memory. `IN TRANSACTIONS OF n ROWS` +counts input rows, so when one row fans out to an unbounded set of edges or attributes (deleting a +node deletes everything attached to it), the per-transaction cost is unbounded no matter how small +`n` is. Batch over the fan-out unit where possible, and cap a fan-out-prone batch with +`min(configured_size, ceiling)` rather than inheriting `query_size_limit` unchanged. + +**Pitfall — one bad item must not hide the rest.** A migration that iterates independent items +(branches, nodes, kinds) wraps each item in its own `try`, collects per-item failures into +`MigrationResult.errors`, and keeps going. A single `try` around the loop aborts on the first +failure, reports one error, and leaves the state of every remaining item unknown — on re-run the +operator cannot tell what was reclaimed and what was never attempted. + **Pitfall — don't carry candidate ids across phases in application memory.** A multi-phase migration (phase 1 computes candidate node/edge ids, phase 2 acts on them — e.g. deleting orphans or restoring metadata for what phase 1 touched) must not hold those candidates in a Python list to drive the later phase. A crash between phases loses that in-memory list, and a resumed run can no longer tell what still needs cleanup. Keep candidate selection and its dependent cleanup database-side: re-derive candidates by the same filter inside the same `CALL ... IN TRANSACTIONS` pass as the write that produces them, rather than collecting ids in Python. Distinct logical phases (e.g. reopening edges vs. deleting edges) can and should stay as separate passes when planner or transaction-memory limits require it — each pass just needs to clean up after its own candidates rather than depending on ids collected by an earlier pass. See [Merge Failure Recovery](../../knowledge/backend/merge-failure-recovery.md) for a worked example. ### Step 4: Beware of Shared Nodes diff --git a/dev/knowledge/backend/async-tasks.md b/dev/knowledge/backend/async-tasks.md index 527d6373fc3..5e7f4cda24d 100644 --- a/dev/knowledge/backend/async-tasks.md +++ b/dev/knowledge/backend/async-tasks.md @@ -95,6 +95,14 @@ async def validate_schema(db: InfrahubDatabase, branch: Branch) -> bool: # ... implementation ``` +### InfrahubBatch is concurrent, not ordered + +The SDK's `InfrahubBatch` runs everything added to it concurrently when executed — grouping tasks +into one batch (or into a "phase" of batches) is not a serialization mechanism. When tasks must not +overlap (e.g. writes touching overlapping vertices whose idempotency guards only protect +*sequential* reruns), cap the batch's max concurrent execution to 1 or run the items in a plain +loop. + ## Naming Conventions ### Workflow and Task Names diff --git a/dev/knowledge/backend/database-schema.md b/dev/knowledge/backend/database-schema.md index 173a53cf5da..5f4518d8abf 100644 --- a/dev/knowledge/backend/database-schema.md +++ b/dev/knowledge/backend/database-schema.md @@ -43,9 +43,16 @@ Application data nodes. Labels: `Node`, `CoreNode`, `{kind}`, plus inherited sch | Property | Type | Description | |----------|------|-------------| | `uuid` | string | UUID | -| `kind` | string | Node type (also in labels) | +| `kind` | string | Concrete node kind (also in labels) | | `branch_support` | string | `"aware"`, `"local"`, or `"agnostic"` | +The `kind` property always holds the *concrete* kind; generic kinds exist only as labels. A Cypher +filter on `n.kind` therefore never matches a generic — it silently returns zero rows. Match +generics via labels, and remember a label-based sum double-counts nodes that inherit several of the +requested generics. When a query input must be concrete-only, accept `list[NodeSchema]` and derive +the kind strings internally, so passing a `GenericSchema` fails type-checking instead of returning +a silent zero at runtime. + ### Relationship Links two Node vertices. Label: `Relationship`. diff --git a/dev/knowledge/backend/events.md b/dev/knowledge/backend/events.md index 5a1584456c2..ab3fddb6d66 100644 --- a/dev/knowledge/backend/events.md +++ b/dev/knowledge/backend/events.md @@ -25,6 +25,14 @@ All events extend `InfrahubEvent` from `backend/infrahub/events/models.py` and c - **related**: Additional context resources (returned by `get_related()`) - **payload**: Event-specific data (returned by `get_event_payload()`) +### Sensitive values are masked only at construction + +The changelog models (`backend/infrahub/core/changelog/models.py`) mask `Password`/ +`HashedPassword` attribute values to `***` in a `model_validator(mode="after")` — which runs only +when the model is constructed. Assigning `value`/`value_previous` on an existing instance bypasses +the mask (`validate_assignment` is not enabled) and leaks the secret into the event payload. Build +changelog entries with their final values; never patch them after construction. + ### Related resources cap The Prefect API rejects any event whose `related` list exceeds diff --git a/dev/knowledge/backend/mutations.md b/dev/knowledge/backend/mutations.md index b01e42afcdc..7c0cb23854a 100644 --- a/dev/knowledge/backend/mutations.md +++ b/dev/knowledge/backend/mutations.md @@ -82,6 +82,22 @@ mutate_upsert() -> handle file idempotency (checksum comparison) ``` +## Transaction Retry + +`retry_db_transaction` (in `backend/infrahub/database/__init__.py`) replays the *entire* wrapped +function when Neo4j raises a `TransientError`. Its placement is therefore semantics-bearing, not +decoration: + +- Wrap only a scope whose writes are fully contained in a transaction the retry can roll back. The + create path is the trap: `create_node()` commits its own transaction and then does post-commit + work (profiles, response reads), so a decorator on the whole mutation replays a commit that + already succeeded and creates a duplicate node. The update path keeps its response build inside + `db.start_transaction()`, which is what makes it replay-safe. +- Skip the retry when `db.is_transaction` — a caller-supplied transaction is already failed after a + `TransientError`, and replaying inside it raises instead of recovering; the caller owns the retry. +- Check whether an already-retried caller reaches the code (upsert dispatches to create/update): + nested retries multiply attempts. + ## Relationship Resolution During Mutations ### RelationshipManager.update() diff --git a/dev/knowledge/backend/query-pattern.md b/dev/knowledge/backend/query-pattern.md index fe3199af113..ab3919011cf 100644 --- a/dev/knowledge/backend/query-pattern.md +++ b/dev/knowledge/backend/query-pattern.md @@ -148,6 +148,14 @@ class MyQuery(Query): self.add_to_query("RETURN n.uuid AS uuid, n.name AS name LIMIT 100") # Manual pagination ``` +Two failure modes on READ queries: + +- `insert_limit = False` without a `LIMIT` of your own is not "no pagination" — `execute()` still + takes the chunked `query_with_size_limit()` path, every chunk re-reads the full result set, and + the loop (which stops on a short page) never advances once results reach `query_size_limit`. +- An auto-paginated query needs an `ORDER BY` over a total order (a unique key). Without one, Neo4j + does not guarantee pages are disjoint, so rows can be skipped or repeated across chunks. + ### Branch-Aware Edge Resolution Every edge in the graph has branch/temporal properties (`branch`, `branch_level`, `from`, `to`, `status`). When traversing multiple edges in a single query, filter each edge independently to resolve the correct active version: @@ -280,29 +288,8 @@ def get_data(self) -> Generator[NodeWithPeers, None, None]: ``` -### Query Method Patterns - -```python -# Multiple results (standard pattern): -# Query returns: RETURN n.uuid AS node_uuid, n.name AS node_name -def get_data(self) -> Generator[MyQueryResult, None, None]: - for result in self.get_results(): - yield MyQueryResult( - uuid=result.get_as_str("node_uuid"), - name=result.get_as_str("node_name"), - ) - -# Single result: -# Query returns: RETURN n.uuid AS node_uuid, n.name AS node_name -def get_data(self) -> MyQueryResult | None: - result = self.get_result() - if result is None: - return None - return MyQueryResult( - uuid=result.get_as_str("node_uuid"), - name=result.get_as_str("node_name"), - ) -``` +For a query expected to yield at most one row, return `MyQueryResult | None` from a +`self.get_result()` check instead of a generator. ### Guidelines @@ -314,25 +301,9 @@ def get_data(self) -> MyQueryResult | None: - Use `get_as_str()`, `get_as_type()` for scalars, `get_as_optional_type()` for nullable values - Use `Generator` return type since `get_results()` returns a generator -### Why Return Only Needed Properties - -Returning entire nodes (`RETURN n`) transfers all properties from the database, even those you don't use. This wastes: - -1. **Network bandwidth** between Neo4j and the application -2. **Memory** for deserializing and storing unused data -3. **CPU cycles** for parsing unnecessary properties - -```cypher --- Bad: Returns all properties of n, r, and p -MATCH (n:Node)-[r:REL]->(p:Peer) -RETURN n, r, p - --- Good: Returns only the 3 properties actually needed -MATCH (n:Node)-[r:REL]->(p:Peer) -RETURN n.uuid AS node_uuid, n.name AS node_name, r.branch AS rel_branch -``` - -Use `elementId(n) AS db_id` when you need the database ID, rather than returning the full node just to access `node.element_id`. +Use `elementId(n) AS db_id` when you need the database ID, rather than returning the full node just +to access `node.element_id` (see [Return Labels](#return-labels) for why whole-node returns waste +bandwidth and memory). ## Internals diff --git a/development/grafana/AGENTS.md b/development/grafana/AGENTS.md new file mode 100644 index 00000000000..82ec7b1761c --- /dev/null +++ b/development/grafana/AGENTS.md @@ -0,0 +1,12 @@ +# Grafana Dashboards + +Rules for editing the bundled dashboards under `provisioning/dashboards/`. + +- Reference only template variables the dashboard itself defines — `${datasource_prometheus}` or + `${datasource_loki}`, never a generic `$datasource`. Grafana resolves an undefined variable to + nothing, and only when the panel or link is used, so the mistake survives a visual check. +- When changing a variable or metric, sweep every surface of the JSON, not just query targets: + drill-down/data links and legend URLs carry `var-=` references that break silently. +- These files are bind-mounted into `docker-compose-observability.yml`, and the standalone variant + embeds them inline. After editing anything under `provisioning/`, regenerate + `docker-compose-observability-standalone.yml` with `python development/convert_compose_standalone.py`. diff --git a/docs/AGENTS.md b/docs/AGENTS.md index c2a71f56b93..49a9781e8b3 100644 --- a/docs/AGENTS.md +++ b/docs/AGENTS.md @@ -31,7 +31,9 @@ Infrahub documentation is organized using the [Diataxis framework](https://diata - `AGENTS.md` – **Specialized instructions for writing topics** - `reference/` – API/configuration reference - `tutorials/` – Learning tutorials - - `media/` – Images and screenshots + - `media/` – Images and screenshots. Export diagrams (Excalidraw) with an opaque white + background, not transparent — the docs render on a dark theme where near-black line work on a + transparent canvas is unreadable, and the white card is the established convention here - `development/` – Developer documentation - `docs.mdx` – Documentation guide with linting rules - `style-guide.mdx` – **Writing style and terminology rules**