Add opt-in CNC scoped introspection - #1719
Conversation
d42cd85 to
8a83d70
Compare
a8fed57 to
fa39446
Compare
45cbb07 to
658736e
Compare
fa39446 to
0e14abd
Compare
658736e to
f7c63ec
Compare
0e14abd to
8a95026
Compare
|
Review complete. 🟡 3 medium 💬 Inline comments (2)
📍 Findings outside the diff (1) — 🟡 1 medium — defects on lines GitHub can't attach comments to🟡 Medium — Await // graphql/server/src/middleware/graphile.ts
351 const preset = await buildPreset(
352 pool,
353 schema || [],
354 anonRole,
355 roleName,
356 opts.api?.introspectionRole,
357 opts.graphile,
358 api.databaseSettings,
359 api.apiId,
360 compute
361 );
362 const creationPromise = observeGraphileBuild(
🧹 Nitpicks (1) — 🟢 1 low
This PR introduces a scoped-introspection feature that restricts the PostGraphile catalog to an app's dependency closure, plus a parameterized SQL query builder, a preset plugin, server middleware, and a perf harness. The feature is well-tested and the SQL is parameterized against injection.
Four findings: a documented-default mismatch for the scoped catalog type, capability-extension namespaces not approved in Reviewed commit: 8a95026 |
There was a problem hiding this comment.
Adds schema-scoped PostgreSQL catalog introspection (graphile-scoped-introspection) with a new scoped query builder, preset plugin, wiring, server middleware, and perf harness, surfaced through graphile-settings and graphql/env.
Key findings
- 🟡 Default scoped catalog type contradicts documented 'all' — scoped-introspection-service.ts:87
- 🟡 Approve capability-extension namespaces in
allowedSchemas— plugin.ts:130 - 🟡 Await
buildPresetbefore registering in-flight promise — graphile.ts:351
| introspectionScopedCatalogTypes: | ||
| introspectionScopedCatalogTypes ?? 'dependency-closure', |
There was a problem hiding this comment.
🟡 bug · medium
Default scoped catalog type contradicts documented 'all'
makeScopedPgService defaults introspectionScopedCatalogTypes to 'dependency-closure' (scoped-introspection-service.ts:88), while the ScopedIntrospectionServiceOptions contract documents the default as 'all' (graphql/types/src/graphile.ts:13). A consumer calling the exported factory without the option silently retains only the dependency closure instead of the documented full catalog, dropping schema types with no error.
📋 Prompt for AI Agents
In graphile/graphile-settings/src/scoped-introspection-service.ts lines 87-88, the fallback introspectionScopedCatalogTypes ?? 'dependency-closure' contradicts the documented default of 'all' in graphql/types/src/graphile.ts line 13. Decide the intended default: if 'all' is correct, change the fallback to ?? 'all' and update graphile/graphile-scoped-introspection/src/plugin.ts line 92 and graphql/server/src/middleware/graphile-introspection.ts line 64 accordingly; otherwise update the doc comment in graphql/types/src/graphile.ts to state the actual 'dependency-closure' default so the public contract matches the implementation.
| allowedSchemas: [ | ||
| ...new Set([...requiredSchemas, ...dependencySchemas, 'pg_catalog']), | ||
| ], |
There was a problem hiding this comment.
🟡 bug · medium
Approve capability-extension namespaces in allowedSchemas
allowedSchemas is built from requiredSchemas, dependency schemas, and pg_catalog only (plugin.ts:130), but the scoped SQL adds installed_extensions.extnamespace for every requested capability extension. When an extension such as pg_trgm lives in a schema outside that set (the default: extensions in public, app introspecting tenant_a), assertScopedNamespaces throws "crossed into unapproved dependency schema(s)" and aborts the schema build for an explicitly requested feature.
📋 Prompt for AI Agents
In graphile/graphile-scoped-introspection/src/plugin.ts around lines 120-134, the allowedSchemas array for scoped-required mode is built from requiredSchemas, dependencySchemas, and 'pg_catalog' only. Because the generated query's scoped_namespaces CTE (scoped-introspection-query.ts:349-351) includes installed_extensions.extnamespace for every requested capability extension, an extension installed in a schema outside that allowed set (e.g. pg_trgm in public while the app introspects tenant_a) causes assertScopedNamespaces (plugin.ts:159) to throw and abort schema building. Fix: resolve each extension in introspectionCapabilityExtensions to its extnamespace and add those namespaces to allowedSchemas before the assertion, so explicitly requested capability extensions do not fail closed.
Summary
graphile-scoped-introspectionpackage undergraphile/.GRAPHILE_INTROSPECTION_MODE=stock|scoped-required; absence defaults tostock.scoped-requiredis selected.Default and opt-in boundaries
stock(default)ConstructivePresetretains upstreamPgIntrospectionPlugin.graphile-settings.makePgServiceis the upstream PostGraphile factory, without CNC service fields or introspection settings.graphile-scoped-introspection, so its pinned upstream contract sentinel cannot affect stock startup.scoped-required(opt-in)ScopedIntrospectionPresetwhile constructing the schema handler.makeScopedPgServiceselects dependency-closure catalog retention, bounded introspection settings, configured dependency schemas, and configured capability extensions.Configuration is owned by
@constructive-io/graphql-envand forwarded through typedGraphileOptions; the plugin package does not readprocess.env. Precedence remains defaults → config file → environment → runtime override, and malformed explicit mode values fail during option resolution.Architecture
The scoped SQL is CNC-owned, static, and parameterized. It is adapted from the MIT-licensed
pg-introspection@1.0.1query structure and does not patch installed Graphile, Dataplan, or pg-introspection packages or import private package subpaths.The introspection mode, catalog policy, and PgService option contracts have a single owner in
@constructive-io/graphql-types. The scoped plugin re-exports those public types for compatibility;graphile-settingsconsumes the canonical contract directly and has no package dependency ongraphile-scoped-introspection.Database clients continue to use the normal three-argument
withPgClientFromPgService(service, settings, callback)lifecycle. Final pool teardown remains the responsibility ofPgService.release().Validation
@constructive-io/graphql-env: 1 suite / 14 tests passed, including default, valid values, malformed values, and config/env/runtime precedence.graphile-settings: 6 non-database suites / 47 tests passed, including upstream-default and scoped service wiring.@constructive-io/graphql-server: 14 suites / 145 tests passed, including proof that stock wiring does not invoke the scoped loader.graphile-scoped-introspection: 6 suites / 18 tests passed.@constructive-io/perf-harness: 6 suites / 11 tests passed, including the explicit worker CLI protocol.git diff --checkagainst the PR base passed.pnpm install --frozen-lockfileand offline frozen-lockfile install passed.graphile-settingshas no runtime reference to the scoped package.28375,28378), matching schema hash67dae5a669c8aaa4a8cbf71a943cc0854f8ad610e835c03204c9e178c2cbe5f3, successful runtime validation in both cases, and no database URL in the report.Follow-up
When Graphile progressive introspection is available, CNC can replace this concentrated opt-in package boundary with the upstream implementation while preserving the server configuration boundary.