From 49ba26373db0c69257cd65a7b23f15d351edf751 Mon Sep 17 00:00:00 2001 From: Naz Date: Fri, 4 Jun 2021 15:38:42 +0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=8F=97=20Added=20"labs"=20setting=20enabl?= =?UTF-8?q?ing=20feature=20flags?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit refs https://github.com/TryGhost/Team/issues/757 refs https://github.com/TryGhost/Team/issues/332 refs https://github.com/TryGhost/Ghost/commit/ea6d656457eb9088c5a121fc8b3a4920a4e1b903 - We have a need a quick way to add features behind flags. The old way of "labs" is the quickest way to achieve this. It has ready tooling around it and well understood pitfalls. This change reintroduces "labs" group & key in settings table in the same shape it used to be (see reffed commit) - Next step will be introducing very basic guard rails to protect from pitfalls previous implementation of "labs" had. This will include an allowlist based input validation for lab's object's data - The labs being an "object" type is an EXCEPTION. Even though it's an antipattern we aim to move away from, for now it's the lowest impact solution that will unblock the use of flags in the system. A proper solution will come at some point. --- .../data/importer/importers/data/settings.js | 8 ++++ .../versions/4.7/03-add-labs-setting.js | 42 +++++++++++++++++++ core/server/data/schema/default-settings.json | 6 +++ test/unit/data/exporter/index_spec.js | 3 +- test/unit/data/schema/integrity_spec.js | 2 +- 5 files changed, 59 insertions(+), 2 deletions(-) create mode 100644 core/server/data/migrations/versions/4.7/03-add-labs-setting.js diff --git a/core/server/data/importer/importers/data/settings.js b/core/server/data/importer/importers/data/settings.js index f82987adb9..da1a07d53f 100644 --- a/core/server/data/importer/importers/data/settings.js +++ b/core/server/data/importer/importers/data/settings.js @@ -4,9 +4,11 @@ const ObjectId = require('bson-objectid').default; const _ = require('lodash'); const BaseImporter = require('./base'); const models = require('../../../../models'); +const defaultSettings = require('../../../schema').defaultSettings; const keyGroupMapper = require('../../../../api/shared/serializers/input/utils/settings-key-group-mapper'); const keyTypeMapper = require('../../../../api/shared/serializers/input/utils/settings-key-type-mapper'); +const labsDefaults = JSON.parse(defaultSettings.labs.labs.defaultValue); const ignoredSettings = ['slack_url', 'members_from_address', 'members_support_address']; // NOTE: drop support in Ghost 5.0 @@ -204,6 +206,12 @@ class SettingsImporter extends BaseImporter { } _.each(this.dataToImport, (obj) => { + if (obj.key === 'labs' && obj.value) { + // Overwrite the labs setting with our current defaults + // Ensures things that are enabled in new versions, are turned on + obj.value = JSON.stringify(_.assign({}, JSON.parse(obj.value), labsDefaults)); + } + // CASE: we do not import "from address" for members settings as that needs to go via validation with magic link if ((obj.key === 'members_from_address') || (obj.key === 'members_support_address')) { obj.value = null; diff --git a/core/server/data/migrations/versions/4.7/03-add-labs-setting.js b/core/server/data/migrations/versions/4.7/03-add-labs-setting.js new file mode 100644 index 0000000000..cee8e158ff --- /dev/null +++ b/core/server/data/migrations/versions/4.7/03-add-labs-setting.js @@ -0,0 +1,42 @@ +const ObjectID = require('bson-objectid'); +const logging = require('../../../../../shared/logging'); +const {createTransactionalMigration} = require('../../utils.js'); + +const MIGRATION_USER = 1; + +module.exports = createTransactionalMigration( + async function up(knex) { + const labsExists = await knex('settings') + .where('key', '=', 'labs') + .first(); + + if (!labsExists) { + logging.info('Adding "labs" record to "settings" table'); + + const now = knex.raw('CURRENT_TIMESTAMP'); + + await knex('settings') + .insert({ + id: ObjectID().toHexString(), + key: 'labs', + value: JSON.stringify({}), + group: 'labs', + type: 'object', + created_at: now, + created_by: MIGRATION_USER, + updated_at: now, + updated_by: MIGRATION_USER + }); + } else { + logging.warn('Skipped adding "labs" record to "settings" table. Record already exists!'); + } + }, + + async function down(knex) { + logging.info('Removing "labs" record from "settings" table'); + + await knex('settings') + .where('key', '=', 'labs') + .del(); + } +); diff --git a/core/server/data/schema/default-settings.json b/core/server/data/schema/default-settings.json index 0742f0c1c8..df9cf8254f 100644 --- a/core/server/data/schema/default-settings.json +++ b/core/server/data/schema/default-settings.json @@ -413,6 +413,12 @@ "type": "string" } }, + "labs": { + "labs": { + "defaultValue": "{}", + "type": "object" + } + }, "slack": { "slack_url": { "defaultValue": "", diff --git a/test/unit/data/exporter/index_spec.js b/test/unit/data/exporter/index_spec.js index 6c63067358..97bf69eca0 100644 --- a/test/unit/data/exporter/index_spec.js +++ b/test/unit/data/exporter/index_spec.js @@ -190,7 +190,8 @@ describe('Exporter', function () { }, 0); // NOTE: if default settings changed either modify the settings keys blocklist or increase allowedKeysLength - const allowedKeysLength = 76; + // This is a reminder to think about the importer/exporter scenarios ;) + const allowedKeysLength = 77; totalKeysLength.should.eql(SETTING_KEYS_BLOCKLIST.length + allowedKeysLength); }); }); diff --git a/test/unit/data/schema/integrity_spec.js b/test/unit/data/schema/integrity_spec.js index e7f417dfb1..4311d8eab5 100644 --- a/test/unit/data/schema/integrity_spec.js +++ b/test/unit/data/schema/integrity_spec.js @@ -34,7 +34,7 @@ describe('DB version integrity', function () { // Only these variables should need updating const currentSchemaHash = '2099a4ba6b9462c5a0aa36a434139e8b'; const currentFixturesHash = '8671672598d2a62e53418c4b91aa79a3'; - const currentSettingsHash = '8a3f69dfa11d56a6c630456a67f3a59a'; + const currentSettingsHash = '4f7fa90d4d545b9ec7eb0b86aa8f15ab'; const currentRoutesHash = '3d180d52c663d173a6be791ef411ed01'; // If this test is failing, then it is likely a change has been made that requires a DB version bump,