You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jclouds.apache.org by Niraj Tolia <no...@github.com> on 2013/10/01 08:06:23 UTC
[jclouds] Trivial: Remove unnecessary for loop in Atmos signing
(#163)
You can merge this Pull Request by running:
git pull https://github.com/maginatics/jclouds atmos
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/163
-- Commit Summary --
* Trivial: Remove unnecessary for loop in Atmos signing
-- File Changes --
M apis/atmos/src/main/java/org/jclouds/atmos/filters/SignRequest.java (3)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/163.patch
https://github.com/jclouds/jclouds/pull/163.diff
Re: [jclouds] Trivial: Remove unnecessary for loop in Atmos signing
(#163)
Posted by Andrew Phillips <no...@github.com>.
> @@ -171,8 +171,7 @@ private void appendPayloadMetadata(HttpRequest request, StringBuilder buffer) {
> void appendHttpHeaders(HttpRequest request, StringBuilder toSign) {
> // Only the value is used, not the header
> // name. If a request does not include the header, this is an empty string.
> - for (String header : new String[] { "Range" })
> - toSign.append(HttpUtils.nullToEmpty(request.getHeaders().get(header)).toLowerCase()).append("\n");
> + toSign.append(HttpUtils.nullToEmpty(request.getHeaders().get("Range")).toLowerCase()).append("\n");
Should this also be `(HttpUtils.nullToEmpty(request.getFirstHeaderOrNull("Range")).toLowerCase()`? Otherwise, how do we know we are getting the correct value for the "Range" header?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/163/files#r6680404
Re: [jclouds] Trivial: Remove unnecessary for loop in Atmos signing
(#163)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #260](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/260/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/163#issuecomment-25427875
Re: [jclouds] Trivial: Remove unnecessary for loop in Atmos signing
(#163)
Posted by Adrian Cole <no...@github.com>.
+1 to merge based on discussion
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/163#issuecomment-25462318
Re: [jclouds] Trivial: Remove unnecessary for loop in Atmos signing
(#163)
Posted by Niraj Tolia <no...@github.com>.
> @@ -171,8 +171,7 @@ private void appendPayloadMetadata(HttpRequest request, StringBuilder buffer) {
> void appendHttpHeaders(HttpRequest request, StringBuilder toSign) {
> // Only the value is used, not the header
> // name. If a request does not include the header, this is an empty string.
> - for (String header : new String[] { "Range" })
> - toSign.append(HttpUtils.nullToEmpty(request.getHeaders().get(header)).toLowerCase()).append("\n");
> + toSign.append(HttpUtils.nullToEmpty(request.getHeaders().get("Range")).toLowerCase()).append("\n");
The above changes preserve the existing behavior of the code (i.e., no functional changes) but, are you assuming there could be multiple Range headers? I do not believe that is legal according to spec but I could be wrong:
The Atmos Programmer's Manual has a link to http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.35. There is also https://svn.tools.ietf.org/svn/wg/httpbis/draft-ietf-httpbis/latest/p1-messaging.html#field.order.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/163/files#r6680665
Re: [jclouds] Trivial: Remove unnecessary for loop in Atmos signing
(#163)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #721](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/721/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/163#issuecomment-25427888
Re: [jclouds] Trivial: Remove unnecessary for loop in Atmos signing
(#163)
Posted by Andrew Phillips <no...@github.com>.
> @@ -171,8 +171,7 @@ private void appendPayloadMetadata(HttpRequest request, StringBuilder buffer) {
> void appendHttpHeaders(HttpRequest request, StringBuilder toSign) {
> // Only the value is used, not the header
> // name. If a request does not include the header, this is an empty string.
> - for (String header : new String[] { "Range" })
> - toSign.append(HttpUtils.nullToEmpty(request.getHeaders().get(header)).toLowerCase()).append("\n");
> + toSign.append(HttpUtils.nullToEmpty(request.getHeaders().get("Range")).toLowerCase()).append("\n");
> but, are you assuming there could be multiple Range headers
No, I am just wondering what the code would do if there _were_ multiple headers, since we're evidently not performing any checks on this. But the `nullToEmpty` function seems to take care of this by returning the _first_ value from the input collection if that is not `null` or empty, so we're actually getting the same functionality here.
Fine to leave as-is, in other words...thanks for explaining!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/163/files#r6690691