Simplify instantiate_record_from_json with a cached per-class lookup table

benchmark/from_json.rb

@@ -0,0 +1,100 @@
+# Benchmark for Resource.from_json / instantiate_record_from_json and Utils.underscore.
+#
+#   ruby -Ilib benchmark/from_json.rb                      # synthetic payloads
+#   PAYLOAD_DIR=/path/to/captures ruby -Ilib benchmark/from_json.rb
+#
+# PAYLOAD_DIR: JSON files captured from a real Kill Bill (e.g. with curl), named
+# <ModelName>.<anything>.json, where ModelName is a class under KillBillClient::Model
+# (e.g. Account.json, Invoice.list.json, InvoicePayment.list.json, AccountTimeline.json).
+# Without it, payloads are generated from the models' declared attributes (plus a few
+# unknown fields), which is only a rough stand-in for real responses.
+#
+# Needs the benchmark-ips gem (gem install benchmark-ips).
+require 'json'
+require 'benchmark/ips'
+require 'killbill_client'
+
+Resource = KillBillClient::Model::Resource
+Model = KillBillClient::Model
+
+UNKNOWN_FIELDS = { 'someNewField' => 'x', 'anotherUnknownField' => 42 }.freeze
+
+def camel(name)
+  KillBillClient::Utils.camelize(name, :lower)
+end
+
+# Fills every declared attribute (and has_one/has_many, down to `depth`) of a model.
+def synthetic(clazz, depth = 2)
+  nested = Resource.class_variable_get(:@@attribute_names)
+  hash = {}
+  clazz.ancestors.each do |ancestor|
+    (ancestor.respond_to?(:json_attributes) ? ancestor.json_attributes : []).each { |attr| hash[camel(attr)] ||= "#{attr}-value" }
+    (nested[ancestor.name] || {}).each do |attr, desc|
+      next if desc[:type].nil? || depth.zero?
+      child = synthetic(desc[:type], depth - 1)
+      hash[camel(attr)] = desc[:cardinality] == :many ? [child, child] : child
+    end
+  end
+  hash.merge(UNKNOWN_FIELDS)
+end
+
+def synthetic_list(clazz, n)
+  Array.new(n) { synthetic(clazz, 1) }
+end
+
+def synthetic_timeline(n)
+  synthetic(Model::AccountTimeline, 0).merge(
+    'account' => synthetic(Model::Account, 0),
+    'invoices' => synthetic_list(Model::Invoice, n),
+    'payments' => synthetic_list(Model::InvoicePayment, n),
+    'bundles' => synthetic_list(Model::Bundle, n / 10)
+  )
+end
+
+def load_payloads
+  dir = ENV['PAYLOAD_DIR']
+  if dir
+    Dir[File.join(dir, '*.json')].sort.map do |file|
+      [File.basename(file, '.json'), Model.const_get(File.basename(file).split('.').first), File.read(file)]
+    end
+  else
+    [
+      ['Account (synthetic)', Model::Account, synthetic(Model::Account, 0).to_json],
+      ['Invoice list x50 (synthetic)', Model::Invoice, synthetic_list(Model::Invoice, 50).to_json],
+      ['InvoicePayment list x50 (synthetic)', Model::InvoicePayment, synthetic_list(Model::InvoicePayment, 50).to_json],
+      ['AccountTimeline, 300 invoices/payments (synthetic)', Model::AccountTimeline, synthetic_timeline(300).to_json]
+    ]
+  end
+end
+
+def allocations
+  GC.stat[:total_allocated_objects].to_i
+end
+
+payloads = load_payloads
+puts "#{RUBY_ENGINE} #{RUBY_VERSION} (#{RUBY_PLATFORM}), killbill_client #{KillBillClient::VERSION}"
+puts
+
+puts 'Per payload: size, objects allocated per parse (CRuby only), exceptions raised per parse'
+payloads.each do |label, clazz, json|
+  Resource.from_json(clazz, json) # warm up (fills caches, if any)
+
+  raised = Hash.new(0)
+  trace = TracePoint.new(:raise) { |tp| raised[tp.raised_exception.class] += 1 }
+  before = allocations
+  trace.enable { Resource.from_json(clazz, json) }
+  allocated = allocations - before
+
+  printf("  %-55s %8d bytes  %9s allocations  exceptions: %s\n", label, json.bytesize,
+         allocated.zero? ? 'n/a' : allocated, raised.empty? ? 0 : raised.inspect)
+end
+puts
+
+Benchmark.ips do |x|
+  x.config(:time => 5, :warmup => 3)
+  payloads.each do |label, clazz, json|
+    x.report("from_json #{label}") { Resource.from_json(clazz, json) }
+  end
+  x.report('Utils.underscore (camelCase key)') { KillBillClient::Utils.underscore('invoicePaymentId') }
+  x.report('Utils.underscore (already underscored)') { KillBillClient::Utils.underscore('invoice_payment_id') }
+end

lib/killbill_client/models/resource.rb

@@ -14,6 +14,11 @@ class Resource
 
       @@attribute_names = {}
 
+      UNKNOWN_JSON_ATTRIBUTE = [].freeze
+      # class => { json key => [setter, cardinality, type] | UNKNOWN_JSON_ATTRIBUTE }
+      @@json_attribute_cache = {}
+      @@json_attribute_cache_lock = Mutex.new
+
       def initialize(hash = nil)
 	@uri = nil
         # Make sure we support ActiveSupport::HashWithIndifferentAccess for Kaui
@@ -143,44 +148,81 @@ def from_json(resource_class, json)
         def instantiate_record_from_json(resource_class, data)
           record = resource_class.send :new
 
-          kb_ancestors = resource_class.ancestors.select { |ancestor| !@@attribute_names[ancestor.name].nil? }
+          # One hash lookup per JSON key (see json_attribute_for)
+          table = @@json_attribute_cache[resource_class] || {}
           data.each do |name, value|
-            name = Utils.underscore name
-            attr_desc = nil
+            entry = table[name] || json_attribute_for(resource_class, name)
+            assign_json_attribute(record, entry, value) unless entry.equal?(UNKNOWN_JSON_ATTRIBUTE)
+          end
+
+          record
+        end
 
-            # Allow for inheritance
-            kb_ancestors.each do |ancestor|
-              attr_desc = @@attribute_names[ancestor.name][name.to_sym]
-              break unless attr_desc.nil?
+        def assign_json_attribute(record, entry, value)
+          type = entry[2]
+          unless type.nil?
+            if entry[1] == :many
+              value = build_many(type, value) if value.is_a?(Array)
+            elsif entry[1] == :one
+              value = instantiate_record_from_json(type, value) if value.is_a?(Hash)
             end
+          end
 
-            unless attr_desc.nil?
-              type = attr_desc[:type]
-              if attr_desc[:cardinality] == :many && !type.nil? && value.is_a?(Array)
-                newValue = []
-                value.each do |val|
-                  if val.is_a?(Hash)
-                    newValue << instantiate_record_from_json(type, val)
-                  else
-                    newValue << val
-                  end
-                end
-                value = newValue
-              elsif attr_desc[:cardinality] == :one && !type.nil? && value.is_a?(Hash)
-                value = instantiate_record_from_json(type, value)
-              end
-            end #end unless attr_desc.nil? or data_elem.blank?
+          # TODO Be lenient for now to support different API formats
+          begin
+            record.send(entry[0], value)
+          rescue StandardError
+            nil
+          end
+        end
 
-            # TODO Be lenient for now to support different API formats
-            record.send("#{Utils.underscore name}=", value) rescue nil
+        def build_many(type, values)
+          records = []
+          values.each do |val|
+            records << (val.is_a?(Hash) ? instantiate_record_from_json(type, val) : val)
+          end
+          records
+        end
 
-          end #end data.each
+        # Resolves (and caches) what to do with a JSON key for a given class:
+        # [setter, cardinality, type] or UNKNOWN_JSON_ATTRIBUTE if the class doesn't have a matching setter.
+        #
+        # The cache is copy-on-write: readers never lock (and never see a Hash being mutated), writers are serialized.
+        def json_attribute_for(resource_class, json_key)
+          @@json_attribute_cache_lock.synchronize do
+            table = @@json_attribute_cache[resource_class] || {}
+            return table[json_key] if table.key?(json_key)
+
+            entry = resolve_json_attribute(resource_class, json_key)
+            @@json_attribute_cache = @@json_attribute_cache.merge(resource_class => table.merge(json_key => entry))
+            entry
+          end
+        end
 
-          record
+        def resolve_json_attribute(resource_class, json_key)
+          name = Utils.underscore json_key
+          setter = "#{name}=".to_sym
+          return UNKNOWN_JSON_ATTRIBUTE unless resource_class.method_defined?(setter) || resource_class.private_method_defined?(setter)
+
+          # Allow for inheritance: the closest ancestor declaring the attribute wins
+          attr_desc = nil
+          resource_class.ancestors.each do |ancestor|
+            declared = @@attribute_names[ancestor.name]
+            next if declared.nil?
+            attr_desc = declared[name.to_sym]
+            break unless attr_desc.nil?
+          end
+
+          [setter, attr_desc && attr_desc[:cardinality], attr_desc && attr_desc[:type]].freeze
+        end
+
+        def clear_json_attribute_cache
+          @@json_attribute_cache_lock.synchronize { @@json_attribute_cache = {} }
         end
 
         def attribute(name)
           send('attr_accessor', name.to_sym)
+          clear_json_attribute_cache
           attributes = @json_attributes ||= []
 
           if respond_to?(:json_attributes, true)
@@ -198,6 +240,7 @@ def has_many(attr_name, type = nil)
           #add it to attribute_names
           @@attribute_names[self.name] ||= {}
           @@attribute_names[self.name][attr_name.to_sym] = {:type => type, :cardinality => :many}
+          clear_json_attribute_cache
         end
 
         def has_one(attr_name, type = nil)
@@ -206,13 +249,15 @@ def has_one(attr_name, type = nil)
           #add it to attribute_names
           @@attribute_names[self.name] ||= {}
           @@attribute_names[self.name][attr_name.to_sym] = {:type => type, :cardinality => :one}
+          clear_json_attribute_cache
         end
 
         #hack to cater the api return attributes and javax attributes without editing gen scripts
         #call only after its declared as a instance_method using attr_accessor
         def create_alias(new_name, old_name)
           alias_method new_name.to_sym, old_name.to_sym #getter
           alias_method "#{new_name}=".to_sym, "#{old_name}=".to_sym #setter
+          clear_json_attribute_cache
         end
 
         # Extract the session id from a response

spec/killbill_client/instantiate_record_from_json_spec.rb

@@ -0,0 +1,143 @@
+require 'spec_helper'
+
+# Test-only models (named, since has_one/has_many metadata is keyed by class name)
+module InstantiateRecordSpec
+  class Leaf < KillBillClient::Model::Resource
+    attribute :id
+    attribute :first_name
+  end
+
+  class Base < KillBillClient::Model::Resource
+    attribute :name
+    has_one :owner, Leaf
+    has_many :leaves, Leaf
+  end
+
+  class Child < Base
+    attribute :extra
+    has_many :more_leaves, Leaf
+  end
+end
+
+describe KillBillClient::Model::Resource do
+  let(:leaf) { InstantiateRecordSpec::Leaf }
+  let(:base) { InstantiateRecordSpec::Base }
+  let(:child) { InstantiateRecordSpec::Child }
+
+  def parse(clazz, hash)
+    described_class.from_json(clazz, hash.to_json)
+  end
+
+  describe '.from_json / instantiate_record_from_json' do
+    it 'maps camelCase keys to attributes' do
+      record = parse(leaf, 'id' => 'a', 'firstName' => 'Bob')
+      expect(record.id).to eq('a')
+      expect(record.first_name).to eq('Bob')
+    end
+
+    it 'instantiates nested has_one' do
+      record = parse(base, 'name' => 'n', 'owner' => { 'id' => 'o', 'firstName' => 'Ann' })
+      expect(record.owner).to be_a(leaf)
+      expect(record.owner.first_name).to eq('Ann')
+    end
+
+    it 'instantiates nested has_many, leaving non-hash elements untouched' do
+      record = parse(base, 'leaves' => [{ 'id' => '1' }, { 'id' => '2' }, 'raw'])
+      expect(record.leaves.map(&:class)).to eq([leaf, leaf, String])
+      expect(record.leaves.first(2).map(&:id)).to eq(%w(1 2))
+      expect(record.leaves.last).to eq('raw')
+    end
+
+    it 'does not instantiate when the JSON shape does not match the declaration' do
+      record = parse(base, 'owner' => 'not-a-hash', 'leaves' => { 'id' => '1' })
+      expect(record.owner).to eq('not-a-hash')
+      expect(record.leaves).to eq({ 'id' => '1' })
+    end
+
+    it 'keeps null values' do
+      record = parse(base, 'owner' => nil, 'leaves' => nil)
+      expect(record.owner).to be_nil
+      expect(record.leaves).to be_nil
+    end
+
+    it 'inherits has_one/has_many declarations from ancestors' do
+      record = parse(child, 'name' => 'n', 'extra' => 'e',
+                            'owner' => { 'id' => 'o' },
+                            'leaves' => [{ 'id' => '1' }],
+                            'moreLeaves' => [{ 'id' => '2' }])
+      expect(record.name).to eq('n')
+      expect(record.extra).to eq('e')
+      expect(record.owner).to be_a(leaf)
+      expect(record.leaves.first).to be_a(leaf)
+      expect(record.more_leaves.first).to be_a(leaf)
+    end
+
+    it 'does not leak a subclass declaration to its parent' do
+      record = parse(base, 'moreLeaves' => [{ 'id' => '2' }])
+      expect(record).not_to respond_to(:more_leaves)
+    end
+
+    it 'ignores unknown fields' do
+      record = parse(base, 'name' => 'n', 'doesNotExist' => 1, 'nested' => { 'a' => 1 })
+      expect(record.name).to eq('n')
+      expect(record).not_to respond_to(:does_not_exist)
+    end
+
+    it 'does not use exceptions as control flow for unknown fields' do
+      parse(base, 'name' => 'warm-up', 'doesNotExist' => 1) # fill the cache
+
+      raised = []
+      trace = TracePoint.new(:raise) { |tp| raised << tp.raised_exception.class }
+      trace.enable { parse(base, 'name' => 'n', 'doesNotExist' => 1, 'alsoUnknown' => 2) }
+
+      expect(raised).to be_empty
+    end
+
+    it 'stays lenient when a setter raises' do
+      klass = Class.new(KillBillClient::Model::Resource) do
+        attribute :ok
+        def boom=(_); raise 'boom'; end
+      end
+      record = described_class.from_json(klass, { 'ok' => 1, 'boom' => 2 }.to_json)
+      expect(record.ok).to eq(1)
+    end
+
+    it 'parses arrays of records' do
+      records = parse(base, [{ 'name' => 'a' }, { 'name' => 'b' }])
+      expect(records.map(&:name)).to eq(%w(a b))
+    end
+
+    it 'picks up declarations added after a first parse' do
+      klass = Class.new(KillBillClient::Model::Resource) { attribute :id }
+      expect(described_class.from_json(klass, { 'id' => 1, 'late' => 2 }.to_json)).not_to respond_to(:late)
+
+      klass.send(:attribute, :late)
+      expect(described_class.from_json(klass, { 'id' => 1, 'late' => 2 }.to_json).late).to eq(2)
+    end
+
+    it 'is safe to use from several threads' do
+      payload = { 'name' => 'n', 'owner' => { 'id' => 'o' }, 'leaves' => [{ 'id' => '1' }], 'unknownKey' => 1 }.to_json
+      results = 8.times.map do
+        Thread.new { 50.times.map { described_class.from_json(base, payload) } }
+      end.flat_map(&:value)
+      expect(results.map { |r| [r.name, r.owner.id, r.leaves.first.id] }.uniq).to eq([%w(n o 1)])
+    end
+  end
+
+  it 'parses an AccountTimeline with real models' do
+    json = {
+      'account' => { 'accountId' => 'acc', 'name' => 'Bob' },
+      'invoices' => [{ 'invoiceId' => 'i1', 'items' => [{ 'invoiceItemId' => 'it1' }] }],
+      'payments' => [{ 'paymentId' => 'p1' }],
+      'bundles' => [{ 'bundleId' => 'b1' }],
+      'somethingNew' => true
+    }.to_json
+    timeline = described_class.from_json(KillBillClient::Model::AccountTimeline, json)
+    expect(timeline.account).to be_a(KillBillClient::Model::Account)
+    expect(timeline.account.account_id).to eq('acc')
+    expect(timeline.invoices.first).to be_a(KillBillClient::Model::Invoice)
+    expect(timeline.invoices.first.items.first).to be_a(KillBillClient::Model::InvoiceItem)
+    expect(timeline.payments.first).to be_a(KillBillClient::Model::InvoicePayment)
+    expect(timeline.bundles.first.bundle_id).to eq('b1')
+  end
+end