feat: [nan-851] improve query to use the nango_config_id #2124
Conversation
|
@bodinsamuel I know you recently went through this query, so it would be great to get another pair of eyes on this. From my testing the query time is reduced further by using the |
| @@ -269,8 +258,13 @@ export const getSyncs = async (nangoConnection: Connection): Promise<(Sync & { s | |||
| .join(SYNC_SCHEDULE_TABLE, function () { | |||
| this.on(`${SYNC_SCHEDULE_TABLE}.sync_id`, `${TABLE}.id`).andOn(`${SYNC_SCHEDULE_TABLE}.deleted`, '=', db.knex.raw('FALSE')); | |||
| }) | |||
| .join(SYNC_CONFIG_TABLE, `${SYNC_CONFIG_TABLE}.sync_name`, `${TABLE}.name`) | |||
There was a problem hiding this comment.
This is bringing back the issue I fixed in #2029
There was a problem hiding this comment.
Not exactly since we're now querying against the config_id @ https://github.com/NangoHQ/nango/pull/2124/files#diff-7be9b1d2fee1ed263243cfe652dbb029abdc165631a55b0677119bf8fb3a743cR265
I traced back my local history and was able to recreate the issue fixed in #2029 locally and under the same conditions that problem doesn't appear with this adjustment that makes the query a bit simpler and easier to read IMO
Let me know if you see any issues with this change locally.
There was a problem hiding this comment.
ah yes didn't saw the where, if you can just add the condition in the join then (like above), its confusing but also change the query plan
There was a problem hiding this comment.
I'm not sure what you mean. There is no join that can happen other than the sync_name
There was a problem hiding this comment.
Sorry that wasn't clear. Putting the join conditions outside the where can change the query plan (depending on some context) and it makes it harder to see what is exactly we are selecting, e.g: I read too fast here and thought we were joining on name only
.join(SYNC_SCHEDULE_TABLE, function () {
this.on(`${SYNC_CONFIG_TABLE}.sync_name`, `${TABLE}.name`)
.andOn(`${SYNC_CONFIG_TABLE}.deleted `, '=', db.knex.raw('FALSE')). [...]
})
khaliqgant
changed the title
Describe your changes
Reduce the complexity of the code since we now have the
_nango_connections.config_idvalue.I went through the production database to ensure that value is always populated. In some cases it was (for about 86 records) and I manually updated the
config_idto point to a correct value using theenvironment_idandprovider_config_keyto match. There were two instances where that value is NULL but those connections were created in 2023 and had no matching configIssue ticket number and link
NAN-851
Checklist before requesting a review (skip if just adding/editing APIs & templates)