Skip to content
This repository was archived by the owner on Feb 9, 2024. It is now read-only.

feat: fire mapRequestToAsset for all requests if explicitly defined #159

Merged
merged 1 commit into from
May 17, 2021
Merged

feat: fire mapRequestToAsset for all requests if explicitly defined #159

merged 1 commit into from
May 17, 2021

Conversation

Cherry
Copy link
Contributor

@Cherry Cherry commented Dec 7, 2020

This PR re-adds the functionality described in #158.

As per the linked issue, this did used to be possible in very early versions of kv-asset-handler, but was broken in a recent update that added support for encoded paths and things. This was technically a breaking change, but it seems that no one else noticed. 😅

I looked at the tests, but didn't feel that adding this to there would serve much benefit, since these are only mocking the most simple of use-cases, and there wouldn't be a real way to "test" the actual functionality. If you've got any suggestions here, let me know. I did leave a few code comments to hopefully prevent this from becoming a regression again the future.

Technically, this once again is a breaking change, as previously the mapRequestToAsset function was ignored if you set it and an exact match was found in the ASSET_MANIFEST. I'd consider this a bug and unexpected behaviour though, and this PR fixes that.

Closes #158

@Cherry
feat: fire mapRequestToAsset for all requests if explicitly defined
8e33241 
This restores the ability to run `mapRequestToAsset` on any requests, even if that specific asset already exists in the `ASSET_MANIFEST`.

So for example if you had an `image.png`, `image.webp` and `image.avif` in your manifest, and wanted to dynamically serve the `webp` or `avif` to supported clients when they requested the `image.png`, there's was no previously no simple way to do this. This commit restores this functionality and will always call a user defined `mapRequestToAsset` if set.

See cloudflare#158 for more info
@famzah
Copy link

famzah commented Jan 18, 2021

I will also appreciate it if this PR gets merged. Besides the use case described in #158, here is mine:

I want to keep my files with ".html" extension on my computer and that's how they end up in the assets manifest. However, I want the URLs to be without ".html" at the end. Rewriting "/test" to "/test.html" is easy because "test" doesn't exist in the assets manifest and my custom mapRequestToAsset is called.

However, how do we deny access to the original name "/test.html"? Right now (with the incorrect behavior) the request is directly served, because "test.html" exists in the assets manifest. My custom mapRequestToAsset is never called and I can't effectively deny the access to this file.

If you merge the PR and fix this, then I have some options:

  • Any direct requests for "test.html" can be rewritten to "non-existing-ever.html" which will trigger a NotFoundError by ASSET_NAMESPACE.get()
  • When I see a URL ending in ".html" I can directly serve a 404 response. Right now, that's impossible because getAssetFromKV accepts the event as first argument and the original request "test.html" is there and being served because it exists in the assets manifest. If this bug was fixed, then I can call getAssetFromKV with a crafted custom mapRequestToAsset which always returns the "404.html" page.

@famzah
Copy link

famzah commented Jan 19, 2021

In order to have maximum flexibility, I'd also add a context object as second argument to mapRequestToAsset which will give us access to important data about the request, and we can also easily add more properties later. I'd put the following properties in context:

  • ASSET_MANIFEST: the value that we already parsed and use in getAssetFromKV
  • ASSET_NAMESPACE: maybe someone could need it
  • rawPathKey: to save the effort (and bugs) of everybody to strip any preceding /'s

@caass caass added this to the 0.1.2 milestone Mar 15, 2021
@Cherry Cherry mentioned this pull request Mar 27, 2021
Closed
@kristianfreeman kristianfreeman merged commit e151f94 into cloudflare:master May 17, 2021
@Cherry Cherry mentioned this pull request May 25, 2021
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kristianfreeman kristianfreeman kristianfreeman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.1.2
Development

Successfully merging this pull request may close these issues.

Serve different assets any given requestKey, even if those are in the ASSET_MANIFEST
4 participants
@Cherry @famzah @kristianfreeman @caass