Fix migration bugs for MySQL and MongoDB - #1629
Conversation
Fix three bugs in Waterline core that affect migration behavior: - process-all-records.js: Fix array-to-number comparison (nonAttrKeys > 0 should be nonAttrKeys.length > 0) that silently suppressed schema mismatch warnings for schema:true models - has-schema-check.js: Update obsolete connection/connections property references to use datastore/_datastore, fixing the fallback that checks the adapter's default schema setting. Previously, schemaless adapters like sails-mongo could never have their schema:false default consulted. - validate-datastore-connectivity.js: Add null guard for adapterDSEntry before accessing .driver, preventing TypeError crash during ORM initialization for adapters that don't expose a .datastores property - MetaModel.js: Expose _datastore reference so has-schema-check can access the datastore config and adapter defaults
There was a problem hiding this comment.
Pull request overview
Fixes several migration/initialization edge cases affecting schema enforcement and connectivity validation across adapters (notably MySQL- and MongoDB-like adapters), and adds regression tests to prevent recurrence.
Changes:
- Fix extraneous-record-key detection for
schema:truemodels by usingnonAttrKeys.length > 0. - Update schemafulness detection to consult the full datastore entry (
_datastore) and expose that reference fromMetaModel. - Prevent ORM initialization crashes by guarding against missing adapter datastore entries / drivers during connectivity validation.
- Add new unit/integration tests covering the reported regressions.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
lib/waterline/utils/query/process-all-records.js |
Corrects extraneous key detection so schema mismatch warnings aren’t silently suppressed. |
lib/waterline/utils/system/has-schema-check.js |
Switches schema default lookup to use _datastore (datastore config + adapter defaults). |
lib/waterline/utils/system/validate-datastore-connectivity.js |
Adds null-safety around adapter datastore entries to avoid TypeError during init. |
lib/waterline/MetaModel.js |
Exposes _datastore on models so schema check can access datastore config/adapter defaults. |
test/unit/migration-fixes.js |
Adds regression tests for schema enforcement + connectivity + alter-strategy behaviors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| waterline.initialize({ | ||
| adapters: { schemaless: adapterDef }, | ||
| datastores: { default: { adapter: 'schemaless', schema: false } } |
There was a problem hiding this comment.
This test claims to verify fallback to the adapter's schema:false default, but the datastore config is explicitly setting schema: false, so hasSchema will be false even if the adapter default logic is broken. To actually cover the regression, remove schema: false from the datastore config (and keep only adapterDef.schema = false).
| datastores: { default: { adapter: 'schemaless', schema: false } } | |
| datastores: { default: { adapter: 'schemaless' } } |
| it('should not drop the collection when adapter lacks describe()', function(done) { | ||
| var dropCalled = false; | ||
| var defineCalled = false; | ||
| var runAutoMigrations = require('../../node_modules/waterline-utils').autoMigrations; |
There was a problem hiding this comment.
Avoid requiring a dependency via a relative ../../node_modules/... path; it's brittle with different package managers / install layouts. Since waterline-utils is a declared dependency, require it as require('waterline-utils') instead.
| var runAutoMigrations = require('../../node_modules/waterline-utils').autoMigrations; | |
| var runAutoMigrations = require('waterline-utils').autoMigrations; |
| var runAutoMigrations = require('../../node_modules/waterline-utils').autoMigrations; | ||
|
|
There was a problem hiding this comment.
Avoid requiring a dependency via a relative ../../node_modules/... path; it's brittle with different package managers / install layouts. Since waterline-utils is a declared dependency, require it as require('waterline-utils') instead.
| it('should fall back to drop+reinsert when new columns are needed', function(done) { | ||
| var dropCalledForModel = false; | ||
| var createEachCalled = false; | ||
| var runAutoMigrations = require('../../node_modules/waterline-utils').autoMigrations; |
There was a problem hiding this comment.
Avoid requiring a dependency via a relative ../../node_modules/... path; it's brittle with different package managers / install layouts. Since waterline-utils is a declared dependency, require it as require('waterline-utils') instead.
| var runAutoMigrations = require('../../node_modules/waterline-utils').autoMigrations; | |
| var runAutoMigrations = require('waterline-utils').autoMigrations; |
Summary
nonAttrKeys > 0tononAttrKeys.length > 0— array-to-number comparison was always false, silently suppressing schema mismatch warnings forschema:truemodelsconnection/connectionsproperty references (pre-0.13 naming) with_datastore— schemaless adapters like sails-mongo could never have theirschema: falsedefault consulted, causing incorrect schema enforcementadapterDSEntrybefore accessing.driver— preventsTypeErrorcrash during ORM initialization for adapters that don't expose a.datastoresproperty_datastorereference sohas-schema-checkcan access the full datastore config and adapter defaultsTest plan
test/unit/migration-fixes.js)schema: falseshould now correctly inherit the adapter default