Got throws an HTTPError for 304 responses

[lukechilds]

Currently implementing an API client that supports caching. It sends if-modified-since headers and will sometimes get a 304 response. If I get a 304, got throws an HTTPError. I can't catch the error and then do my stuff because the error doesn't contain the headers.

I noticed this in the readme:

got.HTTPError

When server response code is not 2xx. Contains statusCode and statusMessage.

Why isn't 304 allowed?

[floatdrop]

@lukechilds got treats redirects as errors only on POST-like methods, otherwise it will follow redirect location (up to 5 redirects before throwing error). Can you post code to reproduce problem?

[lukechilds]

304 isn't a redirect, it means the content hasn't been modified.

I'm doing a GET request with a date set in the if-modified-since header. The API is returning a 304 meaning the resource hasn't changed since the copy in my cache but this causes got to throw an error.

2 secs, just putting some code together.

[floatdrop]

@lukechilds yup, you right – 304 case will be treated as error. But error does contains headers under response property:

In Promise mode, the response is attached to the error.

So you can check for statusCode or response.headers in error object.

Maybe it is better to make this case valid and not throw error thou (if you do not care about redirects, you can achieve this by disabling followRedirects option).

[lukechilds]

const got = require('got')

// Make API request
got('https://onionoo.torproject.org/summary?limit=1')
  .then(res => {

      // Log response info (should be 200)
      console.log(res.statusCode, res.headers)

      // Make another request with if-modified-since set
      // This should give us a 304
      return got('https://onionoo.torproject.org/summary?limit=1', {
        headers: {
          'if-modified-since': res.headers['last-modified']
        }
      })
  })

  // This should run, but got throws an error
  .then(res => console.log(res.statusCode, res.headers))

  // You can see we got a successful 304 response
  .catch(console.log)

[lukechilds]

Oops, just seen your reply.

The headers aren't in the error object, that's my problem. You can see in the output from the above code:

200 { 'last-modified': 'Tue, 20 Dec 2016 10:22:54 GMT',
  'access-control-allow-origin': '*',
  'content-type': 'application/json;charset=utf-8',
  'cache-control': 'public, max-age=300',
  date: 'Tue, 20 Dec 2016 11:13:43 GMT',
  'x-varnish': '295213998',
  age: '0',
  via: '1.1 varnish-v4',
  'content-length': '211',
  connection: 'close',
  'accept-ranges': 'bytes',
  'strict-transport-security': 'max-age=15768000' }
{ HTTPError
    at stream.catch.then.data (/Users/lukechilds/Dev/oss/onionoo-node-client/node_modules/got/index.js:114:13)
    at process._tickCallback (internal/process/next_tick.js:103:7)
  message: 'Response code 304 (Not Modified)',
  host: 'onionoo.torproject.org',
  hostname: 'onionoo.torproject.org',
  method: 'GET',
  path: '/summary?limit=1',
  statusCode: 304,
  statusMessage: 'Not Modified' }

[floatdrop]

@lukechilds response property is hidden from console.log (because it is huge and spamming logs is bad). Try:

.catch((err) => console.log(err.response))

[lukechilds]

Ahhh, thank you, I didn't realise that. Well that gives me a way to resolve this on my end. It definitely seems odd to me that it throws on a 304 though.

Just had a look and it appears to be reading the body stream that's causing the error. Although the console log says it's an HTTPError the line it points to (114) is a ReadError.

Could this be caused by the 304 having no body?

[floatdrop]

@lukechilds it should point on line 115 – maybe in /Users/lukechilds/Dev/oss/onionoo-node-client/node_modules/got/index.js data handler is on 114 (because local version can differ from master branch).

[lukechilds]

Oops, yep just realised that.

[lukechilds]

Would you accept a PR to "whitelist" 304 in the status code check?

[floatdrop]

@lukechilds maybe, I'm still thinking about it. @sindresorhus what do you think?

Also this will be a major change and could be published only in 7.0.0 version.

[lukechilds]

Ok, it's only a simple change, just submitted a PR.

Take it or leave it ¯\_(ツ)_/¯

[julien-f]

The 304 code should be treated differently because it usually does not contain the response content.

[lukechilds]

@julien-f when you say treated differently do you mean treated differently as in not having an error thrown or treated differently as in I need to do something else in my PR?

[julien-f]

@lukechilds It's just my opinion, but if got does not behave differently (rejects) in case of 304, it force users to handle this case or they will find themselves without any content.

I know that it should only happen when the correct headers are set but still, I think it's much more obvious to debug when an error is thrown.

[lukechilds]

@julien-f Ah I see, that's a valid point.

However, I still think it shouldn't throw an error because, like you said, a 304 should only be returned if someone's asking whether their cached version is still good, so they'll be expecting to handle it.

[reconbot]

I hit this recently too, in an aysnc/await context having to do a try/catch on a valid response is a bit awkward. This is a bit of a contrived example but...

async function updateContent(lastModified) {
  const { body } = await got('https://example.com/status', {
    headers: { 'if-modified-since': lastModified }
  })
  if (body) {
    await doSomething(body)
  }
}

// vs

async function updateContent(lastModified) {
  let response
  try {
    response = await got('https://example.com/status', {
      headers: { 'if-modified-since': lastModified }
    })
  } catch (e) {
    if (e.statusCode === 304) {
      response = e.response
    } else {
      throw e
    }
  }
  const { body } = response
  if (body) {
    await doSomething(body)
  }
}

If you're using bluebird

// bluebird catch
async function updateContent(lastModified) {
  const { body } = await Promise.resolve(got('https://example.com/status', {
    headers: { 'if-modified-since': lastModified }
  })).catch({ statusCode: 304 }, e => e.response)
  if (body) {
    await doSomething(body)
  }
}

[floatdrop]

@reconbot you can't rely on response = e.response – 304 will not contain response you expecting to get in return from updateContent.

[reconbot]

I'm expecting no content if it's not modified. That's why I sent the extra header.

[sindresorhus]

I don't think we should throw, but instead make it clear in the readme that the response body might be empty if you choose to send a if-modified-since header.

How does request handle this?

[lukechilds]

@sindresorhus request doesn't throw. It returns as normal with request.body as an empty string.

[sindresorhus]

Seems like what we should do too. I searched the request issue tracker and couldn't find any issues with 403 and JSON, so seems that solution is working fine for them.

[lukechilds]

Good stuff, is #252 ok or do you need anything else?

[sindresorhus]

@lukechilds A test and the mentioned readme update.