fix: undefined passed to query computations

computeQueryParameters was returning view parameters with undefined
values which is not the desirable behaviour.

.changeset/shiny-llamas-teach.md

@@ -0,0 +1,5 @@
+---
+"@latitude-data/server": patch
+---
+
+Fix: undefined parameters were getting passed to query compute requests

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) {

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<string, unknown>
+  forceRefetch: (request: unknown) => Promise<void>
+  fetch: (request: unknown) => Promise<void>
+}
+
 vi.mock('$app/environment', () => {
   return {
     browser: true,
   }
 })
 
-type MockQueryState = {
-  queries: Record<string, unknown>
-  forceRefetch: (request: unknown) => Promise<void>
-  fetch: (request: unknown) => Promise<void>
+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<MockQueryState>
 
   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<string, unknown>,
+      params: Record<string, unknown>
     ): 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<ViewParams>({})
   })
 
@@ -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.'
     )
   })
 })

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,32 +41,68 @@ export const input = (key: string, defaultValue?: unknown): InlineParam => ({
 })
 
 function computeQueryParams(
-  inlineParams: InlineParams,
+  inlineParams: InlineParams
 ): Record<string, unknown> {
   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<string, unknown>
+): Record<string, unknown> {
+  return Object.fromEntries(
+    Object.entries(params).filter(([_, value]) => value !== undefined)
+  )
+}
+
+function composeParams(
+  inlineParams: InlineParams,
+  viewParams: Record<string, unknown>
+): Record<string, unknown> {
+  return Object.entries(inlineParams).reduce<Record<string, unknown>>(
+    (accumulatedParams, [key, inlineParam]) => {
+      const paramValue = resolveParamValue(inlineParam, viewParams)
+      if (paramValue !== undefined) {
+        accumulatedParams[key] = paramValue
+      }
+      return accumulatedParams
     },
-    { ...viewParams },
+    {}
   )
 }
 
+function resolveParamValue(
+  inlineParam: unknown,
+  viewParams: Record<string, unknown>
+): unknown {
+  if (
+    typeof inlineParam === 'object' &&
+    inlineParam !== null &&
+    'callback' in inlineParam
+  ) {
+    return (
+      inlineParam as {
+        callback: (viewParams: Record<string, unknown>) => string
+      }
+    ).callback(viewParams)
+  }
+  return inlineParam
+}
+
 function createMiddlewareKey(
   queryPath: string,
-  inlineParams: InlineParams = {},
+  inlineParams: InlineParams = {}
 ): string {
   const hashedParams = Object.keys(inlineParams)
     .sort()
     .map(
       (paramName) =>
         `${paramName}=${
           inlineParams[paramName].key ?? String(inlineParams[paramName])
-        }`,
+        }`
     )
     .join('&')
   return `query:${queryPath}?${hashedParams}`
@@ -141,7 +179,7 @@ export function useQuery({
   }
 
   const coreQueryKeyStore = writable<string>(
-    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<Promise<QueryResultArray>> {
   const pendingPromise = () => new Promise<QueryResultArray>(() => {})
   const resolvedPromise = (value: QueryResultArray) =>
@@ -224,7 +262,7 @@ export function runQuery(
     useQuery({ query, inlineParams, opts }),
     ($queryResultState, set) => {
       set(queryStateToPromise($queryResultState))
-    },
+    }
   )
 }
 
@@ -244,16 +282,16 @@ 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({
           query: queryInView.queryPath,
           inlineParams: queryInView.inlineParams,
           force,
           skipIfParamsUnchanged,
-        }),
-      ),
+        })
+      )
   )
 }