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/lib/killbill_client/utils.rb b/lib/killbill_client/utils.rb index 88c600d..0235e0a 100644 --- a/lib/killbill_client/utils.rb +++ b/lib/killbill_client/utils.rb @@ -21,7 +21,9 @@ def demodulize(class_name_in_module) def underscore(camel_cased_word) word = camel_cased_word.to_s.dup word.gsub!(/::/, '/') - word.gsub!(/([A-Z]+)([A-Z][a-z])/, '\1_\2') + # Same result as gsub!(/([A-Z]+)([A-Z][a-z])/, '\1_\2'), but linear: the capture group version backtracks + # quadratically on long runs of capitals (polynomial ReDoS, flagged by CodeQL) + word.gsub!(/(?<=[A-Z])(?=[A-Z][a-z])/, '_') word.gsub!(/([a-z\d])([A-Z])/, '\1_\2') word.tr! '-', '_' word.downcase! 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 diff --git a/spec/killbill_client/utils_spec.rb b/spec/killbill_client/utils_spec.rb new file mode 100644 index 0000000..8a9f6ad --- /dev/null +++ b/spec/killbill_client/utils_spec.rb @@ -0,0 +1,41 @@ +require 'spec_helper' + +describe KillBillClient::Utils do + describe '.underscore' do + { + 'accountId' => 'account_id', + 'AccountId' => 'account_id', + 'invoiceID' => 'invoice_id', + 'HTTPServer' => 'http_server', + 'ABCde' => 'ab_cde', + 'ABcDEf' => 'a_bc_d_ef', + 'accountCBAId' => 'account_cba_id', + 'already_underscored' => 'already_underscored', + 'some-dashed-key' => 'some_dashed_key', + 'Foo::BarBaz' => 'foo/bar_baz', + 'plan2Name' => 'plan2_name', + '' => '' + }.each do |input, expected| + it "converts #{input.inspect} to #{expected.inspect}" do + expect(described_class.underscore(input)).to eq(expected) + end + end + + it 'accepts symbols' do + expect(described_class.underscore(:accountId)).to eq('account_id') + end + + it 'does not mutate its argument' do + input = 'accountId' + described_class.underscore(input) + expect(input).to eq('accountId') + end + + it 'runs in linear time on long runs of capital letters' do + # The previous regex took several seconds on this input + started = Time.now + described_class.underscore('A' * 200_000 + '!') + expect(Time.now - started).to be < 1 + end + end +end