Non-destructive alter migration strategy by stuk88 · Pull Request #97 · sailshq/waterline-utils

stuk88 · 2026-04-16T12:52:00Z

Summary

The current alter migration strategy drops tables/collections before re-inserting records. When re-insert fails (validation constraints reject existing data), data is already destroyed. This PR replaces the destructive cycle with a safe, incremental approach.

MongoDB (no describe()): Calls define() for indexes only, never drops the collection. Records stay in place.
MySQL (has describe()): Diffs old vs new schema. Skips drop when schema is compatible. Falls back to drop+reinsert only when new columns are needed.
Fallback path: Writes machine-readable JSON backup before dropping (not util.inspect() text). Sanitizes backup records (strips removed attributes) before re-insert.
Backup format: Changed from .log with util.inspect() to .json with JSON.stringify() so recovery data is programmatically restorable.

Test plan

Run with sails-mongo: verify collections are NOT dropped during alter migration
Run with sails-mysql: verify tables are NOT dropped when schema is unchanged
Run with sails-mysql: verify fallback drop+reinsert works when new columns are added
Verify JSON backup file is written to .tmp/ before destructive operations
Verify migrate: 'drop' still works as before (not affected by this change)
Verify migrate: 'safe' still works as before (not affected by this change)

Related: balderdashy/waterline#1629

Copilot

Pull request overview

This PR updates Waterline’s alter auto-migration strategy to avoid destructive drop/recreate cycles where possible, and to produce machine-restorable JSON backups when destructive fallback is unavoidable.

Changes:

Adds adapter-capability branching: use describe()-based checks for schema-aware adapters; avoid collection drops for adapters without describe().
Introduces a fallback drop+reinsert path that writes a JSON backup before destructive operations and sanitizes records prior to reinsert.
Updates failed-migration backup output to prefer machine-readable JSON instead of util.inspect()-formatted text.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File	Description
lib/auto-migrations/private/run-alter-strategy/index.js	Refactors `alter` to be incremental when possible; adds JSON backup + sanitize on destructive fallback.
lib/auto-migrations/private/run-alter-strategy/private/inform-re-failed-alter-stratagem.js	Changes recovered-record logging to prefer JSON output and `.json` file extension.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-16T12:57:40Z

            var lastRecord = _.last(backupRecords);
            var primaryKeyColumnName = WLModel.schema[primaryKeyAttrName].columnName;
            var sequenceName = WLModel.tableName + '_' + primaryKeyColumnName + '_seq';
            var sequenceValue = lastRecord[primaryKeyColumnName];



sequenceValue is read from lastRecord[primaryKeyColumnName], but backupRecords from WLModel.find() are logical Waterline records keyed by attribute name (unless explicitly using a physical/column-name mode). If columnName differs from the attribute name, this will be undefined, causing setSequence() to be called with the wrong value. Consider using lastRecord[primaryKeyAttrName] (and only falling back to primaryKeyColumnName when records are known to be physical).

Fixed. sequenceValue now reads from lastRecord[primaryKeyAttrName] first (logical attribute name), falling back to lastRecord[primaryKeyColumnName] only when the attribute name key isn't present.

Copilot · 2026-04-16T12:57:40Z

+    // If JSON.stringify fails (e.g. circular refs), fall back to util.inspect
+    logFileContents = util.inspect(backupRecords, { depth: 5 });


logFileContents is always written to a .json file, but the catch path falls back to util.inspect(...), which is not valid JSON. This makes the backup file extension/content inconsistent and undermines “machine-readable” recovery. Consider either (a) always writing valid JSON (e.g., JSON with an inspectFallback string + error info) or (b) switching the extension to .log only for the fallback path.

Suggested change

// If JSON.stringify fails (e.g. circular refs), fall back to util.inspect

logFileContents = util.inspect(backupRecords, { depth: 5 });

// If JSON.stringify fails (e.g. circular refs), fall back to a JSON payload

// that preserves machine readability while including an inspect-based snapshot.

logFileContents = JSON.stringify({

timestamp: new Date().toISOString(),

model: modelIdentity,

operation: operationName,

recordCount: backupRecords.length,

jsonSerializationError: {

message: jsonErr && jsonErr.message,

stack: jsonErr && jsonErr.stack

},

inspectFallback: util.inspect(backupRecords, { depth: 5 })

}, null, 2);

Good catch. The fallback now wraps the util.inspect output in a JSON envelope with inspectFallback and jsonSerializationError fields, so the .json file is always valid JSON regardless of whether serialization succeeded.

Copilot · 2026-04-16T12:57:40Z

+  WLAdapter.define(datastoreName, tableName, tableDDLSpec, function defineCallback(err) {
+    if (err) {
+      // If define fails (e.g. unique index can't be created because of
+      // duplicate values), warn but don't fail fatally.
+      console.warn('\n'+
+        'Warning: When performing `alter` auto-migration on `' + tableName + '`,\n'+
+        'the adapter could not apply all schema constraints (e.g. unique indexes).\n'+
+        'This usually means existing data violates a new uniqueness constraint.\n'+
+        'You may need to resolve duplicates manually.\n'+
+        '\n'+
+        'Error details:\n'+
+        '```\n'+
+        (err.message || util.inspect(err))+'\n'+
+        '```\n'
+      );
+    }
+
+    return done();


migrateWithoutDescribe() currently ignores all errors from WLAdapter.define() and proceeds as if migration succeeded. That can mask real failures (e.g. connection/auth issues) and leave the app running with a partially-applied schema/index state. Consider only downgrading known “constraint/index can’t be applied” cases to warnings, and otherwise propagate the error via done(err).

Agreed. Now only downgrades AdapterError to a warning (constraint violations like duplicate keys). All other errors (connection, auth, etc.) propagate via done(err).

Copilot · 2026-04-16T12:57:41Z

+  WLAdapter.describe(datastoreName, tableName, function describeCallback(err, existingSchema) {
+    if (err) {
+      // describe() failed — table probably doesn't exist yet.
+      // Just define it fresh.
+      WLAdapter.define(datastoreName, tableName, tableDDLSpec, function(err) {
+        if (err) { return done(err); }
+        return done();
+      });
+      return;
+    }


In migrateWithDescribe(), any error from WLAdapter.describe() is treated as “table probably doesn't exist” and triggers define(). describe() can also fail for other reasons (permissions, connectivity, malformed table name, etc.), and treating those as “missing table” can hide the real problem and lead to unexpected behavior. Suggest negotiating the error and only falling back to define() for confirmed “no such table” cases; otherwise return done(err).

Fixed. Now only treats AdapterError from describe() as "table doesn't exist" and falls through to define(). All other errors (permissions, connectivity) propagate immediately.

Copilot · 2026-04-16T12:57:41Z

+    // Schema is compatible (no new columns needed). Just ensure indexes
+    // by calling define() — most adapters handle "IF NOT EXISTS" internally.
+    WLAdapter.define(datastoreName, tableName, tableDDLSpec, function defineCallback(err) {
      if (err) {
-        // Ignore the error if it's an adapter error.  For example, this could error out
-        // on an empty database when the table doesn't yet exist (which is perfectly fine).
-        // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
-        // FUTURE: further negotiate this error and only ignore failure due to "no such table"
-        // (other errors are still relevant and important).  The database-specific piece of
-        // this should happen in the adapter (and where supported, use a newly standardized
-        // footprint from the underlying driver)
-        // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
-        if (err.name === 'AdapterError') {
-
-          // Ignore.
-          //
-          // (But note that we also set backupRecords to an empty array so that it matches
-          // what we'd expect if everything had worked out.)
-          backupRecords = [];
-
-        // But otherwise, this is NOT an adapter error, so still bail w/ a fatal error
-        // (because this means something else completely unexpected has happened.)
-        } else {
-          return next(flaverr({
-            message: 'When attempting to perform the `alter` auto-migration strategy '+
-            'on model `' + WLModel.identity + '`, Sails encountered an error.  '+err.message+'\n'+
-            'Tip: Could there be existing records in the database that are not compatible '+
-            'with a recent change to this model\'s definition?  If so, you might need to '+
-            'migrate them manually or, if you don\'t care about the data, wipe them; e.g. --drop.\n'+
-            '--\n'+
-            'For help with auto-migrations, visit:\n'+
-            ' [?] https://sailsjs.com/docs/concepts/models-and-orm/model-settings#?migrate\n'
-          }, err));
-        }
-      }//>-•
+        // define() failed on an existing table. This can happen if the adapter's
+        // define() uses CREATE TABLE (not IF NOT EXISTS). Fall back.
+        return fallbackDropAndReinsert(WLAdapter, datastoreName, tableName, tableDDLSpec, WLModel, primaryKeyAttrName, done);
+      }


When newColumns.length === 0, any define() error triggers a destructive fallbackDropAndReinsert(). Some define() failures can be non-table-existence issues (e.g. failing to apply a new unique index because existing rows are duplicates). In those cases, falling back to drop+reinsert both destroys the table and is likely to fail again on reinsert, which is worse than warning and leaving data intact. Consider only using the fallback when the error indicates “table already exists / CREATE TABLE failed because exists”, and otherwise either return done(err) or emit a non-fatal warning (similar to migrateWithoutDescribe()).

Agreed — triggering drop+reinsert for an index failure would likely just fail again. Changed to warn and proceed, leaving the data intact.

Copilot · 2026-04-16T12:57:41Z

+    // Write a JSON backup BEFORE dropping, so data is never lost.
+    writeJsonBackup(WLModel.identity, backupRecords, function(backupErr) {
+      if (backupErr) {
+        console.warn('Warning: Could not write JSON backup for `' + WLModel.identity + '`: ' + backupErr.message);


fallbackDropAndReinsert() proceeds to drop() even if writeJsonBackup() fails, which can still result in irreversible data loss (e.g. if reinsert fails and the in-memory process crashes). Given the goal of a non-destructive strategy, consider treating backupErr as fatal (abort before drop()), or at least making this behavior opt-in/explicit.

Suggested change

console.warn('Warning: Could not write JSON backup for `' + WLModel.identity + '`: ' + backupErr.message);

return done(flaverr({

message: 'When attempting to perform the `alter` auto-migration strategy '+

'on model `' + WLModel.identity + '`, Sails could not write the JSON backup '+

'required before the fallback drop-and-reinsert step. Aborting migration '+

'before dropping data to avoid irreversible data loss.\n'+

'--\n'+

'Backup path:\n'+

' ' + path.resolve(process.cwd(), '.tmp/migrate-' + WLModel.identity + '.json') + '\n'+

'Original error: ' + backupErr.message

}, backupErr));

Fixed. If backup can't be written, migration now aborts before drop() with a clear error. No more proceeding to destroy data without a safety net.

Replace the destructive dump-drop-recreate alter migration with a safe, incremental approach that avoids data loss. For schemaless adapters (MongoDB): - Call define() to ensure indexes only, never drop the collection - Records stay in place — MongoDB is schemaless, no physical schema to alter For schema-aware adapters with describe() (MySQL): - Diff existing schema against new model definition - Skip drop entirely when schema is compatible (no new columns) - Fall back to drop+reinsert only when new columns must be added - Write machine-readable JSON backup before any destructive operation - Sanitize backup records (strip removed attributes) before re-insert Also change backup format from util.inspect() text to JSON so recovery data is machine-readable and programmatically restorable.

Copilot AI review requested due to automatic review settings April 16, 2026 12:52

Copilot started reviewing on behalf of stuk88 April 16, 2026 12:52 View session

Copilot AI reviewed Apr 16, 2026

View reviewed changes

stuk88 force-pushed the fix/non-destructive-alter-strategy branch from 2b25a79 to 635111e Compare April 16, 2026 17:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Non-destructive alter migration strategy#97

Non-destructive alter migration strategy#97
stuk88 wants to merge 1 commit intosailshq:masterfrom
stuk88:fix/non-destructive-alter-strategy

stuk88 commented Apr 16, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 16, 2026

Uh oh!

stuk88 Apr 16, 2026

Uh oh!

Copilot AI Apr 16, 2026

Uh oh!

stuk88 Apr 16, 2026

Uh oh!

Copilot AI Apr 16, 2026

Uh oh!

stuk88 Apr 16, 2026

Uh oh!

Copilot AI Apr 16, 2026

Uh oh!

stuk88 Apr 16, 2026

Uh oh!

Copilot AI Apr 16, 2026

Uh oh!

stuk88 Apr 16, 2026

Uh oh!

Copilot AI Apr 16, 2026

Uh oh!

stuk88 Apr 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

		// If JSON.stringify fails (e.g. circular refs), fall back to util.inspect
		logFileContents = util.inspect(backupRecords, { depth: 5 });

-    // If JSON.stringify fails (e.g. circular refs), fall back to util.inspect
-    logFileContents = util.inspect(backupRecords, { depth: 5 });
+    // If JSON.stringify fails (e.g. circular refs), fall back to a JSON payload
+    // that preserves machine readability while including an inspect-based snapshot.
+    logFileContents = JSON.stringify({
+      timestamp: new Date().toISOString(),
+      model: modelIdentity,
+      operation: operationName,
+      recordCount: backupRecords.length,
+      jsonSerializationError: {
+        message: jsonErr && jsonErr.message,
+        stack: jsonErr && jsonErr.stack
+      },
+      inspectFallback: util.inspect(backupRecords, { depth: 5 })
+    }, null, 2);

-        console.warn('Warning: Could not write JSON backup for `' + WLModel.identity + '`: ' + backupErr.message);
+        return done(flaverr({
+          message: 'When attempting to perform the `alter` auto-migration strategy '+
+          'on model `' + WLModel.identity + '`, Sails could not write the JSON backup '+
+          'required before the fallback drop-and-reinsert step.  Aborting migration '+
+          'before dropping data to avoid irreversible data loss.\n'+
+          '--\n'+
+          'Backup path:\n'+
+          ' ' + path.resolve(process.cwd(), '.tmp/migrate-' + WLModel.identity + '.json') + '\n'+
+          'Original error: ' + backupErr.message
+        }, backupErr));

Conversation

stuk88 commented Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

stuk88 Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

stuk88 Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

stuk88 Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

stuk88 Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

stuk88 Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

stuk88 Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

stuk88 commented Apr 16, 2026 •

edited

Loading