-
Notifications
You must be signed in to change notification settings - Fork 465
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
fix(server): Update query to paginate correctly against filter #3012
fix(server): Update query to paginate correctly against filter #3012
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we are due for adding test(s) for this function. The query was already quite involved and I am worried about breaking something here. I am happy to pair if you want to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
packages/shared/lib/services/connection.service.integration.test.ts
Outdated
Show resolved
Hide resolved
.where({ | ||
'_nango_connections.environment_id': environmentId, | ||
'_nango_connections.deleted': false | ||
}) | ||
.orderBy('_nango_connections.created_at', 'desc'); | ||
.orderBy('_nango_connections.created_at', 'desc') | ||
.groupBy('_nango_connections.id', 'end_users.id', '_nango_configs.provider') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL you could groupBy .id
with a row_to_json
, that was the main reason for the subQuery, thanks 🙏🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you very much for adding tests
}); | ||
|
||
const connectionIds = dbConnections.map((c) => c.connection.connection_id); | ||
expect(connectionIds).toEqual([googleOK.connection_id]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you avoid the mapping with something like
expect(dbConnections).toEqual([
expect.objectContaining({
connection: {
connection_id: googleOK.connection_id
}
})
]);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TBonnin It has to be this because of how connection id is nested:
expect(dbConnections).toEqual([
expect.objectContaining({ connection: expect.objectContaining({ connection_id: notion.connection_id }) }),
expect.objectContaining({ connection: expect.objectContaining({ connection_id: google.connection_id }) })
]);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nvm then. this is ugly
Describe your changes
Update the connections list query to filter errored connections appropriately. This was a little tricky! The key element though is the
FILTER
againstjson_agg
. The query I'm targeting roughly looks like this:Issue ticket number and link
https://linear.app/nango/issue/NAN-2134/bug-with-connection-errors
Checklist before requesting a review (skip if just adding/editing APIs & templates)