Lotus::Action::Params should deep symbolise keys of itself

[hanami]

Lotus::Action::Params should deep symbolise keys of itself

[jodosha]

@joneslee85 this is related to #53. We don't handle nested levels of params yet.

[runlevel5]

@jodosha I don't this is related to #53 at all. I think it is not even related to lotus/controller.

On the second look, it turns out that params which is an instance Lotus::Utils::Attributes, and this instance does not perform deep symbolise keys upon being initialised.

I question should we symbolising keys at all? There aren't any merits of doing so? Well there are, if you consider 'no-GC' issue (not an issue with Ruby 2.2) and DDoS merits. I am aware that underneath Lotus::Utils::Attributes is actually Lotus::Utils::Hash anyway. So I'd like to propose that we drop symbolising hash keys to remove the overhead. Thoughts? /cc @stevehodgkiss

[AlfonsoUceda]

I think this behaviour is correct:

params['user'] #=>  { "first_name" => "Sean", "last_name" => "Bean" }
params['user'].class #=>  Lotus::Utils::Hash
params[:user] #=>  { "first_name" => "Sean", "last_name" => "Bean" }
params[:user].class #=>  Lotus::Utils::Hash

and this behaviour I think it isn't supported now because the class returned is Utils::Hash and it isn't a Utils::Attributes:

params['user']['first_name'] #=>  "Sean"
params[:user][:first_name] #=>  "Sean"

I think in Utils::Attributes we should create Utils::Attributes instances for deeped hashes

params['user'] #=>  { "first_name" => "Sean", "last_name" => "Bean" }
params['user'].class #=>  Lotus::Utils::Attributes
params[:user] #=>  { "first_name" => "Sean", "last_name" => "Bean" }
params[:user].class #=>  Lotus::Utils::Attributes

[stevehodgkiss]

Related: hanami/validations#43

[jodosha]

@joneslee85 I think that what @AlfonsoUceda is correct. We need to guarantee that Utils::Attributes deeply converts nested hashes to instances of Utils::Attributes. This should guarantee indifferent access at all the levels.

[1] pry(main)> require 'lotus/utils/attributes'
=> true
[2] pry(main)> attrs = Lotus::Utils::Attributes.new(a: 1, b: { c: 3 })
=> #<Lotus::Utils::Attributes:0x007fd7eca8eff0 @attributes={"a"=>1, "b"=>{"c"=>3}}>
[3] pry(main)> attrs.get('a')
=> 1 # CORRECT
[4] pry(main)> attrs.get(:a)
=> 1 # CORRECT
[5] pry(main)> attrs.get('b')
=> {"c"=>3}
[6] pry(main)> attrs.get('b').get('c')
NoMethodError: undefined method `get' for {"c"=>3}:Lotus::Utils::Hash
from /Users/luca/.gem/ruby/2.2.0/gems/lotus-utils-0.3.3/lib/lotus/utils/hash.rb:270:in `method_missing'  # INCONSISTENT BEHAVIOR
[7] pry(main)> attrs.get('b')[:c]
=> nil # INDIFFERENT ACCESS ISN'T GUARANTEED
[8] pry(main)> attrs.get('b')['c']
=> 3 # INCONSISTENT ACCESS TO NESTED ATTRIBUTES

RE DDoS and symbols: This is still an issue for Ruby as language. The majority of the VMs that we are trying to target are still affected: MRI 2.2-, JRuby and Rubinius. This is still a concern.

@joneslee85 @stevehodgkiss I think that #53 should take account of another issue too. Right now Params#[] delegates to @attributes to return the value. We should guarantee that the returning value will respond to #[] in case of nested params.

• Alternative 1, Alias Utils::Attributes#get with #[].
• Alternative 2, Let Params#[] to return another instance of Params, when the value is a Hash.

[jodosha]

@joneslee85 Yet I think it's still related to #53, because we can't deeply symbolize if we aren't able to whitelist nested params. Still related to DDoS attacks via symbols.

[AlfonsoUceda]

👍 alternative 1

[stevehodgkiss]

#71 adds necessary changes to controller for nested params and also fixes this issue

[jodosha]

This will be fixed with v0.4.0 when we'll only support VMs with 2.2+ mode.
In that case we could use Utils::Hash and safely symbolize params recursively.

[jodosha]

@joneslee85 Does Params#get fixes your problem? With that API, you can safely access nested attributes with the following syntax:

params.get('address.street') # => "Main St."
params.get('some.unknown.params') # => nil

If it helps, please close this ticket. Thank you! 🐈

[runlevel5]

@jodosha doing get helps. But I guess we should document about it.

[tonytonyjan]

Has this issue been resolved already? I can still encounter the same issue during Getting Started Guide.

This test will always fail since params[:book][:title] returns nil:

it 'creates a new book' do
  action.call(params)

  action.book.id.wont_be_nil
  action.book.title.must_equal params[:book][:title]
end

After changing it to params[:book]['title'], the test passed. I'm not sure whether this is a bug or by design.

env:

• ruby (2.3.0)
• hanami (0.7.2)
• hanami-utils (0.7.1)

[jodosha]

@tonytonyjan Thanks for reporting this. That looks like a bug: params provide "indifferent access" both with strings and symbols. They should be interchangeable.

What happens if you do params.get('book.title')?

[tonytonyjan]

NoMethodError: undefined method `get' for #<Hash:0x007ff0d79fbf58>

params should be a Hanami::Action::Params instead of Hash.

What can I do?

source:

# spec/web/controllers/books/create_spec.rb
require 'spec_helper'
require_relative '../../../../apps/web/controllers/books/create'

describe Web::Controllers::Books::Create do
  let(:action) { Web::Controllers::Books::Create.new }
  let(:params) { Hash[book: { title: 'Confident Ruby', author: 'Avdi Grimm' }] }

  before do
    BookRepository.clear
  end

  it 'creates a new book' do
    action.call(params)
    action.book.id.wont_be_nil
    action.book.title.must_equal params[:book]['title']
  end

  it 'redirects the user to the books listing' do
    response = action.call(params)

    response[0].must_equal 302
    response[1]['Location'].must_equal '/books'
  end
end

[tonytonyjan]

I found that even though it's a Hanami::Action::Params, params[:book][:title] returns nil as well. However, get("book.title") works.

it 'creates a new book' do
  action.call(params)
  byebug
  action.book.id.wont_be_nil
  action.book.title.must_equal params[:book][:title]
end

(byebug) params['hanami.action'].params.class
Web::Controllers::Books::Create::Params
(byebug) params['hanami.action'].params.get('book.title')
"Confident Ruby"
(byebug) params['hanami.action'].params[:book][:title]
nil
(byebug) params['hanami.action'].params[:book]['title']
"Confident Ruby"

[jodosha]

@tonytonyjan Sorry, I misread your initial comment.

We can't offer "indifferent access" for that unit test. There is this Hash in test:

let(:params) { Hash[book: { title: 'Confident Ruby', author: 'Avdi Grimm' }] }

That we reference directly below:

it 'creates a new book' do
  # ...
  action.book.title.must_equal params[:book]['title'] # returns nil
end

That is a plain, ol' school, Ruby Hash. If you construct using symbols, it will fail to reach the expected value if you reference the key as string.

~:ruby-2.3.0 ➜ pry
[1] pry(main)> hash = Hash[book: { title: 'Confident Ruby', author: 'Avdi Grimm' }]
=> {:book=>{:title=>"Confident Ruby", :author=>"Avdi Grimm"}}
[2] pry(main)> hash[:book]['title']
=> nil
[3] pry(main)> hash[:book][:title]
=> "Confident Ruby"
[4] pry(main)>

This is exactly the same that is happening in that test.

[jodosha]

@tonytonyjan Speaking of the actual Params class, you're right that still fails.

def call(params)
  puts params[:book]['title'] # => nil
  puts params[:book][:title] # => "Confident Ruby"
end

This is a bug.

[Erol]

Is this still open? I'd like to have a try at it with the PR I just submitted.

[Erol]

hanami/utils#137 should be able to address this issue, such that:

def call(params)
  params['book']['title'] #=> 'Confident Ruby'
  params[:book][:title] #=> 'Confident Ruby'
  params.get('book.title') #=> 'Confident Ruby'
end

🐹

[jodosha]

@Erol @joneslee85 Thanks for taking care of this, but as we planned long time ago, next version of Hanami will support only Ruby 2.2+ with the purpose of avoid indifferent access. All the params will be accessible only with symbols.

[changecase]

Using Ruby 2.2.4p230, accessing the title key via symbol or the get method in the spec still fails. Accessing the title key via string succeeds.

params[:book][:title]    #=> nil
params[:book]['title']   #=> 'Confident Ruby'
params.get('book.title') #=> NoMethodError: undefined method `get'

[Erol]

Are you using hanami-utils:master?

[changecase]

I'd assume the master branch is what's being pulled down as a dependency of the hanami gem in the Gemfile. I'm using rubygems as the source (giving me hanami-utils 0.7.2). However, it looks like rubygems' copy was last updated in May.

Could the issue just be that the hanami-utils on rubygems isn't recent enough?

[cllns]

@changecase In order to get hanami-utils master, in your Gemfile, add (or update, if it's already there):

gem 'hanami-utils', github: 'hanami/utils'

Then run bundle install

[changecase]

Done. Even so, I still get the same set of responses.

params[:book][:title]    #=> nil
params[:book]['title']   #=> 'Confident Ruby'
params.get('book.title') #=> NoMethodError: undefined method `get'

hanami-utils is now at version 0.8.0.

[Erol]

That's odd. Could you check if params is an instance of Hanami::Utils::Attributes?

[changecase]

It is not.

params.is_a? Hanami::Utils::Attributes #=> false

[Erol]

Sorry, I meant params[:book].

Would you also be able to paste your Gemfile.lock? I've tried and it works with the following Gemfile:

source 'https://rubygems.org'

gem 'bundler'
gem 'rake'
gem 'hanami',       '0.7.3'
gem 'hanami-model', '~> 0.5'
gem 'hanami-utils', github: 'hanami/utils'

[jodosha]

@Erol @changecase Hanami::Utils::Attributes is no longer used and it will be deprecated. It was introduced in the first time because we wanted to provide indifferent access. With the next release we will remove indifferent access, by providing symbol only keys.

---

@changecase To try it, you should generate a new project using this:

hanami new --hanami-head=true bookshelf

Then edit Gemfile by adding these gems:

gem 'dry-types',                require: false, github: 'dry-rb/dry-types'
gem 'dry-logic',                require: false, github: 'dry-rb/dry-logic'
gem 'dry-validation',           require: false, github: 'dry-rb/dry-validation'

Please notice that this last step is required because master version of hanami-validations depends on master of dry-* gems. This is a temporary workaround. With hanami-0.8.0 you don't need to worry about fiddling with Gemfile anymore.

[Erol]

Thanks @jodosha