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

Add Prefer: return=minimal support #605

Merged
merged 8 commits into from Aug 9, 2017

Conversation

Projects
None yet
3 participants
@garrensmith
Member

garrensmith commented Jun 21, 2017

Overview

Adding this header allows the client to request that only Non-Essential
or All headers are removed from the response. This will decrease the
size of the response.

The idea is to remove all headers that a non-browser client would not need.
I've had to leave Server header in otherwise Mochiweb adds its very lame/snarky header in its place.

There are two options Non-Essential which removes everything except caching headers. And then All which removes everything except Content-length, Content-Type, Server, Date.

Testing recommendations

curl -v --header "X-Couch-Exclude-Headers: Non-Essential" 'http://dev:5984/_all_dbs

GitHub issue number

This PR will be the number

Checklist

  • Code is written and works correctly;
  • Changes are covered by tests;
  • Documentation reflects the changes;
@rnewson

This comment has been minimized.

Show comment
Hide comment
@rnewson

rnewson Jun 21, 2017

Member

forgot to add, the structure of the change looks good to me. My comments are mainly about configurability. I like the use of a custom header to modify the response, rather than a query parameter. The recommendation these days is not to add X- but there's so much precedence in CouchDB that it might be weird not to always have it. I do think the request header, and its permitted values, are a bit wordy at the same time as being a bit unclear. Perhaps if the filtering were controlled by config this problem goes away.

Member

rnewson commented Jun 21, 2017

forgot to add, the structure of the change looks good to me. My comments are mainly about configurability. I like the use of a custom header to modify the response, rather than a query parameter. The recommendation these days is not to add X- but there's so much precedence in CouchDB that it might be weird not to always have it. I do think the request header, and its permitted values, are a bit wordy at the same time as being a bit unclear. Perhaps if the filtering were controlled by config this problem goes away.

@garrensmith

This comment has been minimized.

Show comment
Hide comment
@garrensmith

garrensmith Jun 21, 2017

Member

Thanks @rnewson I've changed it to support using a config setting.

Member

garrensmith commented Jun 21, 2017

Thanks @rnewson I've changed it to support using a config setting.

@rnewson

This comment has been minimized.

Show comment
Hide comment
@rnewson

rnewson Jun 29, 2017

Member

Take a look at RFC 7240 (https://tools.ietf.org/html/rfc7240).

Prefer: return=minimal

Member

rnewson commented Jun 29, 2017

Take a look at RFC 7240 (https://tools.ietf.org/html/rfc7240).

Prefer: return=minimal

@garrensmith garrensmith changed the title from Add X-Couch-Exclude-Headers support to Add Prefer: return=minimal support Jun 30, 2017

@davisp

Pretty good overall. This is a lot simpler than I expected going into the review so that makes me quite happy. Mostly a few nits other than the case sensitive matching for header filtering.

Show outdated Hide outdated src/chttpd/src/chttpd_prefer_header.erl
Show outdated Hide outdated src/chttpd/src/chttpd_prefer_header.erl
Show outdated Hide outdated src/chttpd/test/chttpd_prefer_header_test.erl
Show outdated Hide outdated src/chttpd/test/chttpd_prefer_header_test.erl
Show outdated Hide outdated rel/overlay/etc/default.ini
filter_headers(Headers, IncludeList) ->
lists:filter(fun({HeaderName, _}) ->
lists:member(HeaderName, IncludeList)

This comment has been minimized.

@davisp

davisp Jul 17, 2017

Member

Seems like you probably want to look into mochiweb's header handling to make this comparison case insensitive.

@davisp

davisp Jul 17, 2017

Member

Seems like you probably want to look into mochiweb's header handling to make this comparison case insensitive.

This comment has been minimized.

@davisp

davisp Jul 17, 2017

Member

Because users may or may not give headers in the same case as we have them specified internally.

@davisp

davisp Jul 17, 2017

Member

Because users may or may not give headers in the same case as we have them specified internally.

This comment has been minimized.

@davisp

davisp Jul 17, 2017

Member

Also that'd make for a good unit test as well.

@davisp

davisp Jul 17, 2017

Member

Also that'd make for a good unit test as well.

garrensmith added some commits Jun 21, 2017

Add X-Couch-Exclude-Headers support
Adding this header allows the client to request that only Non-Essential
or All headers are removed from the response. This will decrease the
size of the response.
Use Header Prefer:return=minimal
Use the Prefer: return=minimal header options from
[rfc7240](https://tools.ietf.org/html/rfc7240). This will reduce the
number of headers in the response. The headers to be included is
configurable via the config.
@rnewson

thanks for pursuing this for so long. I have some comments to address.

I think it would be safer if we identified which headers couchdb sends that are strictly optional, I can't be sure I've listed all the ones I think need to be added back in, though I did read through the standard response header list as part of this review.

Show outdated Hide outdated rel/overlay/etc/default.ini
Show outdated Hide outdated src/chttpd/src/chttpd_prefer_header.erl
@garrensmith

This comment has been minimized.

Show comment
Hide comment
@garrensmith

garrensmith Aug 2, 2017

Member

Thanks @rnewson and @davisp for the reviews on this. I've updated the default.ini and added in the two missing headers. I've also fixed the spacing issue.

Member

garrensmith commented Aug 2, 2017

Thanks @rnewson and @davisp for the reviews on this. I've updated the default.ini and added in the two missing headers. I've also fixed the spacing issue.

@@ -57,6 +57,9 @@ backlog = 512
docroot = {{fauxton_root}}
socket_options = [{recbuf, 262144}, {sndbuf, 262144}, {nodelay, true}]
require_valid_user = false
; List of headers that will be kept when the header Prefer: return=minimal is included in a request.
; If Server header is left out, Mochiweb will add its own one in.
prefer_minimal = Cache-Control, Content-Length, Content-Range, Content-Type, ETag, Server, Transfer-Encoding, Vary

This comment has been minimized.

@rnewson

rnewson Aug 9, 2017

Member

should be commented out by default.

@rnewson

rnewson Aug 9, 2017

Member

should be commented out by default.

@rnewson

This comment has been minimized.

Show comment
Hide comment
@rnewson

rnewson Aug 9, 2017

Member

+1 to merge but please squash down from the 7 commits as appropriate.

Member

rnewson commented Aug 9, 2017

+1 to merge but please squash down from the 7 commits as appropriate.

@rnewson

rnewson approved these changes Aug 9, 2017

@garrensmith

This comment has been minimized.

Show comment
Hide comment
@garrensmith
Member

garrensmith commented Aug 9, 2017

@garrensmith garrensmith merged commit 52da19b into apache:master Aug 9, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

garrensmith added a commit to apache/couchdb-documentation that referenced this pull request Aug 9, 2017

wohali added a commit that referenced this pull request Oct 19, 2017

Add Prefer: return=minimal support (#605)
Use the Prefer: return=minimal header options from
[rfc7240](https://tools.ietf.org/html/rfc7240) to reduce the
number of headers in the response. The header is configurable via the config

jiangphcn added a commit to cloudant/couchdb-documentation that referenced this pull request Mar 16, 2018

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