From 2a9e1826498e7b6f49b3ba548d42311272070249 Mon Sep 17 00:00:00 2001 From: roschaefer Date: Tue, 22 Oct 2019 19:56:02 +0200 Subject: [PATCH 1/2] fix: performance issue with ordering @mattwr18 @aonomike You must never `ORDER BY` a property with a `@cypher` directive. Reason: The order by performance will be terribly poor. See my issue: https://github.com/neo4j-graphql/neo4j-graphql-js/issues/239 And my PR: https://github.com/neo4j-graphql/neo4j-graphql-js/pull/247 --- backend/src/schema/resolvers/posts.js | 5 +- backend/src/schema/resolvers/posts.spec.js | 82 +++++++++++++++------- backend/src/schema/types/type/Post.gql | 35 +++++++++ webapp/pages/index.vue | 2 +- 4 files changed, 95 insertions(+), 29 deletions(-) diff --git a/backend/src/schema/resolvers/posts.js b/backend/src/schema/resolvers/posts.js index 069fb3058..3b3065277 100644 --- a/backend/src/schema/resolvers/posts.js +++ b/backend/src/schema/resolvers/posts.js @@ -234,6 +234,7 @@ export default { const deletePreviousRelationsResponse = await transaction.run( ` MATCH (:User)-[previousRelations:PINNED]->(post:Post) + REMOVE post.pinned DELETE previousRelations RETURN post `, @@ -248,6 +249,7 @@ export default { MATCH (user:User {id: $userId}) WHERE user.role = 'admin' MATCH (post:Post {id: $params.id}) MERGE (user)-[pinned:PINNED {createdAt: toString(datetime())}]->(post) + SET post.pinned = true RETURN post, pinned.createdAt as pinnedAt `, { userId, params }, @@ -276,6 +278,7 @@ export default { const unpinPostTransactionResponse = await transaction.run( ` MATCH (:User)-[previousRelations:PINNED]->(post:Post {id: $params.id}) + REMOVE post.pinned DELETE previousRelations RETURN post `, @@ -293,7 +296,7 @@ export default { }, Post: { ...Resolver('Post', { - undefinedToNull: ['activityId', 'objectId', 'image', 'language', 'pinnedAt'], + undefinedToNull: ['activityId', 'objectId', 'image', 'language', 'pinnedAt', 'pinned'], hasMany: { tags: '-[:TAGGED]->(related:Tag)', categories: '-[:CATEGORIZED]->(related:Category)', diff --git a/backend/src/schema/resolvers/posts.spec.js b/backend/src/schema/resolvers/posts.spec.js index da4a49dba..9106e4eb9 100644 --- a/backend/src/schema/resolvers/posts.spec.js +++ b/backend/src/schema/resolvers/posts.spec.js @@ -582,6 +582,7 @@ describe('UpdatePost', () => { createdAt updatedAt pinnedAt + pinned } } ` @@ -599,7 +600,7 @@ describe('UpdatePost', () => { }) }) - describe('users cannot pin posts', () => { + describe('ordinary users', () => { it('throws authorization error', async () => { await expect(mutate({ mutation: pinPostMutation, variables })).resolves.toMatchObject({ errors: [{ message: 'Not Authorised!' }], @@ -608,7 +609,7 @@ describe('UpdatePost', () => { }) }) - describe('moderators cannot pin posts', () => { + describe('moderators', () => { let moderator beforeEach(async () => { moderator = await user.update({ role: 'moderator', updatedAt: new Date().toISOString() }) @@ -623,7 +624,7 @@ describe('UpdatePost', () => { }) }) - describe('admin can pin posts', () => { + describe('admins', () => { let admin beforeEach(async () => { admin = await user.update({ @@ -634,16 +635,16 @@ describe('UpdatePost', () => { authenticatedUser = await admin.toJson() }) - describe('post created by them', () => { + describe('are allowed to pin posts', () => { beforeEach(async () => { await factory.create('Post', { id: 'created-and-pinned-by-same-admin', author: admin, }) + variables = { ...variables, id: 'created-and-pinned-by-same-admin' } }) it('responds with the updated Post', async () => { - variables = { ...variables, id: 'created-and-pinned-by-same-admin' } const expected = { data: { pinPost: { @@ -667,7 +668,6 @@ describe('UpdatePost', () => { }) it('sets createdAt date for PINNED', async () => { - variables = { ...variables, id: 'created-and-pinned-by-same-admin' } const expected = { data: { pinPost: { @@ -681,6 +681,17 @@ describe('UpdatePost', () => { expected, ) }) + + it('sets redundant `pinned` property for performant ordering', async () => { + variables = { ...variables, id: 'created-and-pinned-by-same-admin' } + const expected = { + data: { pinPost: { pinned: true } }, + errors: undefined, + } + await expect(mutate({ mutation: pinPostMutation, variables })).resolves.toMatchObject( + expected, + ) + }) }) describe('post created by another admin', () => { @@ -748,7 +759,7 @@ describe('UpdatePost', () => { }) }) - describe('removes other pinned post', () => { + describe('pinned post already exists', () => { let pinnedPost beforeEach(async () => { await factory.create('Post', { @@ -756,42 +767,41 @@ describe('UpdatePost', () => { author: admin, }) await mutate({ mutation: pinPostMutation, variables }) + }) + + it('removes previous `pinned` attribute', async () => { + const cypher = 'MATCH (post:Post) WHERE post.pinned IS NOT NULL RETURN post' + pinnedPost = await neode.cypher(cypher) + expect(pinnedPost.records).toHaveLength(1) + variables = { ...variables, id: 'only-pinned-post' } + await mutate({ mutation: pinPostMutation, variables }) + pinnedPost = await neode.cypher(cypher) + expect(pinnedPost.records).toHaveLength(1) + }) + + it('removes previous PINNED relationship', async () => { variables = { ...variables, id: 'only-pinned-post' } await mutate({ mutation: pinPostMutation, variables }) pinnedPost = await neode.cypher( `MATCH (:User)-[pinned:PINNED]->(post:Post) RETURN post, pinned`, ) - }) - - it('leaves only one pinned post at a time', async () => { expect(pinnedPost.records).toHaveLength(1) }) }) describe('PostOrdering', () => { - let pinnedPost, postCreatedAfterPinnedPost, newDate, timeInPast, admin + let pinnedPost, admin beforeEach(async () => { - ;[pinnedPost, postCreatedAfterPinnedPost] = await Promise.all([ + ;[pinnedPost] = await Promise.all([ neode.create('Post', { id: 'im-a-pinned-post', + pinned: true, }), neode.create('Post', { id: 'i-was-created-after-pinned-post', + createdAt: '2019-10-22T17:26:29.070Z', // this should always be 3rd }), ]) - newDate = new Date() - timeInPast = newDate.getDate() - 3 - newDate.setDate(timeInPast) - await pinnedPost.update({ - createdAt: newDate.toISOString(), - updatedAt: new Date().toISOString(), - }) - timeInPast = newDate.getDate() + 1 - newDate.setDate(timeInPast) - await postCreatedAfterPinnedPost.update({ - createdAt: newDate.toISOString(), - updatedAt: new Date().toISOString(), - }) admin = await user.update({ role: 'admin', name: 'Admin', @@ -827,7 +837,7 @@ describe('UpdatePost', () => { ], }, } - variables = { orderBy: ['pinnedAt_asc', 'createdAt_desc'] } + variables = { orderBy: ['pinned_desc', 'createdAt_desc'] } await expect(query({ query: postOrderingQuery, variables })).resolves.toMatchObject( expected, ) @@ -854,6 +864,8 @@ describe('UpdatePost', () => { } createdAt updatedAt + pinned + pinnedAt } } ` @@ -906,16 +918,17 @@ describe('UpdatePost', () => { }) authenticatedUser = await admin.toJson() await admin.relateTo(pinnedPost, 'pinned', { createdAt: new Date().toISOString() }) + variables = { ...variables, id: 'post-to-be-unpinned' } }) it('responds with the unpinned Post', async () => { authenticatedUser = await admin.toJson() - variables = { ...variables, id: 'post-to-be-unpinned' } const expected = { data: { unpinPost: { id: 'post-to-be-unpinned', pinnedBy: null, + pinnedAt: null, }, }, errors: undefined, @@ -925,6 +938,21 @@ describe('UpdatePost', () => { expected, ) }) + + it('unsets `pinned` property', async () => { + const expected = { + data: { + unpinPost: { + id: 'post-to-be-unpinned', + pinned: null, + }, + }, + errors: undefined, + } + await expect(mutate({ mutation: unpinPostMutation, variables })).resolves.toMatchObject( + expected, + ) + }) }) }) }) diff --git a/backend/src/schema/types/type/Post.gql b/backend/src/schema/types/type/Post.gql index f917b2c3e..b4d98ec5c 100644 --- a/backend/src/schema/types/type/Post.gql +++ b/backend/src/schema/types/type/Post.gql @@ -1,3 +1,37 @@ +enum _PostOrdering { + id_asc + id_desc + activityId_asc + activityId_desc + objectId_asc + objectId_desc + title_asc + title_desc + slug_asc + slug_desc + content_asc + content_desc + contentExcerpt_asc + contentExcerpt_desc + image_asc + image_desc + visibility_asc + visibility_desc + deleted_asc + deleted_desc + disabled_asc + disabled_desc + createdAt_asc + createdAt_desc + updatedAt_asc + updatedAt_desc + language_asc + language_desc + pinned_asc + pinned_desc +} + + type Post { id: ID! activityId: String @@ -12,6 +46,7 @@ type Post { visibility: Visibility deleted: Boolean disabled: Boolean + pinned: Boolean disabledBy: User @relation(name: "DISABLED", direction: "IN") createdAt: String updatedAt: String diff --git a/webapp/pages/index.vue b/webapp/pages/index.vue index 6e2d2c0f9..74558184e 100644 --- a/webapp/pages/index.vue +++ b/webapp/pages/index.vue @@ -205,7 +205,7 @@ export default { return { filter: this.finalFilters, first: this.pageSize, - orderBy: ['pinnedAt_asc', this.orderBy], + orderBy: ['pinned_asc', this.orderBy], offset: 0, } }, From 6612e4b7ce302a9e1798187564806c67c1adb708 Mon Sep 17 00:00:00 2001 From: roschaefer Date: Tue, 22 Oct 2019 20:01:17 +0200 Subject: [PATCH 2/2] fix: remove unnecessary pinned orderBy on profile --- webapp/pages/profile/_id/_slug.vue | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/webapp/pages/profile/_id/_slug.vue b/webapp/pages/profile/_id/_slug.vue index 6961046ab..301d504aa 100644 --- a/webapp/pages/profile/_id/_slug.vue +++ b/webapp/pages/profile/_id/_slug.vue @@ -473,7 +473,7 @@ export default { filter: this.filter, first: this.pageSize, offset: 0, - orderBy: ['pinnedAt_asc', 'createdAt_desc'], + orderBy: 'createdAt_desc', } }, update({ profilePagePosts }) {