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

Fastify etag is not returning 304 for matching etag when nginx is used #108

Open
2 tasks done
kp198 opened this issue Jul 10, 2023 · 3 comments
Open
2 tasks done

Comments

@kp198
Copy link

kp198 commented Jul 10, 2023

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the bug has not already been reported

Fastify version

4.15.0

Plugin version

4.2.0

Node.js version

16.5.1

Operating system

Linux

Operating system version (i.e. 20.04, 11.3, 10)

12.6.5

Description

When I have a nginx layer in front of my fastify server, the 304 response is not getting served.
For same data, etag everything works in the local.
I have tried switching off etag generation in nginx layer also.
We gzip the data in nginx, but I am not sure how the 304 might get skipped.

Steps to Reproduce

create a dummy server with nginx and try passing the if-none-match value same as the etag.

Expected Behavior

expect 304 but get 200

@mcollina
Copy link
Member

Thanks for reporting!

Can you provide steps to reproduce? We often need a reproducible example, e.g. some code that allows someone else to recreate your problem by just copying and pasting it. If it involves more than a couple of different file, create a new repository on GitHub and add a link to that. In this case, I recommend also adding a docker-compose file.

@lqqyt2423
Copy link
Contributor

I have the same problem.

Nginx would change etag header to weak etag when using with gzip. https://github.com/aronwoost/TIL-etags-in-nginx

So when compare etag header, this should be considered.

Maybe change the code https://github.com/fastify/fastify-etag/blob/master/index.js#L51 like this https://github.com/jshttp/fresh/blob/master/index.js#L63

@Eomm
Copy link
Member

Eomm commented Aug 28, 2023

Would you like to send a Pull Request to address this issue? Remember to add unit tests.

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

No branches or pull requests

4 participants