feat: Fix getPublicUrl to handle relative and absolute URLs

[Monnte]

This PR updates the getPublicUrl function to handle relative publicUrl values. When a relative URL is provided, it is automatically converted to an absolute URL based on the request context. If an absolute URL is provided, it is returned as-is.

User story:

I encountered an issue while setting up an Nginx proxy with a prefix for the tileserver. The main problem was when I needed to specify the entire absolute URL using -u|--public_url (because when fetching .pbf tiles, the relative URLs didn't work). In scenarios where I don't know the domain the server will be used on, I needed a way to rely on absolute URLs. I believe this PR will help with this scenario, and I'm sure others may face a similar issue. 😄

[Monnte]

Hi @acalcutt I hope you're doing well! Would you mind reviewing this PR when you have a moment?

[acalcutt]

Seems like something is going on with the ci, which i don't think is related to this PR. I will have to figure that out before this can be approved.

[acalcutt]

It does seem like this test is failing. I am not sure that port it is using is valid.

1. Styles
   /styles/test-style/style.json is valid style
   contains expected properties:

   AssertionError: expected 'http://127.0.0.1:37499/test/styles/te…' to equal '/test/styles/test-style/sprite'

   • expected - actual

   -http://127.0.0.1:37499/test/styles/test-style/sprite
   +/test/styles/test-style/sprite

[Monnte]

It does seem like this test is failing. I am not sure that port it is using is valid.

The test if failing because in the "setup.js" is "publicUrl: '/test/'" which due this PR is converted to absolute path.

I propose to check for "contain" instead of "equal" in the test, and also I think it should be not breaking change for the users using this feature, hope so 🙏

[acalcutt]

What about the port that it is getting in that url, 37499. that does not seem correct since the test says it is running on 8888
Listening at http://[::]:8888/. it seems more like a client port than the server port.

Also, I am not sure if all urls that don't have publicurl should have a http url appended to it. some paths are meant to be local paths instead of http paths I think...

[Monnte]

@acalcutt

What about the port that it is getting in that url, 37499. that does not seem correct since the test says it is running on 8888
Listening at http://[::]:8888/. it seems more like a client port than the server port.

The port is taken from the request object used in "getUrlObject" function which I didn't touch, just used as base of absolute path. Don't know how it gets there. There can be some port forwarding in pipline when running tests, don't know really.

Also, I am not sure if all urls that don't have publicurl should have a http url appended to it. some paths are meant to be local paths instead of http paths I think...

The functionality I have added only change how publicUrl is treated, to always create an absolute path and don't use relative paths for urls in styles, for me It makes sense to work like this, becase when I dont specify public url then the "getUrlObject(req).toString()" is used as default which wokrs as absolute path generated from request.

Also I have checked codebase where the getPublicUrl is used, and it is used only for styles, when getting all avaiable styles and then in "fixUrl" function which is used to fix urls like sprites and glyphs etc.. and also the function has the comment ("Replaces local:// URLs with public http(s):// URLs."). And there is no other place where the getPublicUrl is used. And when I use the --public_url with some abolute url like "/tileserver/" it doesnt make sense that I get this url for example ("sprite": "/tileserver/styles/basic-style/sprite", "glyphs": "/tileserver/fonts/{fontstack}/{range}.pbf"), because when I use it Plotly for example it tries to fetch data in relative path based on my current href... So in the end the --public_url is useless without defining the full absolute path. And also when getting the tiles from the data and public_url is relative it didnt work because the library that are used for preview doesnt work with relative urls (see mapbox/mapbox-gl-js#10407 (comment))

I would like to have an option to set some "prefix" like "/tileserver/" to all urls in styels. But if you dont like this change, I can try to do it in some more option way... to use some special character like "$/tileserver/" to detect that i want to use "return new URL(publicUrl, getUrlObject(req)).toString();" instead of basic "return publicUrl;"

WDYT ? 😅

[acalcutt]

I am still concerned with the test that got changed. It seems to me if it was working the same as before the test would not need to change. I still haven't seen a good reason that it looks was getting a client port and adding it to the url.

[Monnte]

@acalcutt It is sad for me that you didn't see the problem with -u|--public_url that is not works with absolute URL ... It seems it was implemented but not tested if it actually works in the end :)

You can try to use is somewhere, and then you will get it...

I would rather fork the repository and adjust it there it and built it myself, because my message here #1472 (comment) seems pointless in the end 😄

AND MAINLY, IF YOU DONT DEFINE THE -u FLAG THEN THE SAME APPROACH IS USED AS I IMPLEMENTED FOR RELATIVE PATH WHEN USING -u FLAG :)

[acalcutt]

Unfortunately I don't have the time right now to set up and environment to test this manually. I don't really like making changes to publicurl because it is not something I use and people are always trying to use it to shoehorn tileserver into different scenarios, so I really can't keep up with what people are using it for an how it would be affected by a change like this.

Possibly if this gets some other reviews, requests, and testers this can be merged, but right now I am not sure #1472 (comment) explains why that port I mentioned is there and why the test should change.

And yes, this feature was probably not tested well before getting merged for the same reason. I don't have a good environment to test these scenarios and make sure this works.

[acalcutt]

@Monnte What is your opinion on the competing PR for this #1464 . Yours seems much simpler at least. right now it looks like the other PR has a similar test failure to what this PR had before the test change.

@maintux-nn could you possibly review this and tell me how it compares to your PR?

[Monnte]

@acalcutt Yep, it seems like the same problem we want to fix, hopefully @maintux-nn will answer as well 👍

[maintux-nn]

Hi @Monnte @acalcutt, apologies for the delayed response. I’ve reviewed this version and it works well. Plus, it’s simpler than mine. I’m happy to go with this one and set mine aside.

Thanks