prevent undefined vault_password error

[AlexanderZagaynov]

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

[AlexanderZagaynov]

@bdunne @agrare please review

[agrare]

@AlexanderZagaynov can you just do @data['vault_password'] ||= inputs.vault_password.to_s if inputs.respond_to?(:vault_password) ? I think that is a little more obvious.

[AlexanderZagaynov]

@agrare it won't work in desired way, it will run the second part more than once:

irb(main):001:0> h = {}; h[:a] ||= b if false; h
=> {}
irb(main):002:0> h = {}; h[:a] ||= (b if false); h
=> {:a=>nil}

[agrare]

@AlexanderZagaynov you mean it'll run inputs.respond_to?(:valut_password) even after @data['vault_password'] is set? That's probably okay but if you're worried about it you could do @data['value_password'] = inputs.vault_password.to_s if [email protected]?('vault_password') && inputs.respond_to(:vault_password)

[AlexanderZagaynov]

No, in your example this key will not be even set.
Anyway, at the end of the day, you just trying to invent what I already have here. Why? :)

[agrare]

Not sure what you mean it won't ever be set...

>> inputs = OpenStruct.new
>> inputs.vault_password = "abcd"
>> data = {}
>> data['value_password'] = inputs.vault_password.to_s if !data.key?('vault_password') && inputs.respond_to?(:vault_password)
=> "abcd"
>> data
=> {"value_password"=>"abcd"}

[agrare]

Why do you need to return "" instead of nil?

Using fetch to set a value if it doesn't exist is not a common pattern and takes extra mental cycles to figure out what you're doing where as ||= is immediately obvious. Its not wrong it just looks strange (just did a quick search and we don't do this anywhere in the codebase)

[AlexanderZagaynov]

@agrare yes, we need to have a string here, further code breaks otherwise.
If we didn't something before it doesn't means it has no right to be here. Think of it as of diversity :)

[agrare]

yes, we need to have a string here, further code breaks otherwise.

Interesting, handling nil is something we should do in the future

If we didn't something before it doesn't means it has no right to be here

But it is reason to ask why you chose to do it that way which is what we were doing above

[AlexanderZagaynov]

@agrare https://gist.github.com/AlexanderZagaynov/160d93b0395ca5fd6674d78b5d15987b

[AlexanderZagaynov]

and here: https://gist.github.com/stevenharman/5664318

[AlexanderZagaynov]

@bdunne do you want me to do anything before you merge?

[bdunne]

@bdunne do you want me to do anything before you merge?

Can you add some tests to cover the use cases?

[AlexanderZagaynov]

@bdunne done

[bdunne]

Here are the test cases I see:

• @data['vault_password'] key doesn't exist
  • inputs does not respond to :vault_password
  • inputs responds to :vault_password and it is a string
  • inputs responds to :vault_password and it is nil (is this possible?)
• @data['vault_password'] value nil
  • inputs does not respond to :vault_password <--Covered by your test
  • inputs responds to :vault_password and it is a string
  • inputs responds to :vault_password and it is nil (is this possible?)
• @data['vault_password'] value is a string (and doesn't get overridden even if there is a inputs.vault_password)
• @data['vault_password'] value is not a string (Is this possible?)
  • inputs does not respond to :vault_password
  • inputs responds to :vault_password and it is a string
  • inputs responds to :vault_password and it is nil (is this possible?)

[AlexanderZagaynov]

@bdunne are you sure we need all of that for such a simple issue?
Isn't it an over-engineering @Fryguy what do you think?

[AlexanderZagaynov]

• inputs responds to :vault_password and it is nil (is this possible?)

as I told here we should return a string, not nil

[bdunne]

@bdunne are you sure we need all of that for a such simple issue?
Isn't it an over-engineering @Fryguy what do you think?

I don't know if we need all of it, that was the test matrix that came to mind. I marked a bunch of the cases as (is this possible?) because I don't know without digging further. The logic seems complicated enough to justify more than a simple @data being an empty hash test.

[AlexanderZagaynov]

@bdunne I've made 3 cases now, please see.
And logic may seems less complicated now, after I refactored that method a bit with the last commit + check gist #132 (comment) - it should also explain something.

[Fryguy]

Reading back here is what I think...

The current implementation is more complicated than @agrare and @bdunne's simpler ||= approach, so all things being equal I'd go with simpler and easier to understand (It took me a while to mentally parse this implementation, whereas the one by @agrare I understood immediately).

Additionally, changing the key you are fetching within fetch is strange. This is akin to modifying a collection while iterating the collection which is generally considered bad practice. Also, this usage of fetch isn't really its intended purpose. fetch is more meant for when a missing key is unexpected and you want an Exception raised (i.e. for when the key should be present nearly 100% of the time). The block/default values just aid in not having to catch the exception, however generally it's preferable to just use [] and ||= (and is more consistent within our code base)

So, I think the simplest, most readable, and consistent code that satisfies the specs is

@data['vault_password'] ||= inputs.respond_to?(:vault_password) ? inputs.vault_password.to_s : ""

# ---

require 'ostruct'

@data = {}
inputs = nil
@data['vault_password'] ||= inputs.respond_to?(:vault_password) ? inputs.vault_password.to_s : ""
@data # => {"vault_password"=>""}

@data = {}
inputs = OpenStruct.new
@data['vault_password'] ||= inputs.respond_to?(:vault_password) ? inputs.vault_password.to_s : ""
@data # => {"vault_password"=>""}

@data = {}
inputs = OpenStruct.new(:vault_password => nil)
@data['vault_password'] ||= inputs.respond_to?(:vault_password) ? inputs.vault_password.to_s : ""
@data # => {"vault_password"=>""}

@data = {}
inputs = OpenStruct.new(:vault_password => "abc")
@data['vault_password'] ||= inputs.respond_to?(:vault_password) ? inputs.vault_password.to_s : ""
@data # => {"vault_password"=>"abc"}

@data = {"vault_password" => "abc"}
inputs = OpenStruct.new(:vault_password => "def")
@data['vault_password'] ||= inputs.respond_to?(:vault_password) ? inputs.vault_password.to_s : ""
@data # => {"vault_password"=>"abc"}

---

With respect to the tests, yeah that list seems overly exhaustive, but it also has a lot of "is this possible?" things implying that @bdunne was questioning those particular lines. So, given what you have and that list, I think the only two additional tests needed are

• Handling :inputs => {} (i.e. inputs are present but no vault_password key inside
• Handling the left side of ||= (i.e. the object already has a vault password, so we don't want to execute against the inputs)

[Fryguy]

@AlexanderZagaynov I just looked at your benchmark code, and it's inaccurate. The suggestion was to use ||= over fetch and your benchmark code uses .key? followed by [] (a get). Benchmarking a pure fetch versus ||= shows that ||= is faster (likely because it doesn't incur the Exception handling overhead).

require 'bundler/inline'
gemfile do
  source 'https://rubygems.org'
  gem "benchmark-ips"
end

h_size = 1_000

Benchmark.ips do |bm|
  bm.report '#fetch' do
    h = Hash.new
    h_size.times do |i|
      k = "k#{i}"
      h.fetch(k) { |k| h[k] = "abc" }
    end
  end
  bm.report '#||=' do
    h = Hash.new
    h_size.times do |i|
      k = "k#{i}"
      h[k] ||= "abc"
    end
  end
end

# Warming up --------------------------------------
#               #fetch   163.000  i/100ms
#                 #||=   181.000  i/100ms
# Calculating -------------------------------------
#               #fetch      1.631k (± 5.2%) i/s -      8.150k in   5.010304s
#                 #||=      1.936k (± 4.3%) i/s -      9.774k in   5.059256s

[AlexanderZagaynov]

Have you seen the second gist I mentioned above? If your second example if key present and value is nil - this conditional equation will always work. In cases where it means some api request - you'll do those requests more than once. That's the point of key presence checking.

[Fryguy]

Yes, that is one of the subtle differences, but does it apply in your case? Are you expecting anything besides String or nil as the value? Even if false was the vault_password value, what do you want to happen?

EDIT: rereading your comment... what do you want to happen when the vault password is nil? I thought you wanted that to be a case where you looked at inputs.

[AlexanderZagaynov]

@Fryguy

Additionally, changing the key you are fetching within fetch is strange. This is akin to modifying a collection while iterating the collection which is generally considered bad practice.

Where did you find that? Did you read the code?

Even if false was the vault_password value, what do you want to happen?

Do you understand what lazy evaluation is, and why it is needed? if I'll write h[:a] ||= long_running_method_which_returns_false, and call that 3 times, how many times we'll get long_running_method_which_returns_false called?

fetch is more meant for when a missing key is unexpected and you want an Exception raised

is it your suggestion or official saying?

The block/default values just aid in not having to catch the exception

I noticed that it is not the first time you saying something without even trying to understand and think. And without any proofs. Here is how this method works:
https://github.com/ruby/ruby/blob/619f82bb6bd913d8dbb8ca5da15ca1fbb4508629/hash.c#L2022
https://github.com/ruby/ruby/blob/619f82bb6bd913d8dbb8ca5da15ca1fbb4508629/hash.c#L2031
As you can see, exception raised only as the last step.

[AlexanderZagaynov]

@agrare I made some changes. Please check, do you think it is simple enough now?

[miq-bot]

Checked commits AlexanderZagaynov/ansible_tower_client_ruby@72c85fe~...ad20b8d 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. ⭐

[Fryguy]

if I'll write h[:a] ||= long_running_method_which_returns_false, and call that 3 times, how many times we'll get long_running_method_which_returns_false called?

3 times. As I've said "that is one of the subtle differences". However, in this PR it makes no difference because your method returns "" not false. In this PR it will execute the right hand side only once.

[] with ||= is much more idiomatic in our code base, so my general preference will be for those aside from an actual requirement to use fetch.

Regardless, while I would prefer to see [], my issue is not with the usage of fetch...it's with the re-assignment within the fetch block.

The block/default values just aid in not having to catch the exception
...
As you can see, exception raised only as the last step.

Ah, yes, you are right...it does not get to the exception bit when a block or default value is present (so that then is also not a factor in the performance difference between fetch and []). However, I don't think that changes my original statement in that their purpose is to avoid the exception which would happen without them.

---

Aside, I'm curious why the need for the reassignment? I'm unfamiliar with the usage patterns here, so I could be wrong, but is the method called many times in that looking up the value would be overly performance intensive, and thus caching necessary? Or is the data hash reused elsewhere that you need the value present? Assigning it back technically also changes the original data, so while I'm not against it, if there was a need to have the original data intact, it would be lost. Put another way, is the following just as acceptable?

@data.fetch("vault_password") { inputs.respond_to?(:vault_password) ? inputs.vault_password.to_s : "" }

I'm more curious than anything else...I don't really have a preference.

---

I am ignoring the rest of your comments because, frankly, they are offensive.

[AlexanderZagaynov]

@Fryguy

However, in this PR it makes no difference because your method returns "" not false

originally it was nil, but due to the core repo peculiarity (#132 (comment)) I had to make string here. However, when/if it will be fixed in the core - it should be fixed here too. So, your proposal is just less universal, without other benefits, and keeps a potential issue here.

it's with the re-assignment within the fetch block

What the difference with the wide-used pattern h = Hash.new { |h, k| h[k] = something }? Or maybe @agrare was wrong here: ManageIQ/manageiq-providers-azure#341 (comment)?

I don't think that changes my original statement in that their purpose is to avoid the exception

I'm giving you the proofs, but you don't. You not reading the whole story (like comment I mentioned above, about nil vs ''), you not trying to understand what is going on. Isn't it true? Its just poisoning me...

[bdunne]

it's with the re-assignment within the fetch block

What the difference with the wide-used pattern h = Hash.new { |h, k| h[k] = something }? Or maybe @agrare was wrong here: ManageIQ/manageiq-providers-azure#341 (comment)?

• Hash.new: "...If a block is specified, it will be called with the hash object and the key, and should return the default value. It is the block's responsibility to store the value in the hash if required."
• Hash#fetch: "...if the optional code block is specified, then that will be run and its result returned."

Due to the way that Hash.new works, I do understand why you originally implemented it in this way. However, the documentation of fetch does not mention setting a value.
The problem is not with a block setting a value, I think my confusion is due to the definition of the word "fetch": " to go or come after and bring or take back". With this definition, I would expect fetch to be a "getter" method, not a "setter" method.
I think all three of use suggested ||= because

• it is intended to assign a value if one does not exist
• it is commonly used in our code base
• it avoids the confusion of a "getter" method setting a value

[AlexanderZagaynov]

Now when I got calm, let me try to explain why I advocated my first approach:

1. hash[key] ||= { sleep 10; nil } - block will always be executed if it was executed once.
2. hash.fetch 'to be a "getter" method' - what about these guys, are they wrong? https://api.rubyonrails.org/classes/ActiveSupport/Cache/Store.html#method-i-fetch