Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deserialize an unexpected property set to null raise error #73

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
59ecbf6
can override scan consistency
CedricCouton Feb 28, 2023
6d4260e
log scan consistency
CedricCouton Mar 1, 2023
c5167ce
add tests
CedricCouton Mar 2, 2023
335ae29
fix usage with a thread local var for // execution
CedricCouton Mar 2, 2023
c037270
fix usage with a thread local var for // execution
CedricCouton Mar 2, 2023
9a57ab3
fix test
CedricCouton Mar 2, 2023
8ac6625
use a DEFAULT_SCAN_CONSISTENCY const
pimpin Mar 2, 2023
c620ef7
allow to have nil value for scan_consistency
pimpin Mar 2, 2023
08ddfe0
Merge pull request #14 from doctolib/can-override-scan-consitency
CedricCouton Mar 2, 2023
7ef991f
Prevent to expose dead callbacks for nested
pimpin Mar 15, 2023
d059dd2
Upgrade Couchbase ruby client version
pimpin Mar 27, 2023
12500dc
Merge pull request #15 from doctolib/do_not_expose_some_callbacks_in_…
pimpin Mar 29, 2023
013ec7b
Update ignored_properties api
pimpin Apr 6, 2023
f44844c
Merge pull request #18 from doctolib/make_ignored_properties_similar_…
pimpin Apr 6, 2023
772e154
test and solve cases of find with multiple ids
CedricCouton Apr 12, 2023
856b5df
Update lib/couchbase-orm/base.rb
CedricCouton Apr 12, 2023
53b5f69
better lisibility
CedricCouton Apr 12, 2023
50f9af8
better lisibility
CedricCouton Apr 12, 2023
bdd4de1
Merge pull request #19 from doctolib/fix-find-array
CedricCouton Apr 12, 2023
02e5e08
Bump Couchbase ruby client to 3.4.2
pimpin Apr 13, 2023
31b5012
Merge pull request #16 from doctolib/upgrade-couchbase-ruby-client
pimpin Apr 13, 2023
33a7d04
Add red test on strict_loading for instance
pimpin May 9, 2023
51b50f4
WIP first implem
pimpin May 9, 2023
1280468
Handle first, last and find
pimpin May 16, 2023
ab1f125
refacto test
pimpin May 16, 2023
c022b29
consistant (and strange) implem
pimpin May 16, 2023
89ecb85
fix
pimpin May 16, 2023
9379e9a
red again... implem is leaky
pimpin May 30, 2023
79b54b9
Fix leaky implem
pimpin May 30, 2023
571c92d
handle habtm relations
pimpin May 30, 2023
52784ed
remove useless code
pimpin May 30, 2023
9f886c5
Cover AR default strict loading usage
pimpin May 30, 2023
dcbfb58
Fix Mapotempo/couchbase-orm/issues/71
pimpin Jun 29, 2023
3bb2292
Merge pull request #25 from doctolib/fix_ignored_properties
pimpin Jun 30, 2023
c1ba1e4
Merge pull request #20 from doctolib/new_feature_strict_loading
pimpin Jul 11, 2023
fcf9e4f
✅ Desrialize an unexpected prop set to null raise
pimpin Jul 12, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion couchbase-orm.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ Gem::Specification.new do |gem|
gem.add_runtime_dependency 'activemodel', ENV["ACTIVE_MODEL_VERSION"] || '>= 5.2'
gem.add_runtime_dependency 'activerecord', ENV["ACTIVE_MODEL_VERSION"] || '>= 5.2'

gem.add_runtime_dependency 'couchbase', '~> 3.3.0'
gem.add_runtime_dependency 'couchbase', '~> 3.4.2'
gem.add_runtime_dependency 'radix', '~> 2.2' # converting numbers to and from any base

gem.add_development_dependency 'rake', '~> 12.2'
Expand Down
4 changes: 4 additions & 0 deletions lib/couchbase-orm/associations.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ def belongs_to(name, **options)
val = if options[:polymorphic]
::CouchbaseOrm.try_load(self.send(ref))
else
raise ActiveRecord::StrictLoadingViolationError, "#{self.class} is marked as strict_loading and #{assoc} cannot be lazily loaded." if strict_loading?

assoc.constantize.find(self.send(ref), quiet: true)
end
instance_variable_set(instance_var, val)
Expand Down Expand Up @@ -81,6 +83,8 @@ def has_and_belongs_to_many(name, **options)
val = if options[:polymorphic]
::CouchbaseOrm.try_load(ref_value) if ref_value
else
raise ActiveRecord::StrictLoadingViolationError, "#{self.class} is marked as strict_loading and #{assoc} cannot be lazily loaded." if strict_loading?

assoc.constantize.find(ref_value) if ref_value
end
val = Array.wrap(val || [])
Expand Down
16 changes: 11 additions & 5 deletions lib/couchbase-orm/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,6 @@ class Document
extend Enum

define_model_callbacks :initialize, :only => :after
define_model_callbacks :create, :destroy, :save, :update

Metadata = Struct.new(:cas)

Expand Down Expand Up @@ -212,6 +211,8 @@ class Base < Document
extend Index
extend IgnoredProperties

define_model_callbacks :create, :destroy, :save, :update

class << self
def connect(**options)
@bucket = BucketProxy.new(::MTLibcouchbase::Bucket.new(**options))
Expand Down Expand Up @@ -241,7 +242,7 @@ def uuid_generator=(generator)
@uuid_generator = generator
end

def find(*ids, quiet: false)
def find(*ids, quiet: false, with_strict_loading: false)
CouchbaseOrm.logger.debug { "Base.find(l##{ids.length}) #{ids}" }

ids = ids.flatten.select { |id| id.present? }
Expand All @@ -253,9 +254,14 @@ def find(*ids, quiet: false)
records = quiet ? collection.get_multi(ids, transcoder: transcoder) : collection.get_multi!(ids, transcoder: transcoder)
CouchbaseOrm.logger.debug { "Base.find found(#{records})" }
records = records.zip(ids).map { |record, id|
self.new(record, id: id) if record
}
records.compact!
next unless record
next if record.error
new(record, id: id).tap do |instance|
if with_strict_loading
instance.strict_loading!
end
end
}.compact
ids.length > 1 ? records : records[0]
end

Expand Down
12 changes: 10 additions & 2 deletions lib/couchbase-orm/n1ql.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ module CouchbaseOrm
module N1ql
extend ActiveSupport::Concern
NO_VALUE = :no_value_specified
DEFAULT_SCAN_CONSISTENCY = :request_plus
# sanitize for injection query
def self.sanitize(value)
if value.is_a?(String)
Expand All @@ -19,6 +20,13 @@ def self.sanitize(value)
end
end

def self.config(new_config = nil)
Thread.current['__couchbaseorm_n1ql_config__'] = new_config if new_config
Thread.current['__couchbaseorm_n1ql_config__'] || {
scan_consistency: DEFAULT_SCAN_CONSISTENCY
}
end

module ClassMethods
# Defines a query N1QL for the model
#
Expand Down Expand Up @@ -49,7 +57,7 @@ def n1ql(name, query_fn: nil, emit_key: [], custom_order: nil, **options)
@indexes[name] = method_opts

singleton_class.__send__(:define_method, name) do |key: NO_VALUE, **opts, &result_modifier|
opts = options.merge(opts).reverse_merge(scan_consistency: :request_plus)
opts = options.merge(opts).reverse_merge(scan_consistency: CouchbaseOrm::N1ql.config[:scan_consistency])
values = key == NO_VALUE ? NO_VALUE : convert_values(method_opts[:emit_key], key)
current_query = run_query(method_opts[:emit_key], values, query_fn, custom_order: custom_order, **opts.except(:include_docs, :key))
if result_modifier
Expand Down Expand Up @@ -116,7 +124,7 @@ def run_query(keys, values, query_fn, custom_order: nil, descending: false, limi
limit = build_limit(limit)
n1ql_query = "select raw meta().id from `#{bucket_name}` where #{where} order by #{order} #{limit}"
result = cluster.query(n1ql_query, Couchbase::Options::Query.new(**options))
CouchbaseOrm.logger.debug { "N1QL query: #{n1ql_query} return #{result.rows.to_a.length} rows" }
CouchbaseOrm.logger.debug "N1QL query: #{n1ql_query} return #{result.rows.to_a.length} rows with scan_consistency : #{options[:scan_consistency]}"
N1qlProxy.new(result)
end
end
Expand Down
2 changes: 1 addition & 1 deletion lib/couchbase-orm/persistence.rb
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ def reload

CouchbaseOrm.logger.debug "Data - Get #{id}"
resp = self.class.collection.get!(id)
assign_attributes(decode_encrypted_attributes(resp.content.except("id"))) # API return a nil id
assign_attributes(decode_encrypted_attributes(resp.content.except("id", *self.class.ignored_properties ))) # API return a nil id
@__metadata__.cas = resp.cas

reset_associations
Expand Down
3 changes: 1 addition & 2 deletions lib/couchbase-orm/proxies/collection_proxy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,7 @@ def get_multi!(*ids, **options)
end

def get_multi(*ids, **options)
result = @proxyfied.get_multi(*ids, Couchbase::Options::GetMulti.new(**options))
result.reject(&:error)
@proxyfied.get_multi(*ids, Couchbase::Options::GetMulti.new(**options))
end

def remove!(id, **options)
Expand Down
36 changes: 23 additions & 13 deletions lib/couchbase-orm/relation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,20 @@ module Relation
extend ActiveSupport::Concern

class CouchbaseOrm_Relation
def initialize(model:, where: where = nil, order: order = nil, limit: limit = nil, _not: _not = false)
CouchbaseOrm::logger.debug "CouchbaseOrm_Relation init: #{model} where:#{where.inspect} not:#{_not.inspect} order:#{order.inspect} limit: #{limit}"
def initialize(model:, where: where = nil, order: order = nil, limit: limit = nil, _not: _not = false, strict_loading: strict_loading = false)
CouchbaseOrm::logger.debug "CouchbaseOrm_Relation init: #{model} where:#{where.inspect} not:#{_not.inspect} order:#{order.inspect} limit: #{limit} strict_loading: #{strict_loading}"
@model = model
@limit = limit
@where = []
@order = {}
@order = merge_order(**order) if order
@where = merge_where(where, _not) if where
@strict_loading = strict_loading
CouchbaseOrm::logger.debug "- #{to_s}"
end

def to_s
"CouchbaseOrm_Relation: #{@model} where:#{@where.inspect} order:#{@order.inspect} limit: #{@limit}"
"CouchbaseOrm_Relation: #{@model} where:#{@where.inspect} order:#{@order.inspect} limit: #{@limit} strict_loading: #{@strict_loading}"
end

def to_n1ql
Expand All @@ -27,8 +28,8 @@ def to_n1ql
end

def execute(n1ql_query)
result = @model.cluster.query(n1ql_query, Couchbase::Options::Query.new(scan_consistency: :request_plus))
CouchbaseOrm.logger.debug { "Relation query: #{n1ql_query} return #{result.rows.to_a.length} rows" }
result = @model.cluster.query(n1ql_query, Couchbase::Options::Query.new(scan_consistency: CouchbaseOrm::N1ql.config[:scan_consistency]))
CouchbaseOrm.logger.debug { "Relation query: #{n1ql_query} return #{result.rows.to_a.length} rows with scan_consistency : #{CouchbaseOrm::N1ql.config[:scan_consistency]}" }
N1qlProxy.new(result)
end

Expand All @@ -51,16 +52,25 @@ def ids
query.to_a
end

def strict_loading
CouchbaseOrm_Relation.new(**initializer_arguments.merge(strict_loading: true))
end

def strict_loading?
!!@strict_loading
end

def first
result = @model.cluster.query(self.limit(1).to_n1ql, Couchbase::Options::Query.new(scan_consistency: :request_plus))
first_id = result.rows.to_a.first
@model.find(first_id) if first_id
result = @model.cluster.query(self.limit(1).to_n1ql, Couchbase::Options::Query.new(scan_consistency: CouchbaseOrm::N1ql.config[:scan_consistency]))
return unless (first_id = result.rows.to_a.first)

@model.find(first_id, with_strict_loading: @strict_loading)
end

def last
result = @model.cluster.query(to_n1ql, Couchbase::Options::Query.new(scan_consistency: :request_plus))
result = @model.cluster.query(to_n1ql, Couchbase::Options::Query.new(scan_consistency: CouchbaseOrm::N1ql.config[:scan_consistency]))
last_id = result.rows.to_a.last
@model.find(last_id) if last_id
@model.find(last_id, with_strict_loading: @strict_loading) if last_id
end

def count
Expand Down Expand Up @@ -89,7 +99,7 @@ def pluck(*fields)
def to_ary
ids = query.results
return [] if ids.empty?
Array(ids && @model.find(ids))
Array(ids && @model.find(ids, with_strict_loading: @strict_loading))
end

alias :to_a :to_ary
Expand Down Expand Up @@ -146,7 +156,7 @@ def build_limit
end

def initializer_arguments
{ model: @model, order: @order, where: @where, limit: @limit }
{ model: @model, order: @order, where: @where, limit: @limit, strict_loading: @strict_loading }
end

def merge_order(*lorder, **horder)
Expand Down Expand Up @@ -233,7 +243,7 @@ def relation

delegate :ids, :update_all, :delete_all, :count, :empty?, :filter, :reduce, :find_by, to: :all

delegate :where, :not, :order, :limit, :all, to: :relation
delegate :where, :not, :order, :limit, :all, :strict_loading, :strict_loading?, to: :relation
end
end
end
10 changes: 8 additions & 2 deletions lib/couchbase-orm/utilities/ignored_properties.rb
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
module CouchbaseOrm
module IgnoredProperties
def ignored_properties=(properties)
@@ignored_properties = properties.map(&:to_s)
end

def ignored_properties(*args)
if args.any?
CouchbaseOrm.logger.warn('Passing aruments to `.ignored_properties` is deprecated. PLease use `.ignored_properties=` intead.')
return send :ignored_properties=, args
end
@@ignored_properties ||= []
return @@ignored_properties if args.empty?
@@ignored_properties += args.map(&:to_s)
end
end
end
2 changes: 1 addition & 1 deletion lib/couchbase-orm/views.rb
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ def view(name, map: nil, emit_key: nil, reduce: nil, **options)
@views[name] = method_opts

singleton_class.__send__(:define_method, name) do |**opts, &result_modifier|
opts = options.merge(opts).reverse_merge(scan_consistency: :request_plus)
opts = options.merge(opts).reverse_merge(scan_consistency: CouchbaseOrm::N1ql.config[:scan_consistency])
CouchbaseOrm.logger.debug("View [#{@design_document}, #{name.inspect}] options: #{opts.inspect}")
if result_modifier
include_docs(bucket.view_query(@design_document, name.to_s, Couchbase::Options::View.new(**opts.except(:include_docs)))).map(&result_modifier)
Expand Down
50 changes: 50 additions & 0 deletions spec/associations_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,13 @@

class Parent < CouchbaseOrm::Base
attribute :name
has_and_belongs_to_many :children
end

class StrictLoadingParent < CouchbaseOrm::Base
attribute :name
has_and_belongs_to_many :children
self.strict_loading_by_default = true
end

class RandomOtherType < CouchbaseOrm::Base
Expand Down Expand Up @@ -209,4 +216,47 @@ class Part < CouchbaseOrm::Base
it_behaves_like "ActiveModel"
end
end

describe 'strict_loading' do
let(:parent) {Parent.create!(name: 'joe')}
let(:child) {Child.create!(name: 'bob', parent_id: parent.id)}
context 'instance strict loading' do
it 'raises StrictLoadingViolationError on lazy loading child relation' do
expect {child.parent.id}.not_to raise_error
expect_strict_loading_error_on_calling_parent(Child.find(child.id).tap{|child| child.strict_loading!})
end
end
context 'scope strict loading' do
it 'raises StrictLoadingViolationError on lazy loading child relation' do
expect_strict_loading_error_on_calling_parent(Child.where(id: child.id).strict_loading.first)
expect_strict_loading_error_on_calling_parent(Child.strict_loading.where(id: child.id).first)
expect_strict_loading_error_on_calling_parent(Child.strict_loading.where(id: child.id).last)
expect_strict_loading_error_on_calling_parent(Child.strict_loading.where(id: child.id).to_a.first)
expect_strict_loading_error_on_calling_parent(Child.strict_loading.all.to_a.first)
end

it 'does not raise StrictLoadingViolationError on lazy loading child relation without declaring it' do
expect_strict_loading_error_on_calling_parent(Child.strict_loading.where(id: child.id).first)
expect { Child.where(id: child.id).last.parent}.not_to raise_error
end

it 'raises StrictLoadingViolationError on lazy loading habtm relation' do
expect {Parent.strict_loading.where(id: parent.id).first.children}.to raise_error(ActiveRecord::StrictLoadingViolationError)
# NB any action called on model class breaks find return type (find return an enumerator instead of a record)
expect {Parent.strict_loading.find(parent.id).first.children}.to raise_error(ActiveRecord::StrictLoadingViolationError)
end

it 'raises StrictLoadingViolationError on lazy loading relation when model is by default strict_loading' do
strict_loading_parent = StrictLoadingParent.create!(name: 'joe')
expect {StrictLoadingParent.where(id: strict_loading_parent.id).first.children}.to raise_error(ActiveRecord::StrictLoadingViolationError)
expect {Parent.find(parent.id).children}.not_to raise_error
# NB any action called on model class breaks find return type (find return an enumerator instead of a record)
expect {Parent.strict_loading.find(strict_loading_parent.id).first.children}.to raise_error(ActiveRecord::StrictLoadingViolationError)
end
end
end

def expect_strict_loading_error_on_calling_parent(child_instance)
expect {child_instance.parent}.to raise_error(ActiveRecord::StrictLoadingViolationError)
end
end
50 changes: 49 additions & 1 deletion spec/base_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ class TimestampTest < CouchbaseOrm::Base
end

class BaseTestWithIgnoredProperties < CouchbaseOrm::Base
ignored_properties :deprecated_property
self.ignored_properties += [:deprecated_property]
attribute :name, :string
attribute :job, :string
end
Expand Down Expand Up @@ -82,6 +82,20 @@ class BaseTestWithIgnoredProperties < CouchbaseOrm::Base
BaseTest.bucket.default_collection.remove 'doc_1'
end

it "raises ActiveModel::UnknownAttributeError on loading objects with unexpected properties even valued to null" do
too_much_properties_valued_to_null_doc = {
type: BaseTest.design_document,
name: 'Pierre',
job: 'dev',
age: nil
}
BaseTest.bucket.default_collection.upsert 'doc_1', too_much_properties_valued_to_null_doc

expect { BaseTest.find_by_id('doc_1') }.to raise_error(ActiveModel::UnknownAttributeError)

BaseTest.bucket.default_collection.remove 'doc_1'
end

it "loads objects even if there is a missing property in doc" do
missing_properties_doc = {
type: BaseTest.design_document,
Expand Down Expand Up @@ -212,6 +226,27 @@ class BaseTestWithIgnoredProperties < CouchbaseOrm::Base
expect(base.created_at).to be_a(Time)
end

it "should find multiple ids at same time" do
base1 = BaseTest.create!(name: 'joe1')
base2 = BaseTest.create!(name: 'joe2')
base3 = BaseTest.create!(name: 'joe3')
expect(BaseTest.find([base1.id, base2.id, base3.id])).to eq([base1, base2, base3])
end

it "should find multiple ids at same time with a not found id with exception" do
base1 = BaseTest.create!(name: 'joe1')
base2 = BaseTest.create!(name: 'joe2')
base3 = BaseTest.create!(name: 'joe3')
expect { BaseTest.find([base1.id, 't', base3.id]) }.to raise_error(Couchbase::Error::DocumentNotFound)
end

it "should find multiple ids at same time with a not found id without exception" do
base1 = BaseTest.create!(name: 'joe1')
base2 = BaseTest.create!(name: 'joe2')
base3 = BaseTest.create!(name: 'joe3')
expect(BaseTest.find([base1.id, 't', 't', base2.id, base3.id], quiet: true)).to eq([base1, base2, base3])
end

describe BaseTest do
it_behaves_like "ActiveModel"
end
Expand All @@ -220,6 +255,15 @@ class BaseTestWithIgnoredProperties < CouchbaseOrm::Base
it_behaves_like "ActiveModel"
end

it 'does not expose callbacks for nested that wont never be called' do
expect{
class InvalidNested < CouchbaseOrm::NestedDocument
before_save {p "this should raise on loading class"}
end

}.to raise_error NoMethodError
end

describe '.ignored_properties' do


Expand Down Expand Up @@ -254,6 +298,10 @@ class BaseTestWithIgnoredProperties < CouchbaseOrm::Base
from(%w[deprecated_property job name type]).
to(%w[job name type])
end

it 'does not raise for reload' do
expect{ loaded_model.reload }.not_to raise_error
end
end
end
end
Loading