Add Prefer: return=minimal support #605

garrensmith · 2017-06-21T09:52:34Z

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 · 2017-06-21T09:56:16Z

src/chttpd/src/chttpd_exclude_headers.erl

+
+
+% List of essential headers for an http request. If we leave out server then Mochiweb adds in its default one
+-define(ESSENTIAL_HEADERS, ["Cache-Control", "ETag", "Content-Type", "Content-Length", "Vary", "Server"]).


list these in alphabetical order.

and probably best to lay them out one line per header, so we can modify it over time.

perhaps it should even come from config?

rnewson · 2017-06-21T10:00:10Z

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 · 2017-06-21T12:44:02Z

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

rnewson · 2017-06-25T12:32:31Z

rel/overlay/etc/default.ini

+; List of headers that will be kept when the header option X-Couch-Exclude-Headers is included in a request
+; 'X-Couch-Exclude-Headers' : "All"
+all = Server
+minimal = Cache-Control, Content-Length, Content-Type, ETag, Server, Vary


will need Transfer-Encoding and perhaps more

Good point. I've added Content-Length, Content-Type and Transfer-Encoding to the all list

rnewson · 2017-06-29T16:21:19Z

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

Prefer: return=minimal

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.

davisp · 2017-07-17T16:53:02Z

src/chttpd/src/chttpd_prefer_header.erl

+-module(chttpd_prefer_header).
+
+
+


Two blank lines between sections and functions please.

davisp · 2017-07-17T16:53:27Z

src/chttpd/src/chttpd_prefer_header.erl

+
+
+
+-export([maybe_return_minimal/2]).


Exports should go one per line so diff's in the future are nicer.

-export([ maybe_return_minimal/2 ]).

davisp · 2017-07-17T16:55:21Z

src/chttpd/test/chttpd_prefer_header_test.erl

+
+
+default_no_exclude_header_test() ->
+    Headers = chttpd_prefer_header:maybe_return_minimal(mock_request([]), default_headers()),


Line length too long.

davisp · 2017-07-17T16:55:51Z

src/chttpd/test/chttpd_prefer_header_test.erl

+-include_lib("eunit/include/eunit.hrl").
+
+
+


Only two blank lines between sections and functions please.

davisp · 2017-07-17T16:57:11Z

rel/overlay/etc/default.ini

@@ -188,6 +188,10 @@ credentials = false
 ; List of accepted methods
 ; methods =

+[prefer_header]


Bike shed: Is this the best place for this, or would it be better as a key in the chttpd section. Something like "prefer_minimal = List-Of-Headers"

You might also add a comment that a user doesn't specify Server here that Mochiweb will add its own regardless.

davisp · 2017-07-17T16:58:01Z

src/chttpd/src/chttpd_prefer_header.erl

+
+filter_headers(Headers, IncludeList) ->
+    lists:filter(fun({HeaderName, _}) -> 
+        lists:member(HeaderName, IncludeList)


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

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

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

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 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.

rnewson · 2017-07-31T11:49:30Z

rel/overlay/etc/default.ini

@@ -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-Type, ETag, Server, Vary


we'll need Transfer-Encoding otherwise views, changes, _all_dbs will be broken.
we'll need Set-Cookie so we can login.
what about CORS related headers?
we'll need Content-Range for range reqs on attachments.
we'll need WWW-Authenticate for users that enable the standard login prompting setting.

Agree on Transfer-Encoding and Content-Range. For cooking and authentication headers I think not since this is aimed at non-browser clients and those are both for browsers (and or, non browsers would just have to be smart enough to not request it when they need to get a cookie.

I don't think CORS is required since this is for non-browser clients. I agree with Paul around needing the auth headers, don't use this header when you need a cookie.

rnewson · 2017-07-31T11:50:01Z

src/chttpd/src/chttpd_prefer_header.erl

+
+-export([
+    maybe_return_minimal/2
+    ]).


this line should be at column 0.

garrensmith · 2017-08-02T09:49:45Z

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.

rnewson · 2017-08-09T08:22:22Z

rel/overlay/etc/default.ini

@@ -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


should be commented out by default.

rnewson · 2017-08-09T08:23:18Z

src/chttpd/src/chttpd_prefer_header.erl

+
+
+get_header_list() ->
+    SectionStr = config:get("chttpd", "prefer_minimal", []),


the default needs to be ""?

I've changed it to "". It seems re:split("", "\\s*,\\s*", [trim, {return, list}]). and re:split([], "\\s*,\\s*", [trim, {return, list}]). return the same result of [[]]

rnewson · 2017-08-09T10:53:23Z

rel/overlay/etc/default.ini

@@ -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


I'm an idiot. I forgot that this is opt-in (you have to pass 'Prefer: return=minimal'). So please uncomment this again. :)

rnewson · 2017-08-09T10:53:45Z

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

garrensmith · 2017-08-09T12:52:56Z

Docs apache/couchdb-documentation#157

This is for apache/couchdb#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

This is for apache/couchdb#605

to minimise response headers for non browser clients. See apache/couchdb#605

This is for apache#605

rnewson reviewed Jun 21, 2017

View reviewed changes

rnewson reviewed Jun 25, 2017

View reviewed changes

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

davisp requested changes Jul 17, 2017

View reviewed changes

garrensmith added 5 commits July 31, 2017 09:18

Add X-Couch-Exclude-Headers support

1014684

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.

fixes from review

bfa946c

add extra headers to default.ini

5fd5143

Use Header Prefer:return=minimal

9f3e745

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.

fixes from reviews

f1b6a96

rnewson requested changes Jul 31, 2017

View reviewed changes

add missing headers and fix spacing

f05b672

rnewson reviewed Aug 9, 2017

View reviewed changes

latest review fixes

70fedd6

rnewson reviewed Aug 9, 2017

View reviewed changes

rnewson approved these changes Aug 9, 2017

View reviewed changes

uncomment in default.ini

c603bc7

garrensmith mentioned this pull request Aug 9, 2017

add prefer: return=minimal documentation apache/couchdb-documentation#157

Merged

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

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

add prefer: return=minimal documentation (#157)

00fe652

This is for apache/couchdb#605

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

add prefer: return=minimal documentation (apache#157)

c75b630

This is for apache/couchdb#605

thilo added a commit to thilo/couch_potato that referenced this pull request Mar 15, 2019

Add default header to couch request

b192524

to minimise response headers for non browser clients. See apache/couchdb#605

thilo mentioned this pull request Mar 15, 2019

Add default header to couch request langalex/couch_potato#129

Merged

langalex pushed a commit to langalex/couch_potato that referenced this pull request Mar 15, 2019

Add default header to couch request (#129)

cd1de22

to minimise response headers for non browser clients. See apache/couchdb#605

thilo mentioned this pull request Jun 5, 2019

Add default header to couch request langalex/couch_potato#136

Merged

garrensmith deleted the header-reductions branch December 12, 2019 17:20

nickva pushed a commit to nickva/couchdb that referenced this pull request Sep 7, 2022

add prefer: return=minimal documentation (apache#157)

3d5d1c5

This is for apache#605

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Prefer: return=minimal support #605

Add Prefer: return=minimal support #605

garrensmith commented Jun 21, 2017

rnewson Jun 21, 2017

rnewson Jun 21, 2017

rnewson Jun 21, 2017

rnewson commented Jun 21, 2017

garrensmith commented Jun 21, 2017

rnewson Jun 25, 2017

garrensmith Jun 26, 2017

rnewson commented Jun 29, 2017

davisp left a comment

davisp Jul 17, 2017

davisp Jul 17, 2017

davisp Jul 17, 2017

davisp Jul 17, 2017

davisp Jul 17, 2017

davisp Jul 17, 2017

davisp Jul 17, 2017

davisp Jul 17, 2017

rnewson left a comment

rnewson Jul 31, 2017

davisp Jul 31, 2017

garrensmith Aug 2, 2017

rnewson Jul 31, 2017

garrensmith commented Aug 2, 2017

rnewson Aug 9, 2017

rnewson Aug 9, 2017

garrensmith Aug 9, 2017

rnewson Aug 9, 2017

rnewson commented Aug 9, 2017

garrensmith commented Aug 9, 2017



		% List of essential headers for an http request. If we leave out server then Mochiweb adds in its default one
		-define(ESSENTIAL_HEADERS, ["Cache-Control", "ETag", "Content-Type", "Content-Length", "Vary", "Server"]).



		default_no_exclude_header_test() ->
		Headers = chttpd_prefer_header:maybe_return_minimal(mock_request([]), default_headers()),



		get_header_list() ->
		SectionStr = config:get("chttpd", "prefer_minimal", []),

		-module(chttpd_prefer_header).




		-export([maybe_return_minimal/2]).

		-include_lib("eunit/include/eunit.hrl").

Add Prefer: return=minimal support #605

Add Prefer: return=minimal support #605

Conversation

garrensmith commented Jun 21, 2017

Overview

Testing recommendations

GitHub issue number

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnewson commented Jun 21, 2017

garrensmith commented Jun 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnewson commented Jun 29, 2017

davisp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnewson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garrensmith commented Aug 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnewson commented Aug 9, 2017

garrensmith commented Aug 9, 2017