Fix URL parsing in normalizeSpriteURL (#4962)

* Revert "fix: order of normalizeSpriteURL and transformRequest in load sprites (#3898)" This reverts commit 54cf54e * Fix URL parsing in normalizeSpriteURL * add documentation --------- Co-authored-by: Harel M <harel.mazor@gmail.com>
maplibre · Nov 7, 2024 · 8ba8f5c · 8ba8f5c
1 parent 928fc0c
commit 8ba8f5c
Show file tree

Hide file tree

Showing 6 changed files with 50 additions and 119 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -8,6 +8,7 @@
 
 ### 🐞 Bug fixes
 - ⚠️ Fix level of detail at high pitch angle by changing which tiles to load ([#3983](https://github.com/maplibre/maplibre-gl-js/issues/3983))
+- ⚠️ Fix URL parsing in `normalizeSpriteURL`, sprite URLs must be absolute.
 - _...Add new stuff here..._
 
 ## 5.0.0-pre.5
@@ -34,7 +35,7 @@
 
 ### 🐞 Bug fixes
 
-- ⚠️ Fix order of normalizeSpriteURL and transformRequest in loadSprite ([#3897](https://github.com/maplibre/maplibre-gl-js/issues/3897))
+- ~~⚠️ Fix order of normalizeSpriteURL and transformRequest in loadSprite ([#3897](https://github.com/maplibre/maplibre-gl-js/issues/3897))~~
 - ⚠️ Remove unminified prod build ([#4906](https://github.com/maplibre/maplibre-gl-js/pull/4906))
 - Fix issue where raster tile source won't fetch updates following request error ([#4890](https://github.com/maplibre/maplibre-gl-js/pull/4890))
 - Fix 3D models in custom layers not being properly occluded by the globe ([#4817](https://github.com/maplibre/maplibre-gl-js/issues/4817))

diff --git a/src/style/load_sprite.test.ts b/src/style/load_sprite.test.ts
@@ -26,6 +26,12 @@ describe('normalizeSpriteURL', () => {
         ).toBe('http://www.foo.com/bar@2x.png?fresh=true');
     });
 
+    test('test relative URL', () => {
+        expect(
+            () => normalizeSpriteURL('/bar?fresh=true', '@2x', '.png')
+        ).toThrow(/Invalid/i);
+    });
+
     test('No Path', () => {
         expect(
             normalizeSpriteURL('http://www.foo.com?fresh=true', '@2x', '.json')
@@ -66,101 +72,9 @@ describe('loadSprite', () => {
 
         const result = await promise;
 
-        expect(transform).toHaveBeenCalledTimes(1);
-        expect(transform).toHaveBeenNthCalledWith(1, 'http://localhost:9966/test/unit/assets/sprite1', 'Sprite');
-
-        expect(Object.keys(result)).toHaveLength(1);
-        expect(Object.keys(result)[0]).toBe('default');
-
-        Object.values(result['default']).forEach(styleImage => {
-            expect(styleImage.spriteData).toBeTruthy();
-            expect(styleImage.spriteData.context).toBeInstanceOf(CanvasRenderingContext2D);
-        });
-
-        expect(server.requests[0].url).toBe('http://localhost:9966/test/unit/assets/sprite1.json');
-        expect(server.requests[1].url).toBe('http://localhost:9966/test/unit/assets/sprite1.png');
-    });
-
-    test('transform of relative url', async () => {
-        const transform = jest.fn().mockImplementation((url, type) => {
-            return {url: `http://localhost:9966${url}`, type};
-        });
-
-        const manager = new RequestManager(transform);
-
-        server.respondWith('GET', 'http://localhost:9966/test/unit/assets/sprite1.json', fs.readFileSync(path.join(__dirname, '../../test/unit/assets/sprite1.json')).toString());
-        server.respondWith('GET', 'http://localhost:9966/test/unit/assets/sprite1.png', bufferToArrayBuffer(fs.readFileSync(path.join(__dirname, '../../test/unit/assets/sprite1.png'))));
-
-        const promise = loadSprite('/test/unit/assets/sprite1', manager, 1, new AbortController());
-
-        server.respond();
-
-        const result = await promise;
-
-        expect(transform).toHaveBeenCalledTimes(1);
-        expect(transform).toHaveBeenNthCalledWith(1, '/test/unit/assets/sprite1', 'Sprite');
-
-        expect(Object.keys(result)).toHaveLength(1);
-        expect(Object.keys(result)[0]).toBe('default');
-
-        Object.values(result['default']).forEach(styleImage => {
-            expect(styleImage.spriteData).toBeTruthy();
-            expect(styleImage.spriteData.context).toBeInstanceOf(CanvasRenderingContext2D);
-        });
-
-        expect(server.requests[0].url).toBe('http://localhost:9966/test/unit/assets/sprite1.json');
-        expect(server.requests[1].url).toBe('http://localhost:9966/test/unit/assets/sprite1.png');
-    });
-
-    test('transform of random Sprite String', async () => {
-        const transform = jest.fn().mockImplementation((url, type) => {
-            return {url: 'http://localhost:9966/test/unit/assets/sprite1', type};
-        });
-
-        const manager = new RequestManager(transform);
-
-        server.respondWith('GET', 'http://localhost:9966/test/unit/assets/sprite1.json', fs.readFileSync(path.join(__dirname, '../../test/unit/assets/sprite1.json')).toString());
-        server.respondWith('GET', 'http://localhost:9966/test/unit/assets/sprite1.png', bufferToArrayBuffer(fs.readFileSync(path.join(__dirname, '../../test/unit/assets/sprite1.png'))));
-
-        const promise = loadSprite('foobar', manager, 1, new AbortController());
-
-        server.respond();
-
-        const result = await promise;
-
-        expect(transform).toHaveBeenCalledTimes(1);
-        expect(transform).toHaveBeenNthCalledWith(1, 'foobar', 'Sprite');
-
-        expect(Object.keys(result)).toHaveLength(1);
-        expect(Object.keys(result)[0]).toBe('default');
-
-        Object.values(result['default']).forEach(styleImage => {
-            expect(styleImage.spriteData).toBeTruthy();
-            expect(styleImage.spriteData.context).toBeInstanceOf(CanvasRenderingContext2D);
-        });
-
-        expect(server.requests[0].url).toBe('http://localhost:9966/test/unit/assets/sprite1.json');
-        expect(server.requests[1].url).toBe('http://localhost:9966/test/unit/assets/sprite1.png');
-    });
-
-    test('transform of relative URL', async () => {
-        const transform = jest.fn().mockImplementation((url, type) => {
-            return {url: `http://localhost:9966${url}`, type};
-        });
-
-        const manager = new RequestManager(transform);
-
-        server.respondWith('GET', 'http://localhost:9966/test/unit/assets/sprite1.json', fs.readFileSync(path.join(__dirname, '../../test/unit/assets/sprite1.json')).toString());
-        server.respondWith('GET', 'http://localhost:9966/test/unit/assets/sprite1.png', bufferToArrayBuffer(fs.readFileSync(path.join(__dirname, '../../test/unit/assets/sprite1.png'))));
-
-        const promise = loadSprite('/test/unit/assets/sprite1', manager, 1, new AbortController());
-
-        server.respond();
-
-        const result = await promise;
-
-        expect(transform).toHaveBeenCalledTimes(1);
-        expect(transform).toHaveBeenNthCalledWith(1, '/test/unit/assets/sprite1', 'Sprite');
+        expect(transform).toHaveBeenCalledTimes(2);
+        expect(transform).toHaveBeenNthCalledWith(1, 'http://localhost:9966/test/unit/assets/sprite1.json', 'SpriteJSON');
+        expect(transform).toHaveBeenNthCalledWith(2, 'http://localhost:9966/test/unit/assets/sprite1.png', 'SpriteImage');
 
         expect(Object.keys(result)).toHaveLength(1);
         expect(Object.keys(result)[0]).toBe('default');
@@ -191,9 +105,11 @@ describe('loadSprite', () => {
         server.respond();
 
         const result = await promise;
-        expect(transform).toHaveBeenCalledTimes(2);
-        expect(transform).toHaveBeenNthCalledWith(1, 'http://localhost:9966/test/unit/assets/sprite1', 'Sprite');
-        expect(transform).toHaveBeenNthCalledWith(2, 'http://localhost:9966/test/unit/assets/sprite2', 'Sprite');
+        expect(transform).toHaveBeenCalledTimes(4);
+        expect(transform).toHaveBeenNthCalledWith(1, 'http://localhost:9966/test/unit/assets/sprite1.json', 'SpriteJSON');
+        expect(transform).toHaveBeenNthCalledWith(2, 'http://localhost:9966/test/unit/assets/sprite1.png', 'SpriteImage');
+        expect(transform).toHaveBeenNthCalledWith(3, 'http://localhost:9966/test/unit/assets/sprite2.json', 'SpriteJSON');
+        expect(transform).toHaveBeenNthCalledWith(4, 'http://localhost:9966/test/unit/assets/sprite2.png', 'SpriteImage');
 
         expect(Object.keys(result)).toHaveLength(2);
         expect(Object.keys(result)[0]).toBe('sprite1');
@@ -267,8 +183,9 @@ describe('loadSprite', () => {
         server.respond();
 
         const result = await promise;
-        expect(transform).toHaveBeenCalledTimes(1);
-        expect(transform).toHaveBeenNthCalledWith(1, 'http://localhost:9966/test/unit/assets/sprite1', 'Sprite');
+        expect(transform).toHaveBeenCalledTimes(2);
+        expect(transform).toHaveBeenNthCalledWith(1, 'http://localhost:9966/test/unit/assets/sprite1@2x.json', 'SpriteJSON');
+        expect(transform).toHaveBeenNthCalledWith(2, 'http://localhost:9966/test/unit/assets/sprite1@2x.png', 'SpriteImage');
 
         expect(Object.keys(result)).toHaveLength(1);
         expect(Object.keys(result)[0]).toBe('default');

diff --git a/src/style/load_sprite.ts b/src/style/load_sprite.ts
@@ -16,9 +16,14 @@ export type LoadSpriteResult = {
 }
 
 export function normalizeSpriteURL(url: string, format: string, extension: string): string {
-    const parsed = new URL(url);
-    parsed.pathname += `${format}${extension}`;
-    return parsed.toString();
+    try {
+        const parsed = new URL(url);
+        parsed.pathname += `${format}${extension}`;
+        return parsed.toString();
+    }
+    catch {
+        throw new Error(`Invalid sprite URL "${url}", must be absolute. Modify style specification directly or use TransformStyleFunction to correct the issue dynamically`)
+    }
 }
 
 export async function loadSprite(
@@ -34,17 +39,11 @@ export async function loadSprite(
     const imagesMap: {[id: string]: Promise<GetResourceResponse<HTMLImageElement | ImageBitmap>>} = {};
 
     for (const {id, url} of spriteArray) {
-        const requestParameters = requestManager.transformRequest(url, ResourceType.Sprite);
-
-        jsonsMap[id] = getJSON<SpriteJSON>({
-            ...requestParameters,
-            url: normalizeSpriteURL(requestParameters.url, format, '.json')
-        }, abortController);
+        const jsonRequestParameters = requestManager.transformRequest(normalizeSpriteURL(url, format, '.json'), ResourceType.SpriteJSON);
+        jsonsMap[id] = getJSON<SpriteJSON>(jsonRequestParameters, abortController);
 
-        imagesMap[id] = ImageRequest.getImage({
-            ...requestParameters,
-            url: normalizeSpriteURL(requestParameters.url, format, '.png')
-        }, abortController);
+        const imageRequestParameters = requestManager.transformRequest(normalizeSpriteURL(url, format, '.png'), ResourceType.SpriteImage);
+        imagesMap[id] = ImageRequest.getImage(imageRequestParameters, abortController);
     }
 
     await Promise.all([...Object.values(jsonsMap), ...Object.values(imagesMap)]);

diff --git a/src/style/style.test.ts b/src/style/style.test.ts
@@ -340,9 +340,11 @@ describe('Style#loadJSON', () => {
 
         await style.once('style.load');
 
-        expect(transformSpy).toHaveBeenCalledTimes(1);
-        expect(transformSpy.mock.calls[0][0]).toBe('http://example.com/sprites/bright-v8');
-        expect(transformSpy.mock.calls[0][1]).toBe('Sprite');
+        expect(transformSpy).toHaveBeenCalledTimes(2);
+        expect(transformSpy.mock.calls[0][0]).toBe('http://example.com/sprites/bright-v8.json');
+        expect(transformSpy.mock.calls[0][1]).toBe('SpriteJSON');
+        expect(transformSpy.mock.calls[1][0]).toBe('http://example.com/sprites/bright-v8.png');
+        expect(transformSpy.mock.calls[1][1]).toBe('SpriteImage');
     });
 
     test('emits an error on non-existant vector source layer', () => new Promise<void>(done => {

diff --git a/src/style/style.ts b/src/style/style.ts
@@ -121,6 +121,7 @@ export type StyleSetterOptions = {
  * - when previous style carries certain 'state' that needs to be carried over to a new style gracefully;
  * - when a desired style is a certain combination of previous and incoming style;
  * - when an incoming style requires modification based on external state.
+ * - when an incoming style uses relative paths, which need to be converted to absolute.
  *
  * @param previous - The current style.
  * @param next - The next style.
@@ -131,8 +132,18 @@ export type StyleSetterOptions = {
  * map.setStyle('https://demotiles.maplibre.org/style.json', {
  *   transformStyle: (previousStyle, nextStyle) => ({
  *       ...nextStyle,
+ *       // make relative sprite path like "../sprite" absolute
+ *       sprite: new URL(nextStyle.sprite, "https://demotiles.maplibre.org/styles/osm-bright-gl-style/sprites/").href,
+ *       // make relative glyphs path like "../fonts/{fontstack}/{range}.pbf" absolute
+ *       glyphs: new URL(nextStyle.glyphs, "https://demotiles.maplibre.org/font/").href,
  *       sources: {
- *           ...nextStyle.sources,
+ *           // make relative vector url like "../../" absolute
+ *           ...nextStyle.sources.map(source => {
+ *              if (source.url) {
+     *              source.url = new URL(source.url, "https://api.maptiler.com/tiles/osm-bright-gl-style/");
+ *              }
+ *              return source;
+ *           }),
  *           // copy a source from previous style
  *           'osm': previousStyle.sources.osm
  *       },

diff --git a/src/util/request_manager.ts b/src/util/request_manager.ts
@@ -7,7 +7,8 @@ export const enum ResourceType {
     Glyphs = 'Glyphs',
     Image = 'Image',
     Source = 'Source',
-    Sprite = 'Sprite',
+    SpriteImage = 'SpriteImage',
+    SpriteJSON = 'SpriteJSON',
     Style = 'Style',
     Tile = 'Tile',
     Unknown = 'Unknown',