From 989d670f93409dce21ca59030be4eb2506aa9ffe Mon Sep 17 00:00:00 2001 From: kpopko Date: Sat, 9 Dec 2023 11:32:19 +0200 Subject: [PATCH 01/12] Fix a case when `v1/action/query` returns only InternalServerError for the queries with excluded fields --- lib/iron_bank/describe/excluded_fields.rb | 60 +++++++++++- .../describe/excluded_fields_spec.rb | 92 +++++++++---------- 2 files changed, 105 insertions(+), 47 deletions(-) diff --git a/lib/iron_bank/describe/excluded_fields.rb b/lib/iron_bank/describe/excluded_fields.rb index 570f254..11da73e 100644 --- a/lib/iron_bank/describe/excluded_fields.rb +++ b/lib/iron_bank/describe/excluded_fields.rb @@ -6,6 +6,7 @@ module Describe # current Zuora tenant, despites Zuora clearly marking these fields as # `true` in their Describe API... /rant # + # rubocop:disable Metrics/ClassLength class ExcludedFields extend Forwardable @@ -87,9 +88,65 @@ def valid_query? info "Successful query for #{object_name}" true - rescue IronBank::InternalServerError, IronBank::BadRequestError => e + rescue IronBank::BadRequestError => e @last_failed_fields = extract_fields_from_exception(e) + false + rescue IronBank::InternalServerError + @last_failed_fields = exctract_from_dividing + + false + end + + def exctract_from_dividing + @working_fields = [] + @failed_fields = [] + query_fields = object.query_fields.clone + + divide_and_execute(query_fields) + # Set initial state for object.query_fields + object.query_fields.concat query_fields + + info "Invalid fields '#{@failed_fields}' for #{object_name} query" + + @failed_fields + end + + # rubocop:disable Metrics/MethodLength + # rubocop:disable Metrics/AbcSize + def divide_and_execute(query_fields) + # Clear state before queries + object.query_fields.clear + # We repeat dividing until only one field has left + @failed_fields.push(query_fields.pop) if query_fields.one? + return if query_fields.empty? + + mid = query_fields.size / 2 + left = query_fields[0..mid - 1] + right = query_fields[mid..] + + if execute_query(left) + @working_fields.concat(left) + else + divide_and_execute(left) + end + + if execute_query(right) + @working_fields.concat(right) + else + divide_and_execute(right) + end + end + # rubocop:enable Metrics/MethodLength + # rubocop:enable Metrics/AbcSize + + def execute_query(fields) + object.query_fields.concat(@working_fields + fields) + + object.where({ id: INVALID_OBJECT_ID }) + + true + rescue IronBank::InternalServerError false end @@ -108,5 +165,6 @@ def extract_fields_from_exception(exception) failed_fields end end + # rubocop:enable Metrics/ClassLength end end diff --git a/spec/iron_bank/describe/excluded_fields_spec.rb b/spec/iron_bank/describe/excluded_fields_spec.rb index e863f26..4859ccf 100644 --- a/spec/iron_bank/describe/excluded_fields_spec.rb +++ b/spec/iron_bank/describe/excluded_fields_spec.rb @@ -16,63 +16,63 @@ it "returns an empty array" do expect(call).to eq([]) end - end - describe "when the object does have unqueryable fields" do - let(:object_name) { "Invoice" } - let(:error_message1) { "invalid field for query: invoice.invalidfield1" } - let(:error_message2) { "invalid field for query: invoice.invalidfield2" } - let(:query_fields) { %w[InvalidField2 InvalidField1 ValidField] } - let(:sorted_fields) { call.sort } + context "with invalid fields" do + let!(:fields_count) { query_fields.size } - before do - allow(object).to receive(:query_fields).and_return(query_fields) - - num_query = 0 - - # Querying `Object#where(id: invalid_id)`: - # - The first time ("InvalidField2") raises an exception - # - The second time ("InvalidField1") raises an exception - # - All following calls are successful - allow(object).to receive(:where).with({ id: invalid_id }) do - num_query += 1 - - # NOTE: This ordering is dictated by the order of `query_fields` - case num_query - when 1 then raise IronBank::InternalServerError, error_message2 - when 2 then raise IronBank::InternalServerError, error_message1 - else anything + before do + allow(object).to receive(:query_fields).and_return(query_fields) + + num_query = 0 + + # Querying `Object#where(id: invalid_id)`: + # - The first 2 queries are initial and failed + # - Plus 2 * invalid fields count and its failed + # - All following calls are successful + allow(object).to receive(:where).with({ id: invalid_id }) do + num_query += 1 + + # NOTE: This ordering is dictated by the order of `query_fields` + case num_query + when 0..invalid_fields_count * 2 then raise IronBank::InternalServerError + else anything + end end end - end - it "makes three queries" do - call + context "when 1 invalid field" do + let(:query_fields) { %w[InvalidField1 Fields2 Field3] } + let(:invalid_fields_count) { 1 } - expect(object).to have_received(:where).exactly(3).times - end + it "makes initial 2 queries and 2 * working query fields count queries" do + call - it "returns the unqueryable fields" do - expect(call).to contain_exactly("InvalidField1", "InvalidField2") - end + expect(object).to have_received(:where).exactly(2 + (invalid_fields_count * 2)).times + end - it "returns a sorted array of unqueryable fields" do - expect(call.sort).to eq(call) - end - end + it "returns the failed field" do + expect(call).to contain_exactly("InvalidField1") + end - describe "when the query results in a `InternalServerError` error" do - let(:object_name) { "Account" } + it "returns a sorted array of unqueryable fields" do + expect(call.sort).to eq(call) + end + end - before do - allow(object). - to receive(:where). - with({ id: invalid_id }). - and_raise(IronBank::InternalServerError) - end + context "when 2 invalid fields" do + let(:query_fields) { %w[InvalidField1 InvalidField2 Field3] } + let(:invalid_fields_count) { 2 } - it "raises a `RuntimeError` error" do - expect { call }.to raise_error(RuntimeError, /Could not parse error/) + it "makes initial 2 queries and 2 * working query fields count queries" do + call + + expect(object).to have_received(:where).exactly(2 + (invalid_fields_count * 2)).times + end + + it "returns the failed fields" do + expect(call).to contain_exactly("InvalidField1", "InvalidField2") + end + end end end end From 50fe07774648c30651d64aeb0a6f7c6bbbeba831 Mon Sep 17 00:00:00 2001 From: kpopko Date: Sun, 10 Dec 2023 12:57:08 +0200 Subject: [PATCH 02/12] Fix review notes --- .rubocop.yml | 5 + lib/iron_bank.rb | 2 + lib/iron_bank/describe/excluded_fields.rb | 103 +----------------- .../excluded_fields/deduce_from_query.rb | 74 +++++++++++++ .../excluded_fields/extract_from_message.rb | 52 +++++++++ .../excluded_fields/deduce_from_query_spec.rb | 31 ++++++ .../extract_from_message_spec.rb | 71 ++++++++++++ .../describe/excluded_fields_spec.rb | 77 ++++++------- 8 files changed, 274 insertions(+), 141 deletions(-) create mode 100644 lib/iron_bank/describe/excluded_fields/deduce_from_query.rb create mode 100644 lib/iron_bank/describe/excluded_fields/extract_from_message.rb create mode 100644 spec/iron_bank/describe/excluded_fields/deduce_from_query_spec.rb create mode 100644 spec/iron_bank/describe/excluded_fields/extract_from_message_spec.rb diff --git a/.rubocop.yml b/.rubocop.yml index fc635e4..7e26583 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -26,6 +26,7 @@ Metrics/AbcSize: Exclude: - lib/iron_bank/associations.rb - lib/iron_bank/client.rb + - lib/iron_bank/describe/excluded_fields/deduce_from_query.rb Metrics/BlockLength: Exclude: @@ -44,9 +45,13 @@ Metrics/MethodLength: - lib/iron_bank/resources/product_rate_plan_charge_tier.rb - lib/iron_bank/error.rb - lib/iron_bank/client.rb + - lib/iron_bank/describe/excluded_fields/deduce_from_query.rb Style/ClassAndModuleChildren: EnforcedStyle: nested + Exclude: + - lib/iron_bank/describe/excluded_fields/deduce_from_query.rb + - lib/iron_bank/describe/excluded_fields/extract_from_message.rb Style/MixinUsage: Exclude: diff --git a/lib/iron_bank.rb b/lib/iron_bank.rb index 1747cdf..1ec2677 100644 --- a/lib/iron_bank.rb +++ b/lib/iron_bank.rb @@ -83,6 +83,8 @@ module Operations; end require "iron_bank/client" require "iron_bank/describe/field" require "iron_bank/describe/excluded_fields" +require "iron_bank/describe/excluded_fields/deduce_from_query" +require "iron_bank/describe/excluded_fields/extract_from_message" require "iron_bank/describe/object" require "iron_bank/describe/related" require "iron_bank/describe/tenant" diff --git a/lib/iron_bank/describe/excluded_fields.rb b/lib/iron_bank/describe/excluded_fields.rb index 11da73e..03b9bbc 100644 --- a/lib/iron_bank/describe/excluded_fields.rb +++ b/lib/iron_bank/describe/excluded_fields.rb @@ -6,32 +6,10 @@ module Describe # current Zuora tenant, despites Zuora clearly marking these fields as # `true` in their Describe API... /rant # - # rubocop:disable Metrics/ClassLength class ExcludedFields extend Forwardable - FAULT_FIELD_MESSAGES = Regexp.union( - # Generic fault field - /invalid field for query: \w+\.(\w+)/, - # Invoice Bill Run is not selectable in ZOQL - /Cannot use the (BillRunId) field in the select clause/, - # Invoice Body, implemented as a separate call - /Can only query one invoice (body) at a time/, - # Catalog tier should only query the price field - /use Price or (DiscountAmount) or (DiscountPercentage)/, - # Catalog plan currencies, implemented as a separate call - /When querying for (active currencies)/, - # Catalog charge rollover balance - /You can only query (RolloverBalance) in particular/, - # (Subscription) charge should only query the price field - %r{ - (OveragePrice), - \ Price, - \ (IncludedUnits), - \ (DiscountAmount) - \ or\ (DiscountPercentage) - }x - ).freeze + INVALID_OBJECT_ID = "InvalidObjectId" private_class_method :new @@ -47,8 +25,6 @@ def call private - INVALID_OBJECT_ID = "InvalidObjectId" - attr_reader :object_name, :last_failed_fields def_delegators "IronBank.logger", :info @@ -88,83 +64,14 @@ def valid_query? info "Successful query for #{object_name}" true - rescue IronBank::BadRequestError => e - @last_failed_fields = extract_fields_from_exception(e) + rescue IronBank::BadRequestError, InternalServerError => e + @last_failed_fields = ExtractFromMessage.call(e.message) || + DeduceFromQuery.call(object) - false - rescue IronBank::InternalServerError - @last_failed_fields = exctract_from_dividing + info "Invalid fields '#{@last_failed_fields}' for #{object_name} query" false end - - def exctract_from_dividing - @working_fields = [] - @failed_fields = [] - query_fields = object.query_fields.clone - - divide_and_execute(query_fields) - # Set initial state for object.query_fields - object.query_fields.concat query_fields - - info "Invalid fields '#{@failed_fields}' for #{object_name} query" - - @failed_fields - end - - # rubocop:disable Metrics/MethodLength - # rubocop:disable Metrics/AbcSize - def divide_and_execute(query_fields) - # Clear state before queries - object.query_fields.clear - # We repeat dividing until only one field has left - @failed_fields.push(query_fields.pop) if query_fields.one? - return if query_fields.empty? - - mid = query_fields.size / 2 - left = query_fields[0..mid - 1] - right = query_fields[mid..] - - if execute_query(left) - @working_fields.concat(left) - else - divide_and_execute(left) - end - - if execute_query(right) - @working_fields.concat(right) - else - divide_and_execute(right) - end - end - # rubocop:enable Metrics/MethodLength - # rubocop:enable Metrics/AbcSize - - def execute_query(fields) - object.query_fields.concat(@working_fields + fields) - - object.where({ id: INVALID_OBJECT_ID }) - - true - rescue IronBank::InternalServerError - false - end - - def extract_fields_from_exception(exception) - message = exception.message - - raise "Could not parse error message: #{message}" unless FAULT_FIELD_MESSAGES.match(message) - - failed_fields = Regexp.last_match. - captures. - compact. - map { |capture| capture.delete(" ") } - - info "Invalid fields '#{failed_fields}' for #{object_name} query" - - failed_fields - end end - # rubocop:enable Metrics/ClassLength end end diff --git a/lib/iron_bank/describe/excluded_fields/deduce_from_query.rb b/lib/iron_bank/describe/excluded_fields/deduce_from_query.rb new file mode 100644 index 0000000..860ada8 --- /dev/null +++ b/lib/iron_bank/describe/excluded_fields/deduce_from_query.rb @@ -0,0 +1,74 @@ +# frozen_string_literal: true + +module IronBank + module Describe + # Makes a binary search by query fields to get failed fields + class ExcludedFields::DeduceFromQuery + INVALID_OBJECT_ID = "InvalidObjectId" + + private_class_method :new + + def self.call(object) + new(object).call + end + + def call + exctract_from_dividing + end + + private + + attr_reader :object + + def initialize(object) + @object = object + end + + def exctract_from_dividing + @working_fields = [] + @failed_fields = [] + query_fields = object.query_fields.clone + + divide_and_execute(query_fields) + # Set initial state for object.query_fields + object.query_fields.concat query_fields + + @failed_fields + end + + def divide_and_execute(query_fields) + # Clear state before queries + object.query_fields.clear + # We repeat dividing until only one field has left + @failed_fields.push(query_fields.pop) if query_fields.one? + return if query_fields.empty? + + mid = query_fields.size / 2 + left = query_fields[0..mid - 1] + right = query_fields[mid..] + + if execute_query(left) + @working_fields.concat(left) + else + divide_and_execute(left) + end + + if execute_query(right) + @working_fields.concat(right) + else + divide_and_execute(right) + end + end + + def execute_query(fields) + object.query_fields.concat(@working_fields + fields) + + object.where({ id: INVALID_OBJECT_ID }) + + true + rescue IronBank::InternalServerError + false + end + end + end +end diff --git a/lib/iron_bank/describe/excluded_fields/extract_from_message.rb b/lib/iron_bank/describe/excluded_fields/extract_from_message.rb new file mode 100644 index 0000000..925aa62 --- /dev/null +++ b/lib/iron_bank/describe/excluded_fields/extract_from_message.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +module IronBank + module Describe + # Extracts failed fields from an exception message. + # Returns from a call if an exception message does not contain failed field + class ExcludedFields::ExtractFromMessage + FAULT_FIELD_MESSAGES = Regexp.union( + # Generic fault field + /invalid field for query: \w+\.(\w+)/, + # Invoice Bill Run is not selectable in ZOQL + /Cannot use the (BillRunId) field in the select clause/, + # Invoice Body, implemented as a separate call + /Can only query one invoice (body) at a time/, + # Catalog tier should only query the price field + /use Price or (DiscountAmount) or (DiscountPercentage)/, + # Catalog plan currencies, implemented as a separate call + /When querying for (active currencies)/, + # Catalog charge rollover balance + /You can only query (RolloverBalance) in particular/, + # (Subscription) charge should only query the price field + / + (OveragePrice), + \ Price, + \ (IncludedUnits), + \ (DiscountAmount) + \ or\ (DiscountPercentage) + /x + ).freeze + + private_class_method :new + + def self.call(message) + new(message).call + end + + def call + return unless FAULT_FIELD_MESSAGES.match(message) + + Regexp.last_match.captures.compact.map { |capture| capture.delete(" ") } + end + + private + + attr_reader :message + + def initialize(message) + @message = message + end + end + end +end diff --git a/spec/iron_bank/describe/excluded_fields/deduce_from_query_spec.rb b/spec/iron_bank/describe/excluded_fields/deduce_from_query_spec.rb new file mode 100644 index 0000000..6d70ea6 --- /dev/null +++ b/spec/iron_bank/describe/excluded_fields/deduce_from_query_spec.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +RSpec.describe IronBank::Describe::ExcludedFields::DeduceFromQuery do + let(:working_fields) { %w[ValidField1 ValidField2] } + let(:failed_fields) { %w[InvalidField1 InvalidField2] } + let(:query_fields) { failed_fields + working_fields } + let(:object) { IronBank::Resources.const_get(object_name) } + let(:invalid_id) { described_class::INVALID_OBJECT_ID } + + subject(:call) { described_class.call(object) } + + before do + allow(object).to receive(:query_fields).and_return(query_fields) + + allow(object).to receive(:where).with({ id: invalid_id }) do + if query_fields.include?("InvalidField1") || query_fields.include?("InvalidField2") + raise IronBank::InternalServerError + end + + true + end + end + + describe "Deduce failed fields from query" do + let(:object_name) { "Product" } + + it "extracts failed fields by binary search" do + expect(call).to contain_exactly(*failed_fields) + end + end +end diff --git a/spec/iron_bank/describe/excluded_fields/extract_from_message_spec.rb b/spec/iron_bank/describe/excluded_fields/extract_from_message_spec.rb new file mode 100644 index 0000000..49bea84 --- /dev/null +++ b/spec/iron_bank/describe/excluded_fields/extract_from_message_spec.rb @@ -0,0 +1,71 @@ +# frozen_string_literal: true + +RSpec.describe IronBank::Describe::ExcludedFields::ExtractFromMessage do + subject(:call) { described_class.call(message) } + + describe "when a message could be processed" do + context "and includes generic fault field" do + let(:message) { "invalid field for query: product.invalidfield" } + + it "returns extracted failed fields" do + expect(call).to include("invalidfield") + end + end + + context "and includes Invoice Bill Run" do + let(:message) { "Cannot use the BillRunId field in the select clause" } + + it "returns extracted failed fields" do + expect(call).to include("BillRunId") + end + end + + context "and includes Invoice Body" do + let(:message) { "Can only query one invoice body at a time" } + + it "returns extracted failed fields" do + expect(call).to include("body") + end + end + + context "and catalog tier should only query the price field" do + let(:message) { "use Price or DiscountAmount or DiscountPercentage" } + + it "returns extracted failed fields" do + expect(call).to include("DiscountAmount", "DiscountPercentage") + end + end + + context "and there are catalog plan currencies" do + let(:message) { "When querying for active currencies" } + + it "returns extracted failed fields" do + expect(call).to include("activecurrencies") + end + end + + context "and included rollover balance" do + let(:message) { "You can only query RolloverBalance in particular" } + + it "returns extracted failed fields" do + expect(call).to include("RolloverBalance") + end + end + + context "and subscription should only query the price field" do + let(:message) { "OveragePrice, Price, IncludedUnits, DiscountAmount or DiscountPercentage" } + + it "returns extracted failed fields" do + expect(call).to_not include("Price") + end + end + end + + describe "when a message could not be processed" do + let(:message) { "some invalid message" } + + it "returns from a call" do + expect(call).to be_nil + end + end +end diff --git a/spec/iron_bank/describe/excluded_fields_spec.rb b/spec/iron_bank/describe/excluded_fields_spec.rb index 4859ccf..58c5cd0 100644 --- a/spec/iron_bank/describe/excluded_fields_spec.rb +++ b/spec/iron_bank/describe/excluded_fields_spec.rb @@ -16,62 +16,53 @@ it "returns an empty array" do expect(call).to eq([]) end + end - context "with invalid fields" do - let!(:fields_count) { query_fields.size } - - before do - allow(object).to receive(:query_fields).and_return(query_fields) + describe "when object has the unqueryable fields" do + let(:object_name) { "Account" } + let(:query_fields) { %w[InvalidField1 Fields2 Field3] } - num_query = 0 + before do + allow(object).to receive(:query_fields).and_return(query_fields) - # Querying `Object#where(id: invalid_id)`: - # - The first 2 queries are initial and failed - # - Plus 2 * invalid fields count and its failed - # - All following calls are successful - allow(object).to receive(:where).with({ id: invalid_id }) do - num_query += 1 + num_query = 0 + allow(object).to receive(:where).with({ id: invalid_id }) do + num_query += 1 - # NOTE: This ordering is dictated by the order of `query_fields` - case num_query - when 0..invalid_fields_count * 2 then raise IronBank::InternalServerError - else anything - end + case num_query + when 1 then raise error_class, error_message + else anything end end + end - context "when 1 invalid field" do - let(:query_fields) { %w[InvalidField1 Fields2 Field3] } - let(:invalid_fields_count) { 1 } - - it "makes initial 2 queries and 2 * working query fields count queries" do - call - - expect(object).to have_received(:where).exactly(2 + (invalid_fields_count * 2)).times - end - - it "returns the failed field" do - expect(call).to contain_exactly("InvalidField1") - end + context "query fails with IronBank::BadRequestError" do + let(:error_class) { IronBank::BadRequestError } + let(:error_message) { "BadRequestError message" } - it "returns a sorted array of unqueryable fields" do - expect(call.sort).to eq(call) - end + before do + allow(described_class::ExtractFromMessage).to receive(:call) end - context "when 2 invalid fields" do - let(:query_fields) { %w[InvalidField1 InvalidField2 Field3] } - let(:invalid_fields_count) { 2 } + it "handles an error with ExtractFromMessage" do + call + expect(described_class::ExtractFromMessage).to have_received(:call).with(error_message) + end + end - it "makes initial 2 queries and 2 * working query fields count queries" do - call + context "query fails with InternalServerError" do + let(:error_class) { IronBank::InternalServerError } + let(:error_message) { "InternalServerError message" } - expect(object).to have_received(:where).exactly(2 + (invalid_fields_count * 2)).times - end + before do + allow(described_class::ExtractFromMessage).to receive(:call) + allow(described_class::DeduceFromQuery).to receive(:call).and_return([]) + end - it "returns the failed fields" do - expect(call).to contain_exactly("InvalidField1", "InvalidField2") - end + it "handles an error with DeduceFromQuery" do + call + expect(described_class::ExtractFromMessage).to have_received(:call).with(error_message) + expect(described_class::DeduceFromQuery).to have_received(:call).with(object) end end end From ba6b6a2f1926b4a1538a56924d9cc7dc76ae9d0b Mon Sep 17 00:00:00 2001 From: kpopko Date: Sun, 10 Dec 2023 13:03:51 +0200 Subject: [PATCH 03/12] Add a comment for new approach with binary search --- lib/iron_bank/schema.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/iron_bank/schema.rb b/lib/iron_bank/schema.rb index 722185c..377e3f9 100644 --- a/lib/iron_bank/schema.rb +++ b/lib/iron_bank/schema.rb @@ -39,6 +39,9 @@ def self.reset end def self.excluded_fields + # When Zuora return InternalServerError we can not extract fields from the a message. + # For this case we are doing binary search through the query fields and it could be + # expensive due to repeatedly querying. @excluded_fields ||= IronBank::Resources.constants.each.with_object({}) do |resource, fields| fields[resource.to_s] = IronBank::Describe::ExcludedFields.call(object_name: resource) From e3b606afd24abfb0afa0ca00aa75ca33c2ad2aa9 Mon Sep 17 00:00:00 2001 From: kpopko Date: Sun, 10 Dec 2023 13:07:55 +0200 Subject: [PATCH 04/12] Add comment to Rakefile for a new approach with binary search --- Rakefile | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Rakefile b/Rakefile index f07370d..b525df2 100644 --- a/Rakefile +++ b/Rakefile @@ -31,6 +31,9 @@ task :excluded_fields, [:filename] do |_t, args| setup_iron_bank destination = args[:filename] || IronBank.configuration.excluded_fields_file + # When Zuora return InternalServerError we can not extract fields from the a message. + # For this case we are doing binary search through the query fields and it could be + # expensive due to repeatedly querying. fields = IronBank::Schema.excluded_fields.sort.to_h File.write(destination, Psych.dump(fields)) From 5b08b9f1a5aa0c74e9f21d1fe7afd78c543277f9 Mon Sep 17 00:00:00 2001 From: kpopko Date: Tue, 26 Dec 2023 10:48:58 +0200 Subject: [PATCH 05/12] Fixed the review notes. Added additional test cases --- .rubocop.yml | 5 -- Rakefile | 11 +++- .../excluded_fields/deduce_from_query.rb | 61 ++++++++++--------- .../excluded_fields/extract_from_message.rb | 7 ++- lib/iron_bank/schema.rb | 11 +++- .../excluded_fields/deduce_from_query_spec.rb | 44 ++++++++----- 6 files changed, 81 insertions(+), 58 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index 7e26583..fc635e4 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -26,7 +26,6 @@ Metrics/AbcSize: Exclude: - lib/iron_bank/associations.rb - lib/iron_bank/client.rb - - lib/iron_bank/describe/excluded_fields/deduce_from_query.rb Metrics/BlockLength: Exclude: @@ -45,13 +44,9 @@ Metrics/MethodLength: - lib/iron_bank/resources/product_rate_plan_charge_tier.rb - lib/iron_bank/error.rb - lib/iron_bank/client.rb - - lib/iron_bank/describe/excluded_fields/deduce_from_query.rb Style/ClassAndModuleChildren: EnforcedStyle: nested - Exclude: - - lib/iron_bank/describe/excluded_fields/deduce_from_query.rb - - lib/iron_bank/describe/excluded_fields/extract_from_message.rb Style/MixinUsage: Exclude: diff --git a/Rakefile b/Rakefile index b525df2..88e33ec 100644 --- a/Rakefile +++ b/Rakefile @@ -31,9 +31,14 @@ task :excluded_fields, [:filename] do |_t, args| setup_iron_bank destination = args[:filename] || IronBank.configuration.excluded_fields_file - # When Zuora return InternalServerError we can not extract fields from the a message. - # For this case we are doing binary search through the query fields and it could be - # expensive due to repeatedly querying. + # NOTE: In some instances the error message from Zuora will not include the + # unqueryable fields that need to be excluded. When that happens IronBank's + # strategy will be to perform a binary search through the fields listed in the + # query -- at the cost of performance due to repeated requests sent to Zuora + # as it tries to identify the offending field. + # + # See: + # - https://github.com/zendesk/iron_bank/pull/107 fields = IronBank::Schema.excluded_fields.sort.to_h File.write(destination, Psych.dump(fields)) diff --git a/lib/iron_bank/describe/excluded_fields/deduce_from_query.rb b/lib/iron_bank/describe/excluded_fields/deduce_from_query.rb index 860ada8..be9098a 100644 --- a/lib/iron_bank/describe/excluded_fields/deduce_from_query.rb +++ b/lib/iron_bank/describe/excluded_fields/deduce_from_query.rb @@ -2,7 +2,11 @@ module IronBank module Describe - # Makes a binary search by query fields to get failed fields + # NOTE: Beware there could be a performance hit as the search repeatedly + # executes queries to perform the search for invalid fields in the query + # included in the query statement. + + # rubocop:disable Style/ClassAndModuleChildren class ExcludedFields::DeduceFromQuery INVALID_OBJECT_ID = "InvalidObjectId" @@ -13,62 +17,59 @@ def self.call(object) end def call - exctract_from_dividing + query_fields = object.query_fields.clone + + divide_and_execute(query_fields) + # Set initial state for object.query_fields + object.query_fields.concat query_fields + + invalid_fields end private - attr_reader :object + attr_reader :object, :valid_fields, :invalid_fields def initialize(object) @object = object - end - - def exctract_from_dividing - @working_fields = [] - @failed_fields = [] - query_fields = object.query_fields.clone - - divide_and_execute(query_fields) - # Set initial state for object.query_fields - object.query_fields.concat query_fields - - @failed_fields + @valid_fields = [] + @invalid_fields = [] end def divide_and_execute(query_fields) # Clear state before queries object.query_fields.clear # We repeat dividing until only one field has left - @failed_fields.push(query_fields.pop) if query_fields.one? + invalid_fields.push(query_fields.pop) if query_fields.one? return if query_fields.empty? - mid = query_fields.size / 2 - left = query_fields[0..mid - 1] - right = query_fields[mid..] + left, right = divide_fields(query_fields) - if execute_query(left) - @working_fields.concat(left) - else - divide_and_execute(left) - end + execute_or_divide_again(left) + execute_or_divide_again(right) + end + + def divide_fields(query_fields) + mid = query_fields.size / 2 + [query_fields[0..mid - 1], query_fields[mid..]] + end - if execute_query(right) - @working_fields.concat(right) + def execute_or_divide_again(fields) + if execute_query(fields) + valid_fields.concat(fields) else - divide_and_execute(right) + divide_and_execute(fields) end end def execute_query(fields) - object.query_fields.concat(@working_fields + fields) - + object.query_fields.concat(valid_fields + fields) object.where({ id: INVALID_OBJECT_ID }) - true rescue IronBank::InternalServerError false end end + # rubocop:enable Style/ClassAndModuleChildren end end diff --git a/lib/iron_bank/describe/excluded_fields/extract_from_message.rb b/lib/iron_bank/describe/excluded_fields/extract_from_message.rb index 925aa62..b8d0c92 100644 --- a/lib/iron_bank/describe/excluded_fields/extract_from_message.rb +++ b/lib/iron_bank/describe/excluded_fields/extract_from_message.rb @@ -2,8 +2,10 @@ module IronBank module Describe - # Extracts failed fields from an exception message. - # Returns from a call if an exception message does not contain failed field + # Extracts invalid fields from an exception message. + # Returns from a call if an exception message does not contain invalid field + + # rubocop:disable Style/ClassAndModuleChildren class ExcludedFields::ExtractFromMessage FAULT_FIELD_MESSAGES = Regexp.union( # Generic fault field @@ -48,5 +50,6 @@ def initialize(message) @message = message end end + # rubocop:enable Style/ClassAndModuleChildren end end diff --git a/lib/iron_bank/schema.rb b/lib/iron_bank/schema.rb index 377e3f9..c369196 100644 --- a/lib/iron_bank/schema.rb +++ b/lib/iron_bank/schema.rb @@ -39,9 +39,14 @@ def self.reset end def self.excluded_fields - # When Zuora return InternalServerError we can not extract fields from the a message. - # For this case we are doing binary search through the query fields and it could be - # expensive due to repeatedly querying. + # NOTE: In some instances the error message from Zuora will not include the + # unqueryable fields that need to be excluded. When that happens IronBank's + # strategy will be to perform a binary search through the fields listed in the + # query -- at the cost of performance due to repeated requests sent to Zuora + # as it tries to identify the offending field. + # + # See: + # - https://github.com/zendesk/iron_bank/pull/107 @excluded_fields ||= IronBank::Resources.constants.each.with_object({}) do |resource, fields| fields[resource.to_s] = IronBank::Describe::ExcludedFields.call(object_name: resource) diff --git a/spec/iron_bank/describe/excluded_fields/deduce_from_query_spec.rb b/spec/iron_bank/describe/excluded_fields/deduce_from_query_spec.rb index 6d70ea6..66c5026 100644 --- a/spec/iron_bank/describe/excluded_fields/deduce_from_query_spec.rb +++ b/spec/iron_bank/describe/excluded_fields/deduce_from_query_spec.rb @@ -1,31 +1,45 @@ # frozen_string_literal: true RSpec.describe IronBank::Describe::ExcludedFields::DeduceFromQuery do - let(:working_fields) { %w[ValidField1 ValidField2] } - let(:failed_fields) { %w[InvalidField1 InvalidField2] } - let(:query_fields) { failed_fields + working_fields } - let(:object) { IronBank::Resources.const_get(object_name) } - let(:invalid_id) { described_class::INVALID_OBJECT_ID } + let(:valid_fields) { %w[ValidField1 ValidField2] } + let(:invalid_fields) { %w[InvalidField1 InvalidField2] } + let(:query_fields) { (valid_fields + invalid_fields).shuffle } + let(:object_name) { "Product" } + let(:object) { IronBank::Resources.const_get(object_name) } + let(:invalid_object_id) { described_class::INVALID_OBJECT_ID } subject(:call) { described_class.call(object) } before do - allow(object).to receive(:query_fields).and_return(query_fields) - - allow(object).to receive(:where).with({ id: invalid_id }) do - if query_fields.include?("InvalidField1") || query_fields.include?("InvalidField2") - raise IronBank::InternalServerError + allow(object). + to receive(:query_fields). + and_return(query_fields) + allow(object). + to receive(:where). + with({ id: invalid_object_id }) do + raise IronBank::InternalServerError unless (query_fields & invalid_fields).none? end + end + + context "when query has valid fields" do + let(:invalid_fields) { [] } - true + it "does not extract fields " do + expect(call).to eq([]) end end - describe "Deduce failed fields from query" do - let(:object_name) { "Product" } + context "when query has invalid fields" do + let(:valid_fields) { [] } + + it "extracts all query fields " do + expect(call).to contain_exactly(*query_fields) + end + end - it "extracts failed fields by binary search" do - expect(call).to contain_exactly(*failed_fields) + context "when query is a mix of shuffled valid and invalid fields" do + it "extracts the invalid fields" do + expect(call).to contain_exactly(*invalid_fields) end end end From 3ac956ced24f03eae57d6d85442aa32f0e049b65 Mon Sep 17 00:00:00 2001 From: Juris Galang Date: Tue, 26 Dec 2023 11:54:51 -0800 Subject: [PATCH 06/12] last_failed_fields -> invalid_fields --- lib/iron_bank/describe/excluded_fields.rb | 31 ++++++++--------------- 1 file changed, 11 insertions(+), 20 deletions(-) diff --git a/lib/iron_bank/describe/excluded_fields.rb b/lib/iron_bank/describe/excluded_fields.rb index 03b9bbc..6fbdf90 100644 --- a/lib/iron_bank/describe/excluded_fields.rb +++ b/lib/iron_bank/describe/excluded_fields.rb @@ -5,7 +5,6 @@ module Describe # Returns an array of non-queryable fields for the given object in the # current Zuora tenant, despites Zuora clearly marking these fields as # `true` in their Describe API... /rant - # class ExcludedFields extend Forwardable @@ -18,21 +17,21 @@ def self.call(object_name:) end def call - remove_last_failure_fields until valid_query? - + remove_invalid_fields until valid_query? (excluded_fields - single_resource_query_fields).sort end private - attr_reader :object_name, :last_failed_fields + attr_reader :object_name, + :invalid_fields def_delegators "IronBank.logger", :info def_delegators :object, :single_resource_query_fields def initialize(object_name) - @object_name = object_name - @last_failed_fields = nil + @object_name = object_name + @invalid_fields = [] end def object @@ -43,16 +42,12 @@ def excluded_fields @excluded_fields ||= object.excluded_fields.dup end - def remove_last_failure_fields - query_fields = object.query_fields - + def remove_invalid_fields + query_fields = object.query_fields failed_fields = query_fields.select do |field| - last_failed_fields.any? { |failed| field.casecmp?(failed) } + invalid_fields.any? { |failed| field.casecmp?(failed) } end - excluded_fields.push(*failed_fields) - - # Remove the field for the next query query_fields.delete_if { |field| failed_fields.include?(field) } end @@ -60,16 +55,12 @@ def valid_query? # Querying using the ID (which is an indexed field) should return an # empty collection very quickly when successful object.where({ id: INVALID_OBJECT_ID }) - info "Successful query for #{object_name}" - true rescue IronBank::BadRequestError, InternalServerError => e - @last_failed_fields = ExtractFromMessage.call(e.message) || - DeduceFromQuery.call(object) - - info "Invalid fields '#{@last_failed_fields}' for #{object_name} query" - + @invalid_fields = ExtractFromMessage.call(e.message) || + DeduceFromQuery.call(object) + info "Invalid fields '#{@invalid_fields}' for #{object_name} query" false end end From 98e81d172ceedaf4399fc9bfa7492c37b7c10401 Mon Sep 17 00:00:00 2001 From: Juris Galang Date: Tue, 26 Dec 2023 11:57:51 -0800 Subject: [PATCH 07/12] ensure value from invalida_fields will always be an array --- lib/iron_bank/describe/excluded_fields.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/iron_bank/describe/excluded_fields.rb b/lib/iron_bank/describe/excluded_fields.rb index 6fbdf90..e439244 100644 --- a/lib/iron_bank/describe/excluded_fields.rb +++ b/lib/iron_bank/describe/excluded_fields.rb @@ -23,8 +23,7 @@ def call private - attr_reader :object_name, - :invalid_fields + attr_reader :object_name, :invalid_fields def_delegators "IronBank.logger", :info def_delegators :object, :single_resource_query_fields @@ -58,8 +57,10 @@ def valid_query? info "Successful query for #{object_name}" true rescue IronBank::BadRequestError, InternalServerError => e - @invalid_fields = ExtractFromMessage.call(e.message) || + @invalid_fields = Array( + ExtractFromMessage.call(e.message) || DeduceFromQuery.call(object) + ) info "Invalid fields '#{@invalid_fields}' for #{object_name} query" false end From b8552974e209cfa4064460049703127550c9a9ac Mon Sep 17 00:00:00 2001 From: Juris Galang Date: Tue, 26 Dec 2023 11:59:25 -0800 Subject: [PATCH 08/12] memoize return value of #object --- lib/iron_bank/describe/excluded_fields.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/iron_bank/describe/excluded_fields.rb b/lib/iron_bank/describe/excluded_fields.rb index e439244..ed65c17 100644 --- a/lib/iron_bank/describe/excluded_fields.rb +++ b/lib/iron_bank/describe/excluded_fields.rb @@ -34,7 +34,7 @@ def initialize(object_name) end def object - IronBank::Resources.const_get(object_name) + @object ||= IronBank::Resources.const_get(object_name) end def excluded_fields From 4bb56bd3ce577e8777eadc12837c35e7c3ae62e3 Mon Sep 17 00:00:00 2001 From: Juris Galang Date: Tue, 26 Dec 2023 12:04:46 -0800 Subject: [PATCH 09/12] minor layout changes --- .../describe/excluded_fields/deduce_from_query.rb | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/lib/iron_bank/describe/excluded_fields/deduce_from_query.rb b/lib/iron_bank/describe/excluded_fields/deduce_from_query.rb index be9098a..3612f03 100644 --- a/lib/iron_bank/describe/excluded_fields/deduce_from_query.rb +++ b/lib/iron_bank/describe/excluded_fields/deduce_from_query.rb @@ -18,11 +18,8 @@ def self.call(object) def call query_fields = object.query_fields.clone - divide_and_execute(query_fields) - # Set initial state for object.query_fields - object.query_fields.concat query_fields - + object.query_fields.concat(query_fields) invalid_fields end @@ -31,20 +28,20 @@ def call attr_reader :object, :valid_fields, :invalid_fields def initialize(object) - @object = object - @valid_fields = [] + @object = object + @valid_fields = [] @invalid_fields = [] end def divide_and_execute(query_fields) - # Clear state before queries + # clear state before queries object.query_fields.clear - # We repeat dividing until only one field has left + + # we repeat dividing until only a single field remains invalid_fields.push(query_fields.pop) if query_fields.one? return if query_fields.empty? left, right = divide_fields(query_fields) - execute_or_divide_again(left) execute_or_divide_again(right) end From e0d77c781c56aeb99c54edfe0fe088856a3324c7 Mon Sep 17 00:00:00 2001 From: Juris Galang Date: Tue, 26 Dec 2023 12:06:22 -0800 Subject: [PATCH 10/12] minor layout changes --- lib/iron_bank/describe/excluded_fields/extract_from_message.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/iron_bank/describe/excluded_fields/extract_from_message.rb b/lib/iron_bank/describe/excluded_fields/extract_from_message.rb index b8d0c92..3dabbe3 100644 --- a/lib/iron_bank/describe/excluded_fields/extract_from_message.rb +++ b/lib/iron_bank/describe/excluded_fields/extract_from_message.rb @@ -39,7 +39,8 @@ def self.call(message) def call return unless FAULT_FIELD_MESSAGES.match(message) - Regexp.last_match.captures.compact.map { |capture| capture.delete(" ") } + Regexp.last_match.captures.compact. + map { |capture| capture.delete(" ") } end private From af3d72f9135bd5ae1cd9f5b6078256d92c4d3013 Mon Sep 17 00:00:00 2001 From: Juris Galang Date: Tue, 26 Dec 2023 12:55:12 -0800 Subject: [PATCH 11/12] minor improvements on describe/excluded_fields_spec.rb --- .../describe/excluded_fields_spec.rb | 101 ++++++++++++------ 1 file changed, 70 insertions(+), 31 deletions(-) diff --git a/spec/iron_bank/describe/excluded_fields_spec.rb b/spec/iron_bank/describe/excluded_fields_spec.rb index 58c5cd0..614b0b1 100644 --- a/spec/iron_bank/describe/excluded_fields_spec.rb +++ b/spec/iron_bank/describe/excluded_fields_spec.rb @@ -10,12 +10,12 @@ let(:object_name) { "Product" } before do - allow(object).to receive(:where).with({ id: invalid_id }) # Successful query + allow(object). + to receive(:where). + with({ id: invalid_id }) end - it "returns an empty array" do - expect(call).to eq([]) - end + it { is_expected.to be_empty } end describe "when object has the unqueryable fields" do @@ -23,46 +23,85 @@ let(:query_fields) { %w[InvalidField1 Fields2 Field3] } before do - allow(object).to receive(:query_fields).and_return(query_fields) + allow(object). + to receive(:query_fields). + and_return(query_fields) num_query = 0 - allow(object).to receive(:where).with({ id: invalid_id }) do - num_query += 1 - - case num_query - when 1 then raise error_class, error_message - else anything + allow(object). + to receive(:where). + with({ id: invalid_id }) do + num_query += 1 + # the ff is necessary since DeduceFromQuery will result in execution + # of additional queries to the object as it tries to identify by + # elimination the unqueryable field(s) in the query. + case num_query + when 1 then raise error_class, error_message + else anything + end end - end end - context "query fails with IronBank::BadRequestError" do - let(:error_class) { IronBank::BadRequestError } - let(:error_message) { "BadRequestError message" } + context "when query fails with an error" do + let(:error_class) do + [ + IronBank::BadRequestError, + IronBank::InternalServerError + ].sample + end + + let(:error_message) { "error message" } before do - allow(described_class::ExtractFromMessage).to receive(:call) + allow(described_class::ExtractFromMessage). + to receive(:call). + and_return(extract_from_message) + allow(described_class::DeduceFromQuery). + to receive(:call). + and_return(deduction_from_query) end - it "handles an error with ExtractFromMessage" do - call - expect(described_class::ExtractFromMessage).to have_received(:call).with(error_message) - end - end + context "when invalid fields can be extracted from message" do + let(:extract_from_message) { %w[InvalidField1] } + let(:deduction_from_query) { double } - context "query fails with InternalServerError" do - let(:error_class) { IronBank::InternalServerError } - let(:error_message) { "InternalServerError message" } + before { subject } - before do - allow(described_class::ExtractFromMessage).to receive(:call) - allow(described_class::DeduceFromQuery).to receive(:call).and_return([]) + it { is_expected.not_to be_empty } + it { is_expected.to eq(extract_from_message) } + + it "will attempt to extract from error message" do + expect(described_class::ExtractFromMessage). + to have_received(:call). + with(error_message) + end + + it "will not attempt to deduce the invalid from query" do + expect(described_class::DeduceFromQuery). + not_to have_received(:call) + end end - it "handles an error with DeduceFromQuery" do - call - expect(described_class::ExtractFromMessage).to have_received(:call).with(error_message) - expect(described_class::DeduceFromQuery).to have_received(:call).with(object) + context "when invalid fields cannot be extracted from message" do + let(:extract_from_message) { nil } + let(:deduction_from_query) { %w[InvalidField1] } + + before { subject } + + it { is_expected.not_to be_empty } + it { is_expected.to eq(deduction_from_query) } + + it "will attempt to extract from error message" do + expect(described_class::ExtractFromMessage). + to have_received(:call). + with(error_message) + end + + it "will attempt to deduce the invalid from query" do + expect(described_class::DeduceFromQuery). + to have_received(:call). + with(object) + end end end end From 2aeab8c5149f2cefb722bda16c0dd1f552614647 Mon Sep 17 00:00:00 2001 From: Juris Galang Date: Tue, 26 Dec 2023 12:55:39 -0800 Subject: [PATCH 12/12] layout adjustment --- .../describe/excluded_fields/deduce_from_query_spec.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/iron_bank/describe/excluded_fields/deduce_from_query_spec.rb b/spec/iron_bank/describe/excluded_fields/deduce_from_query_spec.rb index 66c5026..a9ce14e 100644 --- a/spec/iron_bank/describe/excluded_fields/deduce_from_query_spec.rb +++ b/spec/iron_bank/describe/excluded_fields/deduce_from_query_spec.rb @@ -17,7 +17,8 @@ allow(object). to receive(:where). with({ id: invalid_object_id }) do - raise IronBank::InternalServerError unless (query_fields & invalid_fields).none? + (query_fields & invalid_fields).none? || + raise(IronBank::InternalServerError) end end