feat(seed): add --upsert flag to update existing rows - #1910
Conversation
Seeding a workspace that already holds some of the rows fails the whole
batch for that type on the first duplicate id, inserting nothing and
exiting 1. That leaves no way to add only new rows to an already-seeded
workspace short of trimming the JSONL files by hand.
With --upsert, TailorDB writes use ON CONFLICT ("id") DO UPDATE. The
conflict target is fixed to id because TailorDB accepts only a single
column there, and id is excluded from the update set.
Rows are grouped by key signature before batching. Kysely fills keys
missing from a row with `default`, so mixing rows with different key
sets in one statement would overwrite stored values on conflict.
Built-In IdP users have no upsert primitive and surface an existing user
only as a createUser failure, so --upsert falls back to a name lookup
and update. Without this the whole command would still exit 1 on
re-runs even once TailorDB succeeded.
Default behavior is unchanged.
The upsert fallback cannot distinguish "user already exists" from an unrelated createUser failure, so a validation or permission error was reported only as whatever the lookup or update raised afterwards. Report both causes so the real reason survives.
The usage list already covers --truncate, so a new runtime flag that changes what seed writes belongs next to it.
🦋 Changeset detectedLatest commit: e575aee The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
🗺️ ERD previewNo relevant ERD changes in this update. |
This comment has been minimized.
This comment has been minimized.
![github-actions[bot]](https://avatars.github.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
📖 Docs Quality & Consistency Check
✅ Docs are consistent with the implementation and contain no user-facing internal-detail leaks.
Checked areas:
packages/sdk/docs/generator/builtin.md- Updated documentation for the new--upsertflag- Implementation in
packages/sdk/src/plugin/builtin/seed/- Verified behavior matches documentation packages/sdk/src/cli/commands/generate/seed/bundler.ts- Checked upsert logic for TailorDB rowspackages/sdk/src/plugin/builtin/seed/idp-user-processor.ts- Checked IdP user upsert behaviorpackages/sdk/src/plugin/builtin/seed/seed-data-loader.ts- Checked validation logic.changeset/seed-upsert-flag.md- Verified changeset description matches implementationexample/seed/exec.mjsand template files - Verified generated code consistency
Verified behaviors:
- ✅ Without
--upsert: Seeding fails on duplicate IDs (batch rejected, nothing written) - ✅ With
--upsert: TailorDB rows require anid; new IDs inserted, existing IDs updated - ✅ Optional fields absent from seed data keep their stored values in existing rows
- ✅ Required fields must be present in seed data for upsert (validated before execution)
- ✅ Built-In IdP users looked up by name, then created or updated accordingly
- ✅ Default behavior unchanged (upsert is opt-in)
Internal terms checked:
- ✅ No leaks of
TestExecScript/testExecScriptinto user-facing documentation - ✅ No leaks of internal module names (
parser,configure,cli) into user docs - ✅ JSDoc on exported
seedPluginfunction contains no internal implementation details
Re-run this check by adding the
docs-checklabel to the PR.
| namespace: { type: "string", short: "n" }, | ||
| "skip-idp": { type: "boolean", default: false }, | ||
| truncate: { type: "boolean", default: false }, | ||
| upsert: { type: "boolean", default: false }, |
There was a problem hiding this comment.
Please make id mandatory when --upsert is set.
Generated seed schemas mark id optional (Customer.schema.ts: pickFields([...], { optional: true })), and sqlaccess drops rows whose conflict target is null from the conflict check (service/sqlaccess/src/operator/insert.rs:232-235: "Null values are not considered as conflicts, so they are filtered out"). So a row without id is inserted under a fresh UUID on every run while the output still reads N rows upserted and exits 0.
Please fail on rows missing id, naming the file and line.
If you want lines-db to backfill the ids, LinesDB.sync() is not usable: createStandardSchema has no backward, so the hook-applied values held in SQLite are written straight back and previously omitted optional fields become explicit null (lib/src/database.ts:992-996) -- exactly the value-clobbering --upsert is meant to avoid. JsonlReader + JsonlWriter (both exported) let you add a UUID only to the rows that lack one.
|
|
||
| The `--machine-user` option is required at runtime if `machineUserName` is not configured in the generator options. | ||
|
|
||
| Without `--upsert`, a row whose id already exists fails the whole batch for that type, so nothing is written. Use `--upsert` to add only the new rows to a workspace that is already seeded; existing rows are updated from the seed data, and fields absent from a row keep their stored values. |
There was a problem hiding this comment.
"fields absent from a row keep their stored values" holds only for optional fields, so please say so.
sqlaccess runs the required-field check at insert.rs:66-80, ahead of the ON CONFLICT split at insert.rs:83, so a row missing a required field still fails the whole batch under --upsert with field \x` is required`.
The same wording in the changeset needs the qualifier too.
| @@ -20,11 +58,6 @@ describe("seed-bundler", () => { | |||
| aroundAll(async (runSuite) => { | |||
| await runSuite(); | |||
| delete process.env.TAILOR_SDK_OUTPUT_DIR; | |||
There was a problem hiding this comment.
fs.rmSync(TEST_BUNDLER_BASE, ...) was dropped from this aroundAll, leaving cleanup only in the new describe("seed script upsert behavior") aroundAll (:132-140).
Filter the second suite out -- e.g. pnpm test bundler.test.ts -t "seed-bundler" -- and that hook never runs, so __test_bundler__/ is left behind in the source tree.
Please move the cleanup to a file-level afterAll.
| throw new Error(\`create failed (\${createMessage}); upsert failed (\${updateMessage})\`); | ||
| } | ||
| } | ||
| processed++; |
There was a problem hiding this comment.
created and updated can be counted separately here, so please split them.
Add let created = 0; let updated = 0;, increment each right after a successful create and a successful updateUser, and return both so exec.mjs can print _User: N created, M updated. It also makes a silent overwrite of an existing user visible.
Doing the same on the TailorDB side is not cheap, so IdP-only is enough for this PR: Kysely's .execute() only yields an affected count, and the sqlaccess ON CONFLICT path likewise returns the affected count of a single bulk upsert against its internal table (storage/postgres/mod.rs:668-702), which cannot separate inserted from updated.
|
|
||
| // Rows must share one key set, which groupByColumns guarantees. | ||
| // TailorDB restricts the ON CONFLICT target to a single column, so "id" is the only key. | ||
| const write = (typeName: string, rows: Record<string, unknown>[]) => { |
There was a problem hiding this comment.
Please drop ON CONFLICT here and split into INSERT + UPDATE instead. Sorry about the churn -- recommending ON CONFLICT was my call, so this reversal is on me.
Verified against a live workspace using example's Invoice, which carries two serial fields: --upsert burns one serial value per record per run, conflicting rows included, and the numbers never come back. The four existing rows kept sequentialId 1..4 across runs while a newly added row came out at 21, and at 31 on a later cycle, instead of 5. inject_serial_values runs at insert.rs:61, ahead of the conflict split at insert.rs:83, and nextval is non-transactional. Plain Postgres behaves the same way so this is not a platform bug, but the gaps cannot be cleaned up in place: the only reset path is resetSequences() on truncate (migrator/table.go:353), which wipes the table.
A plain UPDATE draws nothing -- Operator::update never calls inject_serial_values, and live confirmation showed the next insert after an UPDATE advancing by exactly 1. So probe with SELECT "id" ... WHERE "id" IN (...), route new rows through the existing INSERT path, and route existing rows through db.updateTable(t).set(rest).where("id", "=", id). Both statement shapes were accepted by sqlaccess in the same live check.
This also removes groupByColumns entirely: UPDATE only touches the columns you set, so the "Kysely fills missing keys with default" hazard the grouping exists to dodge simply stops applying. And because the split is decided up front, the counts become exact inserted-vs-updated, which also removes the DO NOTHING over-count.
Costs worth weighing: one probe SELECT per type plus one UPDATE per existing row (parser/update.rs has no FROM clause, so no bulk update), a TOCTOU window that a single ON CONFLICT statement did not have, and a permission difference -- the conflict probe deliberately skips read-permission filtering (insert.rs:241-249) in a way a client-side SELECT cannot reproduce.
| await client.createUser(input.users[i]); | ||
| try { | ||
| await client.createUser(input.users[i]); | ||
| } catch (createError) { |
There was a problem hiding this comment.
Please look the user up first rather than inferring existence from a createUser failure.
As written, any createUser failure -- a transient 5xx, a rate limit, a policy rejection -- falls through to userByName + updateUser. If a user with that name happens to exist for an unrelated reason, this silently resets that real user's password and still exits 0. The _User seed schema is exactly { name, password }, so the blast radius is precisely a password reset.
Checking first sidesteps the part that genuinely cannot be done here, which is telling the errors apart. The IdP does return CodeAlreadyExists (service/idp/dataplane/grpc/service.go:227-228), but the runtime flattens it into new Error("createUser failed: " + status.to_string()) (ops/idp.rs, engine/js/idp.js:158-159), leaving only substring matching on a stringified status.
With a lookup first, a successful lookup means the user exists and no guessing is involved; a failed lookup falls through to createUser, so a user that really does exist surfaces as a reported AlreadyExists error instead of an overwrite. That converts a silent destructive write into a loud non-destructive one. userByName throws on not-found (GetUser returns CodeNotFound, service.go:181-185), so it still needs its own try/catch.
The cost is one extra lookup per user, and a create-after-lookup race would surface as an error rather than an overwrite -- both acceptable for a seed tool. It also makes the created/updated split fall out for free, since the branch is decided before any write.
Fixed in 908b4b1. |
This comment has been minimized.
This comment has been minimized.
toiroakr
left a comment
toiroakr
left a comment
There was a problem hiding this comment.
LGTM. All ten review threads are addressed in 503cb06, and the new index.test.ts coverage that pins the generated indentation is a good addition.
The merge is blocked on conflicts with main, so please rebase. Two files conflict, both stemming from fa2a0dea7 (fix(cli): resolve seed bundles from config directory):
packages/sdk/src/plugin/builtin/seed/index.test.ts-- add/add: main added a file with the same name. The twodescribeblocks are independent, so they can sit side by side.packages/sdk/src/cli/commands/generate/seed/bundler.test.ts--bundleSeedScriptgained a third argument (the config directory) on main.bundler.tsitself auto-merges, but the new upsert tests need to pass it.
Ping me once it is rebased and I will merge.
This comment has been minimized.
This comment has been minimized.
toiroakr
left a comment
toiroakr
left a comment
There was a problem hiding this comment.
LGTM. The merge with main resolves both conflicts correctly.
The only thing left is signatures: 0b6cab42c, 9b073817b, 8ce4cebbf and c2e16428a are unsigned, and main requires signed commits. Everything from 785f52b78 onward is fine. Please re-sign them and I will merge.
Done 🙏 |
This comment has been minimized.
This comment has been minimized.
Code Metrics Report (packages/sdk)
Details | | main (c5e1a27) | #1910 (ae0f932) | +/- |
|--------------------|----------------|-----------------|-------|
+ | Coverage | 75.4% | 75.8% | +0.4% |
| Files | 467 | 468 | +1 |
| Lines | 17741 | 17750 | +9 |
+ | Covered | 13390 | 13469 | +79 |
+ | Code to Test Ratio | 1:0.4 | 1:0.4 | +0.0 |
| Code | 120229 | 120861 | +632 |
+ | Test | 56943 | 57460 | +517 |Code coverage of files in pull request scope (45.7% → 75.1%)
SDK Configure Bundle Size
Runtime Performance
Type Performance (instantiations)
Reported by octocov |
Brings in feat/seed-upsert (PR #1910) from main. v2 already extracts seed execution into the sdk-plugin-seed CLI plugin, so the --upsert flag is ported to that architecture instead of merged as generated exec.mjs code: - sdk-plugin-seed: add --upsert to `tailor seed apply`, wiring it through seedNamespace/seedIdpUser and the TailorDB probe-and-update script that bundler.ts already generates - jsonl.ts: enforce requireId/requiredFieldsByType when --upsert is set - seed-type-processor.ts: compute per-type requiredFields for seed-context Also applies review follow-ups from #1910: - treat a _User seed row with only `name` as skipped, not updated - suppress the _User success line when every row fails - preserve the IdP lookup error alongside a create failure - document that --upsert updates run through hooks/validation and fire recordUpdatedTrigger executors # Conflicts: # example/seed/exec.mjs # example/tests/fixtures/expected/seed/exec.mjs # packages/create-sdk/templates/generators/src/seed/exec.mjs # packages/sdk/docs/generator/builtin.md # packages/sdk/src/cli/commands/generate/seed/bundler.test.ts # packages/sdk/src/plugin/builtin/seed/index.test.ts # packages/sdk/src/plugin/builtin/seed/index.ts
Brings in feat/seed-upsert (PR #1910) from main. v2 already extracts seed execution into the sdk-plugin-seed CLI plugin, so the --upsert flag is ported to that architecture instead of merged as generated exec.mjs code: - sdk-plugin-seed: add --upsert to `tailor seed apply`, wiring it through seedNamespace/seedIdpUser and the TailorDB probe-and-update script that bundler.ts already generates - jsonl.ts: enforce requireId/requiredFieldsByType when --upsert is set - seed-type-processor.ts: compute per-type requiredFields for seed-context Also applies review follow-ups from #1910: - treat a _User seed row with only `name` as skipped, not updated - suppress the _User success line when every row fails - preserve the IdP lookup error alongside a create failure - document that --upsert updates run through hooks/validation and fire recordUpdatedTrigger executors # Conflicts: # example/seed/exec.mjs # example/tests/fixtures/expected/seed/exec.mjs # packages/create-sdk/templates/generators/src/seed/exec.mjs # packages/sdk/docs/generator/builtin.md # packages/sdk/src/cli/commands/generate/seed/bundler.test.ts # packages/sdk/src/plugin/builtin/seed/index.test.ts # packages/sdk/src/plugin/builtin/seed/index.ts
Brings in feat/seed-upsert (PR #1910) from main. v2 already extracts seed execution into the sdk-plugin-seed CLI plugin, so the --upsert flag is ported to that architecture instead of merged as generated exec.mjs code: - sdk-plugin-seed: add --upsert to `tailor seed apply`, wiring it through seedNamespace/seedIdpUser and the TailorDB probe-and-update script that bundler.ts already generates - jsonl.ts: enforce requireId/requiredFieldsByType when --upsert is set - seed-type-processor.ts: compute per-type requiredFields for seed-context Also applies review follow-ups from #1910: - treat a _User seed row with only `name` as skipped, not updated - suppress the _User success line when every row fails - preserve the IdP lookup error alongside a create failure - document that --upsert updates run through hooks/validation and fire recordUpdatedTrigger executors # Conflicts: # example/seed/exec.mjs # example/tests/fixtures/expected/seed/exec.mjs # packages/create-sdk/templates/generators/src/seed/exec.mjs # packages/sdk/docs/generator/builtin.md # packages/sdk/src/cli/commands/generate/seed/bundler.test.ts # packages/sdk/src/plugin/builtin/seed/index.test.ts # packages/sdk/src/plugin/builtin/seed/index.ts
Brings in feat/seed-upsert (PR #1910) from main. v2 already extracts seed execution into the sdk-plugin-seed CLI plugin, so the --upsert flag is ported to that architecture instead of merged as generated exec.mjs code: - sdk-plugin-seed: add --upsert to `tailor seed apply`, wiring it through seedNamespace/seedIdpUser and the TailorDB probe-and-update script that bundler.ts already generates - jsonl.ts: enforce requireId/requiredFieldsByType when --upsert is set - seed-type-processor.ts: compute per-type requiredFields for seed-context Also applies review follow-ups from #1910: - treat a _User seed row with only `name` as skipped, not updated - suppress the _User success line when every row fails - preserve the IdP lookup error alongside a create failure - document that --upsert updates run through hooks/validation and fire recordUpdatedTrigger executors # Conflicts: # example/seed/exec.mjs # example/tests/fixtures/expected/seed/exec.mjs # packages/create-sdk/templates/generators/src/seed/exec.mjs # packages/sdk/docs/generator/builtin.md # packages/sdk/src/cli/commands/generate/seed/bundler.test.ts # packages/sdk/src/plugin/builtin/seed/index.test.ts # packages/sdk/src/plugin/builtin/seed/index.ts
2 participants
Summary
seed --upsertadds only the new rows to an already-seeded workspace instead of failing the whole batch on the first duplicate id.Before / After
Re-running seed against a workspace that already holds some rows:
Previously the only workaround was trimming
seed/data/*.jsonldown to the new rows and restoring them afterwards.Behaviour
--upsertNotes
id.--upsertoverwrites existing rows from the seed data, so a workspace with hand-edited data will lose those edits for the fields present in the seed files.