Set Etag automatically for each request
adityapatadia opened this issue · 20 comments
We can provide etag parameter in initial config which is by default set to true. The behaviour will be to automatically set etag header on all successful requests. In fact current implementation is not standards compliant. If I do reply.etag(), it always generates new etag even for same payload. Ideally it should generate same for same payload.
Something like this should be good.
let etag = crypto
.createHash('sha1')
.update(payload)
.digest('base64')
.substring(0, 27);
reply.etag(etag);I found out that this package is doing ETag handling completely wrong. The standard says that server must generate response payload first and then compare it's ETag with the one provided in request. If it's matching, server can ignore sending entire payload and can instead send just 304 response.
This package caches all ETags for certain time and if client sends request with if-none-match header, it just finds if the ETag exists in database. If it exists, it just responds without letting the request process. What is underlying content has changed meanwhile? This I think is a really wrong way to implement. I implemented correct ETag behaviour using Fastify hooks. I hope this library will be updated to be HTTP standards compliant.
This is difficult to implement in a generic way. The approach the plugin takes is to assume that resources are idempotent. Which is to say any request for /foo/bar will always return the same response. This follows from the REST methodology.
If a resource is not idempotent, then it is, in my opinion, better for the application to implement its own method for determining and validating etags.
That being said, if you have a better solution in mind, please submit a PR.
It would be interesting if you could share your own implementation (using fastify hooks)...
This follows from the REST methodology.
This is not accurate. Expressjs do it since years. The only limitation is the current implementation.
Yes it is accurate. /foo/bar should always return the same representation regardless of the number of times it is requested.
Yes it is accurate. /foo/bar should always return the same representation regardless of the number of times it is requested.
Yeah, but with etag it's not needed to respond with a body on subsequent requests when the ressource hasn't changed.
Correct. This plugin does not return the body of the representation when it has not changed.
Just for clarification I have two problems with the current implementation:
- Managing a etag store is wrong. HTTP is stateless.
- The response has to be hashed on every response and then the decision is made whether we want to return the payload or not. This is really important for UX and avoid race-conditions like:
What is underlying content has changed meanwhile? This I think is a really wrong way to implement.
In best case the user hasn't to care about all these things. Have a look at expressjs or koajs.
@jsumners perhaps this plugin has been designed only for complete different use case? I could interpret fastify-cache also as a simple cache. It might be wrong to connect it with compliant HTTP cache?
If you would remove etag and specify a custom header the plugins would make sense to me.
I'm not going to look at Express or Koa. I don't know those frameworks and have never used them. I implemented this plugin by reading the RFC, as is plainly stated in the opening of the README.
I have said it several times: If you think I implemented it incorrectly then submit a PR to fix it, fork it, or whatever. I am of the opinion it is implemented correctly until shown otherwise.
I have said it several times: If you think I implemented it incorrectly then submit a PR to fix it, fork it, or whatever. I am of the opinion it is implemented correctly until shown otherwise.
I don't like your attitude and I have no clue why you are so biased. Before someone will open a PR we should talk about the issue. @adityapatadia and @millette have some great points and it also reflects my opinion about the current implementation.
I'm not going to look at Express or Koa. I don't know those frameworks and have never used them. I implemented this plugin by reading the RFC, as is plainly stated in the opening of the README.
The reason why I mentioned expressjs or koajs is that they have solid implementations of the etag behaviour and it works for years, no magic!
I propose to implement it in a similiar way.
- Managing a etag store is wrong because HTTP is stateless. We should just react based on request/response headers.
- The etag should be generated automatically.
- If a response body can be omitted should be checked by this plugin.
I think this will reflect the usecase of 99% the users. If you have a different intention with this plugin we can create different one e.g fastify-etag
It would be interesting if you could share your own implementation (using fastify hooks)...
fastify.addHook('onSend', (req, res, payload) => {
const etag = crypto
.createHash('sha1')
.update(payload)
.digest('base64')
.substring(0, 27);
if (req.headers['if-none-match'] && req.headers['if-none-match'] == etag) {
res.code(304);
return res.send(null);
} else {
res.header('etag', "W/" + '"' + etag + '"');
}
}I'm not going to look at Express or Koa. I don't know those frameworks and have never used them. I implemented this plugin by reading the RFC, as is plainly stated in the opening of the README.
I have said it several times: If you think I implemented it incorrectly then submit a PR to fix it, fork it, or whatever. I am of the opinion it is implemented correctly until shown otherwise.
There is another RFC in 2014 which states that same URLs can change its response over time: https://tools.ietf.org/html/rfc7232#section-2.3
This is where the ETag header can help clients differentiate between fresh/stale response. As rightly mentioned by @StarpTech express and koajs has been handling this in correct way.
One real use-case is for HTML homepage. If a website homepage changes say everyday, they can provide ETag based on hash of content and clients will receive fresh content when the mechanism is implemented in the way I suggested. In your implementation, clients will keep getting stale responses.
I can also understand that you might not have time to implement this on your own and we are happy to create PR to modify the behaviour, however, you should agree to merge the PR before we put that effort. I have given sample code above. Once you agree, I or may be @StarpTech might be able to create PR.
Yes it is accurate.
/foo/barshould always return the same representation regardless of the number of times it is requested.
It's not always the case. For example, content of https://economictimes.indiatimes.com will be changing everyday and does not necessarily remain same. You can see that the site uses ETag so that it can suggest clients that content has changed. This is not a rare case but a norm in web world.
I should add that above link is India's one of the most read news site.
I'm not going to look at Express or Koa. I don't know those frameworks and have never used them. I implemented this plugin by reading the RFC, as is plainly stated in the opening of the README.
Nowhere in RFC it is stated that response should be cached for 3600000 ms. It was arbitrary number chosen. Basically as @StarpTech says, the cache should be removed and pure content based ETag should be implemented.
I also disagree with creating fork. This is not a third party plugin but a fastify organisation official plugin. If this remains broken like this, less tech savvy users will keep using this plugin and get wrong behaviour than expected.
Please review https://github.com/fastify/fastify-etag.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.