feat: add simpler etag handling to all non-critical endpoints#1836
feat: add simpler etag handling to all non-critical endpoints#1836CommanderStorm merged 32 commits intomaplibre:mainfrom
Conversation
|
I don't super like the importing part as this makes |
CommanderStorm
changed the title
nyurik
left a comment
nyurik
left a comment
There was a problem hiding this comment.
there is a minor nit, and feel free to merge after addressing it
I think a really simple test for this is still necessary. Maybe next PR. |
I added a testcase for this. Whas what I added what you thought of? |
| echo "Testing $(basename "$FILENAME") from $URL" | ||
| # jq before 1.6 had a different float->int behavior, so trying to make it consistent in all | ||
| $CURL "$URL" | jq --sort-keys -e 'walk(if type == "number" then .+0.0 else . end)' > "$FILENAME" | ||
| $CURL --dump-header "$FILENAME.headers" "$URL" | jq --sort-keys -e 'walk(if type == "number" then .+0.0 else . end)' > "$FILENAME" |
There was a problem hiding this comment.
Maybe do a test like we did under the martin/tests, and sure, it's good to test it in test.sh either.
But dump the header for each request it's not a good idea I guess...
There was a problem hiding this comment.
at first I also thought headers might be too flaky, but than it seems headers only have the relatively stable things... with the possible exception of the length? i.e. in theory we could have a slightly different compression between different brotli implementation versions? So lets add it, and if we ever run into issues with the test flakiness, we could filter the header files to remove unstable headers?
There was a problem hiding this comment.
responding to this explicitely:
- If length changes, then the etag changes as well.
- If encoding changes, then the etag and length changes as well.
We cannot have both tests that
- ensure that the etag does not change and
- not check that the etag changes.
But since we have PRs for this kind of dependency changes, this should be fine.
This is not like the build in main being broken by new cippy lints every few months.
nyurik
left a comment
nyurik
left a comment
There was a problem hiding this comment.
i think this is good to go, thanks!
|
Nice |
3 participants
This PR adds etag handling via an actix wrap instruction to all "non-tile" endpoints.
I ommitted some endpoints which I suspect are not called more than once such as
/if the webui is not compiled in.The reason for not being smarter for some is that honestly, the impact of this is less than I expected based on benchmarks in #1834 and that
actix-middleware-etaguses the same hash algorithm.I have not addded tests for this as frankly, this does not seem like a feature that needs deep tests.By request, this PR now includes all stemi-static headers.