Fix a case when v1/action/query returns only InternalServerError

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))

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"

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
     # `<selectable>true</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,74 +17,53 @@ 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
 
       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

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

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

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)

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