-
-
Notifications
You must be signed in to change notification settings - Fork 110
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
Lotus::Action::Params should deep symbolise keys of itself #70
Comments
@joneslee85 this is related to #53. We don't handle nested levels of params yet. |
@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 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 |
I think this behaviour is correct:
and this behaviour I think it isn't supported now because the class returned is Utils::Hash and it isn't a Utils::Attributes:
I think in Utils::Attributes we should create Utils::Attributes instances for deeped hashes
|
Related: hanami/validations#43 |
@joneslee85 I think that what @AlfonsoUceda is correct. We need to guarantee that [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
|
@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. |
👍 alternative 1 |
#71 adds necessary changes to controller for nested params and also fixes this issue |
This will be fixed with |
@joneslee85 Does params.get('address.street') # => "Main St."
params.get('some.unknown.params') # => nil If it helps, please close this ticket. Thank you! 🐈 |
@jodosha doing |
Has this issue been resolved already? I can still encounter the same issue during Getting Started Guide. This test will always fail since 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 env:
|
@tonytonyjan Thanks for reporting this. That looks like a bug: What happens if you do |
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 |
I found that even though it's a 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
|
@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. |
@tonytonyjan Speaking of the actual def call(params)
puts params[:book]['title'] # => nil
puts params[:book][:title] # => "Confident Ruby"
end This is a bug. |
Is this still open? I'd like to have a try at it with the PR I just submitted. |
hanami/utils#137 should be able to address this issue, such that:
🐹 |
@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. |
Using Ruby 2.2.4p230, accessing the params[:book][:title] #=> nil
params[:book]['title'] #=> 'Confident Ruby'
params.get('book.title') #=> NoMethodError: undefined method `get' |
Are you using |
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? |
@changecase In order to get
Then run |
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'
|
That's odd. Could you check if |
It is not. params.is_a? Hanami::Utils::Attributes #=> false |
Sorry, I meant Would you also be able to paste your
|
@Erol @changecase @changecase To try it, you should generate a new project using this: hanami new --hanami-head=true bookshelf Then edit 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 |
Thanks @jodosha |
I posted following nested params to a controller action:
The controller has access to
params
which is instance ofLotus::Utils::Attributes
. It works fine with 1 level hash, but with nested level hash, it turns out to be not the case. Let's see this example:UPDATE: it seems to be this is an issue of
Lotus::Utils::Attributes
, which is lotus/utils. But I want to bring up a discussion here whether we should provide indifferent access to our params hash or not. I don't find indifferent access hash has any particular merits aside of convenient way to access hash. I'd vote for a string hash.The text was updated successfully, but these errors were encountered: