diff --git a/.changeset/shiny-llamas-teach.md b/.changeset/shiny-llamas-teach.md new file mode 100644 index 000000000..64fb9fb5d --- /dev/null +++ b/.changeset/shiny-llamas-teach.md @@ -0,0 +1,5 @@ +--- +"@latitude-data/server": patch +--- + +Fix: undefined parameters were getting passed to query compute requests diff --git a/apps/server/src/lib/autoimports/forms/Select/index.svelte b/apps/server/src/lib/autoimports/forms/Select/index.svelte index b096cc2d5..9f5610d7c 100644 --- a/apps/server/src/lib/autoimports/forms/Select/index.svelte +++ b/apps/server/src/lib/autoimports/forms/Select/index.svelte @@ -83,7 +83,6 @@ $: valuesKey = findValuesKey() $: labelsKey = findLabelsKey() - $: userDefinedItems = options?.map((item) => { if (item && typeof item === 'object' && 'value' in item) { diff --git a/apps/server/src/lib/stores/queries.test.ts b/apps/server/src/lib/stores/queries.test.ts index 6eff8ee73..3727c37f0 100644 --- a/apps/server/src/lib/stores/queries.test.ts +++ b/apps/server/src/lib/stores/queries.test.ts @@ -3,30 +3,32 @@ import { useQuery, init as initQueries } from './queries' import { type ViewParams } from './viewParams' import { get, Writable, writable } from 'svelte/store' import { v4 as uuidv4 } from 'uuid' - -// @ts-expect-error - This imported functions are defined in the mock below, but not in the actual file -import { resetQueryState, mockFetchFn } from '@latitude-data/client' +import { store } from '@latitude-data/client' import { setViewParam } from './viewParams' +type MockQueryState = { + queries: Record + forceRefetch: (request: unknown) => Promise + fetch: (request: unknown) => Promise +} + vi.mock('$app/environment', () => { return { browser: true, } }) -type MockQueryState = { - queries: Record - forceRefetch: (request: unknown) => Promise - fetch: (request: unknown) => Promise +const mockFetchFn = vi.hoisted(() => vi.fn(async () => {})) +const initialQueryState = { + queries: {}, + forceRefetch: mockFetchFn, + fetch: mockFetchFn, +} +const resetQueryState = () => { + mockFetchFn.mockClear() + store.setState(initialQueryState) } - vi.mock('@latitude-data/client', () => { - const mockFetchFn = vi.fn(async () => {}) - const initialQueryState: MockQueryState = { - queries: {}, - forceRefetch: mockFetchFn, - fetch: mockFetchFn, - } let queryState: Writable return { @@ -35,20 +37,19 @@ vi.mock('@latitude-data/client', () => { return queryState.subscribe(subscription) }), getState: vi.fn(() => get(queryState)), + setState: vi.fn((state: MockQueryState) => { + queryState = writable(state) + }), update: vi.fn((updater) => { queryState.update(updater) }), }, createQueryKey: ( queryPath: string, - params: Record, + params: Record ): string => { return queryPath + JSON.stringify(params) }, - resetQueryState: () => { - queryState = writable(initialQueryState) - mockFetchFn.mockReset() - }, mockFetchFn, } }) @@ -74,6 +75,7 @@ describe('useQuery with reactiveParams', () => { beforeEach(() => { vi.useFakeTimers() resetQueryState() + vi.clearAllMocks() mockViewParams = writable({}) }) @@ -135,6 +137,20 @@ describe('useQuery with reactiveParams', () => { expect(mockFetchFn).toHaveBeenCalledTimes(2) }) + it('should not include undefined view parameters to run requests', () => { + setViewParam('foo', undefined) + + const query = uuidv4() + const opts = { reactToParams: 100 } // 100ms debounce time + useQuery({ query, opts }) + vi.runAllTimers() + + // @ts-expect-error - Typescript is wrong here + const lastCallArgs = mockFetchFn.mock.calls[0][0] + // @ts-expect-error - Typescript is wrong here + expect(lastCallArgs!.params).toStrictEqual({}) + }) + it('show a console warning when using the deprecated reactiveToParams option', () => { // Subscribe to a query const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) @@ -154,7 +170,7 @@ describe('useQuery with reactiveParams', () => { // The console should have a warning expect(consoleSpy).toBeCalledWith( - 'The "reactiveToParams" option is deprecated. Please use "reactToParams" instead.', + 'The "reactiveToParams" option is deprecated. Please use "reactToParams" instead.' ) }) }) diff --git a/apps/server/src/lib/stores/queries.ts b/apps/server/src/lib/stores/queries.ts index a3bdc6c9f..77d3308b3 100644 --- a/apps/server/src/lib/stores/queries.ts +++ b/apps/server/src/lib/stores/queries.ts @@ -11,6 +11,8 @@ import { debounce } from 'lodash-es' let loaded = false +// TODO: Refactor this madness + /** * The middlewareQueryStore is a store that keeps track of the queries being * used in the current view, and points to the key in the core query store with @@ -39,24 +41,60 @@ export const input = (key: string, defaultValue?: unknown): InlineParam => ({ }) function computeQueryParams( - inlineParams: InlineParams, + inlineParams: InlineParams ): Record { const viewParams = getAllViewParams() - return Object.entries(inlineParams).reduce( - (params, [key, inlineParam]) => { - params[key] = - typeof inlineParam === 'object' && 'callback' in inlineParam - ? inlineParam.callback(viewParams) - : inlineParam // Inline params can be just a hardcoded value - return params + const sanitizedViewParams = sanitizeParams(viewParams) + const composedParams = composeParams(inlineParams, viewParams) + + return { ...sanitizedViewParams, ...composedParams } +} + +function sanitizeParams( + params: Record +): Record { + return Object.fromEntries( + Object.entries(params).filter(([_, value]) => value !== undefined) + ) +} + +function composeParams( + inlineParams: InlineParams, + viewParams: Record +): Record { + return Object.entries(inlineParams).reduce>( + (accumulatedParams, [key, inlineParam]) => { + const paramValue = resolveParamValue(inlineParam, viewParams) + if (paramValue !== undefined) { + accumulatedParams[key] = paramValue + } + return accumulatedParams }, - { ...viewParams }, + {} ) } +function resolveParamValue( + inlineParam: unknown, + viewParams: Record +): unknown { + if ( + typeof inlineParam === 'object' && + inlineParam !== null && + 'callback' in inlineParam + ) { + return ( + inlineParam as { + callback: (viewParams: Record) => string + } + ).callback(viewParams) + } + return inlineParam +} + function createMiddlewareKey( queryPath: string, - inlineParams: InlineParams = {}, + inlineParams: InlineParams = {} ): string { const hashedParams = Object.keys(inlineParams) .sort() @@ -64,7 +102,7 @@ function createMiddlewareKey( (paramName) => `${paramName}=${ inlineParams[paramName].key ?? String(inlineParams[paramName]) - }`, + }` ) .join('&') return `query:${queryPath}?${hashedParams}` @@ -141,7 +179,7 @@ export function useQuery({ } const coreQueryKeyStore = writable( - get(middlewareQueryStore)[middlewareKey]!.coreQueryKey, + get(middlewareQueryStore)[middlewareKey]!.coreQueryKey ) // Update coreQueryKey when middlewareQueryStore changes middlewareQueryStore.subscribe((state) => { @@ -167,7 +205,7 @@ export function useQuery({ if (opts.reactiveToParams) { console.warn( - 'The "reactiveToParams" option is deprecated. Please use "reactToParams" instead.', + 'The "reactiveToParams" option is deprecated. Please use "reactToParams" instead.' ) } @@ -206,7 +244,7 @@ export function useQuery({ export function runQuery( query: string, inlineParams: InlineParams = {}, - opts: QuerySubscriptionOptions = {}, + opts: QuerySubscriptionOptions = {} ): Readable> { const pendingPromise = () => new Promise(() => {}) const resolvedPromise = (value: QueryResultArray) => @@ -224,7 +262,7 @@ export function runQuery( useQuery({ query, inlineParams, opts }), ($queryResultState, set) => { set(queryStateToPromise($queryResultState)) - }, + } ) } @@ -244,7 +282,7 @@ export async function computeQueries({ Object.values(queriesInView) .filter( (queryInView) => - queryPaths.length === 0 || queryPaths.includes(queryInView.queryPath), + queryPaths.length === 0 || queryPaths.includes(queryInView.queryPath) ) .map((queryInView) => fetchQueryFromCore({ @@ -252,8 +290,8 @@ export async function computeQueries({ inlineParams: queryInView.inlineParams, force, skipIfParamsUnchanged, - }), - ), + }) + ) ) }