keys should be symbolized before merge

[AlexanderZagaynov]

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1718209

[juliancheal]

👍 LGTM

[miq-bot]

@juliancheal unrecognized command 'asign', ignoring...

Accepted commands are: add_label, add_reviewer, assign, close_issue, move_issue, remove_label, rm_label, remove_reviewer, set_milestone

[juliancheal]

@miq-bot assign @abellotti

[AlexanderZagaynov]

@miq-bot add_label ivanchuk/yes

[lpichler]

@AlexanderZagaynov can you add some spec for this fix ?
thanks!

[lpichler]

@AlexanderZagaynov and where is the issue ?

this works:

{'aa' => 'ddd'}.reverse_merge(:u => 'aa').symbolize_keys!

[AlexanderZagaynov]

@lpichler what do you mean "this works"? Try with hsh = { 'a' => 'b' }.reverse_merge(:a => 'c'), and then hsh.symbolize_keys!

[lpichler]

 hsh = { 'a' => 'b' }.reverse_merge(:a => 'c')
hsh.symbolize_keys! 

returns

{
    :a => "b"
}

I just don't understand what is the issue because from code:

in creds:

{
    "password" => "abc123"
}

there you do:

 creds.reverse_merge!(:userid => "whatever")

you have in creds

creds
{
    "password" => "abc123",
       :userid => "testuser"
}

and then finally creds.symbolize_keys! returns:

{
    :password => "abc123",
      :userid => "testuser"
}

what looks same result as with you change.

so what is the issue or what I missed ?

[AlexanderZagaynov]

@lpichler issue is that we have { 'userid' => 'something' } in the hash, and then we trying to merge there :userid. See https://bugzilla.redhat.com/show_bug.cgi?id=1718209#c7

[juliancheal]

@lpichler The problem was, the data from the api request was "userid" => "foo", but in this line https://github.com/ManageIQ/manageiq-api/blob/master/app/controllers/api/hosts_controller.rb#L18
creds.reverse_merge!(:userid => host.authentication_userid(auth_type))

if there was no userid already it would return nil ie. :userid => nil

which would then result in "userid" => "foo", :userid => nil

Once they were symbolised we would only get :userid => nil in the hash.

Therefore when we got to update_authentications method it would delete the new creds we were updating as the userid was nil so it would think it was a delete.

[lpichler]

ah I see thanks @juliancheal for explanation!
@AlexanderZagaynov now I understand

to be honest it is pretty hidden what is going on in code

I would to like prefer something like this (it is pseudocode):

if host.authentication_userid(auth_type)
    use userid from host.authentication_userid(auth_type)
else if there is userid in creds
   use userid from params
else
... 
end

what do you think about it ?
can you also cover it by specs ?

thanks!

[AlexanderZagaynov]

@lpichler I believe the simplest changes are better. This PR is quite obvious, and if we'll start inventions and refactoring, we're at risk to never finish that.
P.S. I would like to refactor it totally, to be honest :)

[skateman]

I'd rather prefer not using mutability here and drop the exclamation marks:

Suggested change

	hash[auth_type.to_sym] = creds
	hash[auth_type.to_sym] = creds.symbolize_keys.reverse_merge(:userid => host.authentication_userid(auth_type))

[AlexanderZagaynov]

I just following existing pattern, trying to KISS

[lpichler]

@lpichler I believe the simplest changes are better. This PR is quite obvious, and if we'll start inventions and refactoring, we're at risk to never finish that.
P.S. I would like to refactor it totally, to be honest :)

it is hard to read from code what has bigger priority if params are comming request or from host - who remembers behaviour of reverse_merge with same keys ?

but ok if you are going to refactor it - so just add some specs and I can merge it ;)

thanks

[jvlcek]

@juliancheal Thank you for explaining the issue above.
@AlexanderZagaynov +1 to keeping this change simple.
@lpichler +1 to add some specs.

Once spec are added LGTM

[AlexanderZagaynov]

@lpichler

who remembers behaviour of reverse_merge with same keys?

keys are not the same, that's the problem. they was string and symbol - it's a different objects. reverse_merge has no any relation to that :)

I added the spec. Here is the result without my change:

Failures:

  1) hosts API editing a host's password with an appropriate role prevents duplicate string/symbol keys mess
     Failure/Error:
       expect do
         post api_host_url(nil, host), :params => gen_request(:edit, options)
       end.to change { host.reload.authentication_password(:default) }.to("abc123")
     
       expected `host.reload.authentication_password(:default)` to have changed to "abc123", but did not change
     # ./spec/requests/hosts_spec.rb:32:in `block (4 levels) in <top (required)>'

Finished in 3.94 seconds (files took 7.11 seconds to load)
1 example, 1 failure

Failed examples:

rspec ./spec/requests/hosts_spec.rb:26 # hosts API editing a host's password with an appropriate role prevents duplicate string/symbol keys mess

With this change it is green.

[lpichler]

keys are not the same, that's the problem. they was string and symbol - it's a different objects. reverse_merge has no any relation to that :)

with you change keys are also same objects.

creds.symbolize!
=> {:userid=>'11'}

creds.reverse_merge!(:userid =>'222')
=>  {:userid=>'11'} or  {:userid=>'222'} ?  it is hard to know.

I still think that using .reverse_merge! does not support readability of code when you can have same keys.

[AlexanderZagaynov]

@lpichler I guess you still didn't understand the issue reason ((

[miq-bot]

Checked commits AlexanderZagaynov/manageiq-api@7842948~...4cf6368 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

[AlexanderZagaynov]

@lpichler maybe if you'll run my spec without that change, you'll get what I mean :)

[jvlcek]

LGTM! Thank you @AlexanderZagaynov
Please merge when green.

[AlexanderZagaynov]

For everybody who don't understand what reverse_merge does: think of it as given_options_hash.apply_defaults(...). If value is given - it will stay on place. If there is no such key - it will be taken from defaults.

[simaishi]

Ivanchuk backport details:

$ git log -1
commit 528cf9fedd907578bf5bc146830a0e54bc2c78d4
Author: Libor Pichler <lpichler@redhat.com>
Date:   Thu Jul 25 17:39:52 2019 +0200

    Merge pull request #631 from AlexanderZagaynov/BZ-1718209_update_host_credentials
    
    keys should be symbolized before merge
    
    (cherry picked from commit 2f85ef4e39f14555d556bfa7e3001f8fd8f18a76)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1718209