-
Notifications
You must be signed in to change notification settings - Fork 646
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.
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
Upgrade Express to v5 + code cleanup #1429
base: master
Are you sure you want to change the base?
Conversation
Co-Authored-By: Andrew Calcutt <[email protected]>
Co-Authored-By: Andrew Calcutt <[email protected]>
Co-Authored-By: Andrew Calcutt <[email protected]>
Co-Authored-By: Andrew Calcutt <[email protected]>
Co-Authored-By: Andrew Calcutt <[email protected]>
Co-Authored-By: Andrew Calcutt <[email protected]>
Co-Authored-By: Andrew Calcutt <[email protected]>
Co-Authored-By: Andrew Calcutt <[email protected]>
Co-Authored-By: Andrew Calcutt <[email protected]>
Co-Authored-By: Andrew Calcutt <[email protected]>
Co-Authored-By: Andrew Calcutt <[email protected]>
Co-Authored-By: Andrew Calcutt <[email protected]>
Co-Authored-By: Andrew Calcutt <[email protected]>
Co-Authored-By: Andrew Calcutt <[email protected]>
Co-Authored-By: Andrew Calcutt <[email protected]>
Co-Authored-By: Andrew Calcutt <[email protected]>
Co-Authored-By: Andrew Calcutt <[email protected]>
Co-Authored-By: Andrew Calcutt <[email protected]>
Co-Authored-By: Andrew Calcutt <[email protected]>
Co-Authored-By: Andrew Calcutt <[email protected]>
Co-Authored-By: Andrew Calcutt <[email protected]>
Co-Authored-By: Andrew Calcutt <[email protected]>
Co-Authored-By: Andrew Calcutt <[email protected]>
Co-Authored-By: Andrew Calcutt <[email protected]>
I just had a short look at the code changes; as far as I have seen it looks OK so far. I build the image and used it with my test config/data and can see a lot of "Handling request" messages on the application console and for some styles I have errors regarding fonts in the browser console. I would like to take a deeper look at that and test verbose logging a bit. May be I can dig out some older scripts, which use the static endpoint, too. In addition I would like to apply my elevation fix to this PR and test again. I hope I will have time in the next few days. |
Do you have verbose enabled in your test config? You should only see the handling messages when verbose is enabled. I added more logging in verbose mode to help troubleshoot this. |
One concert I have is with fonts. I added a lot of validation there to make codeql happy, but i'm not sure they are all good changes. |
I re-checked everything:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I applied and adjusted changes of #1432 in another branch.
Except the coordinates of fetchTileData (which is fixed in the new branch, too) this PR works for me.
src/serve_data.js
Outdated
const intX = parseInt(req.params.x, 10); | ||
const intY = parseInt(req.params.y, 10); | ||
|
||
const fetchTile = await fetchTileData(source, sourceType, z, x, y); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use const fetchTile = await fetchTileData(source, sourceType, z, xy[0], xy[1]);
instead. This was invalid for pmtiles before.
This PR addresses #1412 by upgrade express to v5. It also tries to address #1411 by adding Try/Catch around that function and also adding better verbose logging.
express v5 features a new version of 'path-to-regexp', which changes the syntax to no longer allow optional parameters by surrounding them with ( and )? and putting the type inside ( and ). see https://github.com/pillarjs/path-to-regexp?tab=readme-ov-file#errors
In tileserver-gl we had a lot of spots we were using optional parameters and defining types by regex values, which because of this change of 'path-to-regexp' used by express, are no longer supported
This PR tries to address this change by doing the following
1.) All optional parameters have been changed from the format (/:spriteID)?, to the new format {/:spriteID}
2.) All validation has been removed from the enpoints, For example, :range([\d]+-[\d]+) became :range
3.) Because validation was removed from the endpoints, some of our paths under /styles/ became too similar. I had to merge these into a single path and add in some logic to direct the request the right way.
4.) Also because validation was removed I tried to add in some logic for accepted values for things like tileSize and Scale
5.) The syntax we used in ServeTemplate also has to change. the '$' was no longer supported in the path, so it had to be removed. This lead to a need to use next('route') to redirect request not served by the template.
Other changes.
1.) Tried to sanitize outputs. I still wanted some information in the logs, so it tries to at least sanitize the data to acceptable characters. codeql seems to still not like this.
2.) Tried to change variable style functions to traditional functions for better logging information
3.) Added generated function headers so the code is more informative.
4.) Some status codes have change. for example 400 vs 404 when bad data is provided (test had to be updated for this). Also, when a tile is not found it now returns a 204 No Content to try and allow sparse tile suport.