feat: Provide a way to set raw cookie headers #1414

kgriffs · 2019-01-18T04:23:48Z

Previously, setting raw cookies was not supported without having to subclass Response and override several methods. This patch adds official support for this functionality, while also explicitly disallowing the use of Set-Cookie with certain generic header manipulation methods that have semantics that are incompatible with Set-Cookie.

Fixes #1265

codecov · 2019-01-18T04:32:04Z

Codecov Report

Merging #1414 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #1414   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          38      38           
  Lines        2543    2562   +19     
  Branches      394     400    +6     
======================================
+ Hits         2543    2562   +19

Impacted Files	Coverage Δ
falcon/response.py	`100% <100%> (ø)`	⬆️
falcon/errors.py	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e5dd5ea...5b97a04. Read the comment docs.

codecov · 2019-01-18T04:32:04Z

Codecov Report

Merging #1414 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #1414   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          39      39           
  Lines        2558    2579   +21     
  Branches      395     402    +7     
======================================
+ Hits         2558    2579   +21

Impacted Files	Coverage Δ
falcon/response.py	`100% <100%> (ø)`	⬆️
falcon/errors.py	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 793f304...4632bb0. Read the comment docs.

vytas7 · 2019-01-18T15:59:42Z

falcon/response.py

+        if name == 'set-cookie':
+            raise HeaderNotSupported('This method can not be used to remove cookies')
+
+        # NOTE(kgriffs): normalize name by lowercasing it


Is this inline comment needed? We are not really doing the lowercasing in the line below the note?

👍 Oops, that was some cruft leftover from refactoring.

vytas7 · 2019-01-18T18:08:20Z

falcon/response.py

        Returns:
            str: The value of the specified header if set, or
            the default value if not set.
        """
-        return self._headers.get(name.lower(), default)
+
+        name = name.lower()


...the normalization note could probably be plopped atop this instead (see the other comment deeper in this review), not a requirement from me though.

vytas7 · 2019-01-18T18:11:02Z

falcon/response.py

+            items += self._extra_headers
+
+        # NOTE(kgriffs): It is important to append these after self._extra_headers
+        #   in case the latter containes Set-Cookie headers that should be


spelling containes (sic) ➡️ contains

vytas7 · 2019-01-18T18:17:29Z

falcon/response.py

+        name = name.lower()
+
+        if name == 'set-cookie':
+            raise HeaderNotSupported('This method can not be used to set cookies')


Not a mistake per se, but if I were a native speaker I would probably dare to point out that cannot was a much more usual spelling variant (unless not formed part of another construction, or you wanted to sound emphatic). However, it seems you prefer can not by choice, and I am not very opinionated on this.

👍 I guess I'm a bit old-school sometimes when it comes grammar. I don't mind substituting "cannot" here.

No, absolutely no need if this was your choice. Throughout the codebase, you consistently use can not, and @jmvrbanac writes cannot 🙂

vytas7 · 2019-01-19T01:34:39Z

Looks great, I had just a couple of minor nitpicks 👍

The only real code-related question could be, given Falcon's bare metal nature, whether this change grants an extra attribute on Response. But I don't see a clean solution that could do without it either.

kgriffs · 2019-01-19T20:06:47Z

The only real code-related question could be, given Falcon's bare metal nature, whether this change grants an extra attribute on Response. But I don't see a clean solution that could do without it either.

My thought was to try to minimize the changes at this point. I do think we could make the overall interface a bit friendlier when it comes to cookies. Maybe this is something we could look at for 3.1?

kgriffs · 2019-01-19T20:10:14Z

(Feedback addressed)

vytas7 · 2019-01-19T20:15:15Z

The only real code-related question could be, given Falcon's bare metal nature, whether this change grants an extra attribute on Response. But I don't see a clean solution that could do without it either.

My thought was to try to minimize the changes at this point. I do think we could make the overall interface a bit friendlier when it comes to cookies. Maybe this is something we could look at for 3.1?

An extra attribute in the constructor adds about 40ns, which should be negligible? I am fine with this.

kgriffs · 2019-01-29T20:44:26Z

The only real code-related question could be, given Falcon's bare metal nature, whether this change grants an extra attribute on Response. But I don't see a clean solution that could do without it either.

My thought was to try to minimize the changes at this point. I do think we could make the overall interface a bit friendlier when it comes to cookies. Maybe this is something we could look at for 3.1?

An extra attribute in the constructor adds about 40ns, which should be negligible? I am fine with this.

While 40ns is not much, let me take a quick stab at reducing that a little bit, if nothing else by lazily creating the list object.

vytas7 · 2019-02-02T17:20:32Z

Looking good now!

Previously, setting raw cookies was not supported without having to subclass Response and override several methods. This patch adds official support for this functionality, while also explicitly disallowing the use of Set-Cookie with certain generic header manipulation methods that have semantics that are incompatible with Set-Cookie. BREAKING CHANGE: Previously, several methods in the Response class could be used to attempt to set raw cookie headers. However, due to the Set-Cookie header values not being combinable as a comma-delimited list, this resulted in an unexpected and/or incorrect response being constructed for the user agent in the case that more than one cookie was being set. Fixes falconry#1265

kgriffs · 2019-02-13T01:57:17Z

I forgot to update cookies.rst (now fixed). I also rebased on master and resolved the conflict.

kgriffs requested review from jmvrbanac and vytas7 January 18, 2019 04:24

vytas7 requested changes Jan 18, 2019

View reviewed changes

vytas7 previously approved these changes Jan 19, 2019

View reviewed changes

kgriffs requested a review from nZac January 22, 2019 21:56

kgriffs dismissed vytas7’s stale review via 5c35b3c January 29, 2019 21:00

kgriffs force-pushed the 1265_set_cookie branch from dbd4ff4 to 5c35b3c Compare January 29, 2019 21:00

vytas7 previously approved these changes Feb 2, 2019

View reviewed changes

kgriffs dismissed vytas7’s stale review via a1944e2 February 9, 2019 17:26

kgriffs force-pushed the 1265_set_cookie branch from 5c35b3c to a1944e2 Compare February 9, 2019 17:26

vytas7 previously approved these changes Feb 9, 2019

View reviewed changes

kgriffs dismissed vytas7’s stale review via 03d7835 February 13, 2019 01:53

kgriffs force-pushed the 1265_set_cookie branch from 5bd84a7 to 03d7835 Compare February 13, 2019 01:53

kgriffs force-pushed the 1265_set_cookie branch from 03d7835 to 45d7723 Compare February 13, 2019 01:55

Merge branch 'master' into 1265_set_cookie

a79f417

jmvrbanac approved these changes Feb 14, 2019

View reviewed changes

Merge branch 'master' into 1265_set_cookie

4632bb0

vytas7 approved these changes Feb 14, 2019

View reviewed changes

vytas7 merged commit 75824ba into falconry:master Feb 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Provide a way to set raw cookie headers #1414

feat: Provide a way to set raw cookie headers #1414

kgriffs commented Jan 18, 2019 •

edited

Loading

codecov bot commented Jan 18, 2019

codecov bot commented Jan 18, 2019 •

edited

Loading

vytas7 Jan 18, 2019

kgriffs Jan 19, 2019

vytas7 Jan 18, 2019

vytas7 Jan 18, 2019

vytas7 Jan 18, 2019

kgriffs Jan 19, 2019

vytas7 Jan 19, 2019 •

edited

Loading

vytas7 commented Jan 19, 2019

kgriffs commented Jan 19, 2019

kgriffs commented Jan 19, 2019

vytas7 commented Jan 19, 2019 •

edited

Loading

kgriffs commented Jan 29, 2019

vytas7 commented Feb 2, 2019

kgriffs commented Feb 13, 2019

feat: Provide a way to set raw cookie headers #1414

feat: Provide a way to set raw cookie headers #1414

Conversation

kgriffs commented Jan 18, 2019 • edited Loading

codecov bot commented Jan 18, 2019

Codecov Report

codecov bot commented Jan 18, 2019 • edited Loading

Codecov Report

vytas7 Jan 18, 2019

Choose a reason for hiding this comment

kgriffs Jan 19, 2019

Choose a reason for hiding this comment

vytas7 Jan 18, 2019

Choose a reason for hiding this comment

vytas7 Jan 18, 2019

Choose a reason for hiding this comment

vytas7 Jan 18, 2019

Choose a reason for hiding this comment

kgriffs Jan 19, 2019

Choose a reason for hiding this comment

vytas7 Jan 19, 2019 • edited Loading

Choose a reason for hiding this comment

vytas7 commented Jan 19, 2019

kgriffs commented Jan 19, 2019

kgriffs commented Jan 19, 2019

vytas7 commented Jan 19, 2019 • edited Loading

kgriffs commented Jan 29, 2019

vytas7 commented Feb 2, 2019

kgriffs commented Feb 13, 2019

kgriffs commented Jan 18, 2019 •

edited

Loading

codecov bot commented Jan 18, 2019 •

edited

Loading

vytas7 Jan 19, 2019 •

edited

Loading

vytas7 commented Jan 19, 2019 •

edited

Loading