Fix Broken rendering - No handling of local GeoJSON files #1324 #1326

okimiko · 2024-07-20T14:22:36Z

Add general missing handling of local geojson files during rendering (#1324).

acalcutt · 2024-07-21T10:33:07Z

I'm not sure I would want to take this approach. I kind of liked you other idea of adding a "file://" protocol, where you could set up path for "file" paths like we do for others paths https://tileserver.readthedocs.io/en/latest/config.html#paths

For that to work we would need a path set up similar to
https://github.com/maptiler/tileserver-gl/blob/master/src/server.js#L94
and then we would set up serve_rendered.js to handle the file:// protocol.
In your style config you would then prefix your geojson files with file://, similar to how mbtiles:// or pmtiles://

okimiko · 2024-07-23T13:38:19Z

Yes, that behaviour would have been a bit hacky, because it would require a matching environment to work in rendered and vector mode.

I extended the code a bit to support a file:// urls, but stumpled over the spec a bit: geojson uses "data" instead of "url" because it seems to be inteded, to write inline geojson.

In the current implementation it may be a change with a "bigger" impact, because with /files/ the public interface is extended, but this works without any need of a reverse proxy in both modes.

src/serve_rendered.js

acalcutt · 2024-07-23T21:58:11Z

I like how this looks better now and will try to test it soon.

One thing I was thinking about was an issue we have with the 'icons' path we added. Right now, if you don't specify a path for that it defaults to the tileserver-gl root directory, and in that case allows the user to try and load files in the root directory that are not images and causes a crash. In my own instance I specifically set it to the folder public/resources/images which avoids that issue, but really it should be hardcoded to a path like that by default, not the root folder like we use now.

I was wondering if we should do the same thing with 'files' to prevent inadvertently exposing all source files and maybe make a default folder specifically for that like public/resources/files and use that as the default path if one doesn't exists. If it was specified in the config it would use the alternative path given, but not set it would default to public/resources/files (or possibly just files would be fine)

acalcutt · 2024-07-23T22:12:24Z

For the comment on geojson sources, it seems to accect inline code or a url
https://maplibre.org/maplibre-style-spec/sources/#geojson

In my own project I have used dynamically generated geojson sources by url on pages like
https://wifidb.net/wifidb/opt/map.php?func=user_list&labeled=0&id=65596&sig_label=none

I guess the only question is does maplibre-native seem to work with this new file:// protocol ? the handler for file:// has been added so I assume it should

src/serve_rendered.js

acalcutt · 2024-07-24T00:03:10Z

I am testing locally with this config

test_data_geojson.zip

on windows I am starting this with
node . -c C:\Users\andrew.EIRI\Desktop\test_data_geojson\config.json

vector map

raster map

Note, I did make a few changes locally. I changed 'file://' to 'files://' and updated anywhere in the code that just used 'file' to 'files'. This was for consistency with the other protocol and to get it working. I also made the other path change I commented above.

acalcutt · 2024-07-24T00:07:01Z

One thing to note is depending on your geojson file, this is likely going to affect performance in raster tiles because every tile it makes is basically loading the full geojson file. Were if instead you did like I mentioned before and converted your geojson to mbtiles/pmtiles vector files with tippecanoe, it would only need to download the region you are looking at and would be faster, since tippcannoe has already broken your data down into tiles.

src/serve_rendered.js

src/serve_style.js

docs/config.rst

src/main.js

okimiko · 2024-07-24T08:34:30Z

One thing to note is depending on your geojson file, this is likely going to affect performance in raster tiles because every tile it makes is basically loading the full geojson file. Were if instead you did like I mentioned before and converted your geojson to mbtiles/pmtiles vector files with tippecanoe, it would only need to download the region you are looking at and would be faster, since tippcannoe has already broken your data down into tiles.

Good point! For my current files it seems not to be a problem, but that should be added as note/kept in mind.

I will apply some of the above changes later.

Edit: I added the changes and tested my setup, maybe you can test on Windows again?

src/serve_rendered.js

okimiko · 2024-08-02T22:36:58Z

Merged latest changes (and applied changes due to fix: avoid synchronous calls where possible #1320).
Tested (and fixed) handling of inline geojson

acalcutt · 2024-08-10T03:03:24Z

Dockerfile

@@ -107,8 +107,8 @@ COPY --from=builder /usr/src/app /usr/src/app

 COPY . /usr/src/app

-RUN mkdir -p /data && chown node:node /data
-VOLUME /data
+RUN mkdir -p /data /usr/src/app/public/files && chown node:node /data /usr/src/app/public/files


the only thing I don't really like is making this folder with the dockerfile . since tileserver-gl can run directly without docker, it would need that folder there in that use case also. So maybe we should either make the directory with a default file inside, of maybe find a way to create it with the package.json npm prepare..

I added the folder to the repository (and excluded .gitignore files for docker build).

sounds good to me. this can be removed now right?

I meant can we remove the changes in this file. Since the folder exists we don't need to create it here anymore.

Sorry, I was a bit confused.

In fact the mkdir for public/files may be removed, but due to the '-p' it does not hurt and ensures, that the path, which will be chown'ed and exposed as volume is really there.
If I remove/change it, I would revert the whole file, because the volume depends on the configuration and everything will still work with the default configuration, because the mount may be applied without a exposed volume of an empty directory. (This may be different, if there was a default config file included in the image.)

In short: It's up to you ;-)

I would rather keep it as it was before since it shouldn't really be needed now,

src/server.js

okimiko · 2024-08-12T21:01:42Z

@acalcutt I merged the latest changes, applied the prettier fixes and finally updated the tests to be executed in the main directory, so no further stubbing should be required.

src/serve_rendered.js

okimiko · 2024-08-26T08:40:54Z

@acalcutt: I tidied up a little bit (merge master, lint:*:fix, squash - okimiko@222d6c1). If you want to merge, this may be a bit cleaner than this here. If wanted, I can force push it or we close this here and I create a new, clean PR.

acalcutt · 2024-08-27T01:18:16Z

src/serve_rendered.js

@@ -893,13 +893,13 @@ export const serve_rendered = {
            // console.log('Handling request:', req);
            if (protocol === 'sprites') {
              const dir = options.paths[protocol];
-              const file = unescape(req.url).substring(protocol.length + 3);
+              const file = decodeURI(req.url).substring(protocol.length + 3).replaceAll('%2c', ',');


What is this replacement added for?
replaceAll('%2c', ',')

Sorry, I did not check the code before. Correct would be to use decodeURIComponent as counterpart of encodeURIComponent.
decodeURI / encodeURI does not escape commas, so I added the extra replace after using a style with two fonts configured and receiving ERROR: Font not found: Noto Sans Regular%2cNoto Sans Bold.

I will fix that later today an force push the squashed commit.

acalcutt · 2024-08-27T01:19:46Z

force push or a new PR would be fine with me

- use 'file://' as indicator for local files - add directory as default directory - serve local files at - add documentation for static file serving - add some minor fixes (icon directory, directory checking, decodeURIComponent, extend error message)

okimiko marked this pull request as ready for review July 23, 2024 15:20

okimiko force-pushed the master branch from c3341bf to fc2a697 Compare July 23, 2024 15:21

github-advanced-security bot found potential problems Jul 23, 2024

View reviewed changes

src/serve_rendered.js Fixed Show resolved Hide resolved

acalcutt reviewed Jul 23, 2024

View reviewed changes

src/serve_rendered.js Outdated Show resolved Hide resolved

acalcutt reviewed Jul 24, 2024

View reviewed changes

src/serve_rendered.js Show resolved Hide resolved

acalcutt reviewed Jul 24, 2024

View reviewed changes

src/serve_style.js Outdated Show resolved Hide resolved

acalcutt reviewed Jul 24, 2024

View reviewed changes

docs/config.rst Outdated Show resolved Hide resolved

acalcutt reviewed Jul 24, 2024

View reviewed changes

src/main.js Outdated Show resolved Hide resolved

github-advanced-security bot found potential problems Jul 24, 2024

View reviewed changes

src/serve_rendered.js Fixed Show fixed Hide fixed

okimiko requested a review from acalcutt August 2, 2024 22:37

acalcutt reviewed Aug 10, 2024

View reviewed changes

src/server.js Show resolved Hide resolved

github-advanced-security bot found potential problems Aug 12, 2024

View reviewed changes

src/serve_rendered.js Dismissed Show dismissed Hide dismissed

acalcutt approved these changes Aug 14, 2024

View reviewed changes

github-advanced-security bot found potential problems Aug 20, 2024

View reviewed changes

src/serve_rendered.js Fixed Show fixed Hide fixed

src/serve_rendered.js Fixed Show fixed Hide fixed

acalcutt reviewed Aug 27, 2024

View reviewed changes

okimiko force-pushed the master branch from e5e2dee to 16b0626 Compare August 27, 2024 14:35

Merge branch 'master' into master

ecd7456

Update .gitignore

5d459b5

acalcutt merged commit 44cf365 into maptiler:master Sep 1, 2024
6 checks passed

acalcutt added a commit that referenced this pull request Sep 1, 2024

Fix: Revert test chdir from #1326

098b18e

okimiko mentioned this pull request Sep 1, 2024

Broken rendering - No handling of local GeoJSON files #1324

Closed

acalcutt mentioned this pull request Sep 2, 2024

Fix: Use blank index to make sure 'public/files/' folder exists in npm package #1377

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Broken rendering - No handling of local GeoJSON files #1324 #1326

Fix Broken rendering - No handling of local GeoJSON files #1324 #1326

okimiko commented Jul 20, 2024 •

edited

Loading

acalcutt commented Jul 21, 2024

okimiko commented Jul 23, 2024

acalcutt commented Jul 23, 2024 •

edited

Loading

acalcutt commented Jul 23, 2024 •

edited

Loading

acalcutt commented Jul 24, 2024 •

edited

Loading

acalcutt commented Jul 24, 2024 •

edited

Loading

okimiko commented Jul 24, 2024 •

edited

Loading

okimiko commented Aug 2, 2024

acalcutt Aug 10, 2024

okimiko Aug 10, 2024

acalcutt Aug 10, 2024 •

edited

Loading

acalcutt Aug 10, 2024

okimiko Aug 10, 2024

acalcutt Aug 10, 2024 •

edited

Loading

okimiko Aug 10, 2024

okimiko commented Aug 12, 2024

okimiko commented Aug 26, 2024

acalcutt Aug 27, 2024 •

edited

Loading

okimiko Aug 27, 2024

acalcutt commented Aug 27, 2024

Fix Broken rendering - No handling of local GeoJSON files #1324 #1326

Fix Broken rendering - No handling of local GeoJSON files #1324 #1326

Conversation

okimiko commented Jul 20, 2024 • edited Loading

acalcutt commented Jul 21, 2024

okimiko commented Jul 23, 2024

acalcutt commented Jul 23, 2024 • edited Loading

acalcutt commented Jul 23, 2024 • edited Loading

acalcutt commented Jul 24, 2024 • edited Loading

acalcutt commented Jul 24, 2024 • edited Loading

okimiko commented Jul 24, 2024 • edited Loading

okimiko commented Aug 2, 2024

acalcutt Aug 10, 2024

Choose a reason for hiding this comment

okimiko Aug 10, 2024

Choose a reason for hiding this comment

acalcutt Aug 10, 2024 • edited Loading

Choose a reason for hiding this comment

acalcutt Aug 10, 2024

Choose a reason for hiding this comment

okimiko Aug 10, 2024

Choose a reason for hiding this comment

acalcutt Aug 10, 2024 • edited Loading

Choose a reason for hiding this comment

okimiko Aug 10, 2024

Choose a reason for hiding this comment

okimiko commented Aug 12, 2024

okimiko commented Aug 26, 2024

acalcutt Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

okimiko Aug 27, 2024

Choose a reason for hiding this comment

acalcutt commented Aug 27, 2024

okimiko commented Jul 20, 2024 •

edited

Loading

acalcutt commented Jul 23, 2024 •

edited

Loading

acalcutt commented Jul 23, 2024 •

edited

Loading

acalcutt commented Jul 24, 2024 •

edited

Loading

acalcutt commented Jul 24, 2024 •

edited

Loading

okimiko commented Jul 24, 2024 •

edited

Loading

acalcutt Aug 10, 2024 •

edited

Loading

acalcutt Aug 10, 2024 •

edited

Loading

acalcutt Aug 27, 2024 •

edited

Loading