Repository navigation
feat(bucket-provisioner): provisionBucket returns the bucket row (#2270) - #1914
Conversation
…tamps for reconciliation
|
Review complete. 🟠 1 high · 🟡 1 medium 💬 Inline comments (2)
The change reworks the
Reviewed commit: 9c2f2ed |
There was a problem hiding this comment.
This PR updates the graphile-bucket-provisioner-plugin to make provisionBucket idempotent and return the bucket record with a provisionRequestedAt timestamp instead of a job id, removing jobId from the payload type.
Key findings
- 🟠 Provision SELECT assumes row-returning
_provisionfunction — plugin.ts:203 - 🟡 Remove
jobIdfromProvisionBucketPayloadwithout deprecation — plugin.ts:239
| type ProvisionBucketPayload { | ||
| """The logical bucket row that was queued for reconciliation.""" | ||
| """The logical bucket row whose reconciliation was requested.""" | ||
| bucketId: UUID! | ||
| bucketKey: String! | ||
| """The physical bucket name already recorded, or null when reconciliation has not completed.""" | ||
| physicalName: String | ||
| """The reconciler job enqueued to provision this bucket.""" | ||
| jobId: UUID! | ||
| """When reconciliation was last requested for this bucket.""" | ||
| provisionRequestedAt: Datetime! | ||
| } |
There was a problem hiding this comment.
🟡 api-data · medium
Remove jobId from ProvisionBucketPayload without deprecation
ProvisionBucketPayload.jobId: UUID! is deleted and replaced with provisionRequestedAt: Datetime! with no deprecation window (graphile/graphile-bucket-provisioner-plugin/src/plugin.ts:239-247). Any persisted operation selecting jobId now fails schema validation outright.
The repo itself is a victim: the integration suite still queries and asserts jobId, the schema snapshot still records jobId: UUID!, and the generated SDK still types jobId: string — all three fail against the new schema.
📋 Prompt for AI Agents
In graphile/graphile-bucket-provisioner-plugin/src/plugin.ts lines 239-247 the jobId: UUID! field of ProvisionBucketPayload was replaced by provisionRequestedAt: Datetime! with no deprecation, but several in-repo consumers still reference jobId and will fail against the new schema: graphql/server-test/tests/upload.integration.test.ts (query at line 107, assertions at lines 702, 716, 724-725), graphql/server-test/tests/snapshots/schema-snapshot.test.ts.snap (line 1618), and sdk/constructive-sdk/src/storage/orm/input-types.ts (lines 1901-1915). Update the integration test to request provisionRequestedAt and rework the distinct-job-id assertions around the returned timestamp, refresh the snapshot (or regenerate it), and regenerate the SDK payload types. If a deprecation window is preferred instead, restore jobId as a deprecated field backed by a nullable value.
| `SELECT id, key, physical_name, provision_requested_at FROM ${provisionFunction}($1::uuid)`, | ||
| [bucket.id], | ||
| ); |
There was a problem hiding this comment.
🟠 bug · high
Provision SELECT assumes row-returning _provision function
requestReconciliation now runs SELECT id, key, physical_name, provision_requested_at FROM <private>.<buckets>_provision($1::uuid) (graphile/graphile-bucket-provisioner-plugin/src/plugin.ts:203), which only succeeds if the generated function RETURNS TABLE with those columns. The only provision-function definition in this repo still RETURNS bigint and returns a job id, so the FROM-clause select fails at the SQL level against it.
Impact: every provisionBucket mutation errors with an opaque Postgres message against the current in-repo contract, and the server-test integration suite fails at runtime.
📋 Prompt for AI Agents
In graphile/graphile-bucket-provisioner-plugin/src/plugin.ts lines 201-205, requestReconciliation selects id, key, physical_name, provision_requested_at from the generated <private>.<buckets>_provision(uuid) function, but the only in-repo definition of that function (graphql/server-test/fixtures/seed/simple-seed-storage/auth-provision.sql lines 42-61, a copy of the constructive-db generator output) still RETURNS bigint and returns a job id, so the SELECT fails at parse time against it. Confirm the storage-module generator now emits a row-returning function; if it does, regenerate the seed fixture (and the buckets-table column in that fixture's schema.sql) to the new contract in this same change; if it does not, keep the scalar call form and fetch the stamped row from the buckets table separately. Also consider mapping the mismatch to a clear STORAGE_MODULE_OUTDATED-style error so deployed modules on the old contract fail with a useful message instead of a raw Postgres error.
This comment has been minimized.
This comment has been minimized.
…turning provisionBucket
…resh server-test schema snapshot
Companion to constructive-io/constructive-db#4088. Implements constructive-planning #2270 (option B, approved by Dan).
What changed
<buckets>_provision(bucket_id)in constructive-db no longer enqueues a job and returns its id. It now stamps the bucket'sprovision_requested_atas the caller, under the buckets table's RLS, and returns the bucket row. The bucket's ledger-routed UPDATE trigger then records and enqueues the job, keyed and queued on the bucket so repeated requests collapse into one.provisionBucketfollows that contract:SELECT id, key, physical_name, provision_requested_at FROM <private>.<buckets>_provision($1::uuid);ProvisionBucketPayloaddropsjobIdand gainsprovisionRequestedAt: Datetime!, so the payload is the bucket row:bucketId,bucketKey,physicalName,provisionRequestedAt;STORAGE_BUCKET_PROVISION_EMPTY. The SQL function raises on 0 rows itself, so this check is only a guard and is never reached silently.The README,
types.tsand the tests are updated to match.Before / after
Tests updated first, then run against the old
src/(graphile/graphile-bucket-provisioner-plugin,npx jest):After:
tsc --noEmitand eslint on the package are clean.Also updated to the new contract: the
simple-seed-storagefixture (auth-provision.sqlis now the generator's row-returning output,schema.sqlgainsprovision_requested_at),upload.integration.test.ts(assertsprovisionRequestedAtadvances on a repeat request instead of distinct job ids), and thegraphql/test+server-testschema snapshots.provisionRequestedAtis selected as ISO text so theDatetimescalar keeps sub-second precision (aDatewas serialized viaString(), dropping milliseconds).Written by Devin