Support query parameters in GET requests #409

stanhu · 2018-08-16T16:57:36Z

This makes it possible to send some extra headers (e.g. response-content-disposition):
https://cloud.google.com/storage/docs/xml-api/reference-headers

houndci-bot · 2018-08-16T16:57:54Z

lib/fog/storage/google_xml/requests/get_object_http_url.rb

+                      :host     => @host,
+                      :method   => "GET",
+                      :path     => "#{bucket_name}/#{object_name}"
+                    }), expires)


Layout/IndentHash: Indent the right brace the same as the first position after the preceding left parenthesis.

houndci-bot · 2018-08-16T16:57:54Z

lib/fog/storage/google_xml/requests/get_object_http_url.rb

-                   }, expires)
+
+          https_url(options.merge({
+                      :headers  => {},


Layout/IndentHash: Use 2 spaces for indentation in a hash, relative to the first position after the preceding left parenthesis.

houndci-bot · 2018-08-16T16:57:55Z

lib/fog/storage/google_xml/requests/get_object_http_url.rb

-                     :path     => "#{bucket_name}/#{object_name}"
-                   }, expires)
+
+          https_url(options.merge({


Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.

houndci-bot · 2018-08-16T16:57:55Z

lib/fog/storage/google_xml/utils.rb

-          query = [params[:query]].compact
+          query = if params[:query]
+                    params[:query].compact.map { |k, v| [k.to_s, CGI.escape(v)].join("=") }
+                   else


Layout/ElseAlignment: Align else with if.

houndci-bot · 2018-08-16T16:57:55Z

lib/fog/storage/google_xml/utils.rb

@@ -31,7 +31,12 @@ def url(params, expires)
        def host_path_query(params, expires)
          params[:headers]["Date"] = expires.to_i
          params[:path] = Fog::Google.escape(params[:path]).gsub("%2F", "/")
-          query = [params[:query]].compact
+          query = if params[:query]


Style/ConditionalAssignment: Assign variables inside of conditionals

houndci-bot · 2018-08-16T16:57:55Z

lib/fog/storage/google_xml/requests/get_object_https_url.rb

          raise ArgumentError.new("bucket_name is required") unless bucket_name
          raise ArgumentError.new("object_name is required") unless object_name
-          https_url({
+
+          https_url(options.merge({


Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.

houndci-bot · 2018-08-16T16:57:56Z

lib/fog/storage/google_json/requests/get_object_https_url.rb

                      :headers  => {},
                      :host     => @host,
                      :method   => "GET",
                      :path     => "#{bucket_name}/#{object_name}"
-                    }, expires)
+                    }), expires)


Layout/IndentHash: Indent the right brace the same as the first position after the preceding left parenthesis.

houndci-bot · 2018-08-16T16:57:56Z

lib/fog/storage/google_json/requests/get_object_https_url.rb

          raise ArgumentError.new("bucket_name is required") unless bucket_name
          raise ArgumentError.new("object_name is required") unless object_name

-          https_url({
+          https_url(options.merge({


Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.

houndci-bot · 2018-08-16T16:57:56Z

lib/fog/storage/google_json/requests/get_object_http_url.rb

                     :headers  => {},
                     :host     => @host,
                     :method   => "GET",
                     :path     => "#{bucket_name}/#{object_name}"
-                   }, expires)
+                   }), expires)


Layout/IndentHash: Indent the right brace the same as the first position after the preceding left parenthesis.

houndci-bot · 2018-08-16T16:57:56Z

lib/fog/storage/google_json/requests/get_object_http_url.rb

          raise ArgumentError.new("bucket_name is required") unless bucket_name
          raise ArgumentError.new("object_name is required") unless object_name
-          http_url({
+          http_url(options.merge({


Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.

houndci-bot · 2018-08-16T18:51:08Z

test/unit/storage/test_xml_requests.rb

+    url = @client.get_object_url("bucket",
+                                 "just some file.json",
+                                 Time.now + 2 * 60,
+                                 { query: { "Response-Content-Disposition" => 'inline; filename="test.json"' } })


Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.

stanhu · 2018-08-17T13:20:21Z

@Temikus Would you mind taking a look at this? This is affecting lots of people trying to use GitLab with Google Cloud Storage (https://gitlab.com/gitlab-org/gitlab-ce/issues/49957#note_95023022).

codecov · 2018-08-18T09:07:53Z

Codecov Report

Merging #409 into master will decrease coverage by 0.03%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master     #409      +/-   ##
==========================================
- Coverage   84.14%   84.11%   -0.04%     
==========================================
  Files         339      339              
  Lines        5766     5771       +5     
==========================================
+ Hits         4852     4854       +2     
- Misses        914      917       +3

Impacted Files	Coverage Δ
lib/fog/storage/google_json/models/files.rb	`97.56% <100%> (ø)`	⬆️
...orage/google_json/requests/get_object_https_url.rb	`100% <100%> (ø)`	⬆️
...torage/google_json/requests/get_object_http_url.rb	`100% <100%> (ø)`	⬆️
lib/fog/storage/google_json/models/file.rb	`87.14% <50%> (ø)`	⬆️
lib/fog/storage/google_json/utils.rb	`82.6% <50%> (-7.4%)`	⬇️
...fog/storage/google_json/requests/get_object_url.rb	`63.63% <50%> (ø)`	⬆️
lib/fog/storage/google_json/mock.rb	`75% <50%> (-5%)`	⬇️

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 e505e2b...b54b192. Read the comment docs.

Temikus

Overall looks good, please, check the escape method though. If I'm missing something - just let me know!

Temikus · 2018-08-18T09:05:18Z

lib/fog/storage/google_json/utils.rb

+
+          if params[:query]
+            filtered = params[:query].reject { |k, v| k.nil? || v.nil? }
+            query = filtered.map { |k, v| [k.to_s, CGI.escape(v)].join("=") }


Be aware that CGI.escape inserts + instead of %20 when escaping whitespaces which breaks certain behaviours.

If you plan on whitespace being in the query - check if you need to use Fog::Google.escape instead.
See #357

Temikus · 2018-08-18T09:05:25Z

lib/fog/storage/google_xml/utils.rb

+
+          if params[:query]
+            filtered = params[:query].reject { |k, v| k.nil? || v.nil? }
+            query = filtered.map { |k, v| [k.to_s, CGI.escape(v)].join("=") }


Ditto

Be aware that CGI.escape inserts + instead of %20 when escaping whitespaces which breaks certain behaviours.

If you plan on whitespace being in the query - check if you need to use Fog::Google.escape instead.
See #357

Temikus · 2018-08-18T09:06:02Z

test/unit/storage/test_xml_requests.rb

@@ -12,6 +12,16 @@ def teardown
    Fog.unmock!
  end

+  def test_get_url_path_has_query_params


Thank you for the tests ❤️

I had an equivalent test for the JSON request API, but I saw this error:

NoMethodError: NoMethodError: undefined method `issuer' for nil:NilClass /Users/stanhu/github/fog-google/lib/fog/storage/google_json/utils.rb:30:in `host_path_query' /Users/stanhu/github/fog-google/lib/fog/storage/google_json/utils.rb:10:in `https_url' /Users/stanhu/github/fog-google/lib/fog/storage/google_json/requests/get_object_https_url.rb:9:in `get_object_https_url' /Users/stanhu/github/fog-google/lib/fog/storage/google_json/requests/get_object_url.rb:16:in `get_object_url' /Users/stanhu/github/fog-google/test/unit/storage/test_json_requests.rb:18:in `test_get_url_path_has_query_params'

I assume client.issuer comes from Google::Auth::ServiceAccountCredentials#make_creds, but I punted on further investigation.

Actually, @client is nil in the test. Not sure where that's supposed to come from.

Ok, I did some simple mocking. Not sure if it's ideal. Take a look.

Our mocking is not ideal either as I've just started redoing unit tests after 1.0 upgrade, so no worries :)

Thank you for the tests, I'll refactor the client into something more idiomatic as a part 2.0 refactor.

Temikus · 2018-08-18T09:23:56Z

lib/fog/storage/google_json/requests/get_object_https_url.rb

          raise ArgumentError.new("bucket_name is required") unless bucket_name
          raise ArgumentError.new("object_name is required") unless object_name

-          https_url({
+          https_url(options.merge({


Nit: Make this consistent with the other options.merge

Temikus · 2018-08-18T09:26:57Z

@stanhu And I do see you're blocked on this so I'm willing to work with you to push a release right away.

stanhu · 2018-08-18T20:53:28Z

@Temikus Good catch. I'm not sure why in my end-to-end testing the escaping of the + wasn't an issue, but I think avoiding that is the right behavior.

This makes it possible to send some extra headers (e.g. response-content-disposition): * https://cloud.google.com/storage/docs/xml-api/reference-headers * https://cloud.google.com/storage/docs/json_api/v1/objects/get

houndci-bot · 2018-08-18T21:10:14Z

lib/fog/storage/google_json/mock.rb

        def initialize(options = {})
          shared_initialize(options[:google_project], GOOGLE_STORAGE_JSON_API_VERSION, GOOGLE_STORAGE_JSON_BASE_URL)
+          @client = MockClient.new('test')


Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

Temikus

LGTM, thank you!

Temikus · 2018-08-19T00:24:12Z

Merging this in, I'll prep a release by EOD tomorrow (Sydney time)

This is needed to support query parameters in `Fog::Storage::Google`. See fog/fog-google#409. Relates to https://gitlab.com/gitlab-org/gitlab-ce/issues/49957

houndci-bot reviewed Aug 16, 2018

View reviewed changes

stanhu force-pushed the sh-support-query-params branch from 9a8831a to 78840c4 Compare August 16, 2018 18:50

houndci-bot reviewed Aug 16, 2018

View reviewed changes

stanhu force-pushed the sh-support-query-params branch 3 times, most recently from c677d00 to af6301e Compare August 16, 2018 19:08

Temikus added the integrate label Aug 18, 2018

Temikus reviewed Aug 18, 2018

View reviewed changes

stanhu force-pushed the sh-support-query-params branch from 3cb2bd1 to 588fb5c Compare August 18, 2018 20:46

Support query parameters in GET requests

574dce1

This makes it possible to send some extra headers (e.g. response-content-disposition): * https://cloud.google.com/storage/docs/xml-api/reference-headers * https://cloud.google.com/storage/docs/json_api/v1/objects/get

stanhu force-pushed the sh-support-query-params branch from 588fb5c to 574dce1 Compare August 18, 2018 20:54

Add tests for Storage JSON requests

b54b192

houndci-bot reviewed Aug 18, 2018

View reviewed changes

Temikus approved these changes Aug 19, 2018

View reviewed changes

Temikus merged commit 490b3e4 into fog:master Aug 19, 2018

This was referenced Aug 21, 2018

Fix broken calls to get_object_http_url #412

Merged

Fog::Storage::GoogleXML::GetObjectHttpUrl#get_object_http_url is broken #413

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support query parameters in GET requests #409

Support query parameters in GET requests #409

stanhu commented Aug 16, 2018

houndci-bot Aug 16, 2018

houndci-bot Aug 16, 2018

houndci-bot Aug 16, 2018

houndci-bot Aug 16, 2018

houndci-bot Aug 16, 2018

houndci-bot Aug 16, 2018

houndci-bot Aug 16, 2018

houndci-bot Aug 16, 2018

houndci-bot Aug 16, 2018

houndci-bot Aug 16, 2018

houndci-bot Aug 16, 2018

stanhu commented Aug 17, 2018

codecov bot commented Aug 18, 2018 •

edited

Loading

Temikus left a comment •

edited

Loading

Temikus Aug 18, 2018 •

edited

Loading

Temikus Aug 18, 2018 •

edited

Loading

Temikus Aug 18, 2018

stanhu Aug 18, 2018 •

edited

Loading

stanhu Aug 18, 2018

stanhu Aug 18, 2018

Temikus Aug 19, 2018

Temikus Aug 18, 2018

Temikus commented Aug 18, 2018

stanhu commented Aug 18, 2018

houndci-bot Aug 18, 2018

Temikus left a comment

Temikus commented Aug 19, 2018

Support query parameters in GET requests #409

Support query parameters in GET requests #409

Conversation

stanhu commented Aug 16, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stanhu commented Aug 17, 2018

codecov bot commented Aug 18, 2018 • edited Loading

Codecov Report

Temikus left a comment • edited Loading

Choose a reason for hiding this comment

Temikus Aug 18, 2018 • edited Loading

Choose a reason for hiding this comment

Temikus Aug 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stanhu Aug 18, 2018 • edited Loading

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

Temikus commented Aug 18, 2018

stanhu commented Aug 18, 2018

Choose a reason for hiding this comment

Temikus left a comment

Choose a reason for hiding this comment

Temikus commented Aug 19, 2018

codecov bot commented Aug 18, 2018 •

edited

Loading

Temikus left a comment •

edited

Loading

Temikus Aug 18, 2018 •

edited

Loading

Temikus Aug 18, 2018 •

edited

Loading

stanhu Aug 18, 2018 •

edited

Loading