TileJSON 3.0 support with layer fields discovery and cached /capabilities responses#1071
TileJSON 3.0 support with layer fields discovery and cached /capabilities responses#1071Rub21 wants to merge 15 commits into
Conversation
iwpnd
left a comment
There was a problem hiding this comment.
Hey. This is an update that is well overdue. The implementation is alright. The LayerFielder makes sense. Yet incrementing the version to 3.0.0 is an increment to all providers alike even though they do not (yet) implement the LayerFielder. This in turn makes a capabilities.json from any other provider but the postgis provider an invalid TileJSON v3.
The handle_map_capabilities.go should therefor determine whether the provider implements the LayerFielder and if it doesn't yet, it should respond with a valid TileJSON v2.
This PR lacks some tests. Take a look into the postgis_test.go and snoop around in the testdata we setup locally. It should be straight forward to write a small test that covers the creation of the TileJSON v2 and v3 in handle_map_capabilities_test.go.
All in all, good work. Some nits, some questions, some potential redundancy and the question how to handle providers that do not yet impl a LayerFielder.
Co-authored-by: Ben <iwpnd@users.noreply.github.com>
Co-authored-by: Ben <iwpnd@users.noreply.github.com>
Co-authored-by: Ben <iwpnd@users.noreply.github.com>
Co-authored-by: Ben <iwpnd@users.noreply.github.com>
Co-authored-by: Ben <iwpnd@users.noreply.github.com>
Co-authored-by: Ben <iwpnd@users.noreply.github.com>
* Add testing for TileJSON 3.0.0 * Updates for testing
|
Thanks for taking the time to review this PR and for the helpful suggestions. https://vtiles.staging.openhistoricalmap.org/capabilities |
|
@Rub21 thank you! i still think that the rwmutex will become a bottleneck in high concurrency environments and I have an idea how to address this. if you're in a rush you can keep your fork deployed, as I will not touch on functionality. i will however need some time to work on this and am unsure when I can get to it - this weekend maybe. |
That would be great, Thank you!! |
|
@Rub21 thanks for tackling the code review comments. I'm traveling and wont get a chance to look at this until next week. Does look like govulncheck is failing too. Looks like we need to update go to 1.25.6. I will get this done. |
|
@Rub21 can you please verify that you enabled us to commit to your branch? See here. I simplified the caching from a dual-map + The main fix was versioning logic however, and looking into this in more detail I want to propose a different approach here. We were using TileJSON 3.0.0 if any provider supported field metadata. However if you mix providers, this is causing inconsistent Then I added error handling so failed builds don't get cached permanently - if When I was working through the TileJSON creation itself I was struggling to read the logic, tbh. I added a helper to find layers by ID - and removed that nested loop with the skip flag, and replaced the I have my changes tucked away in a separate commit, that I could contribute here if you give me the permission. |
|
@Rub21 bumping this once more to your attention. |
|
@iwpnd Sorry for the delay. I was a bit stuck on a security issue with the project, but I've gotten back on track this week. I've just sent you an invitation to the repo, so you can commit to my branch. Since the PR was submitted from an organization account, works different i think , you should already have access to the repo and be able to commit. https://github.com/OpenHistoricalMap/tegola, thanks! |
|
@Rub21 @ARolek I added my changes, see previous comment. |
Nice! Sorry I'm running a bit behind on reviews. Will get to it as soon as I can. Thanks for pushing for this one as it will be a great addition! |
gdey
left a comment
There was a problem hiding this comment.
Overall this looks great to me.
I would like to have a discussion on the way map capabilities determines the TileJSON version. I like what we have, but think there should be a simple way for one to upgrade from 2.0 to 3.0, by setting the option and having Tegola let's us know which Layers need to what fields to be implemented.
| // determine TileJSON version based on whether ALL providers implement LayerFielder. | ||
| // we only use TileJSON 3.0.0 if every provider can supply field metadata to ensure | ||
| // consistent capabilities across all layers. If a provider lacks LayerFielder support, | ||
| // we fall back to TileJSON 2.0.0 to maintain predictable behavior for clients. | ||
| // this prevents mixed scenarios where some layers have field metadata and others don't, | ||
| // which would make it difficult for clients to reliably work with field information. | ||
| // NOTE: to be discussed |
There was a problem hiding this comment.
I like this, but I think it would be good to have an option that tells us we want 3.0 TileJSON; if that options is set, we should error out pointing out the layers that don't support the LayerFielder; or maybe have another way. But, I do like the this as a fall back, and should be the default behavior.
There was a problem hiding this comment.
Do you imagine this to provide a warning, but still start tegola with v3 or do you want tegola to err on startup when there's layers that do not yet support v3?
There was a problem hiding this comment.
I think if the option is set that we want 3.0 TileJSON, then it should err on startup; and point out which layers don't support the LayerFielder. If the option is not set then it should use the fallback methods that's in this PR. What do you think?
|
@ARolek let's gooo! |
Update TileJSON version to 3.0.0 and adds the required fields property to VectorLayer.
cc. @1ec5