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
This commit is contained in:
roschaefer 2019-10-22 19:56:02 +02:00
parent f9c36b4e01
commit 2a9e182649
4 changed files with 95 additions and 29 deletions

View File

@ -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)',

View File

@ -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,
)
})
})
})
})

View File

@ -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

View File

@ -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,
}
},