make api reject 304s so caller can implement conditional requests #673
Conversation
|
the location header is not present in a 304. So the first line change is
preventing a 304 from behaving like a redirect.
I thought about passing the the response as fulfilled payload, but it
doesn’t match the schema so, while it’s not an error, rejecting kinda made
since to me at first. What would you suggest? it does not have a body to
parse, the consumer would be responsible for catching and inspecting the
exception to return its cached response.
It seemed a bit much to expect this library to handle caching and 304s
directly.
…On Fri, Dec 22, 2017 at 7:57 PM Gregor Martynus ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Hey I’m sorry but I don’t quite understand your changes? 304 is not an
error and hence shouldn’t be handled like one. Can you elaborate on the
problem you are trying to solve? I can’t merge it like this.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#673 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAjPrY6X1yBFYUf8u4EqWDQpmGfgyzB0ks5tDF4kgaJpZM4RLaaA>
.
|
|
I’m writing a program to query my orgs repos and generate a report. It’s pretty request heavy so i need to add the conditional request headers so i don’t exhaust my api limits. 304s are currently falling because it’s trying to parse a url from a non existant location header. I need to know explicitly that the response was a 304. While not an http error, it is an exception. when rased the caller can know nothing changed. |
gotcha. Could you create a failing test for a 304 and we take it from there? |
|
sure thing. i’ll try to submit one tonight.
…On Sat, Dec 23, 2017 at 2:07 PM Gregor Martynus ***@***.***> wrote:
the location header is not present in a 304
gotcha. Could you create a failing test for a 304 and we take it from
there?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#673 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAjPrSyB9kgUw9neuhuwFbtxNkEcNmWbks5tDV1ogaJpZM4RLaaA>
.
|
|
Merry Christmas! I added a test for 304. It will fail without my original changes. No matter how you expect it to resolve...because the original issue is the code is assuming all 301-307 codes are redirects. 304s do not have a location header, and the url parser blows up if the value passed to it is not a string. My understanding & expectation is that 300s are still not 200's. They are still exceptions that must be dealt with. The library can transparently handle redirects... that's fine, but 304 and 308+ are still exceptions...not necessarily errors, but abort/short=circuit the request in some way. In my opinion, a rejection is appropriate. |
| options.path = Url.parse(res.headers.location, true).path | ||
| httpSendRequest() | ||
| return | ||
| } | ||
|
|
||
| if (res.statusCode >= 400 || res.statusCode < 10) { | ||
| if (res.statusCode === 304 || res.statusCode >= 400 || res.statusCode < 10) { | ||
| callCallback(new error.HttpError(data, res.statusCode, res.headers)) |
There was a problem hiding this comment.
Can you add a 304 status in https://github.com/octokit/node-github/blob/master/lib/error.js#L47-L48? Maybe add a comment to reference this pull request, as it looks a little confusing to have a single 304 besides all the 400+ error codes
There was a problem hiding this comment.
sure thing, i’ll try to do it tomorrow
There was a problem hiding this comment.
I've added the 304 to that file
|
Okay I think I understand the context now. It’s correct that I’d argue caching is within the scope of Octokit libraries, just like pagination and authentication, I’ll have to give it some more thought. I guess the main question is:
Either way, the library does and will allow you to keep that state yourself, so it’s possible that a |
|
Caching is a complicated realm. I would leave that to the consumers to decide on when/how/why to use caching. This library is more pure without that complicating things. With this tweak, opting into caching is pretty trivial, and in my opinion “good enough” :) |
|
Thanks @jsg2021 👍 A new release is on its way |
I was attempting to implement conditional requests and hit an error in this library...