From 65df86932562fcc4d003fd45b74382c8412f408a Mon Sep 17 00:00:00 2001
From: Lukas Stracke <lukas.stracke@sentry.io>
Date: Wed, 16 Aug 2023 10:43:37 +0200
Subject: [PATCH] fix(sveltekit): Avoid invalidating data on route changes in
 `wrapServerLoadWithSentry` (#8801)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The SvelteKit SDK caused server load data to be
invalidated, resulting in said functions being called on every route
change. I initially thought this was related to our Kit-specific client
fetch instrumentation which turned out not to be causing this. Instead,
the culprit is our `wrapServerLoadWithSentry` wrapper:

In the wrapper, we access `event.route.id` to determine the route of the
load function for the span description. Internally, SvelteKit puts a
proxy on certain `event` properties, such as `event.route`. In case any
property of `event.route` was accessed, [SvelteKit marks
this](https://github.com/sveltejs/kit/blob/e133aba479fa9ba0e7f9e71512f5f937f0247e2c/packages/kit/src/runtime/server/page/load_data.js#L111-L124)
internally and send along a flag to the client. On a route change, the
client would [check this
flag](https://github.com/sveltejs/kit/blob/e133aba479fa9ba0e7f9e71512f5f937f0247e2c/packages/kit/src/runtime/client/client.js#L572)
and mark the route as invalidated, thereby causing a [call to the load
function](https://github.com/sveltejs/kit/blob/e133aba479fa9ba0e7f9e71512f5f937f0247e2c/packages/kit/src/runtime/client/client.js#L641)
on each navigation.

---------

Co-authored-by: Kamil Ogórek <kamil.ogorek@gmail.com>
---
 packages/sveltekit/src/server/load.ts       |  6 ++++-
 packages/sveltekit/test/server/load.test.ts | 29 +++++++++++++++++++++
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/packages/sveltekit/src/server/load.ts b/packages/sveltekit/src/server/load.ts
index 0ad28e1cb4eb..04d0137062c6 100644
--- a/packages/sveltekit/src/server/load.ts
+++ b/packages/sveltekit/src/server/load.ts
@@ -119,7 +119,11 @@ export function wrapServerLoadWithSentry<T extends (...args: any) => any>(origSe
 
       addNonEnumerableProperty(event as unknown as Record<string, unknown>, '__sentry_wrapped__', true);
 
-      const routeId = event.route && event.route.id;
+      // Accessing any member of `event.route` causes SvelteKit to invalidate the
+      // server `load` function's data on every route change.
+      // To work around this, we use `Object.getOwnPropertyDescriptor` which doesn't invoke the proxy.
+      // https://github.com/sveltejs/kit/blob/e133aba479fa9ba0e7f9e71512f5f937f0247e2c/packages/kit/src/runtime/server/page/load_data.js#L111C3-L124
+      const routeId = event.route && (Object.getOwnPropertyDescriptor(event.route, 'id')?.value as string | undefined);
 
       const { dynamicSamplingContext, traceparentData, propagationContext } = getTracePropagationData(event);
       getCurrentHub().getScope().setPropagationContext(propagationContext);
diff --git a/packages/sveltekit/test/server/load.test.ts b/packages/sveltekit/test/server/load.test.ts
index fa57dc6b7c46..90980204cf68 100644
--- a/packages/sveltekit/test/server/load.test.ts
+++ b/packages/sveltekit/test/server/load.test.ts
@@ -400,4 +400,33 @@ describe('wrapServerLoadWithSentry calls trace', () => {
 
     expect(mockTrace).toHaveBeenCalledTimes(1);
   });
+
+  it("doesn't invoke the proxy set on `event.route`", async () => {
+    const event = getServerOnlyArgs();
+
+    // simulates SvelteKit adding a proxy to `event.route`
+    // https://github.com/sveltejs/kit/blob/e133aba479fa9ba0e7f9e71512f5f937f0247e2c/packages/kit/src/runtime/server/page/load_data.js#L111C3-L124
+    const proxyFn = vi.fn((target: { id: string }, key: string | symbol): any => {
+      return target[key];
+    });
+
+    event.route = new Proxy(event.route, {
+      get: proxyFn,
+    });
+
+    const wrappedLoad = wrapServerLoadWithSentry(serverLoad);
+    await wrappedLoad(event);
+
+    expect(mockTrace).toHaveBeenCalledTimes(1);
+    expect(mockTrace).toHaveBeenCalledWith(
+      expect.objectContaining({
+        op: 'function.sveltekit.server.load',
+        name: '/users/[id]', // <-- this shows that the route was still accessed
+      }),
+      expect.any(Function),
+      expect.any(Function),
+    );
+
+    expect(proxyFn).not.toHaveBeenCalled();
+  });
 });