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
✨ Cache middleware: Store e2e headers. #1807
✨ Cache middleware: Store e2e headers. #1807
Conversation
I seem to recall that when we were first merging this into Fiber core, we thought about implementing such a feature and decided against it for whatever reason (I don't remember what exactly). While I'm not on the maintainers team anymore, I think this would be a good addition to the cache middleware.
I think it's a good idea to put this behind a flag of some sorts, yes, since this is changing observable behaviour of the cache middleware. I'm struggling to find a situation where it'd be a problem if more headers were preserved from the original request, but I'm think there probably are some. Were this not the case, and it wasn't placed behind a flag, I'd suggest removing explicit caching of the |
Thanks for your prompt answer on this @codemicro 🙏
IMHO, the risks reside in client identity specific headers (such as
I was thinking about it as well and preferred to assess if this PR generates interest from the maintainers and you before going further. I'll create a |
Thanks for the work, Maybe we should do some more research on what were the reasons for not saving and delivering all headers I will have a look at the pull-requests later or tomorrow |
Thanks for answer @ReneWerner87 !
I introduced the
Sounds good to me, let me know what you decide and I'll re-adjust if necessary 😊 P.S: I would be very interested to participate in cache middleware improvement efforts as I'm currently building several projects on-top of Fiber (closed sources for the moment, but will open them at some point) |
Gladly any pull request and improvements both functional and in the area of speed or tests are welcome Of course this also applies to all other parts of the software |
I agree, though if an endpoint is setting cookies, I'd argue that most of the time it's a bad idea to be caching any request to that endpoint.
Typically, I'd structure my application so that I'm specifically including endpoints to be cached as opposed to specifically excluding endpoints. I wonder if this is an approach should be encouraged, as opposed to implementing a feature to remove headers automatically like this?
I think this still has the potential to break things in bizzare edge-cases or when this is used in an unusual manner.
You're welcome! |
In case you believe the flag doesn't offer enough flexibility for the end users, I might have 2 different suggestions that satisfies your two perspectives: First (simple but whitelisting only)The Use cases
Benefits
Second (more complex but blacklisting and/or whitelisting)The
Use cases
Benefits
It feels like we would provide maximum flexibility to the end user without impacting current users. |
Personally, I'm not sure there needs to be flexibility in relation to what headers are included/excluded. Both your proposals here seem a little overcomplicated for this middleware, whereas the current state of this PR with a simple boolean flag looks alright to me. |
Look at it tomorrow, today i have unfortunately no time |
I understand your standpoint and starting with this simple flag instead of increasing the complexity suddenly could be indeed a good option to start with and satisfy the current need 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- please adjust the readme
- please create a pull request for the documentation repository - https://github.com/gofiber/docs/blob/master/api/middleware/cache.md
middleware/cache/cache.go
Outdated
@@ -133,6 +149,20 @@ func New(config ...Config) fiber.Handler { | |||
e.status = c.Response().StatusCode() | |||
e.ctype = utils.CopyBytes(c.Response().Header.ContentType()) | |||
e.cencoding = utils.CopyBytes(c.Response().Header.Peek(fiber.HeaderContentEncoding)) | |||
e.e2eHeaders = make(map[string][]byte) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why convert to string? that would take time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it's impossible to determine both header key and value ahead of time, it feels like a map is needed to compare ignoreHeaders
with response headers with a O(1) complexity.
If you have another approach to recommend, I take it 🙏
As defined in RFC2616 - section-13.5.1, shared caches MUST store end-to-end headers from backend response and MUST be transmitted in any response formed from a cache entry. This commit ensures a stronger consistency between responses served from the handlers & from the cache middleware.
Set flag to prevent e2e headers caching to be the default behavior of the cache middleware. This would otherwise change quite a lot the experience for cache middleware current users.
751553a
to
583802e
Compare
E2E is an acronym commonly associated with test. While in the present case it refers to end-to-end HTTP headers (by opposition to hop-by-hop), this still remains confusing. This commits renames it to a more generic name.
@thylong i added 2 more comments for performance improvements stuff |
This will prevent an extra memory allocation for users not interested in this feature.
Thanks for your review @ReneWerner87 ! Feedbacks:
Requesting your review again 🙂 |
I will do the last point. |
- use set instead of add for the headers - copy value from the headers -> prevent problems with mutable values
the last point is not so easy have added something anyway |
Indeed... Thanks for the the help though ! I'll keep that in mind for later contributions, in case they generate opportunities to make this better. |
Please provide enough information so that others can review your pull request:
As defined in RFC2616 - section-13.5.1, shared caches should store end-to-end headers from backend response and MUST be transmitted in any response formed from a cache entry.
This commit ensures a stronger consistency between responses served from the handlers & from the cache middleware. It also tries to not generate many more memory allocations and ops complexity.
Explain the details for making this change. What existing problem does the pull request solve?
The current cache middleware is minimalist and really fast but doesn't respect several aspects of HTTP/1.1 caching RFCs (RFC2616 & RFC7234).
Non-exhaustive list of unsupported features:
Vary
headerWarning
headerThis causes response consistency issues such as the one described in the following scenario :
You have a simple endpoint with a cache middleware instantiated :
If you
curl -D - http://localhost:8080/
this endpoint, your response will look like the following:Now if you execute the
curl
command again, the second response will look like the following:X-Foobar
response header gets stripped as it's not store in the cache entry.Note
@codemicro @ReneWerner87 @Fenny , do you think this should be optional as it changes the default behaviour of the cache middleware?
I'm hesitating to let this PR setting the new default cache middleware behaviour to make it RFC compliant but it could also catch off guard people already using the middleware. WDYT?