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

Check HTTP Method after mapRequestToAsset #151

Closed
oliverpool opened this issue Sep 30, 2020 · 3 comments · Fixed by #178
Closed

Check HTTP Method after mapRequestToAsset #151

oliverpool opened this issue Sep 30, 2020 · 3 comments · Fixed by #178
Labels
bug Something isn't working
Milestone
0.1.2

Comments

@oliverpool
Copy link
Contributor

oliverpool commented Sep 30, 2020 •
edited
Loading

Currently in getAssetFromKV the HTTP Method is checked before the call to mapRequestToAsset:

kv-asset-handler/src/index.ts

Lines 96 to 111 in 3df430c

const SUPPORTED_METHODS = ['GET', 'HEAD']
if (!SUPPORTED_METHODS.includes(request.method)) {
throw new MethodNotAllowedError(`${request.method} is not a valid request method`)
}
const rawPathKey = new URL(request.url).pathname.replace(/^\/+/, '') // strip any preceding /'s
let pathIsEncoded = false
let requestKey
if (ASSET_MANIFEST[rawPathKey]) {
requestKey = request
} else if (ASSET_MANIFEST[decodeURIComponent(rawPathKey)]) {
pathIsEncoded = true;
requestKey = request
} else {
requestKey = options.mapRequestToAsset(request)
}

I suggest to check the HTTP method after this.

In my case, I want to send an e-mail on a webhook (which is a POST call from an external service - payment provider for instance).
I store my email template along my static site, so that it is uploaded to the KV store.
I want to retrieve the template using getAssetFromKV, (replacing the request path using mapRequestToAsset), however it fails because the method check is done before this.

I currently have an ugly hack, creating a new pseudo-event with a fresh GET request...

I don't see any drawback to moving this check after the if block (mapRequestToAsset is not supposed to have any side-effect, so calling it before failing should not be a big deal9.

I would be happy to make a PR if you think this is not a bad idea.
@aqueenan
Copy link

aqueenan commented Oct 1, 2020 •
edited
Loading

@oliverpool I ran into the same problem and agree with your suggested solution.

For anyone experiencing the same problem, follow @oliverpool's workaround below, it's better than mine because it allows the response to be cached.

async function getResponseFromKV(event, url) {
  let options = {
    mapRequestToAsset: req => new Request(url),
    cacheControl: {
      bypassCache: DEBUG,
    }
  }

  if (event.request.method === 'POST') {
    options.cacheControl.bypassCache = true // Prevents event.waitUntil() from being called
    event = {
      request: new Request(url)
    }
  }

  return await getAssetFromKV(event, options)
}

@oliverpool
Copy link
Contributor Author

@aqueenan thanks for sharing your hack, here is mine :)

async function getAsset(event, uri) {
  const ev = Object.assign(
    {
      waitUntil: p => {
        event.waitUntil(p)
      },
    },
    event,
    {
      request: new Request(`${new URL(event.request.url).origin}/${uri}`),
    },
  )

  return getAssetFromKV(ev)
}

@caass caass added this to the 0.1.2 milestone Mar 15, 2021
@kristianfreeman kristianfreeman added the bug Something isn't working label May 17, 2021
@kristianfreeman
Copy link
Contributor

Great convo here, can someone open a PR to fix this?

oliverpool added a commit to oliverpool/kv-asset-handler that referenced this issue May 18, 2021
@oliverpool
Check HTTP Method after mapRequestToAsset
6c0dd6d 
Closes cloudflare#151
@oliverpool oliverpool mentioned this issue May 18, 2021
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
0.1.2
Development

Successfully merging a pull request may close this issue.

Check HTTP Method after mapRequestToAsset oliverpool/kv-asset-handler
4 participants
@kristianfreeman @oliverpool @caass @aqueenan