feat(tailordb): warning tier and migration script subcommand#1200
Draft
toiroakr wants to merge 4 commits into
Draft
feat(tailordb): warning tier and migration script subcommand#1200toiroakr wants to merge 4 commits into
toiroakr wants to merge 4 commits into
Conversation
In tailordb migrations, columns scheduled for removal (`field_removed` diff) were dropped from the type schema in the Pre-migration phase, before `migrate.ts` ran. This made it impossible for migration scripts to read the about-to-be-deleted column — most notably, joining through a foreign key that is being removed in the same migration would fail with `field 'X' not found`. Extend `applyPreMigrationFieldAdjustments` so that `field_removed` changes re-insert the field with its pre-migration config into the cloned Pre-phase request. The Post-migration phase still sends the final schema (without the field), so the physical column drop happens there as documented. Also update `docs/services/tailordb-migration.md` so the Pre/Post phase descriptions match the new behavior.
🦋 Changeset detectedLatest commit: 37e6827 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 |
⚡ pkg.pr.new@tailor-platform/sdk@tailor-platform/create-sdk
|
This comment has been minimized.
This comment has been minimized.
- Classify field_removed/type_removed as warning-tier (data-loss possible but not breaking) and surface them during 'tailordb migration generate' - Add 'tailordb migration script <number>' subcommand to add a migrate.ts (and db.ts) template to an existing migration directory - Execute migrate.ts whenever the file exists on disk, regardless of whether the diff originally required a script; breaking changes still hard-require one - Extract pre-migration schema computation to a dedicated migrate/pre-migration-schema.ts module
toiroakr
changed the title
This comment has been minimized.
This comment has been minimized.
Address Copilot review on #1200: - loadDiff derives hasWarnings from warnings.length so the two stay consistent even if a hand-edited diff.json sets one side - Add tests asserting compareSnapshots populates warnings/hasWarnings for field_removed and type_removed - Add unit tests for formatWarnings (field, type-level, multiple) - Doc: per-migration script step now reflects file-existence based execution - Rewrite pre-migration-schema header comment to scope to fields only (type_removed is handled in the deploy flow) - Neutralize migrate.ts template docstring so it covers both breaking and warning-tier changes
This comment has been minimized.
This comment has been minimized.
…idation Address Copilot review feedback: - Rename misleading 'breakingChanges' to 'preMigrationChanges' in deploy/tailordb/index.ts since the map also contains warning-tier field_removed changes, not only breaking ones. - Tighten migration number validation in the 'script' subcommand to reject inputs like '1abc' that parseInt would silently accept. - Add CLI argument-schema tests for the new 'script' subcommand.
Code Metrics Report (packages/sdk)
Details | | main (b2bd4aa) | #1200 (a3d0f1e) | +/- |
|--------------------|----------------|-----------------|-------|
- | Coverage | 62.3% | 62.2% | -0.2% |
| Files | 364 | 366 | +2 |
| Lines | 12773 | 12852 | +79 |
+ | Covered | 7967 | 7996 | +29 |
+ | Code to Test Ratio | 1:0.4 | 1:0.4 | +0.0 |
| Code | 83913 | 84323 | +410 |
+ | Test | 35136 | 35338 | +202 |Code coverage of files in pull request scope (55.0% → 54.2%)
SDK Configure Bundle Size
Runtime Performance
Type Performance (instantiations)
Reported by octocov |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
packages/sdk/src/cli/commands/deploy/tailordb/migration.ts:138
- Now that migrations execute
migrate.tswhenever it exists, a user can end up withmigrate.tspresent but missing the companiondb.ts(e.g. manual edits). The bundler will then fail onimport type { Transaction } from "./db"with a low-level build error. Consider validatingdb.tsexistence whenhasScriptis true and surfacing a clear warning/error (similar to the existing breaking-change script check).
// Load the diff to inspect breaking/warning classification
const diff = loadDiff(diffPath);
// The migration script is executed when migrate.ts exists on disk.
// Breaking changes still hard-require a script; warnings (e.g. field_removed)
// may optionally have one added via `tailordb migration script <num>`.
const scriptPath = getMigrationFilePath(migrationsDir, file.number, "migrate");
const hasScript = fs.existsSync(scriptPath);
if (diff.requiresMigrationScript && !hasScript) {
logger.warn(
`Migration ${namespace}/${file.number} requires a script but migrate.ts not found`,
);
continue;
}
Comment on lines
+45
to
+58
| // Accept either the canonical 4-digit form ("0001") or a bare non-negative | ||
| // integer ("1"). Reject anything containing non-digit characters so that | ||
| // inputs like "1abc" don't silently parse to migration 1. | ||
| let migrationNumber: number; | ||
| if (isValidMigrationNumber(options.number)) { | ||
| migrationNumber = parseInt(options.number, 10); | ||
| } else if (/^\d+$/.test(options.number)) { | ||
| migrationNumber = parseInt(options.number, 10); | ||
| } else { | ||
| throw new Error( | ||
| `Invalid migration number format: ${options.number}. Expected 4-digit format (e.g., 0001) or integer (e.g., 1).`, | ||
| ); | ||
| } | ||
|
|
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
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
safe/warning/breaking):field_removedandtype_removedare now reported as warnings duringtailordb migration generate, not silent changes. They are no longer dropped beforemigrate.tsruns — the field/type stays available during the Pre-migration phase so scripts can read it (e.g.innerJointhrough a foreign key being dropped in the same migration), and the physical drop happens in Post-migration.tailordb migration script <number>subcommand to add amigrate.ts(anddb.ts) template to an existing migration directory. Useful for warning-tier changes where you want a custom data migration even though one was not generated automatically.deploy/tailordb/index.tsinto a dedicatedcli/commands/tailordb/migrate/pre-migration-schema.tsmodule.docs/services/tailordb-migration.mdwith the new warning behavior and script subcommand; changeset bumped tominor.Notes
warnings/hasWarningsfields alongside the existingbreakingChanges/hasBreakingChanges.PendingMigrationgains ahasScript: booleanto decouple "run the script" from "the diff required a script".requiresMigrationScript && !hasScriptis preserved — breaking changes without a script still fail loudly.Closes: GitHub Discussion #249