Cleaned up Admin API v2 posts/pages input serializer (#10516)

no issue - make use of filter instead of status=all or data.page - nql was designed to filter data on database layer - do not break v0.1 - we just got rid of the "status" query param, you should use the filter instead - get rid of the ugly condition to remove page field if "fields" param was used - allow filtering on model layer for "findOne" - do not allow filtering for "findOne" on API layer for now - the API controller defines what is allowed - the model layer can allow more by default - we can re-use the powerful filter logic without adding hacks
2024-11-28 14:03:48 +03:00 · 2019-02-22 12:07:34 +01:00 · 2019-02-22 12:07:34 +01:00 · dfd350bd69
commit dfd350bd69
parent c749ce785d
9 changed files with 212 additions and 101 deletions
--- a/core/server/api/v2/utils/serializers/input/pages.js
+++ b/core/server/api/v2/utils/serializers/input/pages.js
@ -25,11 +25,7 @@ function setDefaultOrder(frame) {
    }
 }

-module.exports = {
-    browse(apiConfig, frame) {
-        debug('browse');
-
-        /**
+/**
 * CASE:
 *
 * - the content api endpoints for pages forces the model layer to return static pages only
@ -37,11 +33,27 @@ module.exports = {
 *
 * @TODO: https://github.com/TryGhost/Ghost/issues/10268
 */
+const forcePageFilter = (frame) => {
    if (frame.options.filter) {
        frame.options.filter = `(${frame.options.filter})+page:true`;
    } else {
        frame.options.filter = 'page:true';
    }
+};
+
+const forceStatusFilter = (frame) => {
+    if (!frame.options.filter) {
+        frame.options.filter = 'status:[draft,published,scheduled]';
+    } else if (!frame.options.filter.match(/status:/)) {
+        frame.options.filter = `(${frame.options.filter})+status:[draft,published,scheduled]`;
+    }
+};
+
+module.exports = {
+    browse(apiConfig, frame) {
+        debug('browse');
+
+        forcePageFilter(frame);

        if (localUtils.isContentAPI(frame)) {
            removeMobiledocFormat(frame);
@ -49,10 +61,7 @@ module.exports = {
        }

        if (!localUtils.isContentAPI(frame)) {
-            // @TODO: remove when we drop v0.1
-            if (!frame.options.filter || !frame.options.filter.match(/status:/)) {
-                frame.options.status = 'all';
-            }
+            forceStatusFilter(frame);
        }

        debug(frame.options);
@ -61,7 +70,7 @@ module.exports = {
    read(apiConfig, frame) {
        debug('read');

-        frame.data.page = true;
+        forcePageFilter(frame);

        if (localUtils.isContentAPI(frame)) {
            removeMobiledocFormat(frame);
@ -69,10 +78,7 @@ module.exports = {
        }

        if (!localUtils.isContentAPI(frame)) {
-            // @TODO: remove when we drop v0.1
-            if (!frame.options.filter || !frame.options.filter.match(/status:/)) {
-                frame.data.status = 'all';
-            }
+            forceStatusFilter(frame);
        }

        debug(frame.options);
@ -100,8 +106,8 @@ module.exports = {

        debug('edit');

-        // @NOTE: force not being able to update a page via pages endpoint
-        frame.options.page = true;
+        forceStatusFilter(frame);
+        forcePageFilter(frame);
    },

    destroy(apiConfig, frame) {
--- a/core/server/api/v2/utils/serializers/input/posts.js
+++ b/core/server/api/v2/utils/serializers/input/posts.js
@ -34,11 +34,7 @@ function setDefaultOrder(frame) {
    }
 }

-module.exports = {
-    browse(apiConfig, frame) {
-        debug('browse');
-
-        /**
+/**
 * CASE:
 *
 * - posts endpoint only returns posts, not pages
@ -46,11 +42,27 @@ module.exports = {
 *
 * @TODO: https://github.com/TryGhost/Ghost/issues/10268
 */
+const forcePageFilter = (frame) => {
    if (frame.options.filter) {
        frame.options.filter = `(${frame.options.filter})+page:false`;
    } else {
        frame.options.filter = 'page:false';
    }
+};
+
+const forceStatusFilter = (frame) => {
+    if (!frame.options.filter) {
+        frame.options.filter = 'status:[draft,published,scheduled]';
+    } else if (!frame.options.filter.match(/status:/)) {
+        frame.options.filter = `(${frame.options.filter})+status:[draft,published,scheduled]`;
+    }
+};
+
+module.exports = {
+    browse(apiConfig, frame) {
+        debug('browse');
+
+        forcePageFilter(frame);

        /**
         * ## current cases:
@ -71,10 +83,7 @@ module.exports = {
        }

        if (!localUtils.isContentAPI(frame)) {
-            // @TODO: remove when we drop v0.1
-            if (!frame.options.filter || !frame.options.filter.match(/status:/)) {
-                frame.options.status = 'all';
-            }
+            forceStatusFilter(frame);
        }

        debug(frame.options);
@ -83,7 +92,7 @@ module.exports = {
    read(apiConfig, frame) {
        debug('read');

-        frame.data.page = false;
+        forcePageFilter(frame);

        /**
         * ## current cases:
@ -104,16 +113,13 @@ module.exports = {
        }

        if (!localUtils.isContentAPI(frame)) {
-            // @TODO: remove when we drop v0.1
-            if (!frame.options.filter || !frame.options.filter.match(/status:/)) {
-                frame.data.status = 'all';
-            }
+            forceStatusFilter(frame);
        }

        debug(frame.options);
    },

-    add(apiConfig, frame) {
+    add(apiConfig, frame, options = {add: true}) {
        debug('add');

        if (_.get(frame,'options.source')) {
@ -126,15 +132,17 @@ module.exports = {

        frame.data.posts[0] = url.forPost(Object.assign({}, frame.data.posts[0]), frame.options);

-        // @NOTE: force storing post
+        // @NOTE: force adding post
+        if (options.add) {
            frame.data.posts[0].page = false;
+        }
    },

    edit(apiConfig, frame) {
-        this.add(apiConfig, frame);
+        this.add(apiConfig, frame, {add: false});

-        // @NOTE: force that you cannot update pages via posts endpoint
-        frame.options.page = false;
+        forceStatusFilter(frame);
+        forcePageFilter(frame);
    },

    destroy(apiConfig, frame) {
--- a/core/server/api/v2/utils/serializers/output/utils/mapper.js
+++ b/core/server/api/v2/utils/serializers/output/utils/mapper.js
@ -56,13 +56,6 @@ const mapPost = (model, frame) => {
        });
    }

-    /**
-     * Remove extra data attributes passed for filtering when used with columns/fields as bookshelf doesn't filter it out
-     */
-    if (frame.options.columns && frame.options.columns.indexOf('page') < 0) {
-        delete jsonModel.page;
-    }
-
    return jsonModel;
 };

--- a/core/server/models/base/index.js
+++ b/core/server/models/base/index.js
@ -917,9 +917,16 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({
     * @return {Promise(ghostBookshelf.Model)} Single Model
     */
    findOne: function findOne(data, unfilteredOptions) {
-        var options = this.filterOptions(unfilteredOptions, 'findOne');
+        const options = this.filterOptions(unfilteredOptions, 'findOne');
        data = this.filterData(data);
-        return this.forge(data).fetch(options);
+        const model = this.forge(data);
+
+        // @NOTE: The API layer decides if this option is allowed
+        if (options.filter) {
+            model.applyDefaultAndCustomFilters(options);
+        }
+
+        return model.fetch(options);
    },

    /**
@ -936,17 +943,15 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({
    edit: function edit(data, unfilteredOptions) {
        const options = this.filterOptions(unfilteredOptions, 'edit');
        const id = options.id;
-        let model;
-
-        if (options.hasOwnProperty('page')) {
-            model = this.forge({id: id, page: options.page});
-            delete options.page;
-        } else {
-            model = this.forge({id: id});
-        }
+        const model = this.forge({id: id});

        data = this.filterData(data);

+        // @NOTE: The API layer decides if this option is allowed
+        if (options.filter) {
+            model.applyDefaultAndCustomFilters(options);
+        }
+
        // We allow you to disable timestamps when run migration, so that the posts `updated_at` value is the same
        if (options.importing) {
            model.hasTimestamps = false;
--- a/core/server/models/post.js
+++ b/core/server/models/post.js
@ -701,11 +701,11 @@ Post = ghostBookshelf.Model.extend({
            // whitelists for the `options` hash argument on methods, by method name.
            // these are the only options that can be passed to Bookshelf / Knex.
            validOptions = {
-                findOne: ['columns', 'importing', 'withRelated', 'require'],
+                findOne: ['columns', 'importing', 'withRelated', 'require', 'filter'],
                findPage: ['status', 'staticPages'],
                findAll: ['columns', 'filter'],
                destroy: ['destroyAll', 'destroyBy'],
-                edit: ['page']
+                edit: ['filter']
            };

        // The post model additionally supports having a formats option
@ -753,10 +753,11 @@ Post = ghostBookshelf.Model.extend({
     * @extends ghostBookshelf.Model.findOne to handle post status
     * **See:** [ghostBookshelf.Model.findOne](base.js.html#Find%20One)
     */
-    findOne: function findOne(data, options) {
-        data = _.defaults(data || {}, {
-            status: 'published'
-        });
+    findOne: function findOne(data = {}, options = {}) {
+        // @TODO: remove when we drop v0.1
+        if (!options.filter && !data.status) {
+            data.status = 'published';
+        }

        if (data.status === 'all') {
            delete data.status;
--- a/core/test/regression/api/v0.1/public_api_spec.js
+++ b/core/test/regression/api/v0.1/public_api_spec.js
@ -146,6 +146,18 @@ describe('Public API', function () {
            });
    });

+    it('read post with filter', function () {
+        return request
+            .get(localUtils.API.getApiQuery(`posts/${testUtils.DataGenerator.Content.posts[0].id}/?client_id=ghost-admin&client_secret=not_available&filter=slug:test`))
+            .expect('Content-Type', /json/)
+            .expect('Cache-Control', testUtils.cacheRules.private)
+            .expect(200)
+            .then((res) => {
+                // proofs that "filter" does not work for v0.1
+                localUtils.API.checkResponse(res.body.posts[0], 'post');
+            });
+    });
+
    it('browse posts with inverse filters', function (done) {
        request.get(localUtils.API.getApiQuery('posts/?client_id=ghost-admin&client_secret=not_available&filter=tag:-[bacon,pollo,getting-started]&include=tags'))
            .expect('Content-Type', /json/)
--- a/core/test/regression/api/v2/content/posts_spec.js
+++ b/core/test/regression/api/v2/content/posts_spec.js
@ -139,4 +139,16 @@ describe('Posts', function () {
            .expect('Cache-Control', testUtils.cacheRules.private)
            .expect(404);
    });
+
+    it('can read post with fields', function () {
+        return request
+            .get(localUtils.API.getApiQuery(`posts/${testUtils.DataGenerator.Content.posts[0].id}/?key=${validKey}&fields=title,slug`))
+            .set('Origin', testUtils.API.getURL())
+            .expect('Content-Type', /json/)
+            .expect('Cache-Control', testUtils.cacheRules.private)
+            .expect(200)
+            .then((res)=> {
+                localUtils.API.checkResponse(res.body.posts[0], 'post', null, null, ['id', 'title', 'slug']);
+            });
+    });
 });
--- a/core/test/unit/api/v2/utils/serializers/input/pages_spec.js
+++ b/core/test/unit/api/v2/utils/serializers/input/pages_spec.js
@ -71,37 +71,92 @@ describe('Unit: v2/utils/serializers/input/pages', function () {
    });

    describe('read', function () {
-        it('default', function () {
+        it('content api default', function () {
            const apiConfig = {};
            const frame = {
                options: {
                    context: {}
                },
-                data: {
-                    status: 'all'
-                }
+                data: {}
            };

            serializers.input.pages.read(apiConfig, frame);
-            frame.data.status.should.eql('all');
-            frame.data.page.should.eql(true);
+            frame.options.filter.should.eql('page:true');
        });

-        it('overrides page', function () {
+        it('content api default', function () {
            const apiConfig = {};
            const frame = {
+                apiType: 'content',
                options: {
-                    context: {}
+                    context: {
+                        user: 0,
+                        api_key: {
+                            id: 1,
+                            type: 'content'
                        },
-                data: {
-                    status: 'all',
-                    page: false
                    }
+                },
+                data: {}
            };

            serializers.input.pages.read(apiConfig, frame);
-            frame.data.status.should.eql('all');
-            frame.data.page.should.eql(true);
+            frame.options.filter.should.eql('page:true');
+        });
+
+        it('admin api default', function () {
+            const apiConfig = {};
+            const frame = {
+                apiType: 'admin',
+                options: {
+                    context: {
+                        user: 0,
+                        api_key: {
+                            id: 1,
+                            type: 'admin'
+                        },
+                    }
+                },
+                data: {}
+            };
+
+            serializers.input.pages.read(apiConfig, frame);
+            frame.options.filter.should.eql('(page:true)+status:[draft,published,scheduled]');
+        });
+
+        it('custom page filter', function () {
+            const apiConfig = {};
+            const frame = {
+                options: {
+                    filter: 'page:false',
+                    context: {}
+                },
+                data: {}
+            };
+
+            serializers.input.pages.read(apiConfig, frame);
+            frame.options.filter.should.eql('(page:false)+page:true');
+        });
+
+        it('custom status filter', function () {
+            const apiConfig = {};
+            const frame = {
+                apiType: 'admin',
+                options: {
+                    filter: 'status:draft',
+                    context: {
+                        user: 0,
+                        api_key: {
+                            id: 1,
+                            type: 'admin'
+                        },
+                    }
+                },
+                data: {}
+            };
+
+            serializers.input.pages.read(apiConfig, frame);
+            frame.options.filter.should.eql('(status:draft)+page:true');
        });

        it('remove mobiledoc option from formats', function () {
--- a/core/test/unit/api/v2/utils/serializers/input/posts_spec.js
+++ b/core/test/unit/api/v2/utils/serializers/input/posts_spec.js
@ -35,7 +35,7 @@ describe('Unit: v2/utils/serializers/input/posts', function () {
            };

            serializers.input.posts.browse(apiConfig, frame);
-            should.equal(frame.options.filter, 'page:false');
+            should.equal(frame.options.filter, '(page:false)+status:[draft,published,scheduled]');
        });

        it('combine filters', function () {
@ -135,7 +135,7 @@ describe('Unit: v2/utils/serializers/input/posts', function () {
    });

    describe('read', function () {
-        it('with apiType of "content" it sets data.page to false', function () {
+        it('with apiType of "content" it forces page filter', function () {
            const apiConfig = {};
            const frame = {
                apiType: 'content',
@ -144,25 +144,24 @@ describe('Unit: v2/utils/serializers/input/posts', function () {
            };

            serializers.input.posts.read(apiConfig, frame);
-            frame.data.page.should.eql(false);
+            frame.options.filter.should.eql('page:false');
        });

-        it('with apiType of "content" it overrides data.page to be false', function () {
+        it('with apiType of "content" it forces page false filter', function () {
            const apiConfig = {};
            const frame = {
                apiType: 'content',
-                options: {},
-                data: {
-                    status: 'all',
-                    page: true
-                }
+                options: {
+                    filter: 'page:true'
+                },
+                data: {}
            };

            serializers.input.posts.read(apiConfig, frame);
-            frame.data.page.should.eql(false);
+            frame.options.filter.should.eql('(page:true)+page:false');
        });

-        it('with apiType of "admin" it does not set data.page', function () {
+        it('with apiType of "admin" it forces page & status false filter', function () {
            const apiConfig = {};
            const frame = {
                apiType: 'admin',
@ -178,7 +177,27 @@ describe('Unit: v2/utils/serializers/input/posts', function () {
            };

            serializers.input.posts.read(apiConfig, frame);
-            should.equal(frame.data.page, false);
+            frame.options.filter.should.eql('(page:false)+status:[draft,published,scheduled]');
+        });
+
+        it('with apiType of "admin" it forces page filter & respects custom status filter', function () {
+            const apiConfig = {};
+            const frame = {
+                apiType: 'admin',
+                options: {
+                    context: {
+                        api_key: {
+                            id: 1,
+                            type: 'admin'
+                        }
+                    },
+                    filter: 'status:draft'
+                },
+                data: {}
+            };
+
+            serializers.input.posts.read(apiConfig, frame);
+            frame.options.filter.should.eql('(status:draft)+page:false');
        });

        it('remove mobiledoc option from formats', function () {