From fa321b562d827115c5823f51d1e371fc715ccff6 Mon Sep 17 00:00:00 2001 From: rferg Date: Tue, 9 Jan 2024 11:39:33 -0500 Subject: [PATCH 1/5] include key in where clause when no matching field --- lib/active_force/active_query.rb | 3 ++- spec/active_force/active_query_spec.rb | 12 ++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/lib/active_force/active_query.rb b/lib/active_force/active_query.rb index 74d5775..458ddab 100644 --- a/lib/active_force/active_query.rb +++ b/lib/active_force/active_query.rb @@ -167,7 +167,8 @@ def raise_if_bind_arity_mismatch(expected_var_count, actual_var_count) def build_conditions_from_hash(hash) hash.map do |key, value| - applicable_predicate mappings[key], value + field = mappings.fetch(key, key&.to_s) + applicable_predicate(field, value) end end diff --git a/spec/active_force/active_query_spec.rb b/spec/active_force/active_query_spec.rb index e8d485e..361298e 100644 --- a/spec/active_force/active_query_spec.rb +++ b/spec/active_force/active_query_spec.rb @@ -296,6 +296,18 @@ end end end + + context 'when given attributes Hash with fields that do not exist on the SObject' do + it 'uses the given key in an eq condition' do + expected = 'SELECT Id FROM table_name WHERE (no_attribute = 1) AND (another_one = 2)' + expect(active_query.where(no_attribute: 1, 'another_one' => 2).to_s).to eq(expected) + end + + it 'uses the given key in an in condition' do + expected = 'SELECT Id FROM table_name WHERE (no_attribute IN (1,2))' + expect(active_query.where(no_attribute: [1, 2]).to_s).to eq(expected) + end + end end describe '#not' do From 412a9f249bd67452d8a597fae07cf15deb2f26c2 Mon Sep 17 00:00:00 2001 From: rferg Date: Tue, 9 Jan 2024 11:44:09 -0500 Subject: [PATCH 2/5] update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6fbe243..0843c8f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## Not released +- Include "invalid" attribute keys in query generated with `.where` (https://github.com/Beyond-Finance/active_force/pull/80) - Fix `.update` and `.update!`: include given `nil` valued attributes in request (https://github.com/Beyond-Finance/active_force/pull/79) - Change `.first` to not query the API if records have already been retrieved (https://github.com/Beyond-Finance/active_force/pull/77) From 26dbedf2ce3a1067e2d0ff4781444b9c221138c2 Mon Sep 17 00:00:00 2001 From: rferg Date: Tue, 9 Jan 2024 13:06:22 -0500 Subject: [PATCH 3/5] raise UnknownFieldError when given invalid attribute key --- lib/active_force/active_query.rb | 15 +++++++++--- spec/active_force/active_query_spec.rb | 32 +++++++++++--------------- spec/active_force/sobject_spec.rb | 15 ++++++++++-- 3 files changed, 39 insertions(+), 23 deletions(-) diff --git a/lib/active_force/active_query.rb b/lib/active_force/active_query.rb index 458ddab..6d0d7c9 100644 --- a/lib/active_force/active_query.rb +++ b/lib/active_force/active_query.rb @@ -4,6 +4,13 @@ module ActiveForce class PreparedStatementInvalid < ArgumentError; end + + class UnknownFieldError < StandardError + def initialize(object, field) + super("unknown field '#{field}' for #{object.name}") + end + end + class RecordNotFound < StandardError attr_reader :table_name, :conditions @@ -45,9 +52,9 @@ def count sfdc_client.query(super.to_s).first.expr0 end - def sum field + def sum(field) raise ArgumentError, 'field is required' if field.blank? - raise ArgumentError, "field '#{field}' does not exist on #{sobject}" unless mappings.key?(field.to_sym) + raise UnknownFieldError.new(sobject, field) unless mappings.key?(field.to_sym) sfdc_client.query(super(mappings.fetch(field.to_sym)).to_s).first.expr0 end @@ -167,7 +174,9 @@ def raise_if_bind_arity_mismatch(expected_var_count, actual_var_count) def build_conditions_from_hash(hash) hash.map do |key, value| - field = mappings.fetch(key, key&.to_s) + field = mappings[key] + raise UnknownFieldError.new(sobject, key) if field.blank? + applicable_predicate(field, value) end end diff --git a/spec/active_force/active_query_spec.rb b/spec/active_force/active_query_spec.rb index 361298e..128f83e 100644 --- a/spec/active_force/active_query_spec.rb +++ b/spec/active_force/active_query_spec.rb @@ -2,11 +2,7 @@ describe ActiveForce::ActiveQuery do let(:sobject) do - double("sobject", { - table_name: "table_name", - fields: [], - mappings: mappings - }) + class_double(ActiveForce::SObject, { table_name: 'table_name', fields: [], mappings: mappings, name: 'TableName' }) end let(:mappings){ { id: "Id", field: "Field__c", other_field: "Other_Field" } } let(:client) { double('client', query: nil) } @@ -32,11 +28,6 @@ result = active_query.where("Text_Label = 'foo'").to_a expect(result).to be_a Array end - - it "should decorate the array of objects" do - expect(sobject).to receive(:decorate) - active_query.where("Text_Label = 'foo'").to_a - end end describe '#blank? delegation' do @@ -298,14 +289,9 @@ end context 'when given attributes Hash with fields that do not exist on the SObject' do - it 'uses the given key in an eq condition' do - expected = 'SELECT Id FROM table_name WHERE (no_attribute = 1) AND (another_one = 2)' - expect(active_query.where(no_attribute: 1, 'another_one' => 2).to_s).to eq(expected) - end - - it 'uses the given key in an in condition' do - expected = 'SELECT Id FROM table_name WHERE (no_attribute IN (1,2))' - expect(active_query.where(no_attribute: [1, 2]).to_s).to eq(expected) + it 'raises UnknownFieldError' do + expect { active_query.where(xyz: 1) } + .to raise_error(ActiveForce::UnknownFieldError, /unknown field 'xyz' for #{sobject.name}/i) end end end @@ -341,6 +327,11 @@ new_query = active_query.find_by field: 123 expect(new_query).to be_nil end + + it 'should raise UnknownFieldError if given invalid field' do + expect { active_query.find_by(invalid: true) } + .to raise_error(ActiveForce::UnknownFieldError, /unknown field 'invalid' for #{sobject.name}/i) + end end describe '#find_by!' do @@ -349,6 +340,11 @@ expect { active_query.find_by!(field: 123) } .to raise_error(ActiveForce::RecordNotFound, "Couldn't find #{sobject.table_name} with {:field=>123}") end + + it 'should raise UnknownFieldError if given invalid field' do + expect { active_query.find_by!(invalid: true) } + .to raise_error(ActiveForce::UnknownFieldError, /unknown field 'invalid' for #{sobject.name}/i) + end end describe '#find!' do diff --git a/spec/active_force/sobject_spec.rb b/spec/active_force/sobject_spec.rb index 9f1adf9..6079108 100644 --- a/spec/active_force/sobject_spec.rb +++ b/spec/active_force/sobject_spec.rb @@ -386,9 +386,9 @@ class IceCream < ActiveForce::SObject expect { Whizbang.sum(nil) }.to raise_error(ArgumentError, 'field is required') end - it 'raises ArgumentError if given invalid field' do + it 'raises UnknownFieldError if given invalid field' do expect { Whizbang.sum(:invalid) } - .to raise_error(ArgumentError, /field 'invalid' does not exist on Whizbang/i) + .to raise_error(ActiveForce::UnknownFieldError, /unknown field 'invalid' for Whizbang/i) end it 'sends the correct query to the client' do @@ -419,6 +419,11 @@ class IceCream < ActiveForce::SObject expect(client).to receive(:query).with("SELECT #{Whizbang.fields.join ', '} FROM Whizbang__c WHERE (Id = 123) AND (Text_Label = 'foo') LIMIT 1") Whizbang.find_by id: 123, text: "foo" end + + it 'raises UnknownFieldError if given invalid field' do + expect { Whizbang.find_by(xyz: 1) } + .to raise_error(ActiveForce::UnknownFieldError, /unknown field 'xyz' for Whizbang/) + end end describe "#find_by!" do @@ -426,10 +431,16 @@ class IceCream < ActiveForce::SObject expect(client).to receive(:query).with("SELECT #{Whizbang.fields.join ', '} FROM Whizbang__c WHERE (Id = 123) AND (Text_Label = 'foo') LIMIT 1").and_return([Restforce::Mash.new(Id: 123, text: 'foo')]) Whizbang.find_by! id: 123, text: "foo" end + it "raises if nothing found" do expect(client).to receive(:query).with("SELECT #{Whizbang.fields.join ', '} FROM Whizbang__c WHERE (Id = 123) AND (Text_Label = 'foo') LIMIT 1") expect { Whizbang.find_by! id: 123, text: "foo" }.to raise_error(ActiveForce::RecordNotFound) end + + it 'raises UnknownFieldError if given invalid field' do + expect { Whizbang.find_by!(xyz: 1) } + .to raise_error(ActiveForce::UnknownFieldError, /unknown field 'xyz' for Whizbang/) + end end describe '.find!' do From 105ee37ef4a23093a88b6ccd241d14011d3fb17e Mon Sep 17 00:00:00 2001 From: rferg Date: Tue, 9 Jan 2024 13:07:21 -0500 Subject: [PATCH 4/5] update changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0843c8f..b267389 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ## Not released -- Include "invalid" attribute keys in query generated with `.where` (https://github.com/Beyond-Finance/active_force/pull/80) +- Raise `UnknownFieldError` if `.where` is given non-existent attribute names (https://github.com/Beyond-Finance/active_force/pull/80) - Fix `.update` and `.update!`: include given `nil` valued attributes in request (https://github.com/Beyond-Finance/active_force/pull/79) - Change `.first` to not query the API if records have already been retrieved (https://github.com/Beyond-Finance/active_force/pull/77) From d039d6357988827f34eb84f33041a5ff334fd8ba Mon Sep 17 00:00:00 2001 From: rferg Date: Tue, 9 Jan 2024 13:19:22 -0500 Subject: [PATCH 5/5] fix decorate spec for ActiveQuery --- spec/active_force/active_query_spec.rb | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/spec/active_force/active_query_spec.rb b/spec/active_force/active_query_spec.rb index 128f83e..db2588b 100644 --- a/spec/active_force/active_query_spec.rb +++ b/spec/active_force/active_query_spec.rb @@ -1,8 +1,17 @@ require 'spec_helper' +class TestSObject < ActiveForce::SObject + def self.decorate(records) + records + end +end + describe ActiveForce::ActiveQuery do let(:sobject) do - class_double(ActiveForce::SObject, { table_name: 'table_name', fields: [], mappings: mappings, name: 'TableName' }) + class_double( + TestSObject, + { table_name: 'table_name', fields: [], mappings: mappings, name: 'TableName' } + ) end let(:mappings){ { id: "Id", field: "Field__c", other_field: "Other_Field" } } let(:client) { double('client', query: nil) } @@ -28,6 +37,11 @@ result = active_query.where("Text_Label = 'foo'").to_a expect(result).to be_a Array end + + it "should decorate the array of objects" do + expect(sobject).to receive(:decorate) + active_query.where("Text_Label = 'foo'").to_a + end end describe '#blank? delegation' do