-
-
Notifications
You must be signed in to change notification settings - Fork 11k
🐛 Added migration to update empty strings to null #10428
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
84c5407
e5c332f
e204c12
bcc4780
9d13bd5
f87e6e4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
const Promise = require('bluebird'); | ||
const common = require('../../../../lib/common'); | ||
const schema = require('../../../schema'); | ||
|
||
/* | ||
* [{ | ||
* tableName: 'posts', | ||
* columns: ['custom_excerpt', 'description', 'etc...'] | ||
* }] | ||
* */ | ||
const tablesToUpdate = Object.keys(schema.tables).reduce((tablesToUpdate, tableName) => { | ||
const table = schema.tables[tableName]; | ||
const columns = Object.keys(table).filter(columnName => table[columnName].nullable); | ||
if (!columns.length) { | ||
return tablesToUpdate; | ||
} | ||
return tablesToUpdate.concat({ | ||
tableName, | ||
columns | ||
}); | ||
}, []); | ||
|
||
const createReplace = (connection, from, to) => (tableName, columnName) => { | ||
return connection.schema.hasTable(tableName) | ||
.then((tableExists) => { | ||
if (!tableExists) { | ||
common.logging.warn( | ||
`Table ${tableName} does not exist` | ||
); | ||
return; | ||
} | ||
return connection.schema.hasColumn(tableName, columnName) | ||
.then((columnExists) => { | ||
if (!columnExists) { | ||
common.logging.warn( | ||
`Table '${tableName}' does not have column '${columnName}'` | ||
); | ||
return; | ||
} | ||
|
||
common.logging.info( | ||
`Updating ${tableName}, setting '${from}' in ${columnName} to '${to}'` | ||
); | ||
|
||
return connection(tableName) | ||
.where(columnName, from) | ||
.update(columnName, to); | ||
}); | ||
}); | ||
}; | ||
|
||
module.exports.up = ({connection}) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. DML migrations should use a transaction. |
||
const replaceEmptyStringWithNull = createReplace(connection, '', null); | ||
|
||
return Promise.all( | ||
tablesToUpdate.map(({tableName, columns}) => Promise.all( | ||
columns.map( | ||
columnName => replaceEmptyStringWithNull(tableName, columnName) | ||
) | ||
)) | ||
); | ||
}; | ||
|
||
module.exports.down = ({connection}) => { | ||
const replaceNullWithEmptyString = createReplace(connection, null, ''); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kirrg001 I found something interesting, which is that setting the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cool 👍 |
||
|
||
return Promise.all( | ||
tablesToUpdate.map(({tableName, columns}) => Promise.all( | ||
columns.map( | ||
columnName => replaceNullWithEmptyString(tableName, columnName) | ||
) | ||
)) | ||
); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make this a bit more explicit, maybe the type fo
string
ortext
should be checked here as well?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking this too - though we only update if the existing value is
''
so we shouldn't pick up any fields like that. Maybe it's better to be explicit tho 👍