Repository navigation
feat: sync function-resolution from constructive-db — routing_planes() returns resolver_schema_name - #141
Conversation
…lanes() returns resolver_schema_name
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
|
Review complete. 🟠 1 high · 🟡 1 medium 💬 Inline comments (1)
📍 Findings outside the diff (1) — 🟠 1 high — defects on lines GitHub can't attach comments to🟠 High — Deployed change edited in place without plan-version bump · routing_planes.sql:20–36 · // packages/function-resolution/deploy/schemas/function_resolution/procedures/routing_planes.sql
20 CREATE FUNCTION function_resolution.routing_planes()
21 RETURNS TABLE (
22 database_id uuid,
23 schema_name text,
24 resolver_schema_name text,
25 resolver_function_name text
26 ) AS $$
27 SELECT
28 rm.database_id,
29 s.schema_name::text,
30 s.schema_name::text,
31 rm.resolver_function_nameThe In the pgpm/Sqitch model an already-deployed change must never be modified: databases that deployed the old checksum never receive the new column, and plan-digest comparisons diverge from the committed source. Callers relying on This PR changes
Reviewed commit: 630a1f6 |
There was a problem hiding this comment.
The PR adds a resolver_schema_name output column to the routing_planes() resolver so callers can qualify generated functions from their own schema, but the deploy file was edited in place without a plan-version bump and the new column duplicates schema_name.
Key findings
- 🟠 Deployed change edited in place without plan-version bump — routing_planes.sql:20
- 🟡
resolver_schema_nameduplicatesschema_nameverbatim — routing_planes.sql:30
| SELECT | ||
| rm.database_id, | ||
| s.schema_name::text, | ||
| s.schema_name::text, |
There was a problem hiding this comment.
🟡 bug · medium
resolver_schema_name duplicates schema_name verbatim
The new resolver_schema_name column is populated with the same expression as schema_name (s.schema_name::text, packages/function-resolution/deploy/schemas/function_resolution/procedures/routing_planes.sql:30), making it an exact alias of the existing column.
The new header comment says it is "the schema its resolver is generated into, so callers qualify the resolver from it rather than assuming", but metaschema_modules_public.route_module stores no resolver-schema field, so the value can never differ from schema_name and the documented contract is not implemented by any data path.
If resolver generation ever targets a schema other than the plane's public schema, callers that qualify the resolver from this column will silently get the wrong schema. The updated test asserts resolver_schema_name === schema_name for every fixture, so it cannot detect the divergence.
📋 Prompt for AI Agents
In packages/function-resolution/deploy/schemas/function_resolution/procedures/routing_planes.sql line 30, replace the duplicate s.schema_name::text with an expression that yields the schema the resolver is actually generated into — e.g. add a resolver-schema source column to metaschema_modules_public.route_module (packages/metaschema-modules/deploy/schemas/metaschema_modules_public/tables/route_module/table.sql) and join on it. If a separate field is out of scope, amend the function's header comment (lines 15-19) to state that resolver_schema_name currently equals schema_name so callers do not rely on divergent semantics. Extend packages/function-resolution/tests/request-lane-reads.test.ts (around lines 218-224) with a fixture whose resolver schema differs from the plane's public schema. Mirror any SQL change into packages/function-resolution/sql/pgpm-function-resolution--0.47.6.sql and regenerate the bundle.
…ma_name tracks the readers' current schema
|
Replies to the Tenki review:
|
Summary
This syncs
function-resolutionfrom constructive-db#4174. It edits an existing deploy file only, sopgpm.planand versions are unchanged.function_resolution.routing_planes()gains aresolver_schema_namecolumn. It names the schema each plane's resolver is generated into, so callers qualify the resolver from it instead of assuming the plane's public schema:RETURNS TABLE ( database_id uuid, schema_name text, + resolver_schema_name text, resolver_function_name text ) ... s.schema_name::text, + s.schema_name::text, -- readers still live in the public schema rm.resolver_function_namerequest-lane-reads.test.tspins the new column. It's copied from db with the import swappedconstructive-test→pgsql-test. The SQL and bundle artifacts are regenerated with pgpm 5.18.0.Verified locally:
npx jest __tests__/request-lane-reads.test.ts __tests__/login-only-readers.test.tsgives 52 passed.After merge, publish with
git checkout main && git pull && npx lerna publish. That gives@pgpm/function-resolution@0.47.7, which the GraphQL server's fixture pins consume next.Link to Devin session: https://app.devin.ai/sessions/ea7371d8dbc441c49cb46e16d1d97517
Open in Devin Desktop: https://app.devin.ai/desktop/session/ea7371d8dbc441c49cb46e16d1d97517?variant=devin
Requested by: @pyramation