Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Disable Etag response header for GET /protocol-version#45

Closed
jasonrudolph wants to merge 4 commits intomasterfrom
no-etag-for-protocol-version
Closed

Disable Etag response header for GET /protocol-version#45
jasonrudolph wants to merge 4 commits intomasterfrom
no-etag-for-protocol-version

Conversation

@jasonrudolph
Copy link
Contributor

@jasonrudolph jasonrudolph commented Feb 13, 2018

Motivation: atom/teletype#318 (comment)

TODO

  • Add middleware to disable the ETag response header for GET /protocol-version
  • Add test(s)

Demo

Before

$ curl -i http://localhost:3000/protocol-version
HTTP/1.1 200 OK
X-Powered-By: Express
Access-Control-Allow-Origin: *
Content-Type: application/json; charset=utf-8
Content-Length: 13
ETag: W/"d-MaS+LUY/qwRdJ3/Z0vJ+lNsXnE8"
Date: Tue, 13 Feb 2018 22:16:53 GMT
Connection: keep-alive

{"version":6}

After

$ curl -i http://localhost:3000/protocol-version
HTTP/1.1 200 OK
X-Powered-By: Express
Access-Control-Allow-Origin: *
Content-Type: application/json; charset=utf-8
Content-Length: 13
Date: Tue, 13 Feb 2018 22:16:59 GMT
Connection: keep-alive

{"version":6}

@jasonrudolph
Copy link
Contributor Author

/fyi @as-cii: Here's a potential solution for removing the ETag response header from GET /protocol-version to help address the issue seen in atom/teletype#318 (comment)

- Add tests for the middleware introduced in 17947a1
- Refactor TestServer to extract out the logic for starting and
  destroying an HTTP test server for an Express app. This refactoring
  allows us to create a tiny Express app to test the
  `disableResponseHeaders` middleware. (I started out by attempting to
  unit test the `disableResponseHeaders` middleware using the stub-based
  approach that we used for the other middlewares, but it that approach
  would have required quite a bit of fragile stubbing to stub out all
  the necessary parts of the request and response. Ultimately, it feels
  more robust and more maintainable to test the middleware by using it
  in a small Express app and testing the middleware's effects on that
  app.)
@jasonrudolph jasonrudolph changed the title [WIP] Disable Etag response header for GET /protocol-version Disable Etag response header for GET /protocol-version Mar 12, 2018
@jasonrudolph
Copy link
Contributor Author

@as-cii: I think this is ready for code review. Would you mind reviewing it when you have a moment? It's probably easiest to review the individual commits, where each commit message explains the specific change that's taking place.

I'm especially curious to hear your thoughts on the changes to TestServer and on the introduction of HTTPTestServer, but I certainly appreciated feedback on anything at all. 😄

Copy link
Contributor

@as-cii as-cii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. After a quick search it seems like another option would be to add a Cache-Control: no-store header to the response (see expressjs/express#2472). For instance:

app.get('/protocol-version', function (req, res) {
  res.set('Cache-Control', 'no-store')
  res.send({version: 7})
})

If we make it less general by using the approach above we may even consider avoiding to write tests at all for this particular behavior as it's pretty declarative already. What do you think?

@jasonrudolph
Copy link
Contributor Author

another option would be to add a Cache-Control: no-store header to the response

Nice find, @as-cii. #47 implements that approach. I tested it out in Teletype, and it seems to do the trick (i.e., it prevents the client from caching the response from /protocol-version).

@jasonrudolph
Copy link
Contributor Author

Closing in favor of #47.

@jasonrudolph jasonrudolph deleted the no-etag-for-protocol-version branch March 15, 2018 10:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants