Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: order of normalizeSpriteURL and transformRequest in load sprites #3898

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix: order of normalizeSpriteURL and transformRequest in load sprites #3898

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5457628
removed url validation form normalizeSpriteURL
Kai-W Mar 25, 2024
aa5f46d
changed order of normalize and transform in load sprites
Kai-W Mar 25, 2024
e1fb123
updated normalizeSpriteURL to use URL api
Kai-W Mar 31, 2024
692217c
fix lint
Kai-W Mar 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
- Added const enum for actor messages to improve readability and maintainability. In tsconfig.json, `isolatedModules` flag is set to false in favor of generated JS size. ([#3879](https://github.com/maplibre/maplibre-gl-js/issues/3879))

### 🐞 Bug fixes
- Fix order of normalizeSpriteURL and transformRequest in loadSprite ([#3897](https://github.com/maplibre/maplibre-gl-js/issues/3897))
- _...Add new stuff here..._

## 4.1.2
Expand Down
139 changes: 127 additions & 12 deletions src/style/load_sprite.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,38 @@
import fs from 'fs';
import path from 'path';
import {RequestManager} from '../util/request_manager';
import {loadSprite} from './load_sprite';
import {loadSprite, normalizeSpriteURL} from './load_sprite';
import {type FakeServer, fakeServer} from 'nise';
import {bufferToArrayBuffer} from '../util/test/util';
import {ABORT_ERROR} from '../util/abort_error';
import * as util from '../util/util';

describe('normalizeSpriteURL', () => {
test('concantenates path, ratio, and extension for non-mapbox:// scheme', () => {
expect(
normalizeSpriteURL('http://www.foo.com/bar', '@2x', '.png')
).toBe('http://www.foo.com/bar@2x.png');
});

test('concantenates path, ratio, and extension for file:/// scheme', () => {
expect(
normalizeSpriteURL('file:///path/to/bar', '@2x', '.png')
).toBe('file:///path/to/bar@2x.png');
});

test('normalizes non-mapbox:// scheme when query string exists', () => {
expect(
normalizeSpriteURL('http://www.foo.com/bar?fresh=true', '@2x', '.png')
).toBe('http://www.foo.com/bar@2x.png?fresh=true');
});

test('No Path', () => {
expect(
normalizeSpriteURL('http://www.foo.com?fresh=true', '@2x', '.json')
).toBe('http://www.foo.com/@2x.json?fresh=true');
});
});

describe('loadSprite', () => {

let server: FakeServer;
Expand Down Expand Up @@ -40,9 +66,101 @@ describe('loadSprite', () => {

const result = await promise;

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(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(Object.keys(result)).toHaveLength(1);
expect(Object.keys(result)[0]).toBe('default');
Expand Down Expand Up @@ -73,11 +191,9 @@ describe('loadSprite', () => {
server.respond();

const result = await promise;
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(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(Object.keys(result)).toHaveLength(2);
expect(Object.keys(result)[0]).toBe('sprite1');
Expand Down Expand Up @@ -151,9 +267,8 @@ describe('loadSprite', () => {
server.respond();

const result = await promise;
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(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');
Expand Down
20 changes: 16 additions & 4 deletions src/style/load_sprite.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,12 @@ 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();
}

export async function loadSprite(
originalSprite: SpriteSpecification,
requestManager: RequestManager,
Expand All @@ -28,11 +34,17 @@ export async function loadSprite(
const imagesMap: {[id: string]: Promise<GetResourceResponse<HTMLImageElement | ImageBitmap>>} = {};

for (const {id, url} of spriteArray) {
const jsonRequestParameters = requestManager.transformRequest(requestManager.normalizeSpriteURL(url, format, '.json'), ResourceType.SpriteJSON);
jsonsMap[id] = getJSON<SpriteJSON>(jsonRequestParameters, abortController);
const requestParameters = requestManager.transformRequest(url, ResourceType.Sprite);

jsonsMap[id] = getJSON<SpriteJSON>({
...requestParameters,
url: normalizeSpriteURL(requestParameters.url, format, '.json')
}, abortController);

const imageRequestParameters = requestManager.transformRequest(requestManager.normalizeSpriteURL(url, format, '.png'), ResourceType.SpriteImage);
imagesMap[id] = ImageRequest.getImage(imageRequestParameters, abortController);
imagesMap[id] = ImageRequest.getImage({
...requestParameters,
url: normalizeSpriteURL(requestParameters.url, format, '.png')
}, abortController);
}

await Promise.all([...Object.values(jsonsMap), ...Object.values(imagesMap)]);
Expand Down
8 changes: 3 additions & 5 deletions src/style/style.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,11 +338,9 @@ describe('Style#loadJSON', () => {

await style.once('style.load');

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');
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');
});

test('emits an error on non-existant vector source layer', done => {
Expand Down
21 changes: 0 additions & 21 deletions src/util/request_manager.test.ts
View file
Open in desktop

This file was deleted.

35 changes: 1 addition & 34 deletions src/util/request_manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@ export const enum ResourceType {
Glyphs = 'Glyphs',
Image = 'Image',
Source = 'Source',
SpriteImage = 'SpriteImage',
SpriteJSON = 'SpriteJSON',
Sprite = 'Sprite',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this break the current API of request transform, which is a public API?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Havent thought about that, could be. But if we call first transformRequest we dont have the inforamtion if it is an SpriteImage or SpriteJSON. And we cant first convert the urls to an Image/JSON because we need a absolute URL for normalizeSpriteURL to work

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not saying this shouldn't be fixed, I just want to see if there's a way to do it without a breaking change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise, this will have to go into version 5 breaking change version, which can take a few month to be published.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see some possible solutions but im not sure which is the best one:

  • support relative urls than the order can stay this way. but this was rejected in other PR
  • call transformRequest twice with the same url, once for Image and JSON. this doesnt change the enum but it is a silent api change because the url passed into transformRequest changes from the result of normalizeSpriteURL like http://localhost:9966/test/unit/assets/sprite1@2x.json with the pixel ratio and file extension to the raw url from the style: http://localhost:9966/test/unit/assets/sprite1
  • rewrite normalizeSpriteURL that it can modify the url before transformRequest and do the validation afterwards. Im not sure if this is a good idea because theoretically the sprite url before the transformRequest doesn't need to be any valid url. Simply appending ${format}${extension} to the url can have other sideeffects i cant estimate. I'm still very new to maplibre-gl-js

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the URL api to parse the spriteURl does work for absoulte URLs
For relative URLs we have to provide an additional base parameter to resolve against. The progres gets then resolved (base+relative)
This would be an replacement of the current implementation of absolute URLs but doesn't solve the Problem that relative URLs are parsed and validated before the transformRequest call.

The quick and dirty way:
To use the current system with differnt SpriteImage and SpriteJSON calls we need to extend the provided SpriteUrl with the information of ${format}${extension} before we call Transform.
We could try to modify the URL without parsing just based on String Operations.
This extension is always appended at the path of the URL.
If there are query Parameters thats directly before the first ? or without params at the end of the String
E.g. @2x.png should be inserted:
http://www.foo.com/bar?fresh=true -> http://www.foo.com/bar@2x.png?fresh=true
http://www.foo.com/bar -> http://www.foo.com/bar@2x.png
/bar -> /bar@2x.png
This doesn't need validation before the transformRequest.
This will work for absolute and relative urls.
It will break if its an absolute url and there is no path component
http://www.foo.com-> http://www.foo.com@2x.png
or sombody has arbitrary strings that get converted to urls in the transformRequest method
foobar -> foobar@2x.png

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i tried implementing it in this branch. It works but with the drawbacks mentioned above
main...Kai-W:maplibre-gl-js:test
I still prefer the original solution with no modification of the SpriteURL before the call of transformRequest. It feels like a cleaner solution, but it has a small Public api change

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do public API changes as part of version 5, which we probably will target late this year or early next year.
If the normalize URL is only used in this code path then it makes sense to move it next to it.
Can you better clarify how the current method and the previous method are different? What won't work in the code you wrote (changing the normalizeUrl method)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current Method:
Parse URL with an regex in protocol, authority, path and params. If path is empty set to "/". Append ${format}${extension} to path and then reconstruct the url from the parts.
Afterwards call transformRequest twice with the modifyed urls (.josn + .png)
Works only with correct absolute URLs, otherwise parsing with the regex failes.
Works as well for absoulte URLs with no Path element e.g. http://www.foo.com -> http://www.foo.com/@2x.png/

New normalizeUrl:
Splits the String at the first "?" Assuming the Sting is an URL and a ? would seperate path and params of an URL
Then Appennd ${format}${extension} to the first part (path) and concatinate it back together. Then calling the transformRequest with both modifyed URLs
This works with absolute and relative URLs. But it wont work with arbitrary strings or absolute URLs with no Path. E.g
http://www.foo.com gets http://www.foo.com@2x.png/ is missing a "/" between .com and @2x
But i think thats an edgcase that would not happen in production. It would mean that there is no filename used for the sprites
Additionally with my current implemetation there is no validation of the URL returned by transformRequest.

New Api breaking Change:
First call transformRequest once withoput appending ${format}${extension} (.png/.json) to the Path.
Assume the result of transformRequest must be a valid absolute URL and then use normalizeSpriteURL to generate the .png/ .json urls based on the result of transfomrRequest.
Works with any sprite String.
Api change: transformRequest gets called once without .png/.json in the URL and one generic Sprite ResourceType
Additionaly we could change the normalizeSpriteURL to use the Browser URL API to remove the regex parsing.

An Option could be to merge the new normalize URL now to fix the bug and then merge the api breaking Change for Version 5.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm OK with the last suggestion about merging this fix now and adding a breaking change feature to v5.

Style = 'Style',
Tile = 'Tile',
Unknown = 'Unknown',
Expand All @@ -20,13 +19,6 @@ export const enum ResourceType {
*/
export type RequestTransformFunction = (url: string, resourceType?: ResourceType) => RequestParameters | undefined;

type UrlObject = {
protocol: string;
authority: string;
path: string;
params: Array<string>;
};

export class RequestManager {
_transformRequestFn: RequestTransformFunction;

Expand All @@ -42,33 +34,8 @@ export class RequestManager {
return {url};
}

normalizeSpriteURL(url: string, format: string, extension: string): string {
const urlObject = parseUrl(url);
urlObject.path += `${format}${extension}`;
return formatUrl(urlObject);
}

setTransformRequest(transformRequest: RequestTransformFunction) {
this._transformRequestFn = transformRequest;
}
}

const urlRe = /^(\w+):\/\/([^/?]*)(\/[^?]+)?\??(.+)?/;

function parseUrl(url: string): UrlObject {
const parts = url.match(urlRe);
if (!parts) {
throw new Error(`Unable to parse URL "${url}"`);
}
return {
protocol: parts[1],
authority: parts[2],
path: parts[3] || '/',
params: parts[4] ? parts[4].split('&') : []
};
}

function formatUrl(obj: UrlObject): string {
const params = obj.params.length ? `?${obj.params.join('&')}` : '';
return `${obj.protocol}://${obj.authority}${obj.path}${params}`;
}
Loading