From 143ae857c928a2dbaf89748cd00b290a5c231d80 Mon Sep 17 00:00:00 2001 From: Daniel Lockyer Date: Wed, 12 Oct 2022 21:53:43 +0700 Subject: [PATCH] Removed `bool` type from schema refs https://github.com/TryGhost/Toolbox/issues/441 - we tend to have a mix of `bool` and `boolean` in the schema and migrations, which has become a real nit for me at this point - we don't do any special handling between `bool` and `boolean`, it's just something we pass to Knex - `bool` is an alias for `boolean` but `boolean` is actually documented - https://knexjs.org/guide/schema-builder.html#boolean - this commit switches Ghost to only using `boolean` in the schema and migrations, and removes `bool` from the allowlist in tests to prevent us from adding it again in the future - this should make absolutely no difference to the DB because both resulted in the same column --- .../05-add-members-subscribe-events-table.js | 2 +- ...d-email-only-column-to-posts-meta-table.js | 2 +- .../4.3/06-add-stripe-prices-table.js | 2 +- .../versions/4.42/2022-03-21-17-17-add.js | 4 +-- ...2-03-28-19-26-recreate-newsletter-table.js | 10 +++---- ...12-39-add-track-clicks-column-to-emails.js | 4 +-- ...-feedback-enabled-column-to-newsletters.js | 2 +- ghost/core/core/server/data/schema/schema.js | 28 +++++++++---------- .../core/core/server/data/schema/validator.js | 2 +- .../models/base/plugins/data-manipulation.js | 2 +- .../unit/server/data/schema/integrity.test.js | 2 +- .../unit/server/data/schema/schema.test.js | 4 --- .../unit/server/data/schema/validator.test.js | 10 ------- 13 files changed, 30 insertions(+), 44 deletions(-) diff --git a/ghost/core/core/server/data/migrations/versions/4.0/05-add-members-subscribe-events-table.js b/ghost/core/core/server/data/migrations/versions/4.0/05-add-members-subscribe-events-table.js index a4997f630a..06e04ab980 100644 --- a/ghost/core/core/server/data/migrations/versions/4.0/05-add-members-subscribe-events-table.js +++ b/ghost/core/core/server/data/migrations/versions/4.0/05-add-members-subscribe-events-table.js @@ -3,7 +3,7 @@ const {addTable} = require('../../utils'); module.exports = addTable('members_subscribe_events', { id: {type: 'string', maxlength: 24, nullable: false, primary: true}, member_id: {type: 'string', maxlength: 24, nullable: false, unique: false, references: 'members.id', cascadeDelete: true}, - subscribed: {type: 'bool', nullable: false, defaultTo: true}, + subscribed: {type: 'boolean', nullable: false, defaultTo: true}, created_at: {type: 'dateTime', nullable: false}, source: {type: 'string', maxlength: 50, nullable: true} }); diff --git a/ghost/core/core/server/data/migrations/versions/4.12/01-add-email-only-column-to-posts-meta-table.js b/ghost/core/core/server/data/migrations/versions/4.12/01-add-email-only-column-to-posts-meta-table.js index 16bf97b9dc..d1441053b9 100644 --- a/ghost/core/core/server/data/migrations/versions/4.12/01-add-email-only-column-to-posts-meta-table.js +++ b/ghost/core/core/server/data/migrations/versions/4.12/01-add-email-only-column-to-posts-meta-table.js @@ -1,7 +1,7 @@ const {createAddColumnMigration} = require('../../utils'); module.exports = createAddColumnMigration('posts_meta', 'email_only', { - type: 'bool', + type: 'boolean', nullable: false, defaultTo: false }); diff --git a/ghost/core/core/server/data/migrations/versions/4.3/06-add-stripe-prices-table.js b/ghost/core/core/server/data/migrations/versions/4.3/06-add-stripe-prices-table.js index bf6170fb70..54a8b0fdf7 100644 --- a/ghost/core/core/server/data/migrations/versions/4.3/06-add-stripe-prices-table.js +++ b/ghost/core/core/server/data/migrations/versions/4.3/06-add-stripe-prices-table.js @@ -4,7 +4,7 @@ module.exports = addTable('stripe_prices', { id: {type: 'string', maxlength: 24, nullable: false, primary: true}, stripe_price_id: {type: 'string', maxlength: 255, nullable: false, unique: true}, stripe_product_id: {type: 'string', maxlength: 255, nullable: false, unique: false, references: 'stripe_products.stripe_product_id', cascadeDelete: true}, - active: {type: 'bool', nullable: false}, + active: {type: 'boolean', nullable: false}, nickname: {type: 'string', maxlength: 50, nullable: true}, currency: {type: 'string', maxLength: 3, nullable: false}, amount: {type: 'integer', nullable: false}, diff --git a/ghost/core/core/server/data/migrations/versions/4.42/2022-03-21-17-17-add.js b/ghost/core/core/server/data/migrations/versions/4.42/2022-03-21-17-17-add.js index acd1e0af8b..19f37eab1b 100644 --- a/ghost/core/core/server/data/migrations/versions/4.42/2022-03-21-17-17-add.js +++ b/ghost/core/core/server/data/migrations/versions/4.42/2022-03-21-17-17-add.js @@ -12,7 +12,7 @@ module.exports = addTable('newsletters', { sender_name: {type: 'string', maxlength: 191, nullable: false}, sender_email: {type: 'string', maxlength: 191, nullable: false, validations: {isEmail: true}}, sender_reply_to: {type: 'string', maxlength: 191, nullable: false, validations: {isEmail: true}}, - default: {type: 'bool', nullable: false, defaultTo: false}, + default: {type: 'boolean', nullable: false, defaultTo: false}, status: {type: 'string', maxlength: 50, nullable: false, defaultTo: 'active'}, recipient_filter: { type: 'text', @@ -20,6 +20,6 @@ module.exports = addTable('newsletters', { nullable: false, defaultTo: '' }, - subscribe_on_signup: {type: 'bool', nullable: false, defaultTo: false}, + subscribe_on_signup: {type: 'boolean', nullable: false, defaultTo: false}, sort_order: {type: 'integer', nullable: false, unsigned: true, defaultTo: 0} }); diff --git a/ghost/core/core/server/data/migrations/versions/4.43/2022-03-28-19-26-recreate-newsletter-table.js b/ghost/core/core/server/data/migrations/versions/4.43/2022-03-28-19-26-recreate-newsletter-table.js index d788f6d7cb..6c04db09e9 100644 --- a/ghost/core/core/server/data/migrations/versions/4.43/2022-03-28-19-26-recreate-newsletter-table.js +++ b/ghost/core/core/server/data/migrations/versions/4.43/2022-03-28-19-26-recreate-newsletter-table.js @@ -15,15 +15,15 @@ module.exports = recreateTable('newsletters', { nullable: false, defaultTo: 'members' }, - subscribe_on_signup: {type: 'bool', nullable: false, defaultTo: true}, + subscribe_on_signup: {type: 'boolean', nullable: false, defaultTo: true}, sort_order: {type: 'integer', nullable: false, unsigned: true, defaultTo: 0}, header_image: {type: 'string', maxlength: 2000, nullable: true}, - show_header_icon: {type: 'bool', nullable: false, defaultTo: true}, - show_header_title: {type: 'bool', nullable: false, defaultTo: true}, + show_header_icon: {type: 'boolean', nullable: false, defaultTo: true}, + show_header_title: {type: 'boolean', nullable: false, defaultTo: true}, title_font_category: {type: 'string', maxlength: 191, nullable: false, defaultTo: 'sans_serif', validations: {isIn: [['serif', 'sans_serif']]}}, title_alignment: {type: 'string', maxlength: 191, nullable: false, defaultTo: 'center', validations: {isIn: [['center', 'left']]}}, - show_feature_image: {type: 'bool', nullable: false, defaultTo: true}, + show_feature_image: {type: 'boolean', nullable: false, defaultTo: true}, body_font_category: {type: 'string', maxlength: 191, nullable: false, defaultTo: 'sans_serif', validations: {isIn: [['serif', 'sans_serif']]}}, footer_content: {type: 'text', maxlength: 1000000000, nullable: true}, - show_badge: {type: 'bool', nullable: false, defaultTo: true} + show_badge: {type: 'boolean', nullable: false, defaultTo: true} }); diff --git a/ghost/core/core/server/data/migrations/versions/5.17/2022-09-29-12-39-add-track-clicks-column-to-emails.js b/ghost/core/core/server/data/migrations/versions/5.17/2022-09-29-12-39-add-track-clicks-column-to-emails.js index d4b09bce58..a9b6afb368 100644 --- a/ghost/core/core/server/data/migrations/versions/5.17/2022-09-29-12-39-add-track-clicks-column-to-emails.js +++ b/ghost/core/core/server/data/migrations/versions/5.17/2022-09-29-12-39-add-track-clicks-column-to-emails.js @@ -1,7 +1,7 @@ const {createAddColumnMigration} = require('../../utils'); module.exports = createAddColumnMigration('emails', 'track_clicks', { - type: 'bool', - nullable: false, + type: 'boolean', + nullable: false, defaultTo: false }); diff --git a/ghost/core/core/server/data/migrations/versions/5.19/2022-10-11-10-38-add-feedback-enabled-column-to-newsletters.js b/ghost/core/core/server/data/migrations/versions/5.19/2022-10-11-10-38-add-feedback-enabled-column-to-newsletters.js index e437c31939..6644b84b1a 100644 --- a/ghost/core/core/server/data/migrations/versions/5.19/2022-10-11-10-38-add-feedback-enabled-column-to-newsletters.js +++ b/ghost/core/core/server/data/migrations/versions/5.19/2022-10-11-10-38-add-feedback-enabled-column-to-newsletters.js @@ -1,7 +1,7 @@ const {createAddColumnMigration} = require('../../utils'); module.exports = createAddColumnMigration('newsletters', 'feedback_enabled', { - type: 'bool', + type: 'boolean', nullable: false, defaultTo: false }); diff --git a/ghost/core/core/server/data/schema/schema.js b/ghost/core/core/server/data/schema/schema.js index 5494d8d7d2..5173fb4d30 100644 --- a/ghost/core/core/server/data/schema/schema.js +++ b/ghost/core/core/server/data/schema/schema.js @@ -13,7 +13,7 @@ module.exports = { uuid: {type: 'string', maxlength: 36, nullable: false, unique: true, validations: {isUUID: true}}, name: {type: 'string', maxlength: 191, nullable: false, unique: true}, description: {type: 'string', maxlength: 2000, nullable: true}, - feedback_enabled: {type: 'bool', nullable: false, defaultTo: false}, + feedback_enabled: {type: 'boolean', nullable: false, defaultTo: false}, slug: {type: 'string', maxlength: 191, nullable: false, unique: true}, sender_name: {type: 'string', maxlength: 191, nullable: true}, sender_email: {type: 'string', maxlength: 191, nullable: true}, @@ -25,18 +25,18 @@ module.exports = { nullable: false, defaultTo: 'members' }, - subscribe_on_signup: {type: 'bool', nullable: false, defaultTo: true}, + subscribe_on_signup: {type: 'boolean', nullable: false, defaultTo: true}, sort_order: {type: 'integer', nullable: false, unsigned: true, defaultTo: 0}, header_image: {type: 'string', maxlength: 2000, nullable: true}, - show_header_icon: {type: 'bool', nullable: false, defaultTo: true}, - show_header_title: {type: 'bool', nullable: false, defaultTo: true}, + show_header_icon: {type: 'boolean', nullable: false, defaultTo: true}, + show_header_title: {type: 'boolean', nullable: false, defaultTo: true}, title_font_category: {type: 'string', maxlength: 191, nullable: false, defaultTo: 'sans_serif', validations: {isIn: [['serif', 'sans_serif']]}}, title_alignment: {type: 'string', maxlength: 191, nullable: false, defaultTo: 'center', validations: {isIn: [['center', 'left']]}}, - show_feature_image: {type: 'bool', nullable: false, defaultTo: true}, + show_feature_image: {type: 'boolean', nullable: false, defaultTo: true}, body_font_category: {type: 'string', maxlength: 191, nullable: false, defaultTo: 'sans_serif', validations: {isIn: [['serif', 'sans_serif']]}}, footer_content: {type: 'text', maxlength: 1000000000, nullable: true}, - show_badge: {type: 'bool', nullable: false, defaultTo: true}, - show_header_name: {type: 'bool', nullable: false, defaultTo: true}, + show_badge: {type: 'boolean', nullable: false, defaultTo: true}, + show_header_name: {type: 'boolean', nullable: false, defaultTo: true}, created_at: {type: 'dateTime', nullable: false}, updated_at: {type: 'dateTime', nullable: true} }, @@ -51,7 +51,7 @@ module.exports = { comment_id: {type: 'string', maxlength: 50, nullable: true}, plaintext: {type: 'text', maxlength: 1000000000, fieldtype: 'long', nullable: true}, feature_image: {type: 'string', maxlength: 2000, nullable: true}, - featured: {type: 'bool', nullable: false, defaultTo: false}, + featured: {type: 'boolean', nullable: false, defaultTo: false}, type: {type: 'string', maxlength: 50, nullable: false, defaultTo: 'post', validations: {isIn: [['post', 'page']]}}, status: {type: 'string', maxlength: 50, nullable: false, defaultTo: 'draft', validations: {isIn: [['published', 'draft', 'scheduled', 'sent']]}}, // NOTE: unused at the moment and reserved for future features @@ -103,7 +103,7 @@ module.exports = { frontmatter: {type: 'text', maxlength: 65535, nullable: true}, feature_image_alt: {type: 'string', maxlength: 191, nullable: true, validations: {isLength: {max: 125}}}, feature_image_caption: {type: 'text', maxlength: 65535, nullable: true}, - email_only: {type: 'bool', nullable: false, defaultTo: false} + email_only: {type: 'boolean', nullable: false, defaultTo: false} }, // NOTE: this is the staff table users: { @@ -607,7 +607,7 @@ module.exports = { subscription_id: {type: 'string', maxlength: 255, nullable: false, unique: true}, stripe_price_id: {type: 'string', maxlength: 255, nullable: false, unique: false, index: true, defaultTo: ''}, status: {type: 'string', maxlength: 50, nullable: false}, - cancel_at_period_end: {type: 'bool', nullable: false, defaultTo: false}, + cancel_at_period_end: {type: 'boolean', nullable: false, defaultTo: false}, cancellation_reason: {type: 'string', maxlength: 500, nullable: true}, current_period_end: {type: 'dateTime', nullable: false}, start_date: {type: 'dateTime', nullable: false}, @@ -653,7 +653,7 @@ module.exports = { members_subscribe_events: { id: {type: 'string', maxlength: 24, nullable: false, primary: true}, member_id: {type: 'string', maxlength: 24, nullable: false, unique: false, references: 'members.id', cascadeDelete: true}, - subscribed: {type: 'bool', nullable: false, defaultTo: true}, + subscribed: {type: 'boolean', nullable: false, defaultTo: true}, created_at: {type: 'dateTime', nullable: false}, source: { type: 'string', maxlength: 50, nullable: true, validations: { @@ -673,7 +673,7 @@ module.exports = { id: {type: 'string', maxlength: 24, nullable: false, primary: true}, stripe_price_id: {type: 'string', maxlength: 255, nullable: false, unique: true}, stripe_product_id: {type: 'string', maxlength: 255, nullable: false, unique: false, references: 'stripe_products.stripe_product_id'}, - active: {type: 'bool', nullable: false}, + active: {type: 'boolean', nullable: false}, nickname: {type: 'string', maxlength: 50, nullable: true}, currency: {type: 'string', maxLength: 3, nullable: false}, amount: {type: 'integer', nullable: false}, @@ -723,8 +723,8 @@ module.exports = { reply_to: {type: 'string', maxlength: 2000, nullable: true}, html: {type: 'text', maxlength: 1000000000, fieldtype: 'long', nullable: true}, plaintext: {type: 'text', maxlength: 1000000000, fieldtype: 'long', nullable: true}, - track_opens: {type: 'bool', nullable: false, defaultTo: false}, - track_clicks: {type: 'bool', nullable: false, defaultTo: false}, + track_opens: {type: 'boolean', nullable: false, defaultTo: false}, + track_clicks: {type: 'boolean', nullable: false, defaultTo: false}, submitted_at: {type: 'dateTime', nullable: false}, newsletter_id: {type: 'string', maxlength: 24, nullable: true, references: 'newsletters.id'}, created_at: {type: 'dateTime', nullable: false}, diff --git a/ghost/core/core/server/data/schema/validator.js b/ghost/core/core/server/data/schema/validator.js index 515c3cf7a7..c462f98d84 100644 --- a/ghost/core/core/server/data/schema/validator.js +++ b/ghost/core/core/server/data/schema/validator.js @@ -63,7 +63,7 @@ function validateSchema(tableName, model, options) { // validate boolean columns if (Object.prototype.hasOwnProperty.call(schema[tableName][columnKey], 'type') - && (schema[tableName][columnKey].type === 'bool' || schema[tableName][columnKey].type === 'boolean')) { + && schema[tableName][columnKey].type === 'boolean') { if (!(validator.isBoolean(strVal) || validator.isEmpty(strVal))) { message = tpl(messages.valueMustBeBoolean, { tableName: tableName, diff --git a/ghost/core/core/server/models/base/plugins/data-manipulation.js b/ghost/core/core/server/models/base/plugins/data-manipulation.js index 9078df8e73..2842d94fa9 100644 --- a/ghost/core/core/server/models/base/plugins/data-manipulation.js +++ b/ghost/core/core/server/models/base/plugins/data-manipulation.js @@ -78,7 +78,7 @@ module.exports = function (Bookshelf) { _.each(attrs, function each(value, key) { const tableDef = schema.tables[self.tableName]; const columnDef = tableDef ? tableDef[key] : null; - if (columnDef && (columnDef.type === 'bool' || columnDef.type === 'boolean')) { + if (columnDef?.type === 'boolean') { attrs[key] = value ? true : false; } }); diff --git a/ghost/core/test/unit/server/data/schema/integrity.test.js b/ghost/core/test/unit/server/data/schema/integrity.test.js index 18135bea76..61dcb8892c 100644 --- a/ghost/core/test/unit/server/data/schema/integrity.test.js +++ b/ghost/core/test/unit/server/data/schema/integrity.test.js @@ -35,7 +35,7 @@ const validateRouteSettings = require('../../../../../core/server/services/route */ describe('DB version integrity', function () { // Only these variables should need updating - const currentSchemaHash = 'ea7c1c293556a58c4f928111dc3be3c6'; + const currentSchemaHash = '7f8c1a992b307dff960165b0cfa1d2db'; const currentFixturesHash = '8cf221f0ed930ac1fe8030a58e60d64b'; const currentSettingsHash = '2978a5684a2d5fcf089f61f5d368a0c0'; const currentRoutesHash = '3d180d52c663d173a6be791ef411ed01'; diff --git a/ghost/core/test/unit/server/data/schema/schema.test.js b/ghost/core/test/unit/server/data/schema/schema.test.js index d272041257..1f21c277e9 100644 --- a/ghost/core/test/unit/server/data/schema/schema.test.js +++ b/ghost/core/test/unit/server/data/schema/schema.test.js @@ -7,10 +7,6 @@ const VALID_KEYS = { bigInteger: [ 'nullable' ], - bool: [ - 'nullable', - 'defaultTo' - ], boolean: [ 'nullable', 'defaultTo' diff --git a/ghost/core/test/unit/server/data/schema/validator.test.js b/ghost/core/test/unit/server/data/schema/validator.test.js index 56c394fd28..4f8fb7bb5f 100644 --- a/ghost/core/test/unit/server/data/schema/validator.test.js +++ b/ghost/core/test/unit/server/data/schema/validator.test.js @@ -64,16 +64,6 @@ describe('Validate Schema', function () { ); }); - it('transforms 0 and 1 (bool)', function () { - const post = models.Post.forge(testUtils.DataGenerator.forKnex.createPost({slug: 'test', featured: 0})); - post.get('featured').should.eql(0); - - return validateSchema('posts', post, {method: 'insert'}) - .then(function () { - post.get('featured').should.eql(false); - }); - }); - it('transforms 0 and 1 (boolean)', async function () { const user = models.User.forge(testUtils.DataGenerator.forKnex.createUser({email: 'test@example.com', comment_notifications: 0})); user.get('comment_notifications').should.eql(0);