feat(clients): Always order query params (#2044)

Closes #2041

To be fair, by virtue of the clients having a fixed process by which the
parameters are appended and updated they were already stable, but an
explicit ordering step ensures that changes in the order of those
operations don't break caching between versions, and is generally more
explicit.

.changeset/selfish-rings-mate.md

@@ -0,0 +1,6 @@
+---
+"@electric-sql/client": patch
+"@core/elixir-client": patch
+---
+
+Always use sorted query parameters in official clients to ensure Shape URLs are cached consistently.

packages/elixir-client/lib/electric/client/fetch/request.ex

@@ -80,7 +80,14 @@ defmodule Electric.Client.Fetch.Request do
     %{endpoint: endpoint} = request
 
     if Keyword.get(opts, :query, true) do
-      query = request |> params() |> URI.encode_query(:rfc3986)
+      # Convert map to _ordered_ list of query parameters
+      # to ensure consistent caching
+      query =
+        request
+        |> params()
+        |> Map.to_list()
+        |> List.keysort(0)
+        |> URI.encode_query(:rfc3986)
 
       URI.to_string(%{endpoint | query: query})
     else

packages/elixir-client/test/electric/client/fetch/request_test.exs

@@ -31,6 +31,9 @@ defmodule Electric.Client.Fetch.RequestTest do
 
       params = URI.decode_query(query)
 
+      # should have sorted parameters
+      assert query == "cursor=123948&handle=my-shape&live=true&offset=1234_1&table=my_table"
+
       assert %{
                "table" => "my_table",
                "cursor" => "123948",

packages/typescript-client/src/client.ts

@@ -358,6 +358,9 @@ export class ShapeStream<T extends Row<unknown> = Row>
           fetchUrl.searchParams.set(REPLICA_PARAM, this.#replica as string)
         }
 
+        // sort query params in-place for stable URLs and improved cache hits
+        fetchUrl.searchParams.sort()
+
         let response!: Response
         try {
           response = await this.#fetchClient(fetchUrl.toString(), {

packages/typescript-client/src/fetch.ts

@@ -312,6 +312,7 @@ function getNextChunkUrl(url: string, res: Response): string | void {
 
   nextUrl.searchParams.set(SHAPE_HANDLE_QUERY_PARAM, shapeHandle)
   nextUrl.searchParams.set(OFFSET_QUERY_PARAM, lastOffset)
+  nextUrl.searchParams.sort()
   return nextUrl.toString()
 }
 

packages/typescript-client/test/fetch.test.ts

@@ -218,7 +218,7 @@ describe(`createFetchWithChunkBuffer`, () => {
     expect(result).toBe(initialResponse)
 
     // Check if the next chunk was prefetched
-    const nextUrl = `${baseUrl}&handle=123&offset=456`
+    const nextUrl = sortUrlParams(`${baseUrl}&handle=123&offset=456`)
     expect(mockFetch).toHaveBeenCalledWith(nextUrl, expect.anything())
   })
 
@@ -250,23 +250,23 @@ describe(`createFetchWithChunkBuffer`, () => {
     expect(mockFetch).toHaveBeenCalledTimes(1 + maxPrefetchNum)
     expect(mockFetch).toHaveBeenNthCalledWith(
       2,
-      `${baseUrl}&handle=123&offset=0`,
+      sortUrlParams(`${baseUrl}&handle=123&offset=0`),
       expect.anything()
     )
     expect(mockFetch).toHaveBeenNthCalledWith(
       3,
-      `${baseUrl}&handle=123&offset=1`,
+      sortUrlParams(`${baseUrl}&handle=123&offset=1`),
       expect.anything()
     )
 
     // Second request consumes one of the prefetched responses and
     // next one fires up
-    await fetchWrapper(`${baseUrl}&handle=123&offset=0`)
+    await fetchWrapper(sortUrlParams(`${baseUrl}&handle=123&offset=0`))
     await sleep()
     expect(mockFetch).toHaveBeenCalledTimes(1 + maxPrefetchNum + 1)
     expect(mockFetch).toHaveBeenNthCalledWith(
       4,
-      `${baseUrl}&handle=123&offset=2`,
+      sortUrlParams(`${baseUrl}&handle=123&offset=2`),
       expect.anything()
     )
   })
@@ -297,7 +297,7 @@ describe(`createFetchWithChunkBuffer`, () => {
     expect(result).toBe(initialResponse)
 
     // fetch the next chunk as well
-    const nextUrl = `${baseUrl}&handle=123&offset=456`
+    const nextUrl = sortUrlParams(`${baseUrl}&handle=123&offset=456`)
     const nextResult = await fetchWrapper(nextUrl)
     expect(nextResult).toBe(nextResponse)
 
@@ -339,7 +339,8 @@ describe(`createFetchWithChunkBuffer`, () => {
     expect(result).toBe(initialResponse)
 
     // Prefetch should have been attempted but failed
-    const nextUrl = `${baseUrl}&handle=123&offset=456`
+    const nextUrl = sortUrlParams(`${baseUrl}&handle=123&offset=456`)
+
     expect(mockFetch).toHaveBeenCalledWith(nextUrl, expect.anything())
 
     // One for the main request, one for the prefetch
@@ -381,20 +382,20 @@ describe(`createFetchWithChunkBuffer`, () => {
     expect(mockFetch).toHaveBeenNthCalledWith(1, baseUrl)
     expect(mockFetch).toHaveBeenNthCalledWith(
       2,
-      `${baseUrl}&handle=123&offset=0`,
+      sortUrlParams(`${baseUrl}&handle=123&offset=0`),
       expect.anything()
     )
 
     // once interrupted it should have called the alt + the 2 prefetches
     expect(mockFetch).toHaveBeenNthCalledWith(3, altUrl)
     expect(mockFetch).toHaveBeenNthCalledWith(
       4,
-      `${altUrl}&handle=123&offset=2`,
+      sortUrlParams(`${altUrl}&handle=123&offset=2`),
       expect.anything()
     )
     expect(mockFetch).toHaveBeenNthCalledWith(
       5,
-      `${altUrl}&handle=123&offset=3`,
+      sortUrlParams(`${altUrl}&handle=123&offset=3`),
       expect.anything()
     )
   })
@@ -429,3 +430,9 @@ describe(`createFetchWithChunkBuffer`, () => {
     expect(mockFetch).toHaveBeenCalledTimes(2)
   })
 })
+
+function sortUrlParams(url: string): string {
+  const parsedUrl = new URL(url)
+  parsedUrl.searchParams.sort()
+  return parsedUrl.toString()
+}

packages/typescript-client/test/stream.test.ts

@@ -45,4 +45,35 @@ describe(`ShapeStream`, () => {
       },
     })
   })
+
+  it(`should sort query parameters for stable URLs`, async () => {
+    const eventTarget = new EventTarget()
+    const requestedUrls: Array<string> = []
+    const fetchWrapper = (
+      ...args: Parameters<typeof fetch>
+    ): Promise<Response> => {
+      requestedUrls.push(args[0].toString())
+      eventTarget.dispatchEvent(new Event(`fetch`))
+      return Promise.resolve(Response.error())
+    }
+
+    const aborter = new AbortController()
+    new ShapeStream({
+      url: shapeUrl,
+      table: `foo`,
+      where: `a=1`,
+      columns: [`id`],
+      handle: `potato`,
+      signal: aborter.signal,
+      fetchClient: fetchWrapper,
+    })
+
+    await new Promise((resolve) =>
+      eventTarget.addEventListener(`fetch`, resolve, { once: true })
+    )
+
+    expect(requestedUrls[0].split(`?`)[1]).toEqual(
+      `columns=id&handle=potato&offset=-1&table=foo&where=a%3D1`
+    )
+  })
 })