Skip to content
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

[Bugfix] Serialize boolean false values #132

Merged
merged 4 commits into from
Jan 25, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 0.12.1 - 2019/1/24

* 🐛 [BUGFIX] Fix boolean `false` values getting serialized as `null`. Please see PR [#132](https://github.com/procore/blueprinter/pull/132).

## 0.12.0 - 2019/1/16

* 🚀 [FEATURE] Enables the setting of global `:field_default` and `:association_default` option value in the Blueprinter Configuration that will be used as default values for fields and associations that evaluate to nil. [#128](https://github.com/procore/blueprinter/pull/128). Thanks to [@mcclayton](https://github.com/mcclayton).
Expand Down
4 changes: 2 additions & 2 deletions lib/blueprinter/extractors/hash_extractor.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
module Blueprinter
class HashExtractor < Extractor
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]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

@samsongz samsongz Jan 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end
end
end
2 changes: 1 addition & 1 deletion lib/blueprinter/version.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
module Blueprinter
VERSION = '0.12.0'
VERSION = '0.12.1'
end
3 changes: 2 additions & 1 deletion spec/activerecord_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ class Vehicle < ActiveRecord::Base
end

class User < ActiveRecord::Base
attr_accessor :company, :description, :position
attr_accessor :company, :description, :position, :active
has_many :vehicles
end

Expand All @@ -25,6 +25,7 @@ class User < ActiveRecord::Base
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
t.datetime "deleted_at"
t.boolean "active"
end

create_table "vehicles", force: :cascade do |t|
Expand Down
1 change: 1 addition & 0 deletions spec/factories/model_factories.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
company { 'Procore' }
birthday { Date.new(1994, 3, 4) }
deleted_at { nil }
active { false }
end

factory :vehicle do
Expand Down
3 changes: 2 additions & 1 deletion spec/integrations/base_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@
description: 'A person',
company: 'Procore',
birthday: Date.new(1994, 3, 4),
deleted_at: nil
deleted_at: nil,
active: false
}
end
let(:object_with_attributes) { OpenStruct.new(obj_hash) }
Expand Down
14 changes: 14 additions & 0 deletions spec/integrations/shared/base_render_examples.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,20 @@
it('returns json with specified fields') { should eq(result) }
end

context 'Given blueprint has ::field with all data types' do
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

let(:result) { '{"active":false,"birthday":"1994-03-04","deleted_at":null,"first_name":"Meg","id":' + obj_id + '}' }
let(:blueprint) do
Class.new(Blueprinter::Base) do
field :id # number
field :first_name # string
field :active # boolean
field :birthday # date
field :deleted_at # null
end
end
it('returns json with the correct values for each data type') { should eq(result) }
end

context 'Given blueprint has ::fields' do
let(:result) do
'{"id":' + obj_id + ',"description":"A person","first_name":"Meg"}'
Expand Down