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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃殌 [Feature]: Cache-Control: no-cache #2159

Merged
merged 24 commits into from Oct 21, 2022

Conversation

marcmartin13
Copy link
Contributor

@marcmartin13 marcmartin13 commented Oct 18, 2022

Added noCache function for checking Cache-Control: no-cache in the request header

Description

If the request header contains Cache-Control: no-cache it will skip getting the cached entry and proceeds to cache the response.

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist:

  • I have performed a self-review of my own code

Check if the request header Cache-Control contains no-cache
@welcome
Copy link

welcome bot commented Oct 18, 2022

Thanks for opening this pull request! 馃帀 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

@ReneWerner87
Copy link
Member

@marcmartin13 thanks for the work

can you tell me what is the difference of this function with the Next function? this can also be done with it or ?

thought rather we integrate the fixed without possible change of functionality

can you add more tests so we can make sure this functionality works and we don't have to manually test each time for minor adjustments

@marcmartin13
Copy link
Contributor Author

marcmartin13 commented Oct 18, 2022

@marcmartin13 thanks for the work

can you tell me what is the difference of this function with the Next function? this can also be done with it or ?

thought rather we integrate the fixed without possible change of functionality

can you add more tests so we can make sure this functionality works and we don't have to manually test each time for minor adjustments

Hi @ReneWerner87

1st:
noCache function allows clients to request the most up-to-date response. Technically it just skips getting the cached entry and proceeds to cache the response (always miss). Next function can do that too, BUT once the response is cached you will always get that cached entry until it expires (always hit). This will cause the ETag middleware to return 304 when using "If-None-Match" since it will just always return the cached entry instead of the updated ones.

Screenshot_8
Screenshot_9

2nd:
I've already tried it but it's not working as expected because the cache middleware goes like this.

  1. check/get cached entry
  2. Next function
  3. cache response

It will always return the cached entry (not yet expired) before reaching the Next function.

3rd:
Test_Cache_WithCacheControlNoCacheRequestHeader
Test_Cache_WithETagAndCacheControlNoCacheRequestHeader

My workaround right now is to use skip middleware but I don't think it's a good practice.

I've updated all of my comments because I don't understand my English 馃ぃ

Thanks!

middleware/cache/config.go Outdated Show resolved Hide resolved
@efectn
Copy link
Member

efectn commented Oct 19, 2022

Please update readme and create a PR for docs

@marcmartin13
Copy link
Contributor Author

Please update readme and create a PR for docs

Hi @efectn, I'm sorry but where I can find it, and how to create PR for docs? Thank you

@efectn
Copy link
Member

efectn commented Oct 19, 2022

Please update readme and create a PR for docs

Hi @efectn, I'm sorry but where I can find it, and how to create PR for docs? Thank you

https://github.com/gofiber/docs/tree/master/api/middleware you can take a look here

@marcmartin13
Copy link
Contributor Author

marcmartin13 commented Oct 19, 2022

Hi @ReneWerner87 and @efectn

may I ask if is it ok to add one more feature?

image
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control

@marcmartin13 marcmartin13 requested review from ReneWerner87 and efectn and removed request for ReneWerner87 October 20, 2022 05:08
@marcmartin13 marcmartin13 requested review from ReneWerner87 and removed request for efectn October 20, 2022 06:26
@ReneWerner87 ReneWerner87 self-requested a review October 20, 2022 10:57
Copy link
Member

@ReneWerner87 ReneWerner87 left a comment

Choose a reason for hiding this comment

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

not sure yet if we really should expose the RequestDirective method

@ReneWerner87
Copy link
Member

ReneWerner87 commented Oct 20, 2022

Please update readme and create a PR for docs

  • update readme and create a PR for docs

Copy link
Contributor Author

@marcmartin13 marcmartin13 left a comment

Choose a reason for hiding this comment

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

I've changed it to a normal private function.

Question: is updating readme and docs necessary since it is just a private function?

thanks!

@efectn
Copy link
Member

efectn commented Oct 21, 2022

I've changed it to a normal private function.

Question: is updating readme and docs necessary since it is just a private function?

thanks!

No

@ReneWerner87
Copy link
Member

I've changed it to a normal private function.

Question: is updating readme and docs necessary since it is just a private function?

thanks!

we could already describe the new behavior so that everyone knows when they use the middleware and don't have to look in the code, but maybe you already implicitly assume this behavior

@marcmartin13
Copy link
Contributor Author

not sure yet if we really should expose the RequestDirective method

not sure yet if we really should expose the RequestDirective method

I've changed it to a normal private function.
Question: is updating readme and docs necessary since it is just a private function?
thanks!

we could already describe the new behavior so that everyone knows when they use the middleware and don't have to look in the code, but maybe you already implicitly assume this behavior

yep, you are right. but I don't know where to put the description of this function since readme and docs have only the config description.

@ReneWerner87
Copy link
Member

then just start with the first descriptions

look at the other middlewares and copy the style or look where the description i

@marcmartin13
Copy link
Contributor Author

then just start with the first descriptions

look at the other middlewares and copy the style or look where the description i

on it

@marcmartin13
Copy link
Contributor Author

@ReneWerner87 thanks for helping me

@ReneWerner87
Copy link
Member

@ReneWerner87 thanks for helping me

thx for the contribution^^

@ReneWerner87 ReneWerner87 merged commit c187c6a into gofiber:master Oct 21, 2022
@welcome
Copy link

welcome bot commented Oct 21, 2022

Congrats on merging your first pull request! 馃帀 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

ReneWerner87 added a commit to gofiber/docs that referenced this pull request Oct 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants