From 9d399e839f09000b0cf9c4d9e836973b63a02bee Mon Sep 17 00:00:00 2001 From: Kyle Date: Sat, 14 Nov 2026 21:37:31 +0700 Subject: [PATCH] Simplify instantiate_record_from_json with a cached per-class lookup table Resolve each JSON key once per class into [setter, cardinality, type] (copy-on-write cache, cleared when attributes are declared) instead of walking ancestors, underscoring twice and rescuing NoMethodError for every key of every record. Unknown keys are cached and skipped without raising. Split the method into smaller pieces. Add specs for nested has_one/has_many, inherited metadata, unknown fields and cache invalidation, plus benchmark/from_json.rb (benchmark-ips). Refs #126 Co-Authored-By: Claude Sonnet 5.5 --- benchmark/from_json.rb | 100 ++++++++++++ lib/killbill_client/models/resource.rb | 99 ++++++++---- .../instantiate_record_from_json_spec.rb | 143 ++++++++++++++++++ 3 files changed, 315 insertions(+), 27 deletions(-) create mode 100644 benchmark/from_json.rb create mode 100644 spec/killbill_client/instantiate_record_from_json_spec.rb diff --git a/benchmark/from_json.rb b/benchmark/from_json.rb new file mode 100644 index 0000000..ac61c29 --- /dev/null +++ b/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 +# ..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 diff --git a/lib/killbill_client/models/resource.rb b/lib/killbill_client/models/resource.rb index c752f20..e217681 100644 --- a/lib/killbill_client/models/resource.rb +++ b/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,6 +249,7 @@ 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 @@ -213,6 +257,7 @@ def has_one(attr_name, type = nil) 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 diff --git a/spec/killbill_client/instantiate_record_from_json_spec.rb b/spec/killbill_client/instantiate_record_from_json_spec.rb new file mode 100644 index 0000000..960ca7d --- /dev/null +++ b/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