Refactor track_deletes #1892

TBonnin · 2024-03-21T15:42:53Z

Describe your changes

Refactoring track_deletes logic so we don't need an extra snapshot tables.
The logic is now simple:

upsert adds/updates records (or set deletedAt), including updating the sync_job_id that serves as a generationId
when the sync is finish, if track_deletes == true then every records without a deletedAt that doesn't have the same sync_job_id as the current one is marked as deleted

Issue ticket number and link

https://linear.app/nango/issue/NAN-599/track-deletes-refactoring

Checklist before requesting a review (skip if just adding/editing APIs & templates)

I added tests, otherwise the reason is:
I added observability, otherwise the reason is:
I added analytics, otherwise the reason is:

linear · 2024-03-21T15:42:55Z

NAN-599 track_deletes refactoring

TBonnin · 2024-03-21T15:43:59Z

packages/jobs/lib/crons/autoIdleDemo.integration.test.ts

        await configService.createProviderConfig({
            unique_key: DEMO_GITHUB_CONFIG_KEY,
            provider: 'github',
-            environment_id: env!.id,


unrelated to track_deletes: making environment id not optional, same from sync and syncJob below

TBonnin · 2024-03-21T15:45:35Z

packages/shared/lib/models/Data.ts

@@ -1,14 +1,12 @@
 export interface DataResponse {
    id?: string;
-    [index: string]: unknown | undefined | string | number | boolean | Record<string, string | boolean | number | unknown>;
+    [index: string]: null | undefined | object | string | number | boolean | Record<string, string | boolean | number>;


unrelated but the linter was yelling at me

TBonnin · 2024-03-21T15:46:02Z

packages/shared/lib/models/Data.ts

 }

 export interface UpsertSummary {
    addedKeys: string[];
    updatedKeys: string[];
    deletedKeys?: string[];
-    affectedInternalIds: string[];
-    affectedExternalIds: string[];


not used anywhere

TBonnin · 2024-03-21T15:52:16Z

packages/shared/lib/services/sync/run.service.ts

@@ -401,25 +401,17 @@ export default class SyncRun {
    async finishSync(models: string[], syncStartDate: Date, version: string, totalRunTime: number, trackDeletes?: boolean): Promise<void> {
        let i = 0;
        for (const model of models) {
+            let deletedKeys: string[] = [];
            if (!this.isWebhook && trackDeletes) {


@khaliqgant I have kept the condition here but I am not sure why !this.isWebhook is necessary

Because a webhook and a sync with track deletes share the same config

bodinsamuel

Works well, minor comments 👌🏻
I have one test that I think failed, but I don't know enough the system to be sure it's not what we want.
Run a sync three times:

1. Add record id: 1 ✅
1. Delete record id: 1 ✅
1. Add back record id: 1 ❌
  The record column external_deleted_at is still filled with a date

I guess the next steps after a few weeks is to drop the table and pending_delete column?

packages/shared/lib/services/sync/job.service.ts

packages/shared/lib/services/sync/data/records.service.ts

bodinsamuel · 2024-03-22T10:44:21Z

packages/shared/lib/services/sync/data/records.service.ts

+        })
+        .whereNot({
+            sync_job_id: generation
+        })


I'm wondering about the performance of this query. There is no index currently mapping those conditions, do you have a way to run an explain in prod with a large sync?

Discussed in Slack, it seems alright but If not, an index like this nango_connection_id, model, sync_id, job_id where external_is_deleted = false could work

packages/shared/lib/services/sync/run.service.integration.test.ts

TBonnin · 2024-03-22T12:57:17Z

Works well, minor comments 👌🏻 I have one test that I think failed, but I don't know enough the system to be sure it's not what we want. Run a sync three times:
* 1. Add record id: 1 ✅

* 2. Delete record id: 1 ✅

* 3. Add back record id: 1 ❌
     The record column `external_deleted_at` is still filled with a date 

Good catch. will fix. Question though: should the creation date of a deleted records that is being brought back be reset or not. ie: should it be considered as added again or simply updated?

TBonnin · 2024-03-22T12:58:56Z

I guess the next steps after a few weeks is to drop the table and pending_delete column?

The table will be dropped after the migration and the new table is not going to have a pending_delete column

bodinsamuel

Looks good, very nice improvement 🙏🏻

bodinsamuel · 2024-03-25T08:46:50Z

packages/shared/lib/db/migrations/20240322141605_undelete_records_reset_createdat.cjs

+                END IF;
+                RETURN NEW;
+            END;
+            $$ LANGUAGE plpgsql;


There is no way to have this logic in the code? I'm really not fond of sql logic, it's hard to debug and easy to forget

I am also not a huge fan of the fact it is easy to forget. The alternative would be to fetch all existing records and compare before merging

khaliqgant

Works as expected from my tests, thanks!

TBonnin requested review from bodinsamuel and khaliqgant March 21, 2024 15:42

TBonnin commented Mar 21, 2024

View reviewed changes

TBonnin force-pushed the tbonnin/NAN-599/track-deletes-refactor branch from 37034c1 to 7fcf79b Compare March 21, 2024 20:16

bodinsamuel reviewed Mar 22, 2024

View reviewed changes

TBonnin force-pushed the tbonnin/NAN-599/track-deletes-refactor branch 5 times, most recently from ac3033f to 643b245 Compare March 22, 2024 17:07

bodinsamuel approved these changes Mar 25, 2024

View reviewed changes

TBonnin force-pushed the tbonnin/NAN-599/track-deletes-refactor branch from 643b245 to 953152f Compare March 25, 2024 11:51

khaliqgant approved these changes Mar 25, 2024

View reviewed changes

TBonnin added 11 commits March 25, 2024 09:59

add test for run.service

68acdc7

sync and syncJob ids are not optional

cc9cd24

more tests for run.service

605b503

refactor track_deletes: no need for snapshot table

574bb59

clean mentions of _nango_sync_data_records_deletes table

9bbe8e8

fix: lastSyncDate and trackDelete are not required by persist endpoints

3ac0ec6

fix: only mark as deleted non-deleted records

39c888e

fix: mark as deleted only records from current sync

8a3d5b8

data service: use constants for table and unique key

12238c5

test tweaks + fix bug when readding deleted records

0e7282d

markNonCurrentGenerationRecordsAsDeleted only needs to return ids

8d0cbd3

TBonnin added 2 commits March 25, 2024 09:59

fix: reset createdAt to now for record previously deleted

57ced19

fix: discard records with deleted_at in getAddedKeys knownKeys

f2e2ae7

TBonnin force-pushed the tbonnin/NAN-599/track-deletes-refactor branch from 953152f to f2e2ae7 Compare March 25, 2024 13:59

TBonnin enabled auto-merge (squash) March 25, 2024 14:00

TBonnin merged commit 4ddac7f into master Mar 25, 2024
18 checks passed

TBonnin deleted the tbonnin/NAN-599/track-deletes-refactor branch March 25, 2024 14:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor track_deletes #1892

Refactor track_deletes #1892

TBonnin commented Mar 21, 2024 •

edited

linear bot commented Mar 21, 2024

TBonnin Mar 21, 2024

TBonnin Mar 21, 2024

TBonnin Mar 21, 2024

TBonnin Mar 21, 2024

khaliqgant Mar 21, 2024

bodinsamuel left a comment

bodinsamuel Mar 22, 2024

bodinsamuel Mar 25, 2024

TBonnin commented Mar 22, 2024 •

edited

TBonnin commented Mar 22, 2024

bodinsamuel left a comment

bodinsamuel Mar 25, 2024

TBonnin Mar 25, 2024

khaliqgant left a comment

Refactor track_deletes #1892

Refactor track_deletes #1892

Conversation

TBonnin commented Mar 21, 2024 • edited

Describe your changes

Issue ticket number and link

Checklist before requesting a review (skip if just adding/editing APIs & templates)

linear bot commented Mar 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bodinsamuel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TBonnin commented Mar 22, 2024 • edited

TBonnin commented Mar 22, 2024

bodinsamuel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khaliqgant left a comment

Choose a reason for hiding this comment

TBonnin commented Mar 21, 2024 •

edited

TBonnin commented Mar 22, 2024 •

edited