diff --git a/Rakefile b/Rakefile index f07370d..88e33ec 100644 --- a/Rakefile +++ b/Rakefile @@ -31,6 +31,14 @@ task :excluded_fields, [:filename] do |_t, args| setup_iron_bank destination = args[:filename] || IronBank.configuration.excluded_fields_file + # 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.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 570f254..ed65c17 100644 --- a/lib/iron_bank/describe/excluded_fields.rb +++ b/lib/iron_bank/describe/excluded_fields.rb @@ -5,32 +5,10 @@ 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 - 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 @@ -39,43 +17,36 @@ 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 - INVALID_OBJECT_ID = "InvalidObjectId" - - 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 - IronBank::Resources.const_get(object_name) + @object ||= IronBank::Resources.const_get(object_name) end 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 @@ -83,30 +54,16 @@ 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::InternalServerError, IronBank::BadRequestError => e - @last_failed_fields = extract_fields_from_exception(e) - + rescue IronBank::BadRequestError, InternalServerError => e + @invalid_fields = Array( + ExtractFromMessage.call(e.message) || + DeduceFromQuery.call(object) + ) + info "Invalid fields '#{@invalid_fields}' for #{object_name} query" 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 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..3612f03 --- /dev/null +++ b/lib/iron_bank/describe/excluded_fields/deduce_from_query.rb @@ -0,0 +1,72 @@ +# frozen_string_literal: true + +module IronBank + module Describe + # 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" + + private_class_method :new + + def self.call(object) + new(object).call + end + + def call + query_fields = object.query_fields.clone + divide_and_execute(query_fields) + object.query_fields.concat(query_fields) + invalid_fields + end + + private + + attr_reader :object, :valid_fields, :invalid_fields + + def initialize(object) + @object = object + @valid_fields = [] + @invalid_fields = [] + end + + def divide_and_execute(query_fields) + # clear state before queries + object.query_fields.clear + + # 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 + + def divide_fields(query_fields) + mid = query_fields.size / 2 + [query_fields[0..mid - 1], query_fields[mid..]] + end + + def execute_or_divide_again(fields) + if execute_query(fields) + valid_fields.concat(fields) + else + divide_and_execute(fields) + end + end + + def execute_query(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 new file mode 100644 index 0000000..3dabbe3 --- /dev/null +++ b/lib/iron_bank/describe/excluded_fields/extract_from_message.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +module IronBank + module Describe + # 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 + /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 + # rubocop:enable Style/ClassAndModuleChildren + end +end diff --git a/lib/iron_bank/schema.rb b/lib/iron_bank/schema.rb index 722185c..c369196 100644 --- a/lib/iron_bank/schema.rb +++ b/lib/iron_bank/schema.rb @@ -39,6 +39,14 @@ def self.reset end def self.excluded_fields + # 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 new file mode 100644 index 0000000..a9ce14e --- /dev/null +++ b/spec/iron_bank/describe/excluded_fields/deduce_from_query_spec.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +RSpec.describe IronBank::Describe::ExcludedFields::DeduceFromQuery do + 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_object_id }) do + (query_fields & invalid_fields).none? || + raise(IronBank::InternalServerError) + end + end + + context "when query has valid fields" do + let(:invalid_fields) { [] } + + it "does not extract fields " do + expect(call).to eq([]) + end + end + + 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 + + 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 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 e863f26..614b0b1 100644 --- a/spec/iron_bank/describe/excluded_fields_spec.rb +++ b/spec/iron_bank/describe/excluded_fields_spec.rb @@ -10,69 +10,99 @@ 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 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 } + describe "when object has the unqueryable fields" do + let(:object_name) { "Account" } + 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 - - # 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 + 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 - it "makes three queries" do - call + context "when query fails with an error" do + let(:error_class) do + [ + IronBank::BadRequestError, + IronBank::InternalServerError + ].sample + end - expect(object).to have_received(:where).exactly(3).times - end + let(:error_message) { "error message" } - it "returns the unqueryable fields" do - expect(call).to contain_exactly("InvalidField1", "InvalidField2") - end + before do + 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 "returns a sorted array of unqueryable fields" do - expect(call.sort).to eq(call) - end - end + context "when invalid fields can be extracted from message" do + let(:extract_from_message) { %w[InvalidField1] } + let(:deduction_from_query) { double } - describe "when the query results in a `InternalServerError` error" do - let(:object_name) { "Account" } + before { subject } - before do - allow(object). - to receive(:where). - with({ id: invalid_id }). - and_raise(IronBank::InternalServerError) - end + 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 + + 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 "raises a `RuntimeError` error" do - expect { call }.to raise_error(RuntimeError, /Could not parse error/) + 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 end