Merge pull request #7811 from maiieul/revert-redirect-caching

Revert "fix: solve issue with Cache-Control header deletion (#6991)"

Author: wmertens

.changeset/free-views-boil.md

@@ -0,0 +1,5 @@
+---
+'@builder.io/qwik-city': patch
+---
+
+FIX: redirects no longer take their parent layout's Cache-Control value by default and are instead set to `no-store`. This prevents issues in redirection logic. We might introduce another API to enable caching redirects in the future.

packages/qwik-city/src/middleware/request-handler/request-event.ts

@@ -228,6 +228,7 @@ export function createRequestEvent(
 
     error: <T = any>(statusCode: number, message: T) => {
       status = statusCode;
+      headers.delete('Cache-Control');
       return new ServerError(statusCode, message);
     },
 
@@ -241,8 +242,8 @@ export function createRequestEvent(
         }
         headers.set('Location', fixedURL);
       }
-      // Fallback to 'no-store' when end user is not managing Cache-Control header
-      if (statusCode > 301 && !headers.get('Cache-Control')) {
+      headers.delete('Cache-Control');
+      if (statusCode > 301) {
         headers.set('Cache-Control', 'no-store');
       }
       exit();
@@ -265,6 +266,7 @@ export function createRequestEvent(
     fail: <T extends Record<string, any>>(statusCode: number, data: T): FailReturn<T> => {
       check();
       status = statusCode;
+      headers.delete('Cache-Control');
       return {
         failed: true,
         ...data,

packages/qwik-city/src/middleware/request-handler/request-event.unit.ts

@@ -0,0 +1,93 @@
+import { describe, it, expect, vi } from 'vitest';
+import { createRequestEvent } from './request-event';
+import { RedirectMessage } from './redirect-handler';
+import type { ServerRequestEvent, QwikSerializer } from './types';
+
+const mockQwikSerializer: QwikSerializer = {
+  _deserializeData: vi.fn(),
+  _serializeData: vi.fn(),
+  _verifySerializable: vi.fn(),
+};
+
+function createMockServerRequestEvent(url = 'http://localhost:3000/test'): ServerRequestEvent {
+  const mockRequest = new Request(url);
+
+  return {
+    mode: 'server',
+    url: new URL(url),
+    locale: undefined,
+    platform: {},
+    request: mockRequest,
+    env: {
+      get: vi.fn(),
+    },
+    getClientConn: vi.fn(() => ({ ip: '127.0.0.1' })),
+    getWritableStream: vi.fn(() => {
+      const writer = {
+        write: vi.fn(),
+        close: vi.fn(),
+      };
+      return {
+        getWriter: () => writer,
+        locked: false,
+        pipeTo: vi.fn(),
+      } as any;
+    }),
+  };
+}
+
+function createMockRequestEvent(url = 'http://localhost:3000/test') {
+  const serverRequestEv = createMockServerRequestEvent(url);
+  return createRequestEvent(serverRequestEv, null, [], true, '/', mockQwikSerializer, vi.fn());
+}
+
+describe('request-event redirect', () => {
+  it('should not cache redirects by default', () => {
+    const requestEv = createMockRequestEvent();
+
+    requestEv.headers.set('Cache-Control', 'max-age=3600, public');
+
+    const result = requestEv.redirect(301, '/new-location');
+
+    expect(result).toBeInstanceOf(RedirectMessage);
+    expect(requestEv.headers.get('Location')).toBe('/new-location');
+    expect(requestEv.headers.get('Cache-Control')).toBeNull();
+    expect(requestEv.status()).toBe(301);
+  });
+
+  it('should set Cache-Control to no-store for redirects with status > 301', () => {
+    const requestEv = createMockRequestEvent();
+
+    const result = requestEv.redirect(307, '/new-location');
+
+    expect(result).toBeInstanceOf(RedirectMessage);
+    expect(requestEv.headers.get('Location')).toBe('/new-location');
+    expect(requestEv.headers.get('Cache-Control')).toBe('no-store');
+    expect(requestEv.status()).toBe(307);
+  });
+
+  it('should fix invalid redirect URLs with multiple slashes', () => {
+    const requestEv = createMockRequestEvent();
+
+    const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {});
+
+    const result = requestEv.redirect(302, '/path//with///multiple////slashes');
+
+    expect(result).toBeInstanceOf(RedirectMessage);
+    expect(requestEv.headers.get('Location')).toBe('/path/with/multiple/slashes');
+    expect(consoleSpy).toHaveBeenCalledWith(
+      'Redirect URL /path//with///multiple////slashes is invalid, fixing to /path/with/multiple/slashes'
+    );
+  });
+
+  it('should throw error when trying to redirect after headers are sent', () => {
+    const requestEv = createMockRequestEvent();
+
+    // Trigger getWritableStream to simulate headers being sent
+    requestEv.getWritableStream();
+
+    expect(() => {
+      requestEv.redirect(302, '/should-fail');
+    }).toThrow('Response already sent');
+  });
+});