files.get interface differs significantly between Fog::Storage::GoogleJson and Fog::Storage::GoogleXML

[tcdowney]

We were trying to consume the new JSON API functionality provided in the latest version (1.0.1) of this gem.

Calls to files.get that use the new GoogleJson version of get_object are not working with our existing code that passes the files.get method a block that we use to write the file out to disk. You can see an example of this here.

In the old GoogleXML implementation of this, there is apparent handling of the passed block, whereas in the the new GoogleJson implementation it appears to just return the object directly regardless of whether a block was provided in the call to files.get.

Comments in the new code imply that we need to call this method differently and supply a download_dest option.

Unfortunately, this seems like an inconsistency between the two implementations and makes it more difficult to interoperate with fog-aws which functions similarly to the GoogleXML version. It also makes it very difficult to interoperate with both the GoogleXML and GoogleJson versions themselves since it's not clear to consumers which codepath the gem will end up using. In our application we allow users to provide arbitrary fog connection config so we want to be able to rely on a standardized interface.

Thanks!
Tim Downey && @ericpromislow

[emilymye]

I'll get a fix for handling that block correctly. get_object doesn't actually expect download_dest to be passed in (it's set to a temporary buffer), and the file value is returned as object[:body].

Also, @icco to confirm but I don't think in general the GoogleXML library is being supported anymore. It certainly was not upgraded.

[icco]

GoogleXML is still suported, it just uses a completely different backend. GoogleJSON supports https://cloud.google.com/storage/docs/json_api/v1/ and GoogleXML supports https://cloud.google.com/storage/docs/xml-api/overview

[icco]

Also @tcdowney could you provide an example of how you're calling the code? We'd love to add it to our tests or examples / use it to debug.

[emilymye]

Yeah, it'd be helpful to see the block you're passing in.

@icco it looks like the block was removed in this pass. I think it was reasonable since the block is excon related. If we want to keep it compatible, we'll probably have to wrap the excon response block in an IO stream to pass to the Google request (here). Does that sound reasonable?

[icco]

@emilymye I'm confused, so GoogleXML uses excon, but GoogleJSON just uses the google api client. The main thing is that https://github.com/fog/fog-google/blob/master/lib/fog/storage/google_json/models/files.rb#L34-L41, https://github.com/fog/fog-google/blob/master/lib/fog/storage/google_xml/models/files.rb#L55-L67, and https://github.com/fog/fog-aws/blob/master/lib/fog/aws/models/storage/files.rb#L59-L77 act similarly from an input standpoint. The requests can be wildly different.

[emilymye]

@tcdowney Sorry, I just saw the example in the original comment.

@icco I assumed the passed-in block was the format expected by Excon - each of these file.get methods just pass through the block to the underlying request and for AWS/GoogleXML/the previous google-api-client calls, it gets set as the Excon :response_blockparam . The Google API doesn't take a streaming response block so we'd have to give it an IO stream instead or fake it using the buffer file.

[icco]

@emilymye Yeah. Can we turn a streaming block and transform it into an IO Stream?

[emilymye]

We would basically be creating an IO stream and reading it into the block, and then letting @tcdowney write it back to an IO stream in this case. So, it is POSSIBLE - I just hate it a little. 😭

[tcdowney]

Wow thanks for getting a PR out so quickly @emilymye!! We'll try it out on Monday. 👍

[icco]

@tcdowney I merged into master. I'll do a release once you sign off on it.

[tcdowney]

@icco

We're having some difficulty testing master of this repo because the method signature of directories#get changed here:
2902d55#diff-38f0b74473bc10007c3bcace35c39e54R12

We call it with the max_keys option and it's now raising an ArgumentError:

[2018-01-22 21:48:35+0000] ArgumentError: unknown keyword: max_keys
[2018-01-22 21:48:35+0000] /var/vcap/packages/cloud_controller_ng/cloud_controller_ng/vendor/cache/fog-google-e8ecf6a5f0c7/lib/fog/storage/google_json/models/directories.rb:12:in `get'

Is it possible for it to keep the same interface as the GoogleXML version?

fog-google/lib/fog/storage/google_xml/models/directories.rb

Line 15 in e8ecf6a

:max_keys => "max-keys",

[icco]

@tcdowney totally, sorry, we tried to keep the model interfaces the same. Problem here is it won't actually do anything. We can take in a generic object and ignore the options, but because google severely changed their bucket api, it will act much differently: https://cloud.google.com/storage/docs/json_api/v1/buckets/get

[tcdowney]

@icco

Yeah that's interesting... I think for the interface it's helpful if it can accept what the other implementations allow though I agree it's a little unfortunate that it doesn't actually do anything and is silent about it. 🤷‍♂️ I don't think the Openstack implementation works with max-keys either, but it doesn't error and this would be similar to that behavior at least: https://github.com/fog/fog-openstack/blob/a0dff17ba9a0054c665aa967a32be19506693269/lib/fog/storage/openstack/models/directories.rb#L15

[tcdowney]

One last thing we've seen so far is that files#head was replaced with files#metadata:
da9619b#diff-5d9e2ffaf754e4d81109929c33f36ed6R48

I'm not very familiar with the difference between the two implementations, but it seems like we could alias #head to it so consumers aren't broken? It seems like the intent behind the methods are still pretty much the same (looking at https://cloud.google.com/storage/docs/xml-api/head-object at least)?

Thanks!

[icco]

Yup, what you're saying makes sense. My original goal in this was to keep models pretty close, but was okay with changes when proposed mainly because we really have no idea how people use this library. Glad we can make some minor tweaks and keep if working for you. Thanks for reporting @tcdowney.

[tcdowney]

@icco we're still seeing:

[2018-01-23 17:54:57+0000] NameError: undefined local variable or method `head' for Fog::Storage::GoogleJSON::Files:Class

We made the method names symbols inline in our code here and it works:
06bb0da#diff-5d9e2ffaf754e4d81109929c33f36ed6R55

Thanks for your quick responses!

[tcdowney]

Submitted a PR for the above issue: #295

[icco]

Awesome, thanks. Merged. I've been meaning to write a bot that assigns all PRs to me... Would solve the missing PRs that don't mention me or @fog/fog-google

[tcdowney]

Thanks, can confirm that we can now successfully call files#get with a block.

[icco]

Awesome. Released 1.1.0 with this fix.