Fix accept header overwrite (#4210)

* do not overwrite the Accept header if the user has already set it * debug check that we do not nuke Accept header * stop with maplibre-gl-header stuff * add tests for Accept header preservation * undo local dev changes * add Changelog entry for 4210 * remove local dev code from package.json * lint * make Content-Type and Accept header consistent in ajax test * also stop overwriting user provided Accept header for makeXMLHttpRequest * add tests for file:// url as well * fix ajax file:/// tests * reflect new build size
maplibre · May 31, 2024 · f70ce60 · f70ce60
1 parent 3bec7be
commit f70ce60
Show file tree

Hide file tree

Showing 4 changed files with 74 additions and 3 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,6 +7,7 @@
 - Use Autoprefixer to handle vendor prefixes in CSS ([#4165](https://github.com/maplibre/maplibre-gl-js/pull/4165))
 - Make `aria-label` configurable for Map, Marker and Popup [#4147](https://github.com/maplibre/maplibre-gl-js/pull/4147)
 - Map `<canvas>` is focusable only when interactive [#4147](https://github.com/maplibre/maplibre-gl-js/pull/4147)
+- "Accept" headers set in Request Transformers are not overwritten [#4210](https://github.com/maplibre/maplibre-gl-js/pull/4210)
 - _...Add new stuff here..._
 
 ### 🐞 Bug fixes

diff --git a/src/util/ajax.test.ts b/src/util/ajax.test.ts
@@ -167,5 +167,71 @@ describe('ajax', () => {
             expect(server.requests[0].method).toBe('GET');
             expect(server.requests[0].requestHeaders['Authorization']).toBe('Bearer 123');
         });
+
+        test('should preserve user-specified Accept header', async () => {
+            server.respondWith(request => {
+                // Note that postgrest responds to this type of request with application/geo+json
+                request.respond(200, {'Content-Type': 'application/geo+json'}, '{"foo": "bar"}');
+            });
+
+            const promise = getJSON({url: 'http://example.com/test-params.json', cache: 'force-cache', headers: {'Authorization': 'Bearer 123', 'Accept': 'application/geo+json'}}, new AbortController());
+            server.respond();
+            await promise;
+
+            expect(server.requests).toHaveLength(1);
+            expect(server.requests[0].url).toBe('http://example.com/test-params.json');
+            expect(server.requests[0].method).toBe('GET');
+            expect(server.requests[0].requestHeaders['Authorization']).toBe('Bearer 123');
+            expect(server.requests[0].requestHeaders['Accept']).toBe('application/geo+json');
+        });
+
+        test('should add default Accept header when user has not specified one', async () => {
+            server.respondWith(request => {
+                request.respond(200, {'Content-Type': 'application/json'}, '{"foo": "bar"}');
+            });
+
+            const promise = getJSON({url: 'http://example.com/test-params.json', cache: 'force-cache', headers: {'Authorization': 'Bearer 123'}}, new AbortController());
+            server.respond();
+            await promise;
+
+            expect(server.requests).toHaveLength(1);
+            expect(server.requests[0].url).toBe('http://example.com/test-params.json');
+            expect(server.requests[0].method).toBe('GET');
+            expect(server.requests[0].requestHeaders['Authorization']).toBe('Bearer 123');
+            expect(server.requests[0].requestHeaders['Accept']).toBe('application/json');
+        });
+
+        test('should add default Accept header when user has not specified one, even for file:// requests', async () => {
+            server.respondWith(request => {
+                request.respond(200, {'Content-Type': 'application/json'}, '{"foo": "bar"}');
+            });
+
+            const promise = getJSON({url: 'file:///C:/Temp/abc.json', cache: 'force-cache', headers: {'Authorization': 'Bearer 123'}}, new AbortController());
+            server.respond();
+            await promise;
+
+            expect(server.requests).toHaveLength(1);
+            expect(server.requests[0].url).toBe('file:///C:/Temp/abc.json');
+            expect(server.requests[0].method).toBe('GET');
+            expect(server.requests[0].requestHeaders['Authorization']).toBe('Bearer 123');
+            expect(server.requests[0].requestHeaders['Accept']).toBe('application/json');
+        });
+
+        test('should not add default Accept header when user has already specified one, even for file:// requests', async () => {
+            server.respondWith(request => {
+                request.respond(200, {'Content-Type': 'application/json'}, '{"foo": "bar"}');
+            });
+
+            const promise = getJSON({url: 'file:///C:/Temp/abc.json', cache: 'force-cache', headers: {'Authorization': 'Bearer 123', 'Accept': 'application/geo+json'}}, new AbortController());
+            server.respond();
+            await promise;
+
+            expect(server.requests).toHaveLength(1);
+            expect(server.requests[0].url).toBe('file:///C:/Temp/abc.json');
+            expect(server.requests[0].method).toBe('GET');
+            expect(server.requests[0].requestHeaders['Authorization']).toBe('Bearer 123');
+            expect(server.requests[0].requestHeaders['Accept']).toBe('application/geo+json');
+        });
+
     });
 });
diff --git a/src/util/ajax.ts b/src/util/ajax.ts
@@ -150,7 +150,8 @@ async function makeFetchRequest(requestParameters: RequestParameters, abortContr
         signal: abortController.signal
     });
 
-    if (requestParameters.type === 'json') {
+    // If the user has already set an Accept header, do not overwrite it here
+    if (requestParameters.type === 'json' && !request.headers.has('Accept')) {
         request.headers.set('Accept', 'application/json');
     }
 
@@ -187,7 +188,10 @@ function makeXMLHttpRequest(requestParameters: RequestParameters, abortControlle
         }
         if (requestParameters.type === 'json') {
             xhr.responseType = 'text';
-            xhr.setRequestHeader('Accept', 'application/json');
+            // Do not overwrite the user-provided Accept header
+            if (!requestParameters.headers?.Accept) {
+                xhr.setRequestHeader('Accept', 'application/json');
+            }
         }
         xhr.withCredentials = requestParameters.credentials === 'include';
         xhr.onerror = () => {

diff --git a/test/build/min.test.ts b/test/build/min.test.ts
@@ -36,7 +36,7 @@ describe('test min build', () => {
         const decreaseQuota = 4096;
 
         // feel free to update this value after you've checked that it has changed on purpose :-)
-        const expectedBytes = 791799;
+        const expectedBytes = 792823;
 
         expect(actualBytes - expectedBytes).toBeLessThan(increaseQuota);
         expect(expectedBytes - actualBytes).toBeLessThan(decreaseQuota);