From 547bcc58ccee0af8b0b02bf86c5137269e6baa18 Mon Sep 17 00:00:00 2001 From: David Stosik Date: Thu, 5 Mar 2020 13:34:46 +0900 Subject: [PATCH 1/4] Fix spec label --- spec/active_json/base_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/active_json/base_spec.rb b/spec/active_json/base_spec.rb index 236ba66b..a828330c 100644 --- a/spec/active_json/base_spec.rb +++ b/spec/active_json/base_spec.rb @@ -23,7 +23,7 @@ class Empty < ActiveJSON::Base ; end # Empty JSON expect(State.first.name).to match(/^New York/) end - it 'can load empty yaml' do + it 'can load empty JSON' do expect(Empty.first).to be_nil end end From 3db1f7979d90bd9230d3cb3af3c425c79c2a6301 Mon Sep 17 00:00:00 2001 From: David Stosik Date: Thu, 5 Mar 2020 13:34:59 +0900 Subject: [PATCH 2/4] Write a spec that triggers the bug I'm trying to fix The expectation is that the code won't trigger an exception, but it does: ActiveJSON::Base.create does not fail when the loaded JSON was empty Failure/Error: Empty.create() NoMethodError: undefined method `length' for nil:NilClass # ./lib/active_hash/base.rb:135:in `insert' # ./lib/active_hash/base.rb:497:in `save' # ./lib/active_hash/base.rb:174:in `create' # ./spec/active_json/base_spec.rb:51:in `block (3 levels) in ' --- spec/active_json/base_spec.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/spec/active_json/base_spec.rb b/spec/active_json/base_spec.rb index a828330c..4c5ace25 100644 --- a/spec/active_json/base_spec.rb +++ b/spec/active_json/base_spec.rb @@ -46,6 +46,12 @@ class Empty < ActiveJSON::Base ; end # Empty JSON end end + describe ".create" do + it "does not fail when the loaded JSON was empty" do + Empty.create() + end + end + describe ".delete_all" do context "when called before .all" do it "causes all to not load data" do From 5312cbb1c137e96bdf54f3ddb5e608e46fc97f8a Mon Sep 17 00:00:00 2001 From: David Stosik Date: Thu, 5 Mar 2020 13:37:52 +0900 Subject: [PATCH 3/4] Fix bug where records can't be added to an originally empty ActiveJSON datastore Simple fix would be to replace `@records = nil` with `@records = []`, but the suggested approach as a better impact on the code base, avoiding repetitions such as `@records || []`. --- lib/active_hash/base.rb | 17 +++++++++++------ lib/enum/enum.rb | 2 +- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/lib/active_hash/base.rb b/lib/active_hash/base.rb index b326fd1c..fdcbf569 100644 --- a/lib/active_hash/base.rb +++ b/lib/active_hash/base.rb @@ -117,7 +117,7 @@ def data def data=(array_of_hashes) mark_dirty - @records = nil + records.clear reset_record_index self._data = array_of_hashes if array_of_hashes @@ -135,13 +135,12 @@ def exists?(record) end def insert(record) - @records ||= [] record[:id] ||= next_id validate_unique_id(record) if dirty mark_dirty - add_to_record_index({ record.id.to_s => @records.length }) - @records << record + add_to_record_index({ record.id.to_s => records.length }) + records << record end def next_id @@ -153,6 +152,12 @@ def next_id end end + def records + @records ||= [] + end + + private :records + def record_index @record_index ||= {} end @@ -193,7 +198,7 @@ def create!(attributes = {}) end def all(options = {}) - ActiveHash::Relation.new(self, @records || [], options[:conditions] || {}) + ActiveHash::Relation.new(self, records, options[:conditions] || {}) end delegate :where, :find, :find_by, :find_by!, :find_by_id, :count, :pluck, :pick, :first, :last, :order, to: :all @@ -211,7 +216,7 @@ def transaction def delete_all mark_dirty reset_record_index - @records = [] + records.clear end def fields(*args) diff --git a/lib/enum/enum.rb b/lib/enum/enum.rb index c331f389..4001f026 100644 --- a/lib/enum/enum.rb +++ b/lib/enum/enum.rb @@ -21,7 +21,7 @@ def insert(record) def delete_all if @enum_accessors.present? - @records.each do |record| + records.each do |record| constant = constant_for(record, @enum_accessors) remove_const(constant) if const_defined?(constant, false) end From 4ccff1a011095e5557662f024573b529620e0fc7 Mon Sep 17 00:00:00 2001 From: David Stosik Date: Thu, 5 Mar 2020 13:50:37 +0900 Subject: [PATCH 4/4] Make use of `delete_all` in `data=` for clarity --- lib/active_hash/base.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/active_hash/base.rb b/lib/active_hash/base.rb index fdcbf569..1b7bed91 100644 --- a/lib/active_hash/base.rb +++ b/lib/active_hash/base.rb @@ -116,9 +116,7 @@ def data end def data=(array_of_hashes) - mark_dirty - records.clear - reset_record_index + delete_all self._data = array_of_hashes if array_of_hashes auto_assign_fields(array_of_hashes)