feat!: GraphQL query service over relational read models#127
Open
patrickleet wants to merge 32 commits into
Open
feat!: GraphQL query service over relational read models#127patrickleet wants to merge 32 commits into
patrickleet wants to merge 32 commits into
Conversation
Implements specs/query-service-graphql end-to-end: Phase 1: TableKind + RelationshipDef.target_foreign_key (breaking), dep-free SDL renderer, dctl schema --format graphql. Phase 2: graphql feature with filter DSL, permissions, dialect-portable SQL compiler, SQLite/Postgres executors, dynamic role schemas, Service::with_graphql + POST /graphql. Phase 3: aggregates surface in SDL; per-role allow_aggregations. Phase 4: ReadModelChange broadcast on SqlxRepository, Postgres pg_notify hook, subscription root with one-shot streams. Phase 5: GraphqlCommands + Mutation root dispatch via CommandRequest; GraphqlInput/Output type metadata. Docs: docs/graphql.md + README Query API section. Tests: graphql_sdl, graphql_engine, graphql_sqlite (domain fixture exit), graphql_subscriptions_sqlite (commit notify), graphql_commands. Refs: tasks/graphql-qs-epic, tasks/graphql-qs-00-spike through tasks/graphql-qs-13-docs-skills
Implement ChangeHub-driven live query streams: initial yield, commit-path invalidation via repo broadcast forwarder, debounce, hash-gated push. Tests (shipped path): - subscription_pushes_exactly_once_per_commit (filtered list, SQLite) - hash_gate_no_push_when_result_unchanged - mutation_handler_projection_query_loop (mutation→write plan→query) - GraphqlInput/GraphqlOutput derives + mapping golden Fixes skeptic gaps on stub subscriptions and theater command tests. Refs: tasks/graphql-qs-10-subscriptions, tasks/graphql-qs-12-command-mutations, tasks/graphql-qs-11-gql-type-derives
- Embed distributed-graphql skill (registry/frontmatter/directory sync) - dctl scaffold --query-api emits src/query/ skeleton, graphql feature, build_with_graphql + DATABASE_URL pool wiring - query_api implies read-models and promotes in-memory store to sqlite Evidence: cargo test -p distributed_cli --lib (skills + query_api unit); scaffolded service under target/scaffold-query-api-demo cargo check green. Refs: tasks/graphql-qs-13-docs-skills
Wire queryApi.databaseUrl into generated values/env when --query-api is set. Add scripts/graphql-skill-dry-run.sh for AC2 agent-following-skill evidence. Implements [[tasks/graphql-qs-13-docs-skills]] [[tasks/graphql-qs-epic]]
Add cargo run --example graphiql with seeded SQLite orders, shared graphiql_page() with default identity headers, and scaffold GRAPHIQL env (on by default for local, set 0 to disable). Implements [[tasks/graphql-qs-epic]]
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a feature-gated GraphQL query service over relational read models, including SDL generation, role-based permissions, dialect-specific SQL execution, HTTP and GraphiQL routing, live subscriptions, command mutations, repository notifications, CLI scaffolding, documentation, and integration tests. ChangesGraphQL Query Service
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Client
participant GraphqlHttp
participant GraphqlEngine
participant SqlCompiler
participant SqlExecutor
Client->>GraphqlHttp: POST /graphql
GraphqlHttp->>GraphqlEngine: execute(session, request)
GraphqlEngine->>SqlCompiler: compile_root(selection, permissions)
SqlCompiler->>SqlExecutor: execute_sql(sql_plan)
SqlExecutor-->>GraphqlEngine: GraphQL value or error
GraphqlEngine-->>GraphqlHttp: GraphQL response
GraphqlHttp-->>Client: HTTP response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
distributed_cli/src/generate/service_crate.rs (1)
55-76: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
--query-apiwith an in-memory store generates uncompilable code.
distributed_features()pushes"sqlite"only forStoreTarget::Sqlite(nothing forStoreTarget::InMemory), butservice_rs()'squery_apibranch (see below) maps every non-Postgres store — includingInMemory— todistributed::SqliteRepository. Scaffolding--query-apiwith an in-memory store therefore emits code referencing a type gated behind a Cargo feature that was never enabled, breaking the build. Since GraphQL fundamentally requires a SQL-backed pool, this combination should be rejected during scaffold generation (e.g. inScaffold::from_spec) rather than silently generating broken code.🛡️ Proposed fix: reject the invalid combination early
fn from_spec(spec: ServiceScaffoldSpec) -> Result<Self, ScaffoldError> { let names = ScaffoldNames::new(&spec.name)?; + if spec.query_api && spec.store == StoreTarget::InMemory { + return Err(ScaffoldError::new( + "--query-api requires a SQL-backed store (sqlite or postgres)", + )); + } let models = model_scaffolds(&spec.models)?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@distributed_cli/src/generate/service_crate.rs` around lines 55 - 76, Reject the incompatible query_api and StoreTarget::InMemory combination during scaffold validation, preferably in Scaffold::from_spec, before code generation proceeds. Keep distributed_features unchanged unless needed for validation, and ensure the generator returns a clear error instead of emitting service code that references distributed::SqliteRepository without the required feature.
🧹 Nitpick comments (6)
tests/graphql_sqlite/main.rs (1)
117-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename
permissions_filter_by_claimto reflect what it actually tests.The test only verifies anonymous deny-by-default (no grants → error or null
orders), not claim-based row-level filtering. The development comments at lines 124–129 explain the limitation but should be cleaned up. Consider renaming to something likeanonymous_role_denied_by_defaultand removing the exploratory notes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/graphql_sqlite/main.rs` around lines 117 - 146, Rename the test function permissions_filter_by_claim to reflect its actual anonymous deny-by-default behavior, such as anonymous_role_denied_by_default. Remove the exploratory comments describing unsupported claim-based filtering and builder internals, while preserving the existing setup and assertions.src/table/registry.rs (1)
269-325: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueExplicit
target_foreign_keyskips existence and collision validation.When
target_foreign_keyis explicitly set,resolve_m2m_target_foreign_keyreturns it without checking that the column exists on the join table or that it differs from the source-sideforeign_key. The caller at line 208 does validate existence, so the column check is covered. However, a collision withforeign_key(both pointing to the same column) would produce a semantically broken m2m join and is not caught anywhere. Consider adding a guard in the explicit path.🛡️ Optional guard against source/target FK collision
if let Some(explicit) = relationship.target_foreign_key.as_deref() { if explicit.is_empty() { return Err(TableStoreError::Metadata(format!( "model `{}` relationship `{}` target_foreign_key must not be empty", source.model_name, relationship.field_name ))); } + if explicit == relationship.foreign_key.as_deref().unwrap_or_default() { + return Err(TableStoreError::Metadata(format!( + "model `{}` relationship `{}` target_foreign_key must differ from foreign_key", + source.model_name, relationship.field_name + ))); + } return Ok(explicit.to_string()); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/table/registry.rs` around lines 269 - 325, Add a collision guard in the explicit target_foreign_key branch of resolve_m2m_target_foreign_key: reject the configuration when the explicit target key matches relationship.foreign_key, returning a TableStoreError::Metadata describing the model and relationship. Preserve the existing empty-value validation and explicit-key return behavior for non-colliding values.src/graphql/engine.rs (1)
149-154: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead conditional.
The
ifblock computes a condition and only contains a comment; it has no effect and can be removed for clarity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/graphql/engine.rs` around lines 149 - 154, Remove the no-op conditional in sdl_for_role that checks inner.roles and anonymous_role, leaving the schemas lookup and SDL mapping unchanged.src/graphql/subscribe.rs (1)
41-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNo-op
is_emptyguard inpublish.The
if change.is_empty()block only holds a comment and does nothing, yet the empty set is treated as the "all-dirty" lag signal by consumers (footprint_hits, Line 198). As written, any caller publishing an empty change silently triggers all-dirty re-execution. Either drop the misleading branch or explicitly reject/handle empty publishes so the reserved semantics aren't invoked accidentally.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/graphql/subscribe.rs` around lines 41 - 47, Update Subscription::publish to prevent ordinary callers from sending an empty ReadModelChange, since consumers interpret it as the all-dirty lag signal. Remove the no-op is_empty branch and explicitly reject or otherwise handle empty changes before self.tx.send, while preserving the forwarder’s intentional empty-change behavior through its existing explicit path.src/graphql/schema.rs (1)
631-651: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the dead
resolve_subscription_livehelper. It isn’t referenced outside its own definition; the subscription field already builds the stream inline.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/graphql/schema.rs` around lines 631 - 651, Remove the unused resolve_subscription_live helper and its associated dead code, leaving the existing inline subscription stream construction unchanged.distributed_cli/src/generate/mod.rs (1)
144-154: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
--query-apiwithout--read-modelssilently scaffolds an empty GraphQL API.
self.read_models(used to emitsrc/query/*.rsmodel modules) is only populated whenspec.read_modelsis true, independent ofself.query_api. Nothing here rejectsquery_api=true, read_models=false, so that combination generates a working-but-empty query API (no exposed models) with no warning to the user.Consider validating this combination in
from_spec(or emitting a generation warning) so users get clear feedback instead of a silently empty schema.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@distributed_cli/src/generate/mod.rs` around lines 144 - 154, Validate the configuration in from_spec so query_api cannot be enabled when read_models is disabled, or emit a clear generation warning for that combination. Ensure users are informed that --query-api without --read-models produces an empty schema, while preserving normal query generation when read models are enabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@distributed_cli/src/cli.rs`:
- Around line 643-651: Add a warning in the store-selection block around
args.query_api when --query-api promotes StoreTarget::InMemory to
StoreTarget::Sqlite. Clearly state that in-memory storage is being replaced with
SQLite, while preserving the existing promotion behavior and avoiding warnings
for other store selections.
In `@distributed_cli/src/generate/gitops.rs`:
- Around line 139-153: Update query_api_env_yaml to conditionally emit
DATABASE_URL only when .Values.queryApi.databaseUrl is non-empty, while
retaining the existing query_api and queryApi.enabled checks. Follow the
truthiness-guard pattern used by tracing_env_yaml so an empty default value does
not create the environment variable and build_with_graphql can use its fallback
connection string.
In `@distributed_cli/src/generate/service_crate.rs`:
- Around line 283-341: Update the generated build_with_graphql function so its
routes receive the persistent repository connected from DATABASE_URL, rather
than constructing InMemoryRepository::new(). Reuse the store created by the
{repo_ty}::connect call when building Routes::new().with_dependencies(...),
ensuring command writes and GraphQL queries share the same backing store.
In `@src/graphql/commands.rs`:
- Around line 84-88: Update GraphqlCommands::command to detect an existing
command with the same name before appending to self.commands, and reject
duplicates with a clear API-level error consistent with the method’s existing
return contract. Preserve the current command_name assignment and insertion
behavior for unique names.
In `@src/graphql/compile.rs`:
- Around line 402-411: Update chunked_json_object’s key formatting to
defensively validate or escape each client-controlled response_key before
embedding it as a SQL string literal, including the single-chunk path shown here
and any equivalent chunked path. Preserve valid GraphQL aliases while preventing
quote-containing keys from being emitted unescaped.
- Around line 486-492: Update the BelongsTo arm in the join_pred match within
the relationship compilation logic to compare the target table’s primary-key
column against the source table’s foreign-key column. Keep the existing HasMany
predicate unchanged and use the available target PK and source FK symbols rather
than target-side fk lookup.
- Around line 88-146: Reorder bind collection in compile_root,
compile_relationship_subquery, and compile_m2m_subquery to follow each SQL
statement’s emitted placeholder order: collect projection binds before where
binds, then limit/offset binds. Update RootKind::Aggregate so the duplicated
where_sql interpolation has a corresponding second set of SQLite binds,
preserving correct positional binding.
In `@src/graphql/engine.rs`:
- Around line 201-211: The execute_stream path must handle roles absent from
self.inner.schemas without panicking. Replace the expect("role schema missing")
in execute_stream with the same graceful error behavior used by execute(),
returning an error stream or single error Response while preserving normal
execution for configured roles.
In `@src/graphql/execute.rs`:
- Around line 55-56: Update the SQLite result extraction in the surrounding
execution flow to propagate `row.try_get(0)` decode errors instead of converting
them to `None` with `.ok()` and falling back to `"null"`. Match the
error-surfacing behavior of the Postgres path while preserving the existing
successful text-result handling.
In `@src/graphql/filter.rs`:
- Around line 132-161: Add a direct From<f64> implementation for Operand
alongside the existing numeric conversions, converting the value through
LitValue::from so float literals can be passed to comparison methods without
wrapping them in lit(). Optionally add the equivalent From<f32> implementation
if LitValue supports that conversion.
In `@src/graphql/schema.rs`:
- Line 34: Update build_role_schema to use the disable_introspection parameter
when constructing the role schema, rather than discarding it in the let _
assignment. Ensure the generated anonymous-role schema disables introspection
when the flag is true, while preserving existing behavior when it is false.
In `@src/graphql/sdl.rs`:
- Around line 372-378: Update ensure_object_type to register the same nested
{rel}_aggregate field that emit_object_type emits when options.aggregates is
enabled, including its where argument and aggregate return type. Keep the
dynamic schema’s relationship and top-level aggregate registration unchanged,
and ensure the field is added only for the same has-many/many-to-many
relationships.
In `@tests/graphql_commands/main.rs`:
- Around line 188-191: Update the user-role assertion in the GraphQL command
test to check specifically for the `type Mutation` declaration, matching the
existing anonymous-role assertion, and remove the broad case-insensitive
`Mutation` checks.
In `@tests/graphql_sqlite/main.rs`:
- Around line 73-91: Remove the exploratory
`GraphqlEngine::builder(...).table_schema(...)` block and its comments from
`list_filter_and_by_pk`. Keep the `DistributedProjectManifest::new("orders")`
and `GraphqlEngine::from_manifest` setup, using the single `setup_pool()` result
and the original `schema` without cloning.
---
Outside diff comments:
In `@distributed_cli/src/generate/service_crate.rs`:
- Around line 55-76: Reject the incompatible query_api and StoreTarget::InMemory
combination during scaffold validation, preferably in Scaffold::from_spec,
before code generation proceeds. Keep distributed_features unchanged unless
needed for validation, and ensure the generator returns a clear error instead of
emitting service code that references distributed::SqliteRepository without the
required feature.
---
Nitpick comments:
In `@distributed_cli/src/generate/mod.rs`:
- Around line 144-154: Validate the configuration in from_spec so query_api
cannot be enabled when read_models is disabled, or emit a clear generation
warning for that combination. Ensure users are informed that --query-api without
--read-models produces an empty schema, while preserving normal query generation
when read models are enabled.
In `@src/graphql/engine.rs`:
- Around line 149-154: Remove the no-op conditional in sdl_for_role that checks
inner.roles and anonymous_role, leaving the schemas lookup and SDL mapping
unchanged.
In `@src/graphql/schema.rs`:
- Around line 631-651: Remove the unused resolve_subscription_live helper and
its associated dead code, leaving the existing inline subscription stream
construction unchanged.
In `@src/graphql/subscribe.rs`:
- Around line 41-47: Update Subscription::publish to prevent ordinary callers
from sending an empty ReadModelChange, since consumers interpret it as the
all-dirty lag signal. Remove the no-op is_empty branch and explicitly reject or
otherwise handle empty changes before self.tx.send, while preserving the
forwarder’s intentional empty-change behavior through its existing explicit
path.
In `@src/table/registry.rs`:
- Around line 269-325: Add a collision guard in the explicit target_foreign_key
branch of resolve_m2m_target_foreign_key: reject the configuration when the
explicit target key matches relationship.foreign_key, returning a
TableStoreError::Metadata describing the model and relationship. Preserve the
existing empty-value validation and explicit-key return behavior for
non-colliding values.
In `@tests/graphql_sqlite/main.rs`:
- Around line 117-146: Rename the test function permissions_filter_by_claim to
reflect its actual anonymous deny-by-default behavior, such as
anonymous_role_denied_by_default. Remove the exploratory comments describing
unsupported claim-based filtering and builder internals, while preserving the
existing setup and assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 46112043-8920-4dad-93ad-83bf29e54e1e
📒 Files selected for processing (51)
Cargo.tomlREADME.mddistributed_cli/skills/distributed-graphql/SKILL.mddistributed_cli/src/cli.rsdistributed_cli/src/generate/gitops.rsdistributed_cli/src/generate/mod.rsdistributed_cli/src/generate/service_crate.rsdistributed_cli/src/lib.rsdistributed_cli/src/manifest_harness.rsdistributed_cli/src/skills.rsdistributed_macros/src/graphql_types.rsdistributed_macros/src/lib.rsdistributed_macros/src/read_model.rsdocs/graphql.mdexamples/graphiql.rsscripts/graphql-skill-dry-run.shsrc/graphql/commands.rssrc/graphql/compile.rssrc/graphql/engine.rssrc/graphql/execute.rssrc/graphql/filter.rssrc/graphql/http.rssrc/graphql/mod.rssrc/graphql/naming.rssrc/graphql/permissions.rssrc/graphql/schema.rssrc/graphql/sdl.rssrc/graphql/subscribe.rssrc/graphql/types.rssrc/lib.rssrc/manifest.rssrc/microsvc/http.rssrc/microsvc/mod.rssrc/microsvc/service.rssrc/outbox/table.rssrc/postgres_repo/mod.rssrc/read_model/change.rssrc/read_model/in_memory.rssrc/read_model/mod.rssrc/sqlx_repo/read_model.rssrc/sqlx_repo/repo.rssrc/table/metadata.rssrc/table/mod.rssrc/table/registry.rssrc/table/sql.rssrc/telemetry.rstests/graphql_commands/main.rstests/graphql_engine/main.rstests/graphql_sdl/main.rstests/graphql_sqlite/main.rstests/graphql_subscriptions_sqlite/main.rs
Export GitKB specs/query-layer/* and the query-service-graphql redirect into docs/ for agent and CI discoverability. Normative package includes Agent seams for metrics, AuthZ fixtures, WS, IR, and goldens. Refs: specs/query-layer/index
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (5)
docs/query-layer/authorization.md (1)
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse an h2 for the first subsection.
After the level-1
# Authorizationheading, this heading should be##to satisfy markdownlint MD001 and maintain a valid hierarchy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/query-layer/authorization.md` at line 13, Update the “Authorization: role-based select permissions” heading in the authorization documentation from level 3 to level 2, preserving the existing heading text and hierarchy beneath the level-1 “Authorization” heading.Source: Linters/SAST tools
docs/query-layer/security.md (1)
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse an h2 for the first subsection.
After the level-1
# Security, SQL execution, limits, and errorsheading, this heading should be##to satisfy markdownlint MD001.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/query-layer/security.md` at line 13, Change the “Execution: GraphQL → one SQL statement per root field” heading from level 3 to level 2 so it is the first subsection under the document’s level-1 heading and satisfies markdownlint MD001.Source: Linters/SAST tools
docs/query-layer/surface.md (2)
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse an h2 for the first subsection.
After the level-1
# Surface, schema derivation, layout, and dctlheading, this heading should be##to satisfy markdownlint MD001.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/query-layer/surface.md` at line 13, Update the first subsection heading, “Placement: graphql feature on the core crate,” from level 3 to level 2 so it directly follows the document’s level-1 heading and satisfies markdownlint MD001.Source: Linters/SAST tools
231-231: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSpecify the fenced block language.
The directory-tree example should use a language such as
text(```text) so markdownlint MD040 passes and renderers handle it consistently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/query-layer/surface.md` at line 231, Update the directory-tree fenced code block in the documentation to declare the text language, using the existing example content unchanged and ensuring the fence is labeled for markdownlint MD040 compliance.Source: Linters/SAST tools
docs/query-layer/quality.md (1)
15-25: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd explicit metrics and timeout tests to the quality bar.
The observability spec requires a GraphQL metrics assertion and a long-running SQLite statement that maps to
TIMEOUT, but the test-plan table has no entries for either. Add both tests so these contracts are part of the required release evidence.Also applies to: 30-40
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/query-layer/quality.md` around lines 15 - 25, Add explicit quality-bar entries for a GraphQL metrics assertion and a long-running SQLite statement that maps to TIMEOUT, placing them in the appropriate test-plan phases alongside the existing GraphQL engine/SQLite coverage. Ensure both observability contracts are listed as required release evidence.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/query-layer/architecture.md`:
- Line 13: Update the Markdown structure in architecture.md: change the
`graphql` feature heading to the appropriate heading level relative to its
surrounding sections, and add a `text` language identifier to the module-layout
code fence. Preserve the documented content and layout.
- Around line 21-24: Resolve the conflicting GraphQL feature-gating requirements
across the architecture specification and implementation plan: choose either
compilation failure without a SQL store or successful compilation using an
uninhabited GraphqlPool, then update the relevant documented sections so they
consistently describe the same behavior.
In `@docs/query-layer/authorization.md`:
- Around line 62-72: Align the authorization specification with the runtime’s
current empty-role behavior: document that grant-less roles receive a schema
containing the `_empty` field, which returns the error “FORBIDDEN: role has no
GraphQL grants.” Update the surrounding claims about no schema, no schema
access, and the fixed error message while preserving the documented behavior for
roles with granted models and anonymous filters.
In `@docs/query-layer/http.md`:
- Line 13: Fix the Markdown lint violations in the service integration
documentation: adjust the heading containing Service::with_graphql to the
appropriate heading level, and add a language identifier such as text to the
directory-tree fenced code block. Keep the documented content unchanged.
- Around line 71-77: Reconcile the HTTP and subscription contracts throughout
the document by clearly labeling shipped versus planned behavior: align GraphiQL
defaults and non-GraphiQL GET responses, consistently state whether WebSocket
upgrades are shipped, and remove conflicting delivery disclaimers. Update the
debounce/coalescing description to preserve exactly-once delivery for every
distinct commit, explicitly explaining how coalescing cannot drop committed
events.
In `@docs/query-layer/implementation.md`:
- Around line 101-114: Update the fenced code block containing the src/graphql
module layout to specify the text language identifier immediately after the
opening fence, while leaving the directory tree content unchanged.
In `@docs/query-layer/observability.md`:
- Line 13: Update the Observability heading in the documentation so it uses the
appropriate heading level beneath the document’s top-level “Observability”
heading, correcting the reported hierarchy violation while preserving the
surrounding content.
- Around line 43-45: Align the SQLite timeout result in the observability
documentation with the stable GraphQL error contract: either add TIMEOUT to the
documented closed set of GraphQL error codes or rename the SQLite result to an
already-supported code. Ensure both specifications expose the same timeout
value.
In `@docs/query-layer/quality.md`:
- Line 13: Update the “Test plan (files, per phase)” heading in the
documentation to the appropriate heading level beneath the existing “# Quality
bar” heading, replacing the current ### marker while preserving the heading
text.
In `@docs/query-layer/relationships.md`:
- Line 13: Update the “Many-to-many traversal (normative)” heading to the
appropriate heading level beneath the document’s “# Relationships” heading,
replacing the current ### marker while preserving the heading text.
- Around line 59-77: Reconcile m2m through-table discovery with from_manifest’s
TableKind::ReadModel filtering so manifest-defined operational through tables
are not unexpectedly omitted. Either enforce that every m2m through table is a
ReadModel, or register non-ReadModel through tables as shadow catalog entries
that remain non-exposed while supporting traversal; update the documented
catalog requirement and related discovery behavior consistently.
In `@docs/query-layer/security.md`:
- Line 320: Update the “Slow query” row in the security documentation to avoid
claiming timeout support for both dialects: document Postgres-only timeout
enforcement with SQLite best-effort guards, unless an actual validated SQLite
deadline mechanism is implemented first.
In `@docs/query-layer/surface.md`:
- Around line 129-130: Update the Timestamp row in the query-layer wire-format
table to state that v1 exposes the stored Postgres text form as-is, matching the
policy in the security documentation; remove the unresolved normalization
language and leave the Bytes policy unchanged.
---
Nitpick comments:
In `@docs/query-layer/authorization.md`:
- Line 13: Update the “Authorization: role-based select permissions” heading in
the authorization documentation from level 3 to level 2, preserving the existing
heading text and hierarchy beneath the level-1 “Authorization” heading.
In `@docs/query-layer/quality.md`:
- Around line 15-25: Add explicit quality-bar entries for a GraphQL metrics
assertion and a long-running SQLite statement that maps to TIMEOUT, placing them
in the appropriate test-plan phases alongside the existing GraphQL engine/SQLite
coverage. Ensure both observability contracts are listed as required release
evidence.
In `@docs/query-layer/security.md`:
- Line 13: Change the “Execution: GraphQL → one SQL statement per root field”
heading from level 3 to level 2 so it is the first subsection under the
document’s level-1 heading and satisfies markdownlint MD001.
In `@docs/query-layer/surface.md`:
- Line 13: Update the first subsection heading, “Placement: graphql feature on
the core crate,” from level 3 to level 2 so it directly follows the document’s
level-1 heading and satisfies markdownlint MD001.
- Line 231: Update the directory-tree fenced code block in the documentation to
declare the text language, using the existing example content unchanged and
ensuring the fence is labeled for markdownlint MD040 compliance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9ef9d9c7-77ce-4236-97ac-23f51f272666
📒 Files selected for processing (13)
docs/graphql.mddocs/query-layer/architecture.mddocs/query-layer/authorization.mddocs/query-layer/decisions.mddocs/query-layer/http.mddocs/query-layer/implementation.mddocs/query-layer/index.mddocs/query-layer/observability.mddocs/query-layer/quality.mddocs/query-layer/relationships.mddocs/query-layer/security.mddocs/query-layer/surface.mddocs/query-service-graphql.md
✅ Files skipped from review due to trivial changes (3)
- docs/query-service-graphql.md
- docs/query-layer/decisions.md
- docs/graphql.md
Mirror specs/query-layer/state for progress tracking against the package. Refs: specs/query-layer/state
- Validate GraphQL response keys; where/filter max_depth; sanitize client errors - Preserve JSON-looking string columns; SQLite statement timeout; no write txn on SELECT - Wire distributed_graphql_request_* metrics; honor introspection_for_anonymous - Scaffold GraphiQL off in production envs; claim AuthZ + relationship e2e suites - Fill graphql_compile/http/postgres tests; fix skills init count for graphql skill Implements [[tasks/graphql-qs-harden-1]] P0 + P1 (P2 deferred)
Increment where/filter boolean nesting depth; prove restricted role cannot select denied columns and nested _and exceeds max_depth with stable errors. Implements [[tasks/graphql-qs-harden-4]] [[tasks/graphql-qs-harden-7]]
Reflects column allowlist e2e, where max_depth, last-reviewed, and rollup posture for harden epic. Implements [[tasks/graphql-qs-harden-1]]
Specs live in GitKB (specs/query-layer/*) only. Drop docs/query-layer/ and docs/query-service-graphql.md; point docs/graphql.md at the KB. Implements [[tasks/graphql-qs-harden-1]]
- harden-12: anonymous __schema denied when introspection_for_anonymous(false) - harden-13: statement_timeout ZERO yields client TIMEOUT via execute path - harden-11: graphiql_enabled_from_env_vars(production) + HTTP GET 405; scaffold calls shared graphiql_enabled_from_env Implements [[tasks/graphql-qs-harden-1]]
Extract apply_statement_timeout + client_error_for_execute_err with paused-clock / pure unit tests. Integration holds BEGIN EXCLUSIVE so the engine SELECT cannot race past the budget (no Duration::ZERO flake). Implements [[tasks/graphql-qs-harden-1]]
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/metrics.rs (1)
591-600: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
reset()does not clear GraphQL metric maps.The
#[cfg(test)] reset()method clears all other metric maps but omitsgraphql_request_totalandgraphql_request_duration. This inconsistency could cause stale GraphQL metrics to persist across tests that callreset_for_tests(), leading to test interference.Proposed fix
fn reset(&self) { self.lock(&self.service_info).clear(); self.lock(&self.dispatch_total).clear(); self.lock(&self.dispatch_duration).clear(); self.lock(&self.transport_messages_total).clear(); self.lock(&self.transport_failures_total).clear(); self.lock(&self.outbox_messages_total).clear(); self.lock(&self.outbox_pending_messages).clear(); self.lock(&self.outbox_oldest_pending_age_seconds).clear(); + self.lock(&self.graphql_request_total).clear(); + self.lock(&self.graphql_request_duration).clear(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/metrics.rs` around lines 591 - 600, Update the test-only reset method to also clear the graphql_request_total and graphql_request_duration metric maps, alongside the existing metrics in reset, so reset_for_tests removes all metric state between tests.
🧹 Nitpick comments (1)
src/telemetry.rs (1)
116-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding a
metric_labels::ROOT_FIELDconstant for consistency.All other entries in
ALLOWED_METRIC_LABELSreferencesuper::metric_labels::*constants, but"root_field"is a raw string literal. The same raw string appears inGraphqlCounterKey::labels()andGraphqlHistogramKey::labels()insrc/metrics.rs. A named constant would prevent drift if the label name ever changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/telemetry.rs` around lines 116 - 117, Define a metric_labels::ROOT_FIELD constant for the "root_field" label, then replace the raw string in ALLOWED_METRIC_LABELS and in GraphqlCounterKey::labels() and GraphqlHistogramKey::labels() with that constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/graphql/execute.rs`:
- Around line 70-71: Update the SQLite result-column handling around row.try_get
in the execute flow to propagate decode or driver errors instead of converting
them to None and falling back to "null". Return the error in the same manner as
the Postgres path, while preserving the existing "null" fallback only for an
actual SQL NULL value.
In `@tests/graphql_compile/main.rs`:
- Around line 32-38: Update the SQLite setup in the test around the sqlx::query
call to avoid semicolon-separated CREATE TABLE and INSERT statements; execute
them separately or use sqlx::raw_sql(), ensuring the INSERT runs before the
existing row-count assertion.
In `@tests/graphql_postgres/main.rs`:
- Around line 40-46: Split the database seeding in the test setup into separate
sqlx::query calls: execute the CREATE TABLE statement first, then execute the
INSERT statement in a second call, preserving the existing pool and
expect("seed") error handling.
---
Outside diff comments:
In `@src/metrics.rs`:
- Around line 591-600: Update the test-only reset method to also clear the
graphql_request_total and graphql_request_duration metric maps, alongside the
existing metrics in reset, so reset_for_tests removes all metric state between
tests.
---
Nitpick comments:
In `@src/telemetry.rs`:
- Around line 116-117: Define a metric_labels::ROOT_FIELD constant for the
"root_field" label, then replace the raw string in ALLOWED_METRIC_LABELS and in
GraphqlCounterKey::labels() and GraphqlHistogramKey::labels() with that
constant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 62cca900-2a90-49ca-b4fa-fd29179ab508
📒 Files selected for processing (17)
Cargo.tomlREADME.mddistributed_cli/src/generate/mod.rsdistributed_cli/src/generate/service_crate.rsdistributed_cli/tests/cli_skills_init.rsdocs/graphql.mdsrc/graphql/compile.rssrc/graphql/engine.rssrc/graphql/execute.rssrc/graphql/mod.rssrc/graphql/schema.rssrc/metrics.rssrc/telemetry.rstests/graphql_compile/main.rstests/graphql_harden/main.rstests/graphql_http/main.rstests/graphql_postgres/main.rs
✅ Files skipped from review due to trivial changes (3)
- distributed_cli/tests/cli_skills_init.rs
- docs/graphql.md
- README.md
🚧 Files skipped from review as they are similar to previous changes (6)
- src/graphql/mod.rs
- src/graphql/engine.rs
- distributed_cli/src/generate/mod.rs
- distributed_cli/src/generate/service_crate.rs
- src/graphql/schema.rs
- src/graphql/compile.rs
Collaborator
Author
|
🤖 Agent dispatch: Branch: # Watch live:
atc watch --id tasks--graphql-qs-epic@review-fix@1783828313788-e4e0
# View logs:
atc logs tasks--graphql-qs-epic@review-fix@1783828313788-e4e0
# Attach to tmux:
tmux attach -t tasks--graphql-qs-epic@review-fix@1783828313788-e4e0 |
Implements [[tasks--graphql-qs-epic]]
Collaborator
Author
Review-fix summaryPushed review-fix commit
Additional review flywheel fixes in the same commit: corrected has-many joins when parent PK and child FK names differ, prevented bidirectional relationship schema recursion, applied many-to-many client relationship filters instead of dropping them, expanded subscription table-footprint tracking, and cleaned up clippy issues across the touched GraphQL/test files. Build status:
Confidence: 100%. Human review needed: none flagged. |
|
I'll re-review the updated diff to verify each fix lands as described, particularly the bind-order reordering, BelongsTo join direction fix, and nested relationship aggregate execution, since those are the highest-risk changes. ᕮᕭ ᐡ•ᴥ•ᐡ nice work getting through the whole flywheel list ✅ Action performedReview finished.
|
Modularize tests/graphql_harden (authz/inject/dos/errors/transport). Add A2/A3 claim isolation on by_pk + aggregate, A5 nested column deny, S*/D*/E*/T* cases, max_bool_width breadth DoS bound. Fixes: empty by_pk → null (fetch_optional); resolve_root returns None for JSON null so nullable by_pk does not break non-null children. Implements [[tasks/graphql-qs-redteam-1]] [[tasks/graphql-qs-redteam-2]] [[tasks/graphql-qs-redteam-3]] [[tasks/graphql-qs-redteam-4]] [[tasks/graphql-qs-redteam-5]] [[tasks/graphql-qs-redteam-6]] [[tasks/graphql-qs-redteam-7]]
…lects
SQLite positional `?` must follow SQL text order: nested has_many LIMIT/
OFFSET binds appear in SELECT before outer WHERE. Revert where-before-
projection for ByPk. Regression: parents_by_pk { children { … } }.
Implements [[tasks/graphql-qs-redteam-1]]
Centralize relationship join SQL (HasMany/BelongsTo/m2m helpers), dialect JSON/ILIKE fragments via DialectOps, and bind application shared by SQLite/Postgres executors. Adds pure unit tests for join matrix, dialect ops, parse_claim, and sanitize_compile_error. Implements [[tasks/graphql-qs-quality-1]] ship set (dedup-2/3/4, unit-2/6/7, maintain-3).
- Reject PG-only JSON ops (_contains/_has_key/_contained_in) on SQLite - Reject multi-column PK at GraphqlEngine build (v1 single-col policy) - Refine metrics status: timeout|bad_request|forbidden|internal|error - Shared tests/support/graphql fixtures for harden suite - Soft-skip contract tests (schema gate + compile defense-in-depth) - Residual red-team: A8 nested grant, A12 rel where, S9, E4 Implements [[tasks/graphql-qs-quality-2]] children 3–8.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/graphql_sqlite/main.rs (1)
314-345: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUnnecessary
.clone()and stale exploratory comments — same pattern already cleaned up elsewhere in this file.
schemaisn't reused after line ~318, so.table_schema(schema.clone())can just takeschema. The multi-line "Value-based path... Spec API: .permission requires..." comment block reads like leftover developer notes rather than documentation — the same category of issue a prior review already had removed fromlist_filter_and_by_pkin this file (addressed in commit39edfc1), just in a different function this time.🧹 Proposed cleanup
async fn permissions_filter_by_claim() { let schema = orders_schema(); let manifest = - distributed::DistributedProjectManifest::new("orders").table_schema(schema.clone()); + distributed::DistributedProjectManifest::new("orders").table_schema(schema); let pool = setup_pool().await; - // Value-based path: grant_all then we need typed permission — use builder - // with table_schema upgrade. from_manifest exposes all ReadModel tables. - // Use permission via a hand-built approach: grant_all for user is full; - // for restricted, register with filter via engine builder internals... - // Spec API: .permission requires RelationalReadModelIncludes. - // For fixture without derive, use grant_all and a second role without grants. + // `from_manifest` exposes all read-model tables; grant_all("user") is full access, + // "anonymous" is left ungranted to exercise the no-grants path below. let engine = GraphqlEngine::from_manifest(&manifest, pool)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/graphql_sqlite/main.rs` around lines 314 - 345, In permissions_filter_by_claim, pass schema directly to DistributedProjectManifest::table_schema instead of cloning it, since it is not reused. Remove the stale exploratory comment block above the engine construction, keeping only comments that document the test’s intended behavior.
🧹 Nitpick comments (1)
tests/graphql_harden/authz.rs (1)
200-208: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
RelationshipDeffixture construction.The parent→children
HasManyrelationship literal is built identically in botha5_nested_relationship_column_allowlist_deniesandparent_child_engine. Extracting a shared helper avoids fixture drift between the two.♻️ Proposed extraction
+fn parent_with_children_relationship() -> TableSchema { + let mut parent = ParentView::schema().clone(); + parent.relationships = vec![RelationshipDef { + field_name: "children".into(), + kind: RelationshipKind::HasMany, + target_model: "ChildView".into(), + foreign_key: Some("parent_id".into()), + through: None, + target_foreign_key: None, + }]; + parent +} + async fn a5_nested_relationship_column_allowlist_denies() { ... - let mut parent = ParentView::schema().clone(); - parent.relationships = vec![RelationshipDef { - field_name: "children".into(), - kind: RelationshipKind::HasMany, - target_model: "ChildView".into(), - foreign_key: Some("parent_id".into()), - through: None, - target_foreign_key: None, - }]; + let parent = parent_with_children_relationship();(apply similarly in
parent_child_engine)Also applies to: 256-263
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/graphql_harden/authz.rs` around lines 200 - 208, Extract the duplicated parent-to-children RelationshipDef construction into a shared fixture helper, then use that helper when assigning parent.relationships in both a5_nested_relationship_column_allowlist_denies and parent_child_engine. Preserve the existing HasMany configuration and avoid maintaining separate relationship literals.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/graphql_harden/inject.rs`:
- Around line 105-123: Update s5_order_by_junk_direction_is_safe so the order_by
direction uses a deliberately invalid or garbage enum value rather than the
valid asc value. Keep the existing execution, SQL-leak assertion, and
conditional successful-response handling unchanged so the test actually covers
invalid direction input.
In `@tests/support/graphql.rs`:
- Around line 50-72: Update seed_orders to configure SqlitePoolOptions with
max_connections(1) before connecting to the in-memory SQLite database, ensuring
all queries use the same physical connection and see the seeded orders table.
---
Outside diff comments:
In `@tests/graphql_sqlite/main.rs`:
- Around line 314-345: In permissions_filter_by_claim, pass schema directly to
DistributedProjectManifest::table_schema instead of cloning it, since it is not
reused. Remove the stale exploratory comment block above the engine
construction, keeping only comments that document the test’s intended behavior.
---
Nitpick comments:
In `@tests/graphql_harden/authz.rs`:
- Around line 200-208: Extract the duplicated parent-to-children RelationshipDef
construction into a shared fixture helper, then use that helper when assigning
parent.relationships in both a5_nested_relationship_column_allowlist_denies and
parent_child_engine. Preserve the existing HasMany configuration and avoid
maintaining separate relationship literals.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e739371-009f-4092-8717-6d9d49cdc1b3
📒 Files selected for processing (34)
distributed_cli/src/cli.rsdistributed_cli/src/generate/gitops.rsdistributed_cli/src/generate/mod.rsdistributed_cli/src/generate/service_crate.rssrc/graphql/commands.rssrc/graphql/compile.rssrc/graphql/engine.rssrc/graphql/execute.rssrc/graphql/filter.rssrc/graphql/http.rssrc/graphql/permissions.rssrc/graphql/schema.rssrc/graphql/subscribe.rssrc/graphql/types.rssrc/postgres_repo/mod.rssrc/telemetry.rstests/graphql_commands/main.rstests/graphql_compile/main.rstests/graphql_engine/main.rstests/graphql_harden/authz.rstests/graphql_harden/common.rstests/graphql_harden/dos.rstests/graphql_harden/errors.rstests/graphql_harden/inject.rstests/graphql_harden/main.rstests/graphql_harden/residual.rstests/graphql_harden/softskip.rstests/graphql_harden/transport.rstests/graphql_http/main.rstests/graphql_postgres/main.rstests/graphql_sdl/main.rstests/graphql_sqlite/main.rstests/graphql_subscriptions_sqlite/main.rstests/support/graphql.rs
🚧 Files skipped from review as they are similar to previous changes (18)
- src/postgres_repo/mod.rs
- src/telemetry.rs
- tests/graphql_compile/main.rs
- tests/graphql_postgres/main.rs
- src/graphql/commands.rs
- tests/graphql_subscriptions_sqlite/main.rs
- distributed_cli/src/cli.rs
- src/graphql/http.rs
- src/graphql/subscribe.rs
- src/graphql/permissions.rs
- src/graphql/execute.rs
- tests/graphql_sdl/main.rs
- src/graphql/filter.rs
- src/graphql/schema.rs
- src/graphql/types.rs
- distributed_cli/src/generate/mod.rs
- tests/graphql_http/main.rs
- src/graphql/compile.rs
Comment on lines
+105
to
+123
| async fn s5_order_by_junk_direction_is_safe() { | ||
| let pool = seed_orders().await; | ||
| let engine = engine_all_columns(pool); | ||
| let s = session("user", "x"); | ||
| // Unknown enum may fail GraphQL validation or coerce — either way no SQL leak. | ||
| let resp = engine | ||
| .execute( | ||
| &s, | ||
| Request::new( | ||
| r#"{ orders(order_by: [{ status: asc }]) { order_id status } }"#, | ||
| ), | ||
| ) | ||
| .await; | ||
| assert_no_sql_leak(&resp); | ||
| if resp.errors.is_empty() { | ||
| let data = serde_json::to_value(&resp.data).unwrap(); | ||
| assert!(!data["orders"].as_array().unwrap().is_empty()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test doesn't actually exercise a "junk" order_by direction.
The doc comment claims to test a garbage/invalid direction, but asc is a valid order_by enum value — this doesn't exercise the "junk direction" scenario the test name/comment promises, so S5 coverage is effectively a no-op for its stated threat.
🐛 Proposed fix to actually exercise an invalid direction
- Request::new(
- r#"{ orders(order_by: [{ status: asc }]) { order_id status } }"#,
- ),
+ Request::new(
+ r#"{ orders(order_by: [{ status: totally_bogus_direction }]) { order_id status } }"#,
+ ),📝 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.
Suggested change
| async fn s5_order_by_junk_direction_is_safe() { | |
| let pool = seed_orders().await; | |
| let engine = engine_all_columns(pool); | |
| let s = session("user", "x"); | |
| // Unknown enum may fail GraphQL validation or coerce — either way no SQL leak. | |
| let resp = engine | |
| .execute( | |
| &s, | |
| Request::new( | |
| r#"{ orders(order_by: [{ status: asc }]) { order_id status } }"#, | |
| ), | |
| ) | |
| .await; | |
| assert_no_sql_leak(&resp); | |
| if resp.errors.is_empty() { | |
| let data = serde_json::to_value(&resp.data).unwrap(); | |
| assert!(!data["orders"].as_array().unwrap().is_empty()); | |
| } | |
| } | |
| async fn s5_order_by_junk_direction_is_safe() { | |
| let pool = seed_orders().await; | |
| let engine = engine_all_columns(pool); | |
| let s = session("user", "x"); | |
| // Unknown enum may fail GraphQL validation or coerce — either way no SQL leak. | |
| let resp = engine | |
| .execute( | |
| &s, | |
| Request::new( | |
| r#"{ orders(order_by: [{ status: totally_bogus_direction }]) { order_id status } }"#, | |
| ), | |
| ) | |
| .await; | |
| assert_no_sql_leak(&resp); | |
| if resp.errors.is_empty() { | |
| let data = serde_json::to_value(&resp.data).unwrap(); | |
| assert!(!data["orders"].as_array().unwrap().is_empty()); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/graphql_harden/inject.rs` around lines 105 - 123, Update
s5_order_by_junk_direction_is_safe so the order_by direction uses a deliberately
invalid or garbage enum value rather than the valid asc value. Keep the existing
execution, SQL-leak assertion, and conditional successful-response handling
unchanged so the test actually covers invalid direction input.
Comment on lines
+50
to
+72
| pub async fn seed_orders() -> sqlx::SqlitePool { | ||
| let pool = SqlitePoolOptions::new() | ||
| .connect("sqlite::memory:") | ||
| .await | ||
| .unwrap(); | ||
| sqlx::query( | ||
| "CREATE TABLE orders ( | ||
| order_id TEXT PRIMARY KEY, | ||
| customer_id TEXT NOT NULL, | ||
| status TEXT NOT NULL, | ||
| total_cents INTEGER NOT NULL, | ||
| note TEXT NOT NULL | ||
| ); | ||
| INSERT INTO orders VALUES | ||
| ('o1', 'tenant-a', 'open', 100, '{\"looks\":\"json\"}'), | ||
| ('o2', 'tenant-a', 'shipped', 200, 'plain'), | ||
| ('o3', 'tenant-b', 'open', 50, 'x');", | ||
| ) | ||
| .execute(&pool) | ||
| .await | ||
| .unwrap(); | ||
| pool | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how the engine acquires SQLite connections, and whether any in-memory pool
# helper already caps max_connections or uses a shared cache.
rg -n "max_connections|cache=shared|\.acquire\(" src/graphql/engine.rs src/sqlx_repo/repo.rs
rg -n "SqlitePoolOptions::new\(\)" tests -g '*.rs' -A3Repository: hops-ops/distributed
Length of output: 5657
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the helper and the engine call path around SQLite pool usage.
ast-grep outline tests/support/graphql.rs --view expanded
printf '\n---\n'
ast-grep outline tests/graphql_engine/main.rs --view expanded
printf '\n---\n'
ast-grep outline src/sqlx_repo/repo.rs --view expanded
printf '\n=== tests/support/graphql.rs ===\n'
cat -n tests/support/graphql.rs | sed -n '1,180p'
printf '\n=== tests/graphql_engine/main.rs ===\n'
cat -n tests/graphql_engine/main.rs | sed -n '1,220p'
printf '\n=== src/sqlx_repo/repo.rs ===\n'
cat -n src/sqlx_repo/repo.rs | sed -n '1280,1375p'Repository: hops-ops/distributed
Length of output: 25044
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect GraphqlEngine's execution path and any pool/connection handling it performs.
fd -a 'graphql' src tests | sed -n '1,120p'
printf '\n=== outlines ===\n'
ast-grep outline src/graphql --view expanded || true
printf '\n=== search execute / acquire / transaction / connection usage ===\n'
rg -n "impl GraphqlEngine|async fn execute|fn execute\\(|pool\\.acquire\\(|acquire\\(|begin\\(|transaction\\(" src/graphql -g '*.rs' -A4 -B4
printf '\n=== search for SqlitePoolOptions patterns in graphql source ===\n'
rg -n "SqlitePoolOptions::new\\(|cache=shared|max_connections\\(" src/graphql tests/support tests/graphql_* -g '*.rs' -A3 -B3Repository: hops-ops/distributed
Length of output: 28904
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the SQLite execution path in GraphqlEngine to see whether it acquires
# a pooled connection for each query.
cat -n src/graphql/execute.rs | sed -n '1,220p'Repository: hops-ops/distributed
Length of output: 9706
Cap the in-memory SQLite pool at one connection
GraphqlEngine::execute reads through fetch_optional(pool), so concurrent GraphQL queries can grab different physical connections. With sqlite::memory: and no max_connections(1) or shared-cache URI, later connections can see a fresh empty database.
🔒 Proposed fix
pub async fn seed_orders() -> sqlx::SqlitePool {
let pool = SqlitePoolOptions::new()
+ .max_connections(1)
.connect("sqlite::memory:")
.await
.unwrap();📝 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.
Suggested change
| pub async fn seed_orders() -> sqlx::SqlitePool { | |
| let pool = SqlitePoolOptions::new() | |
| .connect("sqlite::memory:") | |
| .await | |
| .unwrap(); | |
| sqlx::query( | |
| "CREATE TABLE orders ( | |
| order_id TEXT PRIMARY KEY, | |
| customer_id TEXT NOT NULL, | |
| status TEXT NOT NULL, | |
| total_cents INTEGER NOT NULL, | |
| note TEXT NOT NULL | |
| ); | |
| INSERT INTO orders VALUES | |
| ('o1', 'tenant-a', 'open', 100, '{\"looks\":\"json\"}'), | |
| ('o2', 'tenant-a', 'shipped', 200, 'plain'), | |
| ('o3', 'tenant-b', 'open', 50, 'x');", | |
| ) | |
| .execute(&pool) | |
| .await | |
| .unwrap(); | |
| pool | |
| } | |
| pub async fn seed_orders() -> sqlx::SqlitePool { | |
| let pool = SqlitePoolOptions::new() | |
| .max_connections(1) | |
| .connect("sqlite::memory:") | |
| .await | |
| .unwrap(); | |
| sqlx::query( | |
| "CREATE TABLE orders ( | |
| order_id TEXT PRIMARY KEY, | |
| customer_id TEXT NOT NULL, | |
| status TEXT NOT NULL, | |
| total_cents INTEGER NOT NULL, | |
| note TEXT NOT NULL | |
| ); | |
| INSERT INTO orders VALUES | |
| ('o1', 'tenant-a', 'open', 100, '{\"looks\":\"json\"}'), | |
| ('o2', 'tenant-a', 'shipped', 200, 'plain'), | |
| ('o3', 'tenant-b', 'open', 50, 'x');", | |
| ) | |
| .execute(&pool) | |
| .await | |
| .unwrap(); | |
| pool | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/support/graphql.rs` around lines 50 - 72, Update seed_orders to
configure SqlitePoolOptions with max_connections(1) before connecting to the
in-memory SQLite database, ensuring all queries use the same physical connection
and see the seeded orders table.
Default GraphqlEngineBuilder.strict_where to true so unknown or ungranted client where/order_by keys fail the request instead of soft-skipping. Opt-in .strict_where(false) restores compile soft-skip. Sanitize maps unknown/ungranted filter messages to invalid filter. Tests: pure compile_order_by matrix; harden softskip/inject fail-closed contracts; claim AuthZ unchanged. Implements [[tasks/graphql-qs-ship-strict-1]].
Share portable vs Postgres JSON comparison ops in naming and gate both dynamic schema and SDL so SQLite never advertises _contains / _contained_in / _has_key. SdlOptions::sqlite()/postgres() align artifacts with engine dialect. Compile reject remains defense-in-depth. Tests: harden dialect JSON unknown-field on SQLite; SDL omit/include matrix; naming unit matrix. Implements [[tasks/graphql-qs-ship-dialect-1]].
Add relationship-aware complexity estimation before SQL compile so multi-level has_many trees are bounded by max_complexity (default 500), not only max_depth. async-graphql dynamic fields remain flat; our estimator multiplies list relationship child cost by list_fanout (5). Tests: dos d8 nested fan-out under/over budget; pure weight formula. deep_parse limiting deferred as unnecessary given weights. Implements [[tasks/graphql-qs-ship-complexity-1]].
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/graphql/complexity.rs (1)
263-286: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnit test doesn't exercise the real estimator — it re-derives the formula.
nested_has_many_costs_more_than_flat_scalarsrecomputes the cost by hand from the same weights and never callsestimate_root_complexity/estimate_field, so it can't catch a regression in the actual traversal logic (only that the arithmetic constants relate as expected). Theparent_schema()/child_schema()/grand_schema()/selbuilders are constructed only to be discarded vialet _ = (...).The end-to-end path is covered by
dos.rs, but a focused unit test on this DoS-protection code would be more valuable. Consider either drivingestimate_root_complexitywith the built schemas + aSelectionNodetree, or dropping the unused builders to avoid implying coverage that isn't there.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/graphql/complexity.rs` around lines 263 - 286, Update nested_has_many_costs_more_than_flat_scalars to exercise the real complexity estimator by passing the existing parent_schema, child_schema, grand_schema, and sel values into estimate_root_complexity (and related traversal APIs as needed), then assert on those returned costs. Remove the hand-recomputed formulas and discarded let _ binding, while preserving the existing comparisons between flat, one-level, and three-level selections.src/graphql/naming.rs (1)
93-122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the shared comparison-op constants in
src/graphql/sdl.rs.
emit_comparison_exphardcodes_eqthrough_is_null, soPORTABLE_COMPARISON_OPScan drift from the generated SDL. Iterating the shared constant here would keep SDL and runtime schema in sync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/graphql/naming.rs` around lines 93 - 122, The SDL comparison-expression generation in emit_comparison_exp currently hardcodes the portable operators, allowing it to diverge from PORTABLE_COMPARISON_OPS. Replace that hardcoded operator list with iteration over the shared PORTABLE_COMPARISON_OPS constant, while preserving the existing handling for string-specific and Postgres JSON operators.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/graphql/complexity.rs`:
- Around line 263-286: Update nested_has_many_costs_more_than_flat_scalars to
exercise the real complexity estimator by passing the existing parent_schema,
child_schema, grand_schema, and sel values into estimate_root_complexity (and
related traversal APIs as needed), then assert on those returned costs. Remove
the hand-recomputed formulas and discarded let _ binding, while preserving the
existing comparisons between flat, one-level, and three-level selections.
In `@src/graphql/naming.rs`:
- Around line 93-122: The SDL comparison-expression generation in
emit_comparison_exp currently hardcodes the portable operators, allowing it to
diverge from PORTABLE_COMPARISON_OPS. Replace that hardcoded operator list with
iteration over the shared PORTABLE_COMPARISON_OPS constant, while preserving the
existing handling for string-specific and Postgres JSON operators.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d087a348-2d13-42ce-ae4b-b3e6a42080f6
📒 Files selected for processing (13)
src/graphql/compile.rssrc/graphql/complexity.rssrc/graphql/engine.rssrc/graphql/mod.rssrc/graphql/naming.rssrc/graphql/schema.rssrc/graphql/sdl.rstests/graphql_harden/dialect.rstests/graphql_harden/dos.rstests/graphql_harden/inject.rstests/graphql_harden/main.rstests/graphql_harden/softskip.rstests/graphql_sdl/main.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/graphql_harden/main.rs
- tests/graphql_sdl/main.rs
- src/graphql/sdl.rs
- src/graphql/mod.rs
- src/graphql/engine.rs
- src/graphql/compile.rs
- src/graphql/schema.rs
Implement IdentityMode (TrustedProxy/OidcBearer/Hybrid/DevHeaders) with JWT access-token validation, claim→Session mapping, HTTP 401 path, and scaffold OidcBearer defaults. Always-on mock JWKS suite covers fixtures F1–F10; Zitadel e2e is env-gated and skips offline. Implements [[tasks/graphql-qs-ship-identity-1]]
Move public_oidc_identity_from_env into the library; scaffold always calls it. Unset OIDC_* uses placeholder issuer with require_auth — ambient headers rejected. Tests drive shipped env_vars path; generated query mod asserts no DevHeaders default.
Add integration-graphql workflow with always-on F1–F10 suite and a live Zitadel compose job (JWT-bearer bootstrap). Wire into on-pr-quality and on-push-main version gate. Offline ZITADEL_E2E path still skips cleanly.
Root cause: FirstInstance could not write machinekey on bind mount
(permission denied). Fix with world-writable machinekey dir + root user,
shared oidc-zitadel-up.sh entrypoint for local and CI, bootstrap jq fixes,
and project-scoped token audience for JWT validation.
Local: ./scripts/oidc-zitadel-up.sh && source graphql-oidc.env && cargo test
--test graphql_oidc_zitadel --features graphql,sqlite,metrics
/debug/ready returns before management routes accept traffic, causing HTTP 503 on projects/_search in GHA. Retry token exchange and management calls until 200; document that probe is early and management wait is authoritative.
Shared E1–E8 runner; Keycloak/Authentik compose+bootstrap live suites; Okta gate hard-fail contract; multi-audience + issuer slash tolerance; CI offline matrix + live Keycloak/Authentik jobs. Implements [[tasks/graphql-oidc-providers-1]]
Only wipe machinekey/e2e; auto-recreate compose volume if admin SA key is missing so JWT-bearer live E1–E8 stays reproducible.
Strict E1 requires engine role + row isolation; E5 uses signed expired JWT via static JWKS (ValidationError::Expired). Wire Keycloak realm roles, Authentik groups on M2M users, Zitadel project-scoped roles + projects:roles scope. Scan urn:zitadel:iam:org:project:*:roles in claim map. Addresses verifier gaps on soft-pass E1 and unsigned E5.
Remove cloud-only Okta suite and offline matrix entry. Compose-backed providers remain: Zitadel, Keycloak, Authentik. Generic OIDC still works with any Okta tenant in production.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the GraphQL query-service epic (
tasks/graphql-qs-epic/specs/query-layer/*): Hasura-style read-only GraphQL over relational read models, with deny-by-default RBAC, dialect SQL compilation (SQLite + Postgres), commit-path live subscriptions, command mutations, CLI skill/scaffold, GraphiQL, and provider-portable OIDC identity with live multi-IdP e2e.Engine & surface
by_pk/ aggregates / where / order_by / relationshipsdctl schema --format graphqlSDL artifactmax_complexity)Live queries & mutations
CommandRequestdispatch → projection → queryCLI & DX
distributed-graphqlskill,dctl scaffold --query-api, gitopsDATABASE_URL/queryApi.databaseUrlcargo run --example graphiql --features "graphql,sqlite"→ http://127.0.0.1:4000/graphqlGRAPHIQL=0to disable; off in prod scaffold)Identity & OIDC (shipped)
TrustedProxy·OidcBearer·Hybrid·DevHeadersalg=none)sub, roles/groups, Zitadel project-scoped roles)OidcBearer(never ambient DevHeaders) — D6azp/client_idfallback (Keycloak client_credentials)Multi-provider live e2e (compose-backed only)
Shared E1–E8 contract (
tests/graphql_oidc_common) against shipped GraphQL HTTP +OidcBearer:./scripts/oidc-zitadel-up.shZITADEL_E2E=1./scripts/oidc-keycloak-up.shKEYCLOAK_E2E=1./scripts/oidc-authentik-up.shAUTHENTIK_E2E=1CI (
.github/workflows/integration-graphql.yaml)Wired into PR quality pipeline:
cargo test)Breaking
feat!metadata batch:TableSchema/ relationships /TableKind(operational tables never exposed on GraphQL)Test plan
Local live e2e
Notes
OIDC_ISSUER/OIDC_AUDIENCE(and JWKS); scaffold rejects ambient headers without a valid Bearer.specs/query-layer/*(identity, oidc-e2e, oidc-zitadel, oidc-keycloak, oidc-authentik).Implements [[tasks/graphql-qs-epic]] · [[tasks/graphql-qs-ship-identity-1]] · [[tasks/graphql-oidc-providers-1]]