From b25da62ccae0a4523d6036f3adcf6446f7d23dbd Mon Sep 17 00:00:00 2001 From: kirrg001 Date: Thu, 7 Feb 2019 20:01:47 +0100 Subject: [PATCH] Ensured defaults when creating resources no issue - the model & api layer suffered from missing fields when creating resources - usually there is only a handful of fields which are required to insert a resource - the other fields are nullable and/or get defaults assigned - the API only returned the configured default fields and the fields you have sent to the API - this resulted in a response with missing fields - if you have listend on "created" event, the same happend - you received a model with missing fields - we now set the undefined fields to null on purpose to ensure a full model for both cases @NOTE: There is no endpoint to serve webhooks (not for v0.1, not for v2). Exposing the secret is required if an integration fetches it's api keys and it's webhooks. The secret is currently un-used and not implemented. --- core/server/models/base/index.js | 22 +++++++++++++++++-- core/test/acceptance/old/admin/tags_spec.js | 9 ++------ core/test/acceptance/old/admin/utils.js | 8 ------- .../acceptance/old/admin/webhooks_spec.js | 2 +- core/test/regression/api/v0.1/tags_spec.js | 9 ++------ core/test/regression/api/v0.1/utils.js | 8 ------- core/test/regression/api/v2/admin/utils.js | 8 ------- 7 files changed, 25 insertions(+), 41 deletions(-) diff --git a/core/server/models/base/index.js b/core/server/models/base/index.js index 99324a0096..34a228625c 100644 --- a/core/server/models/base/index.js +++ b/core/server/models/base/index.js @@ -328,9 +328,27 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({ } } - model._changed = _.cloneDeep(model.changed); + return Promise.resolve(this.onValidate(model, attr, options)) + .then(() => { + /** + * @NOTE: + * + * The API requires only specific attributes to send. If we don't set the rest explicitly to null, + * we end up in a situation that on "created" events the field set is incomplete, which is super confusing + * and hard to work with if you trigger internal events, which rely on full field set. This ensures consistency. + * + * @NOTE: + * + * Happens after validation to ensure we don't set fields which are not nullable on db level. + */ + _.each(Object.keys(schema.tables[this.tableName]), (columnKey) => { + if (model.get(columnKey) === undefined) { + model.set(columnKey, null); + } + }); - return Promise.resolve(this.onValidate(model, attr, options)); + model._changed = _.cloneDeep(model.changed); + }); }, /** diff --git a/core/test/acceptance/old/admin/tags_spec.js b/core/test/acceptance/old/admin/tags_spec.js index 9a4e65eaa6..6553ba82da 100644 --- a/core/test/acceptance/old/admin/tags_spec.js +++ b/core/test/acceptance/old/admin/tags_spec.js @@ -99,13 +99,8 @@ describe('Tag API', function () { should.exist(jsonResponse); should.exist(jsonResponse.tags); jsonResponse.tags.should.have.length(1); - // @TODO: model layer has no defaults for these properties - localUtils.API.checkResponse(jsonResponse.tags[0], 'tag', ['url'], [ - 'feature_image', - 'meta_description', - 'meta_title', - 'parent' - ]); + + localUtils.API.checkResponse(jsonResponse.tags[0], 'tag', ['url']); testUtils.API.isISO8601(jsonResponse.tags[0].created_at).should.be.true(); }); }); diff --git a/core/test/acceptance/old/admin/utils.js b/core/test/acceptance/old/admin/utils.js index 318cb5c1d2..455b5c9ab5 100644 --- a/core/test/acceptance/old/admin/utils.js +++ b/core/test/acceptance/old/admin/utils.js @@ -61,14 +61,6 @@ const expectedProperties = { , webhook: _(schema.webhooks) .keys() - .without( - 'name', - 'last_triggered_at', - 'last_triggered_error', - 'last_triggered_status', - 'secret', - 'integration_id' - ) }; _.each(expectedProperties, (value, key) => { diff --git a/core/test/acceptance/old/admin/webhooks_spec.js b/core/test/acceptance/old/admin/webhooks_spec.js index a916123d9b..4d8889cd0b 100644 --- a/core/test/acceptance/old/admin/webhooks_spec.js +++ b/core/test/acceptance/old/admin/webhooks_spec.js @@ -44,7 +44,7 @@ describe('Webhooks API', function () { should.exist(jsonResponse.webhooks); - localUtils.API.checkResponse(jsonResponse.webhooks[0], 'webhook', ['name', 'secret']); + localUtils.API.checkResponse(jsonResponse.webhooks[0], 'webhook'); jsonResponse.webhooks[0].event.should.equal(webhookData.event); jsonResponse.webhooks[0].target_url.should.equal(webhookData.target_url); diff --git a/core/test/regression/api/v0.1/tags_spec.js b/core/test/regression/api/v0.1/tags_spec.js index cd0b079cf1..56c74189dc 100644 --- a/core/test/regression/api/v0.1/tags_spec.js +++ b/core/test/regression/api/v0.1/tags_spec.js @@ -88,13 +88,8 @@ describe('Tag API', function () { should.exist(jsonResponse); should.exist(jsonResponse.tags); jsonResponse.tags.should.have.length(1); - // @TODO: model layer has no defaults for these properties - localUtils.API.checkResponse(jsonResponse.tags[0], 'tag', null, [ - 'feature_image', - 'meta_description', - 'meta_title', - 'parent' - ]); + + localUtils.API.checkResponse(jsonResponse.tags[0], 'tag'); testUtils.API.isISO8601(jsonResponse.tags[0].created_at).should.be.true(); }); }); diff --git a/core/test/regression/api/v0.1/utils.js b/core/test/regression/api/v0.1/utils.js index 9b77e024cc..8100dfaba3 100644 --- a/core/test/regression/api/v0.1/utils.js +++ b/core/test/regression/api/v0.1/utils.js @@ -72,14 +72,6 @@ const expectedProperties = { webhook: { default: _(schema.webhooks) .keys() - .without( - 'name', - 'last_triggered_at', - 'last_triggered_error', - 'last_triggered_status', - 'secret', - 'integration_id' - ) .value() } }; diff --git a/core/test/regression/api/v2/admin/utils.js b/core/test/regression/api/v2/admin/utils.js index 100943e7e7..cff21dd144 100644 --- a/core/test/regression/api/v2/admin/utils.js +++ b/core/test/regression/api/v2/admin/utils.js @@ -58,14 +58,6 @@ const expectedProperties = { , webhook: _(schema.webhooks) .keys() - .without( - 'name', - 'last_triggered_at', - 'last_triggered_error', - 'last_triggered_status', - 'secret', - 'integration_id' - ) }; _.each(expectedProperties, (value, key) => {