From d209a4d0137da28136bed3927823302c0f0cf3d1 Mon Sep 17 00:00:00 2001 From: kirrg001 Date: Tue, 10 Apr 2018 00:20:19 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20=20Fixed=20importer=20bug:=20can?= =?UTF-8?q?'t=20resolve=20authors=20relation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit closes #9547 - you setup a blog with the following owner: - email: test@ghost.org - name: test - slug: test - now you import a JSON db file, which holds the exact same owner - this owner won't be imported, because it's a duplicate - but the slug is different (!) - the importer tries to find a matching existing user, but won't find anything - the importer then send an empty authors array `post.authors=[]` into the model layer - this is not allowed -> this would mean, you are actively trying to unset all authors --- .../data/importer/importers/data/posts.js | 19 ++++++++- core/server/models/relations/authors.js | 14 +++---- .../data/importer/importers/data_spec.js | 7 ++-- core/test/unit/models/post_spec.js | 28 +++++++++++++ .../utils/fixtures/export/export-authors.json | 42 ++++++++++++++++++- 5 files changed, 98 insertions(+), 12 deletions(-) diff --git a/core/server/data/importer/importers/data/posts.js b/core/server/data/importer/importers/data/posts.js index 45f8f14537..9b9c4a9236 100644 --- a/core/server/data/importer/importers/data/posts.js +++ b/core/server/data/importer/importers/data/posts.js @@ -74,6 +74,12 @@ class PostsImporter extends BaseImporter { * Replace all identifier references. */ replaceIdentifiers() { + const ownerUserId = _.find(this.requiredExistingData.users, (user) => { + if (user.roles[0].name === 'Owner') { + return true; + } + }).id; + const run = (postToImport, postIndex, targetProperty, tableName) => { if (!postToImport[targetProperty] || !postToImport[targetProperty].length) { return; @@ -104,7 +110,7 @@ class PostsImporter extends BaseImporter { return; } - // CASE: search through existing data + // CASE: search through existing data by unique attribute let existingObject = _.find(this.requiredExistingData[tableName], {slug: objectInFile.slug}); if (existingObject) { @@ -117,6 +123,17 @@ class PostsImporter extends BaseImporter { this.dataToImport[postIndex][targetProperty] = _.filter(this.dataToImport[postIndex][targetProperty], ((object, index) => { return indexesToRemove.indexOf(index) === -1; })); + + // CASE: we had to remove all the relations, because we could not match or find the relation reference. + // e.g. you import a post with multiple authors. Only the primary author is assigned. + // But the primary author won't be imported and we can't find the author in the existing database. + // This would end in `post.authors = []`, which is not allowed. There must be always minimum one author. + // We fallback to the owner user. + if (targetProperty === 'authors' && !this.dataToImport[postIndex][targetProperty].length) { + this.dataToImport[postIndex][targetProperty] = [{ + id: ownerUserId + }]; + } }; _.each(this.dataToImport, (postToImport, postIndex) => { diff --git a/core/server/models/relations/authors.js b/core/server/models/relations/authors.js index 105390127f..0f1aae9a28 100644 --- a/core/server/models/relations/authors.js +++ b/core/server/models/relations/authors.js @@ -105,6 +105,13 @@ module.exports.extendModel = function extendModel(Post, Posts, ghostBookshelf) { */ model.unset('author'); + // CASE: you can't delete all authors + if (model.get('authors') && !model.get('authors').length) { + throw new common.errors.ValidationError({ + message: 'At least one author is required.' + }); + } + // CASE: `post.author_id` has changed if (model.hasChanged('author_id')) { // CASE: you don't send `post.authors` @@ -126,13 +133,6 @@ module.exports.extendModel = function extendModel(Post, Posts, ghostBookshelf) { } } - // CASE: you can't delete all authors - if (model.get('authors') && !model.get('authors').length) { - throw new common.errors.ValidationError({ - message: 'At least one author is required.' - }); - } - // CASE: if you change `post.author_id`, we have to update the primary author // CASE: if the `author_id` has change and you pass `posts.authors`, we already check above that // the primary author id must be equal diff --git a/core/test/integration/data/importer/importers/data_spec.js b/core/test/integration/data/importer/importers/data_spec.js index 8674015d70..9145badab8 100644 --- a/core/test/integration/data/importer/importers/data_spec.js +++ b/core/test/integration/data/importer/importers/data_spec.js @@ -1858,7 +1858,7 @@ describe('Import (new test structure)', function () { const posts = importedData[0].posts, users = importedData[1].users; - // owner + 3 imported users + // owner + 3 imported users + 1 duplicate users.length.should.eql(4); users[0].slug.should.eql(exportData.data.users[0].slug); users[1].slug.should.eql(exportData.data.users[1].slug); @@ -1872,9 +1872,10 @@ describe('Import (new test structure)', function () { posts[0].authors.length.should.eql(1); posts[0].authors[0].id.should.eql(users[2].id); - posts[1].author.should.eql(users[0].id); + // falls back to owner user, because target reference could not be resolved (duplicate) + posts[1].author.should.eql(testUtils.DataGenerator.Content.users[0].id); posts[1].authors.length.should.eql(1); - posts[1].authors[0].id.should.eql(users[0].id); + posts[1].authors[0].id.should.eql(testUtils.DataGenerator.Content.users[0].id); posts[2].author.should.eql(users[1].id); posts[2].authors.length.should.eql(3); diff --git a/core/test/unit/models/post_spec.js b/core/test/unit/models/post_spec.js index b9ee69872e..8750e9e359 100644 --- a/core/test/unit/models/post_spec.js +++ b/core/test/unit/models/post_spec.js @@ -281,6 +281,34 @@ describe('Unit: models/post', function () { }); }); + it('[not allowed] with empty authors ([]), without author_id', function () { + const post = testUtils.DataGenerator.forKnex.createPost(); + delete post.author_id; + post.authors = []; + + return models.Post.add(post, {withRelated: ['author', 'authors']}) + .then(function () { + 'Expected error'.should.eql(false); + }) + .catch(function (err) { + (err instanceof common.errors.ValidationError).should.eql(true); + }); + }); + + it('[not allowed] with empty authors ([]), with author_id', function () { + const post = testUtils.DataGenerator.forKnex.createPost(); + post.author_id.should.eql(testUtils.DataGenerator.forKnex.users[0].id); + post.authors = []; + + return models.Post.add(post, {withRelated: ['author', 'authors']}) + .then(function () { + 'Expected error'.should.eql(false); + }) + .catch(function (err) { + (err instanceof common.errors.ValidationError).should.eql(true); + }); + }); + it('with authors, with author_id', function () { const post = testUtils.DataGenerator.forKnex.createPost(); post.author_id.should.eql(testUtils.DataGenerator.forKnex.users[0].id); diff --git a/core/test/utils/fixtures/export/export-authors.json b/core/test/utils/fixtures/export/export-authors.json index 7bca573401..94f116d66b 100644 --- a/core/test/utils/fixtures/export/export-authors.json +++ b/core/test/utils/fixtures/export/export-authors.json @@ -247,6 +247,34 @@ "created_by": "1", "updated_at": "2017-09-01T12:30:59.000Z", "updated_by": "1" + }, + { + "id": "5951f5fca366002ebd5dbef11", + "name": "Joe Blogg's", + "slug": "joe-bloggs-1", + "ghost_auth_access_token": null, + "ghost_auth_id": null, + "password": "$2a$10$.pZeeBE0gHXd0PTnbT/ph.GEKgd0Wd3q2pWna3ynTGBkPKnGIKABC", + "email": "jbloggs@example.com", + "profile_image": "/content/images/2017/05/authorlogo.jpeg", + "cover_image": "/content/images/2017/05/authorcover.jpeg", + "bio": "I'm Joe's father, the good looking one!", + "website": "http://joebloggs.com", + "location": null, + "facebook": null, + "twitter": null, + "accessibility": null, + "status": "active", + "locale": "en_US", + "visibility": "public", + "meta_title": null, + "meta_description": null, + "tour": "[\"getting-started\",\"using-the-editor\",\"static-post\",\"featured-post\",\"upload-a-theme\"]", + "last_seen": "2017-07-01T12:30:37.000Z", + "created_at": "2017-09-01T12:29:51.000Z", + "created_by": "1", + "updated_at": "2017-09-01T12:30:59.000Z", + "updated_by": "1" } ], "posts_authors": [ @@ -291,11 +319,23 @@ "post_id": "59a952be7d79ed06b0d21129", "author_id": "5951f5fca366002ebd5dbefff", "sort_order": 1 - }, { + }, + { "id": "5a8c0aa22a49c40927b18479", "post_id": "59a952be7d79ed06b0d21129", "author_id": "5951f5fca366002ebd5dbef10", "sort_order": 2 + }, + { + "id": "5a8c0aa22a49c40927b18479", + "post_id": "59a952be7d79ed06b0d21129", + "author_id": "5951f5fca366002ebd5dbef10", + "sort_order": 2 + }, + { + "id": "5a8c0aa22a49c40927b18480", + "post_id": "59a952be7d79ed06b0d21128", + "author_id": "5951f5fca366002ebd5dbef11" } ] }