Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support HTTP POST requests #44

Merged
merged 2 commits into from
Aug 1, 2021

Conversation

corneliusludmann
Copy link
Contributor

We at Gitpod would like to use this great Caddy plugin for caching HTTP POST requests. Unfortunately, only GET and HEAD requests are supported yet. This PR adds the possibility to configure this plugin to cache HTTP POST requests as well.

This PR provides 2 commits:

  • The first commit allows to configure which HTTP methods are cached.
  • The second commit adds two new variables to the cache key template: http.request.contentlength and http.request.bodyhash.

For caching POST requests it's important to distinguish requests with different body contents. The second commit adds http.request.contentlength and http.request.bodyhash. For the body hash it's important that the cache key is calculated before the body has been read (before it's empty). Therefore the key is passed to fetchUpstream.

We would be happy if you would accept this change and thus expand the possibilities of the use of this plugin. Please let me know if you need anything else from me.

For caching POST requests it's important to distinguish requests between different body contents. This commit adds `http.request.contentlength` and `http.request.bodyhash`. For the body hash it's important that the cache key is calculated before the body has been read (before it's empty). Therefore the key is passed to `fetchUpstream`.
@sillygod
Copy link
Owner

@corneliusludmann hi,

This is an interesting design. However, I don't know whether it's appropriate to cache the HTTP request with the post method or not.

I found the part of rfc

Responses to this method are not cacheable unless the response includes appropriate Cache-Control or Expires header fields. However,
the 303 (See Other) response can be used to direct the user agent to
retrieve a cacheable resource.

It seems like the post method can be cached under the specified HTTP headers.

well, another rfc 7231 mentions

this specification defines GET, HEAD, and POST as
cacheable, although the overwhelming majority of cache
implementations only support GET and HEAD.

So I think it's ok to add this feature.

I will take some time to review this one. Thanks for this great work!

response.go Show resolved Hide resolved
Copy link
Owner

@sillygod sillygod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

response.go Show resolved Hide resolved
@sillygod sillygod merged commit a00fa59 into sillygod:master Aug 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants