Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve notification query performance by reducing db calls #2470

Closed
wants to merge 7 commits into from
Closed

Improve notification query performance by reducing db calls #2470

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b2ccc1b
Better debugging
roschaefer Dec 9, 2019
3beef5e
Reduce database statements in notifications
roschaefer Dec 10, 2019
deefc4e
Improve notification query performance
roschaefer Dec 10, 2019
12236c9
Convert to transaction function/refactor
mattwr18 Dec 10, 2019
0a50a02
Follow @roschaefer suggestion/remove guard clauses
mattwr18 Dec 10, 2019
4f0df2f
Remove disable from query request
mattwr18 Dec 10, 2019
cc7cd6b
Fix backend tests
roschaefer Dec 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 54 additions & 29 deletions backend/src/schema/resolvers/helpers/Resolver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,16 @@
import { getNeode } from '../../../bootstrap/neo4j'
import log from './databaseLogger'

export const undefinedToNullResolver = list => {
const resolvers = {}
list.forEach(key => {
resolvers[key] = async (parent, params, context, resolveInfo) => {
resolvers[key] = async parent => {
return typeof parent[key] === 'undefined' ? null : parent[key]
}
})
return resolvers
}

export default function Resolver(type, options = {}) {
const instance = getNeode()
const {
idAttribute = 'id',
undefinedToNull = [],
Expand All @@ -22,32 +21,49 @@ export default function Resolver(type, options = {}) {
} = options

const _hasResolver = (resolvers, { key, connection }, { returnType }) => {
return async (parent, params, context, resolveInfo) => {
return async (parent, params, { driver, cypherParams }, resolveInfo) => {
if (typeof parent[key] !== 'undefined') return parent[key]
const id = parent[idAttribute]
const statement = `MATCH(:${type} {${idAttribute}: {id}})${connection} RETURN related`
const result = await instance.cypher(statement, { id })
let response = result.records.map(r => r.get('related').properties)
if (returnType === 'object') response = response[0] || null
return response
const session = driver.session()
const readTxResultPromise = session.readTransaction(async txc => {
const cypher = `
MATCH(:${type} {${idAttribute}: $id})${connection}
RETURN related {.*} as related
`
const result = await txc.run(cypher, { id, cypherParams })
log(result)
return result.records.map(r => r.get('related'))
})
try {
let response = await readTxResultPromise
if (returnType === 'object') response = response[0] || null
return response
} finally {
session.close()
}
}
}

const booleanResolver = obj => {
const resolvers = {}
for (const [key, condition] of Object.entries(obj)) {
resolvers[key] = async (parent, params, { cypherParams }, resolveInfo) => {
resolvers[key] = async (parent, params, { cypherParams, driver }, resolveInfo) => {
if (typeof parent[key] !== 'undefined') return parent[key]
const result = await instance.cypher(
`
${condition.replace('this', 'this {id: $parent.id}')} as ${key}`,
{
parent,
cypherParams,
},
)
const [record] = result.records
return record.get(key)
const id = parent[idAttribute]
const session = driver.session()
const readTxResultPromise = session.readTransaction(async txc => {
const nodeCondition = condition.replace('this', 'this {id: $id}')
const cypher = `${nodeCondition} as ${key}`
const result = await txc.run(cypher, { id, cypherParams })
log(result)
const [response] = result.records.map(r => r.get(key))
return response
})
try {
return await readTxResultPromise
} finally {
session.close()
}
}
}
return resolvers
Expand All @@ -56,16 +72,25 @@ export default function Resolver(type, options = {}) {
const countResolver = obj => {
const resolvers = {}
for (const [key, connection] of Object.entries(obj)) {
resolvers[key] = async (parent, params, context, resolveInfo) => {
resolvers[key] = async (parent, params, { driver, cypherParams }, resolveInfo) => {
if (typeof parent[key] !== 'undefined') return parent[key]
const id = parent[idAttribute]
const statement = `
MATCH(u:${type} {${idAttribute}: {id}})${connection}
RETURN COUNT(DISTINCT(related)) as count
`
const result = await instance.cypher(statement, { id })
const [response] = result.records.map(r => r.get('count').toNumber())
return response
const session = driver.session()
const readTxResultPromise = session.readTransaction(async txc => {
const id = parent[idAttribute]
const cypher = `
MATCH(u:${type} {${idAttribute}: $id})${connection}
RETURN COUNT(DISTINCT(related)) as count
`
const result = await txc.run(cypher, { id, cypherParams })
log(result)
const [response] = result.records.map(r => r.get('count').toNumber())
return response
})
try {
return await readTxResultPromise
} finally {
session.close()
}
}
}
return resolvers
Expand Down
15 changes: 15 additions & 0 deletions backend/src/schema/resolvers/helpers/databaseLogger.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import Debug from 'debug'
const debugCypher = Debug('human-connection:neo4j:cypher')
const debugStats = Debug('human-connection:neo4j:stats')

export default function log(response) {
const { statement, counters, resultConsumedAfter, resultAvailableAfter } = response.summary
const { text, parameters } = statement
debugCypher('%s', text)
debugCypher('%o', parameters)
debugStats('%o', counters)
debugStats('%o', {
resultConsumedAfter: resultConsumedAfter.toNumber(),
resultAvailableAfter: resultAvailableAfter.toNumber(),
})
}
34 changes: 25 additions & 9 deletions backend/src/schema/resolvers/notifications.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import log from './helpers/databaseLogger'

const resourceTypes = ['Post', 'Comment']

const transformReturnType = record => {
Expand Down Expand Up @@ -42,16 +44,29 @@ export default {
}
const offset = args.offset && typeof args.offset === 'number' ? `SKIP ${args.offset}` : ''
const limit = args.first && typeof args.first === 'number' ? `LIMIT ${args.first}` : ''
const cypher = `
MATCH (resource {deleted: false, disabled: false})-[notification:NOTIFIED]->(user:User {id:$id})
${whereClause}
RETURN resource, notification, user
${orderByClause}
${offset} ${limit}
`

const readTxResultPromise = session.readTransaction(async transaction => {
const notificationsTransactionResponse = await transaction.run(
`
MATCH (resource {deleted: false, disabled: false})-[notification:NOTIFIED]->(user:User {id:$id})
${whereClause}
WITH user, notification, resource,
[(resource)<-[:WROTE]-(author:User) | author {.*}] as authors,
[(resource)-[:COMMENTS]->(post:Post)<-[:WROTE]-(author:User) | post{.*, author: properties(author)} ] as posts
WITH resource, user, notification, authors, posts,
resource {.*, __typename: labels(resource)[0], author: authors[0], post: posts[0]} as finalResource
RETURN notification {.*, from: finalResource, to: properties(user)}
${orderByClause}
${offset} ${limit}
`,
{ id: currentUser.id },
)
log(notificationsTransactionResponse)
return notificationsTransactionResponse.records.map(record => record.get('notification'))
})
try {
const result = await session.run(cypher, { id: currentUser.id })
return result.records.map(transformReturnType)
const notifications = await readTxResultPromise
return notifications
} finally {
session.close()
}
Expand All @@ -68,6 +83,7 @@ export default {
RETURN resource, notification, user
`
const result = await session.run(cypher, { resourceId: args.id, id: currentUser.id })
log(result)
const notifications = await result.records.map(transformReturnType)
return notifications[0]
} finally {
Expand Down
9 changes: 7 additions & 2 deletions backend/src/schema/resolvers/notifications.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@ describe('given some notifications', () => {
data: {
notifications: expect.arrayContaining(expected),
},
errors: undefined,
})
})
})
Expand Down Expand Up @@ -233,7 +234,10 @@ describe('given some notifications', () => {
`
await expect(
mutate({ mutation: deletePostMutation, variables: { id: 'p3' } }),
).resolves.toMatchObject({ data: { DeletePost: { id: 'p3', deleted: true } } })
).resolves.toMatchObject({
data: { DeletePost: { id: 'p3', deleted: true } },
errors: undefined,
})
authenticatedUser = await user.toJson()
}

Expand All @@ -242,11 +246,12 @@ describe('given some notifications', () => {
query({ query: notificationQuery, variables: { ...variables, read: false } }),
).resolves.toMatchObject({
data: { notifications: [expect.any(Object), expect.any(Object)] },
errors: undefined,
})
await deletePostAction()
await expect(
query({ query: notificationQuery, variables: { ...variables, read: false } }),
).resolves.toMatchObject({ data: { notifications: [] } })
).resolves.toMatchObject({ data: { notifications: [] }, errors: undefined })
})
})
})
Expand Down
55 changes: 28 additions & 27 deletions backend/src/schema/resolvers/reports.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import log from './helpers/databaseLogger'

const transformReturnType = record => {
return {
...record.get('report').properties,
Expand All @@ -11,12 +13,11 @@ const transformReturnType = record => {
export default {
Mutation: {
fileReport: async (_parent, params, context, _resolveInfo) => {
let createdRelationshipWithNestedAttributes
const { resourceId, reasonCategory, reasonDescription } = params
const { driver, user } = context
const session = driver.session()
const reportWriteTxResultPromise = session.writeTransaction(async txc => {
const reportTransactionResponse = await txc.run(
const reportWriteTxResultPromise = session.writeTransaction(async transaction => {
const reportTransactionResponse = await transaction.run(
`
MATCH (submitter:User {id: $submitterId})
MATCH (resource {id: $resourceId})
Expand All @@ -36,23 +37,23 @@ export default {
reasonDescription,
},
)
log(reportTransactionResponse)
return reportTransactionResponse.records.map(transformReturnType)
})
try {
const txResult = await reportWriteTxResultPromise
if (!txResult[0]) return null
createdRelationshipWithNestedAttributes = txResult[0]
const [createdRelationshipWithNestedAttributes] = await reportWriteTxResultPromise
if (!createdRelationshipWithNestedAttributes) return null
return createdRelationshipWithNestedAttributes
} finally {
session.close()
}
return createdRelationshipWithNestedAttributes
},
},
Query: {
reports: async (_parent, params, context, _resolveInfo) => {
const { driver } = context
const session = driver.session()
let reports, orderByClause, filterClause
let orderByClause, filterClause
switch (params.orderBy) {
case 'createdAt_asc':
orderByClause = 'ORDER BY report.createdAt ASC'
Expand Down Expand Up @@ -81,8 +82,8 @@ export default {
params.offset && typeof params.offset === 'number' ? `SKIP ${params.offset}` : ''
const limit = params.first && typeof params.first === 'number' ? `LIMIT ${params.first}` : ''

const reportReadTxPromise = session.readTransaction(async tx => {
const allReportsTransactionResponse = await tx.run(
const reportReadTxPromise = session.readTransaction(async transaction => {
const allReportsTransactionResponse = await transaction.run(
`
MATCH (report:Report)-[:BELONGS_TO]->(resource)
WHERE (resource:User OR resource:Post OR resource:Comment)
Expand All @@ -100,16 +101,15 @@ export default {
${offset} ${limit}
`,
)
log(allReportsTransactionResponse)
return allReportsTransactionResponse.records.map(record => record.get('report'))
})
try {
const txResult = await reportReadTxPromise
if (!txResult[0]) return null
reports = txResult
const reports = await reportReadTxPromise
return reports
} finally {
session.close()
}
return reports
},
},
Report: {
Expand All @@ -118,23 +118,23 @@ export default {
const session = context.driver.session()
const { id } = parent
let filed
const readTxPromise = session.readTransaction(async tx => {
const allReportsTransactionResponse = await tx.run(
const readTxPromise = session.readTransaction(async transaction => {
const filedReportsTransactionResponse = await transaction.run(
`
MATCH (submitter:User)-[filed:FILED]->(report:Report {id: $id})
RETURN filed, submitter
MATCH (submitter:User)-[filed:FILED]->(report:Report {id: $id})
RETURN filed, submitter
`,
{ id },
)
return allReportsTransactionResponse.records.map(record => ({
log(filedReportsTransactionResponse)
return filedReportsTransactionResponse.records.map(record => ({
submitter: record.get('submitter').properties,
filed: record.get('filed').properties,
}))
})
try {
const txResult = await readTxPromise
if (!txResult[0]) return null
filed = txResult.map(reportedRecord => {
const filedReports = await readTxPromise
filed = filedReports.map(reportedRecord => {
const { submitter, filed } = reportedRecord
const relationshipWithNestedAttributes = {
...filed,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

roschaefer Authored by roschaefer
Dec 10, 2019

Outdated (history rewrite) - original diff

@@ -83,35 +82,35 @@ export default {
         params.offset && typeof params.offset === 'number' ? `SKIP ${params.offset}` : ''
       const limit = params.first && typeof params.first === 'number' ? `LIMIT ${params.first}` : ''
 
-      const reportReadTxPromise = session.readTransaction(async tx => {
-        const cypher = `
-          MATCH (report:Report)-[:BELONGS_TO]->(resource)
-          WHERE (resource:User OR resource:Post OR resource:Comment)
-          ${filterClause}
-          WITH report, resource,
-          [(submitter:User)-[filed:FILED]->(report) |  filed {.*, submitter: properties(submitter)} ] as filed,
-          [(moderator:User)-[reviewed:REVIEWED]->(report) |  reviewed {.*, moderator: properties(moderator)} ] as reviewed,
-          [(resource)<-[:WROTE]-(author:User) | author {.*} ] as optionalAuthors,
-          [(resource)-[:COMMENTS]->(post:Post) | post {.*} ] as optionalCommentedPosts,
-          resource {.*, __typename: labels(resource)[0] } as resourceWithType
-          WITH report, optionalAuthors, optionalCommentedPosts, reviewed, filed,
-          resourceWithType {.*, post: optionalCommentedPosts[0], author: optionalAuthors[0] } as finalResource
-          RETURN report {.*, resource: finalResource, filed: filed, reviewed: reviewed }
-          ${orderByClause}
-          ${offset} ${limit}
-        `
-        const allReportsTransactionResponse = await tx.run(cypher)
+      const reportReadTxPromise = session.readTransaction(async transaction => {
+        const allReportsTransactionResponse = await transaction.run(
+          `
+            MATCH (report:Report)-[:BELONGS_TO]->(resource)
+            WHERE (resource:User OR resource:Post OR resource:Comment)
+            ${filterClause}
+            WITH report, resource,
+            [(submitter:User)-[filed:FILED]->(report) |  filed {.*, submitter: properties(submitter)} ] as filed,
+            [(moderator:User)-[reviewed:REVIEWED]->(report) |  reviewed {.*, moderator: properties(moderator)} ] as reviewed,
+            [(resource)<-[:WROTE]-(author:User) | author {.*} ] as optionalAuthors,
+            [(resource)-[:COMMENTS]->(post:Post) | post {.*} ] as optionalCommentedPosts,
+            resource {.*, __typename: labels(resource)[0] } as resourceWithType
+            WITH report, optionalAuthors, optionalCommentedPosts, reviewed, filed,
+            resourceWithType {.*, post: optionalCommentedPosts[0], author: optionalAuthors[0] } as finalResource
+            RETURN report {.*, resource: finalResource, filed: filed, reviewed: reviewed }
+            ${orderByClause}
+            ${offset} ${limit}
+          `,
+        )
         log(allReportsTransactionResponse)
         return allReportsTransactionResponse.records.map(record => record.get('report'))
       })
       try {
-        const txResult = await reportReadTxPromise
-        if (!txResult[0]) return null
-        reports = txResult
+        const reports = await reportReadTxPromise
+        if (!reports) return []

How could reports ever be not an array?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

roschaefer Authored by roschaefer
Dec 10, 2019

If we run into an error I would prefer HTTP 500

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mattwr18 Authored by mattwr18
Dec 10, 2019

So, revert it to return null?

Expand All @@ -152,23 +152,24 @@ export default {
const session = context.driver.session()
const { id } = parent
let reviewed
const readTxPromise = session.readTransaction(async tx => {
const allReportsTransactionResponse = await tx.run(
const readTxPromise = session.readTransaction(async transaction => {
const reviewedReportsTransactionResponse = await transaction.run(
`
MATCH (resource)<-[:BELONGS_TO]-(report:Report {id: $id})<-[review:REVIEWED]-(moderator:User)
RETURN moderator, review
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mattwr18 Authored by mattwr18
Dec 10, 2019

Outdated (history rewrite) - original diff

@@ -153,14 +155,14 @@ export default {
       const { id } = parent
       let reviewed
       const readTxPromise = session.readTransaction(async tx => {
-        const allReportsTransactionResponse = await tx.run(
-          `
-            MATCH (resource)<-[:BELONGS_TO]-(report:Report {id: $id})<-[review:REVIEWED]-(moderator:User)
-            RETURN moderator, review
-            ORDER BY report.updatedAt DESC, review.updatedAt DESC
-          `,
-          { id },
-        )
+        const cypher = `
+          MATCH (resource)<-[:BELONGS_TO]-(report:Report {id: $id})<-[review:REVIEWED]-(moderator:User)
+          RETURN moderator, review
+          ORDER BY report.updatedAt DESC, review.updatedAt DESC
+        `
+        const params = { id }
+        const allReportsTransactionResponse = await tx.run(cypher, params)
+        log(allReportsTransactionResponse)

same question as your other PR @roschaefer
why so much refactoring just to add log??

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

roschaefer Authored by roschaefer
Dec 10, 2019

I want to see the database calls being made when I set DEBUG=human-connection:neo4j:*. So I implemented this helper function which outputs the cypher statement and/or the database call statistics, the counters. We should always call the log method from now on when we implement database calls.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mattwr18 Authored by mattwr18
Dec 10, 2019

that part, I get... I don't understand how why that needed so much refactoring
I have refactored it and pushed it up.
Would this somehow not work with the code you added...
It would be nice to know how to use as well
If I run DEBUG=human-connection:neo4j:* yarn dev it throws this error

No matches for wildcard “DEBUG=human-connection:neo4j:*”. See `help expand`.
env DEBUG=human-connection:neo4j:* yarn dev

so I'm assuming the wildcard needs to be replaced by something, but what? Ahh
it takes either human-connection:neo4j:cypher or human-connection:neo4j:stats??

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

roschaefer Authored by roschaefer
Dec 10, 2019

On fish shell:

env DEBUG="human-connection:neo4j:*" yarn run dev

if you don't escape the * your shell believes it's a file glob pattern.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mattwr18 Authored by mattwr18
Dec 10, 2019

great thanks @roschaefer
then run a query and it will give me some crazy feedback?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mattwr18 Authored by mattwr18
Dec 10, 2019

ahhh, ok here are the result of running a reports query

2019-12-10T16:04:20.346Z human-connection:neo4j:cypher 
            MATCH (report:Report)-[:BELONGS_TO]->(resource)
            WHERE (resource:User OR resource:Post OR resource:Comment)
            
            WITH report, resource,
            [(submitter:User)-[filed:FILED]->(report) |  filed {.*, submitter: properties(submitter)} ] as filed,
            [(moderator:User)-[reviewed:REVIEWED]->(report) |  reviewed {.*, moderator: properties(moderator)} ] as reviewed,
            [(resource)<-[:WROTE]-(author:User) | author {.*} ] as optionalAuthors,
            [(resource)-[:COMMENTS]->(post:Post) | post {.*} ] as optionalCommentedPosts,
            resource {.*, __typename: labels(resource)[0] } as resourceWithType
            WITH report, optionalAuthors, optionalCommentedPosts, reviewed, filed,
            resourceWithType {.*, post: optionalCommentedPosts[0], author: optionalAuthors[0] } as finalResource
            RETURN report {.*, resource: finalResource, filed: filed, reviewed: reviewed }
            ORDER BY report.createdAt DESC
             
          
2019-12-10T16:04:20.348Z human-connection:neo4j:cypher {}
2019-12-10T16:04:20.349Z human-connection:neo4j:stats StatementStatistics { _stats: { nodesCreated: 0, nodesDeleted: 0, relationshipsCreated: 0, relationshipsDeleted: 0, propertiesSet: 0, labelsAdded: 0, labelsRemoved: 0, indexesAdded: 0, indexesRemoved: 0, constraintsAdded: 0, constraintsRemoved: 0 } }
2019-12-10T16:04:20.349Z human-connection:neo4j:stats { resultConsumedAfter: 2, resultAvailableAfter: 56 }

ORDER BY report.updatedAt DESC, review.updatedAt DESC
`,
{ id },
)
return allReportsTransactionResponse.records.map(record => ({
log(reviewedReportsTransactionResponse)
return reviewedReportsTransactionResponse.records.map(record => ({
review: record.get('review').properties,
moderator: record.get('moderator').properties,
}))
})
try {
const txResult = await readTxPromise
reviewed = txResult.map(reportedRecord => {
const reviewedReports = await readTxPromise
reviewed = reviewedReports.map(reportedRecord => {
const { review, moderator } = reportedRecord
const relationshipWithNestedAttributes = {
...review,
Expand Down
Loading