Fix calculation of the resource part in "files" URIs not featuring trailing slash
#259
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…trailing slash Without the `/` at the end of e.g. `/v1/<tenant>/files/export`, the value of the `resource` attribute in preparation for handling the request with `FileRequestHandler`, becomes identical to the URI. While not critical with current code, the condition does then cascade into additional computation in e.g. `any_path_islink` (and potentially other places that depend on the value of the `resource` attribute), and there's been cases of `any_path_islink` raising errors which would abort the request. Again, it's a matter of interpretation whether such requests where `any_path_islink` aborts, should be aborted since no listing would succeed for these anyway, but in any case I thought the `resource` attribute should be computed "correctly". The latter in quotes because I can't be sure there's not intention for the attribute to ever have valid values like `v1/<tenant>/files/export`, but what this change has going for it is that `resource` value will be the same between URIs that only differ by presence of the terminating slash. And if `resource` should bear empty string value for `/v1/<tenant/files/export` URIs _without_ the trailing slash, then one could argue it shouldn't in the very least be `/v1/<tenant>/files/export` for URIs _with_ the trailing slash, an assertion which holds for this change. This correction should also prove beneficial longer term -- with the rest of the code that may potentially depend on "sane" values of `resource`. Of course all this hinges on allowing URIs like `.../export` _without_ the trailing slash, but [current documentation lists these explicitly](http://github.com/unioslo/tsd-api-docs/blob/master/integration/file-api.md#simple-file-download), so I assume they are indeed allowed. An alternative is adding the trailing slash with e.g. a reverse proxy ahead of the API, so the API can simply assume the trailing slash. But since the API is more of an authority on the semantics and category of the URIs that, after all, originate with it, I thought it's more fitting to be handling this in the API and not in a reverse proxy.
amn
force-pushed
the
fix-files-uri-resource-part-calc
branch
from
9ecc154
to
9c5c336
Compare
|
(Pushed the signed equivalent of the commit, the one pushed first wasn't signed because it was created from a patch I mailed myself since I am at work, aka "reasons") |
haatveit
approved these changes
Oct 14, 2024
|
Would it make sense to add a test for this case? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Without the
/at the end of e.g./v1/<tenant>/files/export, the value of theresourceattribute in preparation for handling the request withFileRequestHandler, becomes identical to the URI. While not critical with current code, the condition does then cascade into additional computation in e.g.any_path_islink(and potentially other places that depend on the value of theresourceattribute), and there's been cases ofany_path_islinkraising errors which would abort the request. Again, it's a matter of interpretation whether such requests whereany_path_islinkaborts, should be aborted since no listing would succeed for these anyway, but in any case I thought theresourceattribute should be computed "correctly". The latter in quotes because I can't be sure there's not intention for the attribute to ever have valid values likev1/<tenant>/files/export, but what this change has going for it is thatresourcevalue will be the same between URIs that only differ by presence of the terminating slash. And ifresourceshould bear empty string value for/v1/<tenant/files/export/URIs, then one could argue it should also hold the same value for/v1/<tenant>/files/export(no trailing slash), an assertion which will hold with this change.This correction should also prove beneficial longer term -- with the rest of the code that may potentially depend on "sane" values of
resource.Of course all this hinges on allowing URIs like
.../exportwithout the trailing slash, but current documentation lists these explicitly, so I assume they are indeed allowed.An alternative is adding the trailing slash with e.g. a reverse proxy ahead of the API, so the API can simply assume the trailing slash. But since the API is more of an authority on the semantics and category of the URIs that, after all, originate with it, I thought it's more fitting to be handling this in the API and not in a reverse proxy.
Should fix #256.