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

Support If-Match and If-Unmodified-Since #130

Closed
jcready opened this issue Feb 12, 2017 · 23 comments
Closed

Support If-Match and If-Unmodified-Since #130

jcready opened this issue Feb 12, 2017 · 23 comments
Assignees

Comments

@jcready
Copy link
Contributor

jcready commented Feb 12, 2017

If-Match

The server will send back the requested resource only if it matches one of the listed ETags otherwise the response will be a 412 (Precondition Failed) error.

The "If-Match" header field makes the request method conditional on the recipient origin server either having at least one current representation of the target resource, when the field-value is "*", or having a current representation of the target resource that has an entity-tag matching a member of the list of entity-tags provided in the field-value.

If-Unmodified-Since

The server will send back the requested resource only if it has not been last modified after the given date. If the request has been modified after the given date, the response will be a 412 (Precondition Failed) error.

The "If-Unmodified-Since" header field makes the request method conditional on the selected representation's last modification date being earlier than or equal to the date provided in the field-value. This field accomplishes the same purpose as If-Match for cases where the user agent does not have an entity-tag for the representation.

A recipient MUST ignore If-Unmodified-Since if the request contains an If-Match header field; the condition in If-Match is considered to be a more accurate replacement for the condition in If-Unmodified-Since, and the two are only combined for the sake of interoperating with older intermediaries that might not implement If-Match.

@dougwilson
Copy link
Contributor

Besides just adding a kitchen sink of supporting every optional thing, I have a question as to what benefit it adds over the current 304 Not Modified. I ask because, as it even suggests in the specs, that those precondition checks are only really useful for state-modifying methods, which this modules doesn't support anyway, so just wanted to get your thoughts on what use-cases you have in mind.

@jcready
Copy link
Contributor Author

jcready commented Feb 12, 2017

I'm not sure what you're referring to when you reference the "current 304 Not Modified." I assume you're referring to this bit:

// conditional GET support
if (this.isConditionalGET() && this.isCachable() && this.isFresh()) {
  this.notModified()
  return
}

The above will not respect clients that send requests with the If-Match or If-Unmodified-Since headers. Neither the isConditionalGET() or isFresh() methods even look at these headers.

I ask because, as it even suggests in the specs, that those precondition checks are only really useful for state-modifying methods, which this modules doesn't support anyway

Both headers are useful for "resumable" downloads. Both sections linked say the following (where "safe methods" are GET or HEAD):

It can also be used with safe methods to abort a request if the selected representation does not match one already stored (or partially stored) from a prior request.

@dougwilson
Copy link
Contributor

The above will not respect clients that send requests with the If-Match or If-Unmodified-Since headers. Neither the isConditionalGET() or isFresh() methods even look at these headers.

That's correct, because those are supposed to send 412, not 304 :)

Both headers are useful for "resumable" downloads. Both sections linked say the following (where "safe methods" are GET or HEAD):

Ah, I see, didn't know that. Of course, I'm not sure of the usefulness since If-Match explicitly states to never match a weak ETag and this module will only ever generate weak ETags, since it just uses the mod time and size to make the tag, which doesn't guarantee the state of the file. It seems like a client may always end up getting 412 if we follow the specs ?

@jcready
Copy link
Contributor Author

jcready commented Feb 12, 2017

Very true, but a developer might set the ETag header themselves before calling send() or when the headers event is emitted. Also I'm planning on opening another ticket to support strong ETags ;) The goal will be to expose a strongEtag option or something where when true it will essentially create the file ReadStream twice, once to generate a strong ETag and a second time to actually send the data (assuming no preconditions failed).

@dougwilson
Copy link
Contributor

This feature is causing a lot of issue reports. I am inclined to pull the feature back out.

@jcready
Copy link
Contributor Author

jcready commented Mar 4, 2017

Maybe limit it to only GET/HEAD requests? It's up to you.

@jcready
Copy link
Contributor Author

jcready commented Mar 4, 2017

Are these issues being posted somewhere else?

@dougwilson
Copy link
Contributor

Because this is a dependency of Express, it's been in the Express IRC channel, the Express Gitter, one on the Express issue tracker, and another on the serve-static issue tracker (which also depends on this), and that's only the ones I know of. It has only been a few days since this got added to Express. As far as I can tell, they are doing GET/HEAD requests, so not sure on how that would change anything.

@jcready
Copy link
Contributor Author

jcready commented Mar 5, 2017

Ah, is it just the Date.parse() issue? It seems like that could be solved with a check that the request header actually exists first.

@dougwilson
Copy link
Contributor

That is the only confirmed cause yet, I'm still waiting to hear back from everyone. I also though that, but that wouldn't help with If-Unmodified-Since: foo

@dougwilson
Copy link
Contributor

Commit efdb43a is the solution that I think would help work-around the issue.

@jcready
Copy link
Contributor Author

jcready commented Mar 5, 2017

That looks good to me!

@jcready
Copy link
Contributor Author

jcready commented Mar 5, 2017

I think you could also just cast the "parsed" date to a number return Number(Date.parse(date)).

@dougwilson
Copy link
Contributor

dougwilson commented Mar 5, 2017

Well, Number(null) is 0, which you cannot distinguish from Date.parse('Thu, 01 Jan 1970 00:00:00 GMT')

@jcready
Copy link
Contributor Author

jcready commented Mar 5, 2017

Ah, good point.

@dougwilson
Copy link
Contributor

haha. Yea, that commit was the "least bad" fix I could think of that both (a) worked and (b) didn't add an untestable branch without forcing me to override Date.parse in the tests 🤷‍♂️

@jcready
Copy link
Contributor Author

jcready commented Mar 5, 2017

Wow, that datejs module doesn't even return a number for Date.parse() when it is "valid", it returns a "Date" instance: https://github.com/abritinthebay/datejs/blob/master/specs/Parsing-spec.js

@dougwilson
Copy link
Contributor

Hm.... Well, then...

@dougwilson
Copy link
Contributor

@dougwilson
Copy link
Contributor

The open issue in datejs: abritinthebay/datejs#263

@dougwilson
Copy link
Contributor

I'm still thinking on it, but I feel inclined to just at least degrade to just having the features be unsupported for this weird behavior, which I think the previous commit accomplishes. As in, if datejs is used, since Date.parse will only return non-numbers, it will just act like all dates are invalid and everything will just fall through.

@jcready
Copy link
Contributor Author

jcready commented Mar 5, 2017

Yeah, I'm surprised datejs hasn't caused more issues when used with express. I think your commit is a good compromise though. It should resolve the immediate issue.

@dougwilson
Copy link
Contributor

👍 feel free to PR anything if you think of something later, though :) Just releasing to make some people's pain go away from the upgrade, haha.

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

No branches or pull requests

2 participants