From 68b0c1c0e5a22632ad38736ccd118cedce4ef374 Mon Sep 17 00:00:00 2001 From: psteinroe Date: Mon, 11 Mar 2024 16:33:59 +0100 Subject: [PATCH 1/2] fix: dedupe queries on the same relation with different fkeys when normalizing and denormalizing --- .changeset/cuddly-zebras-crash.md | 5 ++ .../build-mutation-fetcher-response.spec.ts | 35 +++++++++ .../fetch/build-normalized-query.spec.ts | 19 ++++- .../__tests__/filter/denormalize.spec.ts | 20 +++++ .../fetch/build-mutation-fetcher-response.ts | 8 +- .../src/fetch/build-normalized-query.ts | 10 ++- packages/postgrest-core/src/fetch/dedupe.ts | 24 ++++++ .../postgrest-core/src/filter/denormalize.ts | 19 ++++- .../use-update-mutation.integration.spec.tsx | 76 +++++++++++++++++++ .../20240311151146_add_multi_fkeys.sql | 2 + 10 files changed, 208 insertions(+), 10 deletions(-) create mode 100644 .changeset/cuddly-zebras-crash.md create mode 100644 supabase/migrations/20240311151146_add_multi_fkeys.sql diff --git a/.changeset/cuddly-zebras-crash.md b/.changeset/cuddly-zebras-crash.md new file mode 100644 index 00000000..e78e8c35 --- /dev/null +++ b/.changeset/cuddly-zebras-crash.md @@ -0,0 +1,5 @@ +--- +"@supabase-cache-helpers/postgrest-core": patch +--- + +fix: dedupe queries on the same relation with different fkeys when normalizing and denormalizing diff --git a/packages/postgrest-core/__tests__/fetch/build-mutation-fetcher-response.spec.ts b/packages/postgrest-core/__tests__/fetch/build-mutation-fetcher-response.spec.ts index d06cdaf4..8545c983 100644 --- a/packages/postgrest-core/__tests__/fetch/build-mutation-fetcher-response.spec.ts +++ b/packages/postgrest-core/__tests__/fetch/build-mutation-fetcher-response.spec.ts @@ -7,6 +7,41 @@ import { PostgrestParser } from '../../src/postgrest-parser'; const c = createClient('https://localhost', 'any'); describe('buildMutationFetcherResponse', () => { + it('should work with dedupe alias on the same relation', () => { + const q = c + .from('campaign') + .select( + 'created_by:employee!created_by_employee_id(display_name),updated_by:employee!updated_by_employee_id(display_name)', + ) + .eq('id', 'some-id'); + + const query = buildNormalizedQuery({ + queriesForTable: () => [new PostgrestParser(q)], + }); + + expect(query).toBeTruthy(); + + expect( + buildMutationFetcherResponse( + { + d_0_employee: { + display_name: 'one', + }, + d_1_employee: { + display_name: 'two', + }, + }, + { userQueryPaths: query!.userQueryPaths, paths: query!.paths }, + ), + ).toEqual({ + normalizedData: { + 'employee!created_by_employee_id.display_name': 'one', + 'employee!updated_by_employee_id.display_name': 'two', + }, + userQueryData: undefined, + }); + }); + it('should work with dedupe alias and user-defined alias', () => { const q = c.from('contact').select('some,value').eq('test', 'value'); diff --git a/packages/postgrest-core/__tests__/fetch/build-normalized-query.spec.ts b/packages/postgrest-core/__tests__/fetch/build-normalized-query.spec.ts index 570f8e1b..96d83f0e 100644 --- a/packages/postgrest-core/__tests__/fetch/build-normalized-query.spec.ts +++ b/packages/postgrest-core/__tests__/fetch/build-normalized-query.spec.ts @@ -111,7 +111,7 @@ describe('buildNormalizedQuery', () => { ], })?.selectQuery, ).toEqual( - 'something,the,user,queries,some_relation!hint2(test),test,some,value,another_test,other,some_relation!hint1(test)', + 'something,the,user,queries,d_0_some_relation:some_relation!hint2(test),test,some,value,another_test,other,d_1_some_relation:some_relation!hint1(test)', ); }); @@ -372,6 +372,23 @@ describe('buildNormalizedQuery', () => { }); }); + it('should work with multiple fkeys to the same table', () => { + const q1 = c + .from('campaign') + .select( + 'created_by:employee!created_by_employee_id(display_name),updated_by:employee!updated_by_employee_id(display_name)', + ) + .eq('id', 'some-id'); + + expect( + buildNormalizedQuery({ + queriesForTable: () => [new PostgrestParser(q1)], + })?.selectQuery, + ).toEqual( + 'id,d_0_employee:employee!created_by_employee_id(display_name),d_1_employee:employee!updated_by_employee_id(display_name)', + ); + }); + it('should dedupe with hints and alias and filter', () => { const q1 = c .from('contact') diff --git a/packages/postgrest-core/__tests__/filter/denormalize.spec.ts b/packages/postgrest-core/__tests__/filter/denormalize.spec.ts index 99ccd3ba..15eb112b 100644 --- a/packages/postgrest-core/__tests__/filter/denormalize.spec.ts +++ b/packages/postgrest-core/__tests__/filter/denormalize.spec.ts @@ -27,6 +27,26 @@ describe('denormalize', () => { }); }); + it('should work with multiple aliased fkeys to the same table', () => { + const paths = parseSelectParam( + 'created_by:employee!created_by_employee_id(display_name),updated_by:employee!updated_by_employee_id(display_name)', + ); + + expect( + denormalize(paths, { + 'employee!created_by_employee_id.display_name': 'one', + 'employee!updated_by_employee_id.display_name': 'two', + }), + ).toEqual({ + created_by: { + display_name: 'one', + }, + updated_by: { + display_name: 'two', + }, + }); + }); + it('should set null if relation is null', () => { expect( denormalize( diff --git a/packages/postgrest-core/src/fetch/build-mutation-fetcher-response.ts b/packages/postgrest-core/src/fetch/build-mutation-fetcher-response.ts index 42e2d297..312eabce 100644 --- a/packages/postgrest-core/src/fetch/build-mutation-fetcher-response.ts +++ b/packages/postgrest-core/src/fetch/build-mutation-fetcher-response.ts @@ -68,10 +68,10 @@ export const normalizeResponse = (paths: Path[], obj: R): R => { return { ...prev, ...flatten({ - [curr.path]: normalizeResponse( - curr.paths, - value as Record, - ), + // add hint to path if it has dedupe alias + // can happen if the same relation is queried multiple times via different fkeys + [`${curr.path}${curr.alias?.startsWith('d_') && curr.declaration.split('!').length > 1 ? `!${curr.declaration.split('!')[1]}` : ''}`]: + normalizeResponse(curr.paths, value as Record), }), }; }, {} as R); diff --git a/packages/postgrest-core/src/fetch/build-normalized-query.ts b/packages/postgrest-core/src/fetch/build-normalized-query.ts index 2857969e..420ed974 100644 --- a/packages/postgrest-core/src/fetch/build-normalized-query.ts +++ b/packages/postgrest-core/src/fetch/build-normalized-query.ts @@ -1,5 +1,5 @@ import { buildSelectStatement } from './build-select-statement'; -import { buildDedupePath } from './dedupe'; +import { buildDedupePath, buildDedupePathToFirst } from './dedupe'; import { extractPathsFromFilters } from '../lib/extract-paths-from-filter'; import { parseSelectParam } from '../lib/parse-select-param'; import { FilterDefinitions, Path } from '../lib/query-types'; @@ -99,14 +99,18 @@ export const buildNormalizedQuery = ({ // dedupe paths by adding an alias to the shortest path, // e.g. inbox_id,inbox_id(name) -> d_0:inbox_id,inbox_id(name), let dedupeCounter = 0; - paths = paths.map((p, _, a) => { + paths = paths.map((p, idx, a) => { // check if there is path that starts with the same declaration but is longer // e.g. path is "inbox_id", and there is an "inbox_id(name)" in the cache - if (a.some((i) => i.path.startsWith(`${p.path}.`))) { + if (a.some((i, itemIdx) => i.path.startsWith(`${p.path}.`))) { // if that is the case, add our dedupe alias to the query // the alias has to be added to the last path element only, // e.g. relation_id.some_id -> relation_id.d_0_some_id:some_id return buildDedupePath(dedupeCounter++, p); + } else if (a.some((i, itemIdx) => idx !== itemIdx && i.path === p.path)) { + // check if there is an exact match. this can only happen for different declarations on the same path. + // add dedupe to first path element + return buildDedupePathToFirst(dedupeCounter++, p); } else { // otherwise, leave the path as is return p; diff --git a/packages/postgrest-core/src/fetch/dedupe.ts b/packages/postgrest-core/src/fetch/dedupe.ts index 59b7a15c..0bbb4d09 100644 --- a/packages/postgrest-core/src/fetch/dedupe.ts +++ b/packages/postgrest-core/src/fetch/dedupe.ts @@ -28,3 +28,27 @@ export const buildDedupePath = (idx: number, p: Path) => { .join('.'), }; }; + +// adds dedupe alias to first path element +export const buildDedupePathToFirst = (idx: number, p: Path) => { + return { + path: p.path, + declaration: p.declaration + .split('.') + .map((el, i) => { + const withoutAlias = el.split(':').pop() as string; + const withoutHint = withoutAlias.split('!').shift() as string; + if (i === 0) { + return `${[DEDUPE_ALIAS_PREFIX, idx, withoutHint].join( + '_', + )}:${withoutAlias}`; + } + return withoutAlias; + }) + .join('.'), + alias: p.path + .split('.') + .map((el, i) => (i === 0 ? [DEDUPE_ALIAS_PREFIX, idx, el].join('_') : el)) + .join('.'), + }; +}; diff --git a/packages/postgrest-core/src/filter/denormalize.ts b/packages/postgrest-core/src/filter/denormalize.ts index ad7dfc64..333212e0 100644 --- a/packages/postgrest-core/src/filter/denormalize.ts +++ b/packages/postgrest-core/src/filter/denormalize.ts @@ -53,10 +53,18 @@ export const denormalize = >( const flatNestedObjectOrArray = Object.entries(obj).reduce< Record> | Record >((prev, [k, v]) => { - const isNested = k.startsWith(`${curr.path}.`); + const isNested = + k.startsWith(`${curr.path}.`) || + (k.includes('!') && + k.startsWith(`${removeFirstAlias(curr.declaration)}.`)); + if (!isNested) return prev; // either set to key, or to idx.key - const flatKey = k.slice(curr.path.length + 1); + // is either path.key or path!hint.key + const flatKey = k.slice( + (k.includes('!') ? removeFirstAlias(curr.declaration) : curr.path) + .length + 1, + ); const maybeIdx = flatKey.match(/^\b\d+\b/); if (maybeIdx && isFlatNestedArray(prev)) { isArray = true; @@ -98,3 +106,10 @@ export const denormalize = >( const isFlatNestedArray = ( obj: Record> | Record, ): obj is Record> => true; + +const removeFirstAlias = (key: string): string => { + const split = key.split(':'); + if (split.length === 1) return key; + split.shift(); + return split.join(':'); +}; diff --git a/packages/postgrest-swr/__tests__/mutate/use-update-mutation.integration.spec.tsx b/packages/postgrest-swr/__tests__/mutate/use-update-mutation.integration.spec.tsx index 685a755b..a1f8ca61 100644 --- a/packages/postgrest-swr/__tests__/mutate/use-update-mutation.integration.spec.tsx +++ b/packages/postgrest-swr/__tests__/mutate/use-update-mutation.integration.spec.tsx @@ -238,4 +238,80 @@ describe('useUpdateMutation', () => { await screen.findByText([NOTE_1, NOTE_2].join(','), {}, { timeout: 10000 }); await screen.findByText('success: true', {}, { timeout: 10000 }); }); + + it('should work with multiple fkeys', async () => { + const USERNAME = `${testRunPrefix}-multi-fkeys`; + const NOTE = `${testRunPrefix}-multi-note`; + const NOTE_UPDATED = `${testRunPrefix}-multi-note-updated`; + + const { data: contact } = await client + .from('contact') + .insert({ username: USERNAME }) + .select('id') + .single() + .throwOnError(); + + const { data: contactNote } = await client + .from('contact_note') + .insert({ + contact_id: contact!.id, + text: NOTE, + updated_by_contact_id: contact!.id, + created_by_contact_id: contact!.id, + }) + .select('id') + .single() + .throwOnError(); + + function Page() { + const [success, setSuccess] = useState(false); + const { data } = useQuery( + client + .from('contact_note') + .select('id,text') + .ilike('text', `${testRunPrefix}%`), + { + revalidateOnFocus: false, + revalidateOnReconnect: false, + }, + ); + const { trigger: update } = useUpdateMutation( + client.from('contact_note'), + ['id'], + 'id,text', + { + onSuccess: () => setSuccess(true), + onError: (error) => console.error(error), + }, + ); + return ( +
+
+ await update({ + id: contactNote!.id, + text: NOTE_UPDATED, + }) + } + /> + + {(data ?? []) + .map((d) => d.text) + .sort() + .join(',')} + + {`success: ${success}`} +
+ ); + } + + renderWithConfig(, { provider: () => provider }); + await screen.findByText([NOTE].join(','), {}, { timeout: 10000 }); + + fireEvent.click(screen.getByTestId('update')); + + await screen.findByText([NOTE_UPDATED].join(','), {}, { timeout: 10000 }); + await screen.findByText('success: true', {}, { timeout: 10000 }); + }); }); diff --git a/supabase/migrations/20240311151146_add_multi_fkeys.sql b/supabase/migrations/20240311151146_add_multi_fkeys.sql new file mode 100644 index 00000000..e87dbec3 --- /dev/null +++ b/supabase/migrations/20240311151146_add_multi_fkeys.sql @@ -0,0 +1,2 @@ +alter table public.contact_note add column created_by_contact_id uuid references public.contact on delete set null; +alter table public.contact_note add column updated_by_contact_id uuid references public.contact on delete set null; From f5fa849294bfc2d5e25ea2d23458132010aedc4a Mon Sep 17 00:00:00 2001 From: psteinroe Date: Mon, 11 Mar 2024 16:43:04 +0100 Subject: [PATCH 2/2] fix: test --- .../__tests__/mutate/use-update-mutation.integration.spec.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/postgrest-swr/__tests__/mutate/use-update-mutation.integration.spec.tsx b/packages/postgrest-swr/__tests__/mutate/use-update-mutation.integration.spec.tsx index a1f8ca61..325762c9 100644 --- a/packages/postgrest-swr/__tests__/mutate/use-update-mutation.integration.spec.tsx +++ b/packages/postgrest-swr/__tests__/mutate/use-update-mutation.integration.spec.tsx @@ -269,7 +269,7 @@ describe('useUpdateMutation', () => { client .from('contact_note') .select('id,text') - .ilike('text', `${testRunPrefix}%`), + .ilike('text', `${testRunPrefix}-multi-note%`), { revalidateOnFocus: false, revalidateOnReconnect: false,