object validation for DataHandlerBase#normalize_hash #7264
Conversation
jeremymv2
changed the title
Signed-off-by: Jeremy J. Miller <[email protected]>
jeremymv2
force-pushed
the
jeremymv2/fix_normalize_hash
branch
from
7b897c1 to
7acbb49
Compare
| @@ -64,10 +64,14 @@ def normalize_hash(object, defaults) | |||
| # Make a normalized result in the specified order for diffing | |||
| result = {} | |||
| defaults.each_pair do |key, default| | |||
| result[key] = object.has_key?(key) ? object[key] : default | |||
| result[key] = object.is_a?(Hash) && object.key?(key) ? object[key] : default | |||
There was a problem hiding this comment.
This is redundant. The new conditional block does the same thing and add the exception handler. We probably should just remove this block entirely.
There was a problem hiding this comment.
Hmm, I am not in agreement with this statement. If we do what you are suggesting, then we end up with a resulting Hash whose keys values are replaced with default values when the key exists in both the original and the defaults Hashes. We want the value from the original Hash preserved 🙂
Quick overview, the normalization here is a three step process:
- Create a new Hash (
results) to hold the normalized data - Iterate over the
defaultsHash, adding values toresultsbut preferring already existing values fromobjectwhen the key exists in both. - Iterate over the
objectHash, adding values toresultsthat don't already exist
We can't change step 2 as it's essential. I've added another commit with a 3rd rspec test showing what the importance of step 2 is: Add items into result but prefer the items that already exist in object over the default value from defaults.
There was a problem hiding this comment.
I think this would benefit from an updated comment describing exactly the above.
There was a problem hiding this comment.
@rhass did I make a convincing enough argument to change your mind? 
There was a problem hiding this comment.
Oh, I failed to mention another important part in overviewing! 🤦♂️ Previously when the line was just object.key?(key) to the left of the ? we'd get a undefined method 'has_key?' for nil:NilClass error whenever object was nil
That's because Ruby calls the has_key? method of a nil object. The NilClass does not have such a method, but it does have the method .is_a? (which it inherits from Object)
When we combine object.is_a?(Hash) && object.has_key?(key) we can guard the right side of the && knowing Ruby evaluates the left side of the && first and stops evaluation if the left is false. When object is nil object.is_a?(Hash) evaluates to false so Ruby doesn't try to evaluate object.has_key?(key), assigns the value from the last part of the ternary and doesn't raise a runtime undefined method exception.
irb(main):005:0> object = nil
=> nil
irb(main):006:0> object.has_key?('foo')
Traceback (most recent call last):
2: from /opt/chefdk/embedded/bin/irb:11:in `<main>'
1: from (irb):6
NoMethodError (undefined method `has_key?' for nil:NilClass)
irb(main):007:0> object.is_a?(Hash) && object.has_key?('foo')
=> false
irb(main):008:0>
Signed-off-by: Jeremy J. Miller <[email protected]>
Signed-off-by: Jeremy J. Miller <[email protected]>
itmustbejj
dismissed
rhass’s stale review
After discussing with @jm, the reason for the additional is_a?(Hash) check is to handle nil values raising noMethod errors for key? method
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Signed-off-by: Jeremy J. Miller [email protected]
Description
Chef Server responses to ChefFS requests pass through a data handler pipeline where they get normalized via
DataHandlerBase#normalize_hash. Without this change, the function will blow up withundefined method 'has_key?' for nil:NilClasswhen the Chef Server response contains unexpected non-json null character.Specifically, this change adds input validation checks to the
DataHandlerBase#normalize_hashmethod so that it can survive being givennulldata responses from older Chef Servers and continue processing the remainder of the objects.If passed anything other than a
Hashobject, the function will not try to iterate over it, but simply return a different Hash filled in with the object's default/normalized values, also informing the user that defaults are being used.Quick overview, the normalization here is a three step process:
results) to hold the normalized data.defaultsHash, adding values toresultsbut preferring already existing values fromobjectwhen the key exists in both.objectHash, adding values toresultsthat don't already exist.Issues Resolved
ZD #18855
This error was encountered when running
knife ec backup ..The (very old version of) Chef Server was returning empty data for one node - when looking at the
serialized_objectit only had\x00. This was causing anilvalue to be passed in as the first argument tonormalize_hash.Check List