[Bugfix] Serialize boolean false values #132
Conversation
- Currently there is a bug in the gem that incorrectly renders false boolean values - This adds the attributes and spec to demonstrate the failure - A following commit will fix this bug and pass the specs
- When HashExtractor accessed a field name in the object that resulted in a `false` boolean value, the method should return _that_ value. Instead it would trigger second part of the OR Operator, which would try to lookup `object["false"]` which does not exist thus returning `nil`
| @@ -10,6 +10,20 @@ | |||
| it('returns json with specified fields') { should eq(result) } | |||
| end | |||
|
|
|||
| context 'Given blueprint has ::field with all data types' do | |||
There was a problem hiding this comment.
I wanted to demonstrate the failing boolean test first, but I'd be happy to extend this spec to ensure we have coverage of array and hash field data types as well
| def extract(field_name, object, local_options, options = {}) | ||
| object[field_name] || object[field_name.to_s] | ||
| def extract(field_name, object, _local_options, _options = {}) | ||
| object[field_name] |
There was a problem hiding this comment.
Thanks for noticing this problem!
However this may introduce another issue. So the original intent for object[field_name] || object[field_name.to_s] was to access a hash key that is a String if a Symbol key did not exist.
Can we account for String key access as well?
There was a problem hiding this comment.
I looked for other evidence (beyond this method) that string keys were explicitly supported by the library and couldn't find any in either the public documentation or the tests, please feel free to point out if I've missed something I haven't spent a ton of time w/ this gem.
if string keys are currently supported then it appears to be an implicit and untested byproduct of the code rather than an explicit feature. I could not find any tests that have string field keys and the comments in base.rb https://github.com/procore/blueprinter/blob/64546aa3fe137c28d8209be0cad8e658ca92a74e/lib/blueprinter/base.rb#L24 suggest that the method field should be a symbol.
if that's an expected piece of functionality it feels like following this up to explicitly add tests or documentation would be the right way to maintain it
There was a problem hiding this comment.
Yes, I realized this yesterday too that i did not have any tests for String keys. I originally created the HashExtractor with the intent of accessing both String and Symbol keys, but I only added tests for Symbol keys. That was my oversight.
OK, I'm fine with dropping the implicit support of the String key access in this PR.
Another PR can be made afterwards to explicitly support String keys.
Proposed fix for #130
Currently blueprinter does not serialize boolean
falsevalues, returningnullinsteadChange log