pquerna / cachecontrol Goto Github PK

Heya,

I was wondering if there was some functionality which allows reversing the parse process to generate a Cache-Control string value from a ResponseCacheDirectives struct?

My usecase is that I would like to act as a proxy, parse the response header coming from my downstream service, check if the max-age is set, and if the value is unset or too low I will modify it before proxying it to the user.

I ended up just doing this:

// enforce a default CacheControl max-age value if:
// 	1. A default value is configured for this endpoint.
// 	2. The existing 'Cache-Control' header is unset or 'public'.
// 	3. The existing 'max-age' value is unset or lower than the default.
if e.DefaultAge > 0 {
	policy, _ := cacheobject.ParseResponseCacheControl(resp.Header.Get("Cache-Control"))
	if policy.Public && int(policy.MaxAge) < e.DefaultAge {
		// policy.MaxAge = cacheobject.DeltaSeconds(e.DefaultAge)
		resp.Header.Set("Cache-Control", fmt.Sprintf("public, max-age:%d", e.DefaultAge))
	}
}

I was hoping to find something more elegant than the fmt.Sprintf("public, max-age:%d", e.DefaultAge) line because presumably it's more complex than just ignoring the other properties?

some browser can not work well in the case of this caching

The Vary header can change the cacheability of a response. This library could add some helpers to account for it. A dev would probably want to create a cache key based on the headers in Vary

Proposal:

1. Add additional return value to CachableResponse of []string that is the list of headers in the Vary header. E.g. ["Accept-Language", "Accept-Encoding"]
2. Add helper to extract headers from a http.Request based on the Vary list
3. Add normalization helpers for certain headers. See https://developer.fastly.com/reference/http/http-headers/Accept-Language/

The code reads:

	if respHeaders.Get("Expires") != "" {
		expiresHeader, err = http.ParseTime(respHeaders.Get("Expires"))
		if err != nil {
			// sometimes servers will return `Expires: 0` or `Expires: -1` to
			// indicate expired content
			expiresHeader = time.Time{}
		}
		expiresHeader = expiresHeader.UTC()
	}

The comment says that Expires:0 or Expires:-1 means expired content. But the code doesn't treat it any differently than no Expires header.

When building on a 32 bits x86 or 32 bits ARM system, the following error occurs:

Testing: "/builddir/build/BUILD/cachecontrol-fd225a25f1983d116db58553068677ba03f51fa9/_build/src/github.com/pquerna/cachecontrol/cacheobject"
+ GOPATH=/builddir/build/BUILD/cachecontrol-fd225a25f1983d116db58553068677ba03f51fa9/_build:/usr/share/gocode
+ go test -buildmode pie -compiler gc -ldflags '-extldflags '\''-Wl,-z,relro  -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld '\'''
# github.com/pquerna/cachecontrol/cacheobject
./directive_test.go:283:43: constant 9223372036854775807 overflows int
FAIL	github.com/pquerna/cachecontrol/cacheobject [build failed]

(I'm packaging for Fedora 28 and above. You can see the full build log at https://koji.fedoraproject.org/koji/taskinfo?taskID=27014801)

It looks like this package does not support the less common Surrogate-Control header sometimes used by CDNs.

The logic to support Surrogate-Control would basically the same as Cache-Control, but the header is specifically meant for reverse proxy <-> origin server controls (Cache-Control is meant for end browsers).

See: https://www.w3.org/TR/edge-arch/

Often used by CDNs:
https://docs.fastly.com/guides/tutorials/cache-control-tutorial
https://www.nuevocloud.com/documentation/getting-started/cache-headers-cache-control-surrogate-control-and-expires

See nicolasazrak/caddy-cache#24 for discussion.

I've been dealing with some parsing issues as some servers don't conform to HTTP Caching spec. Since the error vars are exported I could I just handle it in my code, but it seems to me that having something like this:

cachecontrol.CachableResponse(req, res, cachecontrol.Options{StrictParsing: false})

would be pretty beneficial. Then, these are errors could be ignored and defaults would be assumed for the fields:

ErrQuoteMismatch
ErrMaxAgeDeltaSeconds 
ErrMaxStaleDeltaSeconds
ErrMinFreshDeltaSeconds
ErrNoCacheNoArgs
ErrNoStoreNoArgs
ErrNoTransformNoArgs
ErrOnlyIfCachedNoArgs
ErrMustRevalidateNoArgs
ErrPublicNoArgs
ErrProxyRevalidateNoArgs

If no-cache,no-store,max-age=0,must-revalidate
is received by func parse(value string, cd cacheDirective) error
as a value it reports the following error
strconv.ParseUint: parsing "0,must-revalidate": invalid syntax

Currently the cacheobject.Reason type is an int. This works well enough for the library itself, but it's a shame that it doesn't log very well.

For example, right now if parsing a no-store header I get the following log message:

{"level":"warn","reasons":[9],"time":"2021-08-30T17:08:41+01:00","message":"JWK keys are not cacheable"}

The "reasons":[9] part in particular is not very useful.

If the cacheobject.Reason type was instead a human readable string then the log messages here would be much more useful, without actually compromising anything about using the library.

Currently only the expiration time is exposed in the API.

There are many use cases in which a fuller model of a response should be available, such as responding with stale content under certain conditions -- cachecontrol should make it easier to understand these cases for users.

Ooops, it appears that I've created pretty much entirely the same library:

https://github.com/lox/cachecontrol

Any interest in combining / collaborating?

When passing in a response with just the Date header set, the return value is:
reasons: nil
expires: time.Time{} (a zero value)
err: nil

I would expect the reasons to be something like ReasonResponseUncachableByDefault since passing just a Date header doesn't meet any caching spec