Add application setting to disable ETag #1421

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
8 participants
Contributor

kavu commented Nov 18, 2012

Add application setting app.disable('etag'); to disable ETag. Manual ETag addition via res.set('etag', 'somestring);' still works.

Please, check this feature.

Btw, it's my first experience in Express and JS BDD, so, please, if there are any issues or mistakes (for ex., I am not sure about proper spec placement), let me know, maybe I'll can fix 'em.

Thanks!

Owner

tj commented Nov 20, 2012

looks good for the most part, I'll add a few comments, also I'm curious of the use-case for disabling them all together (not saying it's invalid, just curious), I know one guy requested this for his api

lib/response.js
- if (!this.get('ETag')) {
- this.set('ETag', etag(body));
+ if (app.settings['etag']) {
+ if (len > 1024) {
@tj

tj Nov 20, 2012

Owner

might as well do if (app.settings.etag && len > ..)

test/res.etag.js
+var express = require('../')
+ , request = require('./support/http');
+
+describe('res', function(){
@tj

tj Nov 20, 2012

Owner

these should still be in the res.send.js file, with describe('"etag" setting', function...) inside, then you can do describe('when enabled', ...) and describe('when disabled', ...) etc, the rest looks good

test/res.etag.js
+ request(app)
+ .get('/')
+ .end(function(err, res){
+ res.headers.should.not.have.property('ETag');
@tj

tj Nov 20, 2012

Owner

node always lowercases so this could be a false-positive so I'd change this one to 'etag'

Contributor

kavu commented Nov 22, 2012

@visionmedia, fixed issues that you pointed me out.

I'm curious of the use-case for disabling them all together (not saying it's invalid, just curious), I know one guy requested this for his api

Yeap! I need to disable it for exactly the same thing — very thin API with minimum data transmitted to client.

Owner

tj commented Dec 6, 2012

not 100% sure I get the use-case though, just because it's a minimal API means you want to opt-out of caching exact responses?

Contributor

kavu commented Dec 6, 2012

I want to minimize http headers footprint, leave the very minimal just to make it work

On 06.12.2012, at 4:50, TJ Holowaychuk notifications@github.com wrote:

not 100% sure I get the use-case though, just because it's a minimal API means you want to opt-out of caching exact responses?


Reply to this email directly or view it on GitHub.

Owner

tj commented Dec 6, 2012

but etags speed things up, that's likely a very large premature optimization

Contributor

kavu commented Dec 6, 2012

etags speed things up,

AFAIK, it speeds client-server communication if client support and respect that fields (ex. Cache-Control, Etag, Expires). I'll have such ugly API client that won't even try to cache :-(

Owner

tj commented Dec 6, 2012

well shaving off one header field isn't going to increase throughput

Contributor

kavu commented Dec 6, 2012

Em… Is it problem to merge this pull request?

Max Riveiro

On Friday, 7 December 2012 г. at 2:49, TJ Holowaychuk wrote:

well shaving off one header field isn't going to increase throughput


Reply to this email directly or view it on GitHub (visionmedia#1421 (comment)).

Owner

tj commented Dec 6, 2012

I did already, but now im questioning its usefulness haha

Contributor

kavu commented Dec 6, 2012

Thanks!

Well, i have and an impression that Etags question is matter of opinion, for example http://developer.yahoo.com/performance/rules.html#etags — "The problem with ETags is that they typically are constructed using attributes that make them unique to a specific server hosting a site. ETags won't match when a browser gets the original component from one server and later tries to validate that component on a different server, a situation that is all too common on Web sites that use a cluster of servers to handle requests." I am trying to disable it at all in my Nginx configuration for the Rails app. Surely, I may be wrong, but i can enable it any time, so… Time will show

Max Riveiro

On Friday, 7 December 2012 г. at 2:54, TJ Holowaychuk wrote:

I did already, but now im questioning it's usefulness haha


Reply to this email directly or view it on GitHub (visionmedia#1421 (comment)).

Owner

tj commented Dec 6, 2012

that's talking about serving of files, since some include the inode number etc. I'll revert this for now

@tj tj closed this Dec 6, 2012

@brenkehoe brenkehoe referenced this pull request in tjanczuk/iisnode Dec 17, 2012

Closed

Image encoding problem #251

rajbala commented Mar 4, 2013

FWIW: We're also looking for a means of disabling the etag header for our web services API that's written using Express. Any chance of this pull request being add back to Express?

Owner

tj commented Mar 4, 2013

@rajbala just not sure I understand the use-case, I can't think of any reason the etag would get in the way

rajbala commented Mar 4, 2013

Here's an example: Developers consume our API at certain tiers. The free tier allows them 10,000 requests while the paid tiers provide more requests. We need to respond to each request with results regardless of whether it should be a 304 or not. They're allowed a certain number of requests and it's their prerogative as to how they use them.

Owner

tj commented Mar 4, 2013

hmm I'd still count it as a request but the only thing that changes really with the etag is they get a faster "response", you can still +1 the request count or whatever you need to internally

dakami commented Mar 4, 2013

There's drama around ETags. Obviously should be on by default but you can't insist they're present.

dakami commented Mar 4, 2013

(Some people abused ETags and now there's requests to eliminate them. This is why we can't have nice things.)

Isn't skipping cache validation a valid use case? What if my app generates content which is guaranteed not to change until its expiration? I'd like to at least suggest that clients respect Expires, rather than that they should send If-None-Match.

If not disabling ETag, then what is the recommended solution for this situation? Generate a static file in middleware and hand the request off to a static server?

Owner

tj commented Apr 10, 2013

@numbers1311407 you can set a max-age / expires on any response that you like, the ETag doesn't get in the way of that

brett19 commented May 17, 2013

Express uses crc32 to generate etag's if there is not one already tagged on the response. For my use-case, we are sending a fairly large amount of data frequently, and the calculation of these etags is actually consuming a significant amount of processing time, additionally, our data already contains a form of etag within it which nullifies the need for it. Any way you would be willing to pull this again?

Contributor

caridy commented May 17, 2013

@brett19 maybe what you need is to take control over the utils' etag method, so you can replace it with something more simple. As today, that's difficult to do though.

Owner

tj commented May 17, 2013

@brett19 fair enough, we'll just have to rebase

Contributor

kavu commented May 17, 2013

@visionmedia, ping me if you need me to update this PR up to the current master branch.

Owner

tj commented Jun 2, 2013

@kavu that would be great thanks man

dmmalam commented Aug 5, 2013

The main use case is when setting far-futures expires with cachebusting filename revs. In that case ETags are just unnecessary.
See h5bp reccomends https://github.com/h5bp/server-configs-node/blob/master/lib/layers/headers.js#L254

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment