From 69d07c892cbe5b959a1dbc1217cf19934c215d9a Mon Sep 17 00:00:00 2001 From: Simon Backx Date: Tue, 14 Mar 2023 10:58:58 +0100 Subject: [PATCH] Fixed getLazyRelation returning new model for optional relations (#16411) no issue When using `getLazyRelation` on an optional relation that is not set, it will return a newly created model instead of a model from the database. - Adds a new require option to `getLazyRelation`, that throws an error if the relation is not set (off by default to match existing use cases) - This caused a bug (not visible because we always pass a newsletter id) in email previews, where when the newsletter id was not explicitly set, it would use `newsletter = (await post.getLazyRelation('newsletter')) ?? (await this.models.Newsletter.getDefaultNewsletter());`, which always returned the first one, and could return a newly initiated newsletter with all properties set to undefined. - Some page snapshots are altered by this, because the usage of `getLazyRelation` on a post no longer sets the email relation to some new model. --- .../server/models/base/plugins/relations.js | 22 +++ .../__snapshots__/email-previews.test.js.snap | 139 ++++++++++++++++-- .../__snapshots__/pages.test.js.snap | 7 - .../__snapshots__/posts.test.js.snap | 7 - .../unit/server/models/base/relations.test.js | 58 ++++++++ 5 files changed, 207 insertions(+), 26 deletions(-) diff --git a/ghost/core/core/server/models/base/plugins/relations.js b/ghost/core/core/server/models/base/plugins/relations.js index 6884017ed9..91bc08b3d1 100644 --- a/ghost/core/core/server/models/base/plugins/relations.js +++ b/ghost/core/core/server/models/base/plugins/relations.js @@ -1,3 +1,5 @@ +const errors = require('@tryghost/errors'); + /** * @param {import('bookshelf')} Bookshelf */ @@ -9,6 +11,7 @@ module.exports = function (Bookshelf) { * @param {string} name Name of the relation to load * @param {Object} [options] Options to pass to the fetch when not yet loaded (or when force refreshing) * @param {boolean} [options.forceRefresh] If true, the relation will be fetched again even if it has already been loaded. + * @param {boolean} [options.require] Off by default. Throws an error if relation is not found. * @returns {Promise} */ getLazyRelation: async function (name, options = {}) { @@ -18,13 +21,32 @@ module.exports = function (Bookshelf) { } if (!this[name]) { + if (options.require) { + throw new errors.NotFoundError(); + } return undefined; } + + // Explicitly set require to false if it's not set, because default for .fetch is true (not false) + if (options.require) { + options.require = true; + } else { + options.require = false; + } + // Not yet loaded, or force refresh // Note that we don't use .refresh on the relation on options.forceRefresh // Because the relation can also be a collection, which doesn't have a refresh method const instance = this[name](); await instance.fetch(options); + + if (!instance.id && !(instance instanceof Bookshelf.Collection)) { + // Some weird behaviour in Bookshelf allows to just return a newly created model instance instead of throwing an error + if (options.require) { + throw new errors.NotFoundError(); + } + return undefined; + } this.relations[name] = instance; return instance; } diff --git a/ghost/core/test/e2e-api/admin/__snapshots__/email-previews.test.js.snap b/ghost/core/test/e2e-api/admin/__snapshots__/email-previews.test.js.snap index 3d322da90c..5fde66f9b1 100644 --- a/ghost/core/test/e2e-api/admin/__snapshots__/email-previews.test.js.snap +++ b/ghost/core/test/e2e-api/admin/__snapshots__/email-previews.test.js.snap @@ -370,6 +370,19 @@ table.body figcaption a { + + + - @@ -474,6 +490,23 @@ This is the actual post content... + + + +Ghost [http://127.0.0.1:2369/] + + +Default Newsletter [http://127.0.0.1:2369/] + + + + + + + + + + Post with email-only card [http://127.0.0.1:2369/p/d52c42ae-2755-455c-80ec-70b2ec55c904/] @@ -519,10 +552,13 @@ Another email card with a similar replacement, Jamie -Ghost © 2023 – Unsubscribe [http://127.0.0.1:2369/unsubscribe/?uuid=example-uuid] +Ghost © 2023 – Unsubscribe [http://127.0.0.1:2369/unsubscribe/?uuid=example-uuid&newsletter=requested-newsletter-uuid] +https://ghost.org/ + + @@ -545,7 +581,7 @@ exports[`Email Preview API Read can read post email preview with email card and Object { "access-control-allow-origin": "http://127.0.0.1:2369", "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", - "content-length": "14934", + "content-length": "18863", "content-type": "application/json; charset=utf-8", "content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/, "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, @@ -829,6 +865,19 @@ table.body figcaption a { + + + - @@ -933,6 +989,23 @@ This is my custom excerpt! + + + +Ghost [http://127.0.0.1:2369/] + + +Default Newsletter [http://127.0.0.1:2369/] + + + + + + + + + + HTML Ipsum [http://127.0.0.1:2369/html-ipsum/] @@ -955,6 +1028,9 @@ By Joe Bloggs – 1 Jan 2015 – View online → [http://127.0.0.1:2369/html-ips + + + HTML Ipsum Presents Pellentesque habitant morbi tristique senectus et netus et malesuada fames ac turpis egestas. Vestibulum tortor quam, feugiat vitae, ultricies eget, tempor sit amet, ante. Donec eu libero sit amet quam egestas semper. Aenean ultricies mi vitae est. Mauris placerat eleifend leo. Quisque sit amet est et sapien ullamcorper pharetra. Vestibulum erat wisi, condimentum sed, commodo vitae, ornare sit amet, wisi. Aenean fermentum, elit eget tincidunt condimentum, eros ipsum rutrum orci, sagittis tempus lacus enim ac dui. Donec non enim [\\\\\\"#\\\\\\"] in turpis pulvinar facilisis. Ut felis. @@ -991,10 +1067,13 @@ Header Level 3 -Ghost © 2023 – Unsubscribe [http://127.0.0.1:2369/unsubscribe/?uuid=example-uuid] +Ghost © 2023 – Unsubscribe [http://127.0.0.1:2369/unsubscribe/?uuid=example-uuid&newsletter=requested-newsletter-uuid] +https://ghost.org/ + + @@ -1017,7 +1096,7 @@ exports[`Email Preview API Read can read post email preview with fields 4: [head Object { "access-control-allow-origin": "http://127.0.0.1:2369", "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", - "content-length": "19049", + "content-length": "23658", "content-type": "application/json; charset=utf-8", "content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/, "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, @@ -1332,6 +1411,19 @@ table.body figcaption a {
+ + + + + + + + +
+
@@ -388,7 +401,7 @@ table.body figcaption a {
+

Hey Jamie %%{unknown}%%

Welcome to your first Ghost email!

This is the actual post content...

Another email card with a similar replacement, Jamie

@@ -406,9 +419,12 @@ table.body figcaption a { 		- + + + +
Ghost © 2023 – UnsubscribeGhost © 2023 – Unsubscribe
\\"Powered
+ + +
+ + + + + + + + +
+
@@ -846,8 +895,12 @@ table.body figcaption a {
+

HTML Ipsum Presents

Pellentesque habitant morbi tristique senectus et netus et malesuada fames ac turpis egestas. Vestibulum tortor quam, feugiat vitae, ultricies eget, tempor sit amet, ante. Donec eu libero sit amet quam egestas semper. Aenean ultricies mi vitae est. Mauris placerat eleifend leo. Quisque sit amet est et sapien ullamcorper pharetra. Vestibulum erat wisi, condimentum sed, commodo vitae, ornare sit amet, wisi. Aenean fermentum, elit eget tincidunt condimentum, eros ipsum rutrum orci, sagittis tempus lacus enim ac dui. Donec non enim in turpis pulvinar facilisis. Ut felis.

Header Level 2

  1. Lorem ipsum dolor sit amet, consectetuer adipiscing elit.
  2. Aliquam tincidunt mauris eu risus.

Lorem ipsum dolor sit amet, consectetur adipiscing elit. Vivamus magna. Cras in mi at felis aliquet congue. Ut a est eget ligula molestie gravida. Curabitur massa. Donec eleifend, libero at sagittis mollis, tellus est malesuada tellus, at luctus turpis elit sit amet quam. Vivamus pretium ornare est.

Header Level 3

  • Lorem ipsum dolor sit amet, consectetuer adipiscing elit.
  • Aliquam tincidunt mauris eu risus.
#header h1 a{display: block;width: 300px;height: 80px;}
@@ -865,9 +918,12 @@ table.body figcaption a { 		- + + + +
Ghost © 2023 – UnsubscribeGhost © 2023 – Unsubscribe
\\"Powered
+ + + - @@ -1436,6 +1531,23 @@ Testing links in email excerpt and apostrophes ' + + + +Ghost [http://127.0.0.1:2369/] + + +Default Newsletter [http://127.0.0.1:2369/] + + + + + + + + + + Post with email-only card [http://127.0.0.1:2369/p/d52c42ae-2755-455c-80ec-70b2ec55c904/] @@ -1475,10 +1587,13 @@ Testing links [https://ghost.org] in email excerpt and apostrophes ' -Ghost © 2023 – Unsubscribe [http://127.0.0.1:2369/unsubscribe/?uuid=example-uuid] +Ghost © 2023 – Unsubscribe [http://127.0.0.1:2369/unsubscribe/?uuid=example-uuid&newsletter=requested-newsletter-uuid] +https://ghost.org/ + + @@ -1514,7 +1629,7 @@ exports[`Email Preview API Read has custom content transformations for email com Object { "access-control-allow-origin": "http://127.0.0.1:2369", "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", - "content-length": "14700", + "content-length": "18629", "content-type": "application/json; charset=utf-8", "content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/, "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, diff --git a/ghost/core/test/e2e-webhooks/__snapshots__/pages.test.js.snap b/ghost/core/test/e2e-webhooks/__snapshots__/pages.test.js.snap index 7c71b5d387..2cac167806 100644 --- a/ghost/core/test/e2e-webhooks/__snapshots__/pages.test.js.snap +++ b/ghost/core/test/e2e-webhooks/__snapshots__/pages.test.js.snap @@ -51,7 +51,6 @@ Object { "created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, "custom_excerpt": null, "custom_template": null, - "email": Object {}, "excerpt": null, "feature_image": null, "feature_image_alt": null, @@ -1833,9 +1832,6 @@ Object { "created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, "custom_excerpt": null, "custom_template": null, - "email": Object { - "opened_count": null, - }, "excerpt": null, "feature_image": null, "feature_image_alt": null, @@ -2051,9 +2047,6 @@ Object { "created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, "custom_excerpt": null, "custom_template": null, - "email": Object { - "opened_count": null, - }, "excerpt": null, "feature_image": null, "feature_image_alt": null, diff --git a/ghost/core/test/e2e-webhooks/__snapshots__/posts.test.js.snap b/ghost/core/test/e2e-webhooks/__snapshots__/posts.test.js.snap index 5e154803f2..df3035c4d8 100644 --- a/ghost/core/test/e2e-webhooks/__snapshots__/posts.test.js.snap +++ b/ghost/core/test/e2e-webhooks/__snapshots__/posts.test.js.snap @@ -51,7 +51,6 @@ Object { "created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, "custom_excerpt": null, "custom_template": null, - "email": Object {}, "email_only": false, "email_segment": "all", "email_subject": null, @@ -1914,9 +1913,6 @@ Object { "created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, "custom_excerpt": null, "custom_template": null, - "email": Object { - "opened_count": null, - }, "email_only": false, "email_segment": "all", "email_subject": null, @@ -2156,9 +2152,6 @@ Object { "created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, "custom_excerpt": null, "custom_template": null, - "email": Object { - "opened_count": null, - }, "email_only": false, "email_segment": "all", "email_subject": null, diff --git a/ghost/core/test/unit/server/models/base/relations.test.js b/ghost/core/test/unit/server/models/base/relations.test.js index 5677737fda..d7550f8002 100644 --- a/ghost/core/test/unit/server/models/base/relations.test.js +++ b/ghost/core/test/unit/server/models/base/relations.test.js @@ -1,6 +1,7 @@ const should = require('should'); const sinon = require('sinon'); const models = require('../../../../../core/server/models'); +const assert = require('assert'); describe('Models: getLazyRelation', function () { before(function () { @@ -62,6 +63,7 @@ describe('Models: getLazyRelation', function () { throw new Error('Called twice'); } rel = this; + rel.id = 'test123'; // we need to set an id return this; }); @@ -79,6 +81,54 @@ describe('Models: getLazyRelation', function () { fetchStub.calledTwice.should.be.true(); }); + it('can handle fetch of model without id for optional relations', async function () { + var OtherModel = models.Base.Model.extend({ + tableName: 'other_models' + }); + + const TestModel = models.Base.Model.extend({ + tableName: 'test_models', + other() { + return this.belongsTo(OtherModel, 'other_id', 'id'); + } + }); + let rel = null; + sinon.stub(OtherModel.prototype, 'fetch').callsFake(function () { + if (rel !== null) { + throw new Error('Called twice'); + } + rel = this; + return this; + }); + + const modelA = TestModel.forge({id: '1'}); + should.not.exist(await modelA.getLazyRelation('other')); + }); + + it('throws for model without id for optional relations with require', async function () { + var OtherModel = models.Base.Model.extend({ + tableName: 'other_models' + }); + + const TestModel = models.Base.Model.extend({ + tableName: 'test_models', + other() { + return this.belongsTo(OtherModel, 'other_id', 'id'); + } + }); + let rel = null; + sinon.stub(OtherModel.prototype, 'fetch').callsFake(function () { + if (rel !== null) { + throw new Error('Called twice'); + } + rel = this; + return this; + }); + + const modelA = TestModel.forge({id: '1'}); + await assert.rejects(modelA.getLazyRelation('other', {require: true})); + }); + it('returns undefined for nonexistent relations', async function () { const TestModel = models.Base.Model.extend({ tableName: 'test_models' @@ -86,4 +136,12 @@ describe('Models: getLazyRelation', function () { const modelA = TestModel.forge({id: '1'}); should.not.exist(await modelA.getLazyRelation('other')); }); + + it('throws for nonexistent relations with require', async function () { + const TestModel = models.Base.Model.extend({ + tableName: 'test_models' + }); + const modelA = TestModel.forge({id: '1'}); + await assert.rejects(modelA.getLazyRelation('other', {require: true})); + }); });
+ + + + + + + + +
+
@@ -1350,7 +1442,7 @@ table.body figcaption a {
+

Testing links in email excerpt and apostrophes '

@@ -1368,9 +1460,12 @@ table.body figcaption a { 		- + + + +
Ghost © 2023 – UnsubscribeGhost © 2023 – Unsubscribe
\\"Powered