Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[OpenStack] Decode body to array #2578

Merged
merged 2 commits into from Jan 24, 2014
Merged

[OpenStack] Decode body to array #2578

merged 2 commits into from Jan 24, 2014

Conversation

kzaitsev
Copy link
Contributor

Hi, im use Selectel (http://storage.selectel.ru) cloud with openstack swift (auth api v1.0), when i try to get_container, im got an error:

NoMethodError: undefined method `each' for #<String:0x007ff7aaaf9f98>
    from /Users/bugagazavr/Documents/Ruby/fog/lib/fog/openstack/models/storage/directories.rb:31:in `get'
    from (irb):2
    from /Users/bugagazavr/.rvm/gems/ruby-2.1.0/gems/railties-4.1.0.beta1/lib/rails/commands/console.rb:90:in `start'
    from /Users/bugagazavr/.rvm/gems/ruby-2.1.0/gems/railties-4.1.0.beta1/lib/rails/commands/console.rb:9:in `start'
    from /Users/bugagazavr/.rvm/gems/ruby-2.1.0/gems/railties-4.1.0.beta1/lib/rails/commands/commands_tasks.rb:69:in `console'
    from /Users/bugagazavr/.rvm/gems/ruby-2.1.0/gems/railties-4.1.0.beta1/lib/rails/commands/commands_tasks.rb:40:in `run_command!'
    from /Users/bugagazavr/.rvm/gems/ruby-2.1.0/gems/railties-4.1.0.beta1/lib/rails/commands.rb:17:in `<top (required)>'
    from bin/rails:4:in `require'
    from bin/rails:4:in `<main>'

Because selectel cloud send me data.body as string, not array

>> id.directories.get('awesome', :preffix => 'system')
#<Excon::Response:0x007fa8dd463d58
 @body=
  "[{\"hash\": \"b996ceaba5a3f8ba5c18131062380436\", \"last_modified\": \"2014-01-15T18:12:06.865730\", \"bytes\": 295, \"name\": \"system/member/photo/12/test.png\", \"content_type\": \"image/png\"}]",

This is a small fix that allows you to convert a string into an array if it is not.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.59%) to 68.31% when pulling 3c7eb60 on Bugagazavr:master into 5dad9a5 on fog:master.

@krames
Copy link
Member

krames commented Jan 15, 2014

I wonder what is causing this. What version of Excon are you using?

@kzaitsev
Copy link
Contributor Author

@krames

Using excon (0.31.0)

@krames
Copy link
Member

krames commented Jan 15, 2014

@Bugagazavr I am trying to determine if this issue is just with the code you patched or if it is a much larger problem.

When I try to retrieve a list of directories with my openstack instance along with the latest fog master and excon 0.31.0, I am not experiencing an issue.

What version of ruby are you using?

@kzaitsev
Copy link
Contributor Author

@krames

ruby 2.1.0p0 (2013-12-25 revision 44422) [x86_64-darwin13.0]

@krames
Copy link
Member

krames commented Jan 15, 2014

I am using 1.9.3. Let me give that a shot.

@kzaitsev
Copy link
Contributor Author

@krames
i switched to ruby 1.9.3

ruby 1.9.3p484 (2013-11-22 revision 43786) [x86_64-darwin13.0.0]

when im use fog from master or rubygems and start precompile assets for rails ( asset_sync )

bugagazavr@kirill ~/Documents/Ruby/teobit (master●●)$ RAILS_ENV=production bundle exec rake assets:precompile
cp public/assets/chosen-sprite-fa9df9bbee9f1ab89799379cb153636e.png public/assets/chosen-sprite.png
cp public/assets/chosen-sprite@2x-5975a8658625306b2570c7c4146f8595.png public/assets/chosen-sprite@2x.png
rake aborted!
undefined method `each' for #<String:0x007fa50680cc00>
/Users/bugagazavr/.rvm/gems/ruby-1.9.3-p484/gems/fog-1.19.0/lib/fog/openstack/models/storage/directories.rb:27:in `get'
/Users/bugagazavr/.rvm/gems/ruby-1.9.3-p484/bundler/gems/asset_sync-cd88696d4819/lib/asset_sync/storage.rb:20:in `bucket'
/Users/bugagazavr/.rvm/gems/ruby-1.9.3-p484/bundler/gems/asset_sync-cd88696d4819/lib/asset_sync/storage.rb:99:in `get_remote_files'
/Users/bugagazavr/.rvm/gems/ruby-1.9.3-p484/bundler/gems/asset_sync-cd88696d4819/lib/asset_sync/storage.rb:211:in `upload_files'
/Users/bugagazavr/.rvm/gems/ruby-1.9.3-p484/bundler/gems/asset_sync-cd88696d4819/lib/asset_sync/storage.rb:233:in `sync'
/Users/bugagazavr/.rvm/gems/ruby-1.9.3-p484/bundler/gems/asset_sync-cd88696d4819/lib/asset_sync/asset_sync.rb:29:in `block in sync'
/Users/bugagazavr/.rvm/gems/ruby-1.9.3-p484/bundler/gems/asset_sync-cd88696d4819/lib/asset_sync/asset_sync.rb:51:in `call'
/Users/bugagazavr/.rvm/gems/ruby-1.9.3-p484/bundler/gems/asset_sync-cd88696d4819/lib/asset_sync/asset_sync.rb:51:in `with_config'
/Users/bugagazavr/.rvm/gems/ruby-1.9.3-p484/bundler/gems/asset_sync-cd88696d4819/lib/asset_sync/asset_sync.rb:28:in `sync'
/Users/bugagazavr/.rvm/gems/ruby-1.9.3-p484/bundler/gems/asset_sync-cd88696d4819/lib/tasks/asset_sync.rake:5:in `block (2 levels) in <top (required)>'
/Users/bugagazavr/.rvm/gems/ruby-1.9.3-p484/bundler/gems/asset_sync-cd88696d4819/lib/tasks/asset_sync.rake:28:in `block in <top (required)>'
/Users/bugagazavr/.rvm/gems/ruby-1.9.3-p484/bin/ruby_executable_hooks:15:in `eval'
/Users/bugagazavr/.rvm/gems/ruby-1.9.3-p484/bin/ruby_executable_hooks:15:in `<main>'
Tasks: TOP => assets:sync
(See full trace by running task with --trace)

Maybe this selectel problem?

@krames
Copy link
Member

krames commented Jan 15, 2014

Maybe. Let's try one more thing.

Can you add EXCON_DEBUG=true to the front of your command and share the output with me. This will show the raw http requests going to our OpenStack cloud, so you will want to remove any sensitive information.

@kzaitsev
Copy link
Contributor Author

@krames i put all excon debug to gist https://gist.github.com/Bugagazavr/8446663#file-fog-txt
i apologize file list is large.

@krames
Copy link
Member

krames commented Jan 16, 2014

Looking at your response it does seem like OpenStack is responding appropriately:

excon.response  {:body=>"[{\"hash\": \"ce57f8c0298ccef30834b4211c2cce05\", \"last_modified\": \"2014-01-15T19:06:25.171580\", \"bytes\": 326251, \"name\": \"assets/portfolio/yakimanka/big-b98fe3cc2755fd1e4a555df5daf29501.png\", ...

I suspect this might be caused by something in your environment. Can I see your Gemfile and Gemfile.lock?

@kzaitsev
Copy link
Contributor Author

@krames
Copy link
Member

krames commented Jan 17, 2014

@Bugagazavr I tried to use your Gemfile to pull down your configuration and I am having problems with rails

Bundler::GemspecError: Could not read gem at /Users/kyle.rames/.rvm/gems/ruby-2.1.0/cache/rails-4.1.0.beta1.gem. It may be corrupted.
An error occurred while installing rails (4.1.0.beta1), and Bundler cannot continue.
Make sure that `gem install rails -v '4.1.0.beta1'` succeeds before bundling.

@geemus I am having problems trying to duplicate @Bugagazavr's problem. It doesn't make sense to me that you would have to JSON decode Excon response data. Any idea what's going on here? Should we merge this or do you think this could be an environmental problem?

@kzaitsev
Copy link
Contributor Author

@krames I think this is rails 4.1, because json reworked in this version ( where I heard it? ).
But amazon S3 works fine.

@krames
Copy link
Member

krames commented Jan 17, 2014

@Bugagazavr have you experienced any other issues using fog with rails 4.1?

@kzaitsev
Copy link
Contributor Author

@krames no, only OpenStack issue

@krames
Copy link
Member

krames commented Jan 17, 2014

@geemus I was not able to duplicate this issue, but because this is a benign change I am inclined to merge it. What do you think we should do here?

I do think there is a bigger issue that is going to resurface it's head at some point though.

@geemus
Copy link
Member

geemus commented Jan 20, 2014

@krames the request method itself should do this decode. At a guess, I'd say the response we are getting here doesn't match application/json as per https://github.com/fog/fog/blob/master/lib/fog/openstack/storage.rb#L175. Maybe that is something to dig in to more? Otherwise I would presume the decode would have already happened?

@krames
Copy link
Member

krames commented Jan 21, 2014

@Bugagazavr Based on @geemus comment I took another look at the json decoding and I noticed that your context-type is being returned as application/json; charset=utf-8. I tested the Openstack code and it appears that Fog is properly reading that content type.

My second thought is that it might be something wrong with your json library. Can you execute the following gist and send me the results?

@kzaitsev
Copy link
Contributor Author

@krames
Copy link
Member

krames commented Jan 22, 2014

I would like to try one more thing. Can I get you to require this patch and then provide me with a gist of the output of code? Thanks for your help on this!

@kzaitsev
Copy link
Contributor Author

@krames
Copy link
Member

krames commented Jan 23, 2014

Interesting!! I was expecting to see something output for type here. That would read me to believe that there is something wrong with the regular expression on this line. I did notice in that your content-type is coming back as application/json; charset=utf-8, but I would have assumed that our regular expression r{application/json} would have been able to identify that.

I updated my gist to include more debugging information around the regular expression. Can I get you to re-run it and send me the output?

Just so you know I am looking for an output like this:

[/v1/AUTH_b5bf8e689bc64844b1d08094a2f2bdd5/] empty? true parse_json: true type: 0
[/v1/AUTH_b5bf8e689bc64844b1d08094a2f2bdd5/] content-type: 'application/json; charset=utf-8'
[/v1/AUTH_b5bf8e689bc64844b1d08094a2f2bdd5/]  %r{application/json} '0'
[/v1/AUTH_b5bf8e689bc64844b1d08094a2f2bdd5/] /application/json/ '0'
[/v1/AUTH_b5bf8e689bc64844b1d08094a2f2bdd5/] /application/json/i '0'
[/v1/AUTH_b5bf8e689bc64844b1d08094a2f2bdd5/] /json/ '12'
[/v1/AUTH_b5bf8e689bc64844b1d08094a2f2bdd5/] DECODING

@kzaitsev
Copy link
Contributor Author

@krames https://gist.github.com/Bugagazavr/8580140
looks like selectel not send content type, but just regexp not working for application/json; charset=utf-8 :[

bugagazavr@Kirills-Mac-Pro ~/Documents/Ruby/teobit_fog (master●●)$ curl -i https://83201.selcdn.ru/awesome/\?format\=json \
> -H "X-Auth-Token: SOMETOKEN"
HTTP/1.1 200 OK
Date: Thu, 23 Jan 2014 15:19:32 GMT
Server: Selectel_Storage/1.0
content-length: 84650
x-container-object-count: 392
accept-ranges: bytes
x-container-bytes-used: 13127082
x-timestamp: 1389806016.94234
x-container-meta-type: public
x-container-meta-cache-control: public
content-type: application/json; charset=utf-8
X-Container-Domains: 
Access-Control-Allow-Origin: *
Access-Control-Expose-Headers: x-container-object-count, x-container-bytes-used, x-timestamp, x-container-meta-type, x-container-meta-cache-control, x-container-domains

[{"hash": ...

@krames
Copy link
Member

krames commented Jan 23, 2014

Try adding an accept header to your curl command -H "Accept: application/json". It might make a difference.

@kzaitsev
Copy link
Contributor Author

bugagazavr@Kirills-Mac-Pro ~/Documents/Ruby/teobit_fog (master●●)$ curl -i https://83201.selcdn.ru/awesome/\?format\=json \
-H "X-Auth-Token: SOMETOKEN" \
> -H "Accept: application/json"
HTTP/1.1 200 OK
Date: Thu, 23 Jan 2014 15:33:55 GMT
Server: Selectel_Storage/1.0
content-length: 84650
x-container-object-count: 392
accept-ranges: bytes
x-container-bytes-used: 13127082
x-timestamp: 1389806016.94234
x-container-meta-type: public
x-container-meta-cache-control: public
content-type: application/json; charset=utf-8
X-Container-Domains: 
Access-Control-Allow-Origin: *
Access-Control-Expose-Headers: x-container-object-count, x-container-bytes-used, x-timestamp, x-container-meta-type, x-container-meta-cache-control, x-container-domains

[{"hash": ...

@krames
Copy link
Member

krames commented Jan 23, 2014

Looking back at both your curl commands, I do see content-type in both of them. So I think Selectel is returning the right data.

My current thought is that we need to revise the regular expression on this line. I threw a couple different variations of the regular expression in my updated patch. Do you want to run that or play around with the regular expression yourself to get it properly work?

@kzaitsev
Copy link
Contributor Author

@krames ok im test it, maybe use:

response.headers['Content-Type'].include?('application/json')
bugagazavr@Kirills-Mac-Pro ~/Documents/Ruby/teobit_fog (master●●)$ irb
>> content_type = 'application/json; charset=utf-8'
=> "application/json; charset=utf-8"
>> content_type.include?('application/json')
=> true

@geemus
Copy link
Member

geemus commented Jan 23, 2014

@krames hey, in your code you are looking for response.headers['Content-Type'] and in this case it is not capitalized (so it won't even have a value to match against). If you change to use response.get_header('Content-Type') it should do a case-insensitive header lookup (for which I suspect we'll see better luck with that regex). Sorry I didn't catch that sooner, hopefully that will get us back on track though.

@krames
Copy link
Member

krames commented Jan 23, 2014

@geemus Thanks for the tip! Let me update the gist accordingly.

@krames
Copy link
Member

krames commented Jan 23, 2014

@Bugagazavr I updated my patch based on @geemus suggestion. Let's see if this works any better.

https://gist.github.com/krames/8559613

@kzaitsev
Copy link
Contributor Author

@krames yeah it works

bugagazavr@Kirills-Mac-Pro ~/Documents/Ruby/teobit_fog (master●●)$ RAILS_ENV=production rake assets:precompile
USING PATCHED CARRIERWAVE
[fog][WARNING] PATCHING Fog::Storage::OpenStack to added debugging information to JSON decoding
cp public/assets/chosen-sprite-d097caf71641f3afb04998af85b36b33.png public/assets/chosen-sprite.png
cp public/assets/chosen-sprite-fa9df9bbee9f1ab89799379cb153636e.png public/assets/chosen-sprite.png
cp public/assets/chosen-sprite@2x-5975a8658625306b2570c7c4146f8595.png public/assets/chosen-sprite@2x.png
cp public/assets/chosen-sprite@2x-f574a81d811e99d629b5b35cbfee5dae.png public/assets/chosen-sprite@2x.png
[/teobit] empty? true parse_json: true type: 0
[/teobit] content-type: 'application/json; charset=utf-8'
[/teobit]  %r{application/json} '0'
[/teobit] /application/json/ '0'
[/teobit] /application/json/i '0'
[/teobit] /json/ '12'
[/teobit] DECODING
[/teobit] empty? true parse_json: true type: 0
[/teobit] content-type: 'application/json; charset=utf-8'
[/teobit]  %r{application/json} '0'
[/teobit] /application/json/ '0'
[/teobit] /application/json/i '0'
[/teobit] /json/ '12'
[/teobit] DECODING

@krames
Copy link
Member

krames commented Jan 23, 2014

@Bugagazavr excellent!! Do you want to revise your PR to use get_header? Or do you want me to create a new PR with the fix in it?

@kzaitsev
Copy link
Contributor Author

@krames i send fix in my PR. Need rebase commits?

@krames
Copy link
Member

krames commented Jan 23, 2014

@Bugagazavr Thanks! It would be nice if you would rebase, but it's not necessary.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.19%) when pulling 27ecbc2 on Bugagazavr:master into 5dad9a5 on fog:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 27ecbc2 on Bugagazavr:master into 5dad9a5 on fog:master.

* 'master' of github.com:fog/fog: (58 commits)
  make disassociate_address mock idempotent, by not requiring instance data
  Fixes fog#2586
  A hack to fix the Claim#messages= hack on 1.8.7.
  Replace the JSON round-trip with #stringify.
  Missed a chance to use queue.claim!.
  Include the oldest and newest message in stats.
  I guess there isn't really a better place.
  Don't use `&:to_h` style enumerations.
  Only extend the TTL of a MockMessage.
  Make the delete_message mock consistent.
  Yep, just did that.
  Er, actually enable queues_tests, too.
  Refactor PATH_BASE into a constant.
  Enable the queues_tests in mocking mode.
  Er, *actually* enable the messages_tests for mocks.
  Enable the queue_tests in mocking mode.
  Enable the messages_tests in mocking mode.
  Enable the message_tests in mocking mode.
  Enable the claims_tests in mocking mode.
  Enable model tests for Claims.
  ...
@kzaitsev
Copy link
Contributor Author

im squash my commits in one

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling f7c22c8 on Bugagazavr:master into 5a601ca on fog:master.

krames pushed a commit that referenced this pull request Jan 24, 2014
[OpenStack] Decode body to array
@krames krames merged commit 28e8259 into fog:master Jan 24, 2014
@krames
Copy link
Member

krames commented Jan 24, 2014

@Bugagazavr Thanks for all the help on this! 👍

@geemus
Copy link
Member

geemus commented Jan 24, 2014

Thanks for both of your patience and help working through this!

@krames
Copy link
Member

krames commented Jan 24, 2014

@geemus I searched though the code base and it appears that there are a couple other places in fog where this is an issue. I am going to try to submit a pr for it later today.

@geemus
Copy link
Member

geemus commented Jan 24, 2014

@krames - good call, hadn't even thought about that. I suppose in some cases it may not matter as much (ie AWS is unlikely to suddenly change capitalization, though it may vary between different openstack providers).

@krames
Copy link
Member

krames commented Jan 31, 2014

@geemus I grepped through the fog code base and this problem seems to be particularly prevalent. Rather than update all these references to get_header, I think we should update response.header to be case insensitive to prevent this from happening in the future.

I created the following excon issue excon/excon#366 for this.

@geemus
Copy link
Member

geemus commented Jan 31, 2014

Thanks!

On Fri, Jan 31, 2014 at 1:43 PM, Kyle Rames notifications@github.comwrote:

@geemus https://github.com/geemus I grepped through the fog code base
and this problem seems to be particularly prevalent. Rather than update all
these references to get_header, I think we should update response.headerto be case insensitive to prevent this from happening in the future.

I created the following excon issue excon/excon#366https://github.com/geemus/excon/issues/366for this.

Reply to this email directly or view it on GitHubhttps://github.com//pull/2578#issuecomment-33835411
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants