Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
15 changes: 15 additions & 0 deletions lib/couchbase-orm/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,10 @@ class Document

class MismatchTypeError < RuntimeError; end

# Configuration option to control whether unknown attributes should raise an error
# Set to false to silently ignore unknown attributes during mass assignment
class_attribute :raise_on_unknown_attributes, default: true

def initialize(model = nil, ignore_doc_type: false, **attributes)
CouchbaseOrm.logger.debug { "Initialize model #{model} with #{attributes.to_s.truncate(200)}" }
@__metadata__ = Metadata.new
Expand Down Expand Up @@ -100,6 +104,17 @@ def []=(key, value)
send(:"#{key}=", value)
end

# Handle assignment to unknown attributes based on raise_on_unknown_attributes configuration
# If raise_on_unknown_attributes is false, unknown attributes are silently ignored
# If raise_on_unknown_attributes is true (default), ActiveModel::UnknownAttributeError is raised
def attribute_writer_missing(name, value)
if self.class.raise_on_unknown_attributes
super
else
CouchbaseOrm.logger.warn "Ignoring unknown attribute '#{name}' for #{self.class.name}"
end
end
Comment on lines +117 to +123

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The value parameter is not used in this method. It's a good practice in Ruby to prefix unused parameters with an underscore (_) to signal this intent to other developers and to satisfy linters.

        def attribute_writer_missing(name, _value)
            if self.class.raise_on_unknown_attributes
                super
            else
                CouchbaseOrm.logger.warn "Ignoring unknown attribute '#{name}' for #{self.class.name}"
            end
        end


protected

def serialized_attributes
Expand Down
13 changes: 12 additions & 1 deletion lib/couchbase-orm/persistence.rb
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,18 @@ def update_attribute(name, value)

def assign_attributes(hash)
hash = hash.with_indifferent_access if hash.is_a?(Hash)
super(hash.except("type"))

# Filter unknown attributes if raise_on_unknown_attributes is false
if !self.class.raise_on_unknown_attributes
known_attrs = hash.slice(*self.class.attribute_names).except("type")
unknown_attrs = hash.keys - self.class.attribute_names - ["type"]
if unknown_attrs.any?
CouchbaseOrm.logger.warn "Ignoring unknown attribute(s) for #{self.class.name}: #{unknown_attrs.join(', ')}"
end
super(known_attrs)
else
super(hash.except("type"))
end
end

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This override of assign_attributes is a good approach for handling unknown attributes during mass assignment. However, I have a couple of suggestions for improvement:

  1. Consistency: This method is defined in the Persistence module, which is included in CouchbaseOrm::Base but not in CouchbaseOrm::NestedDocument. This leads to inconsistent behavior, as NestedDocument will fall back to the default implementation and log unknown attributes one by one via attribute_writer_missing. To ensure consistent behavior for all models, I recommend moving this method override to the CouchbaseOrm::Document class in lib/couchbase-orm/base.rb.

  2. Performance and Readability: The implementation can be slightly optimized for better performance and clarity. The current approach for finding unknown attributes can be inefficient, and there's a redundant call to .except("type").

Here is a refactored version that addresses the performance/readability point. After applying this, you should also move the method as described above.

        def assign_attributes(hash)
            hash = hash.with_indifferent_access if hash.is_a?(Hash)

            if self.class.raise_on_unknown_attributes
                super(hash.except("type"))
            else
                # Filter unknown attributes
                known_names = self.class.attribute_names
                known_attrs = hash.slice(*known_names)

                # Use a Set for efficient lookup of unknown keys
                known_names_set = known_names.to_set
                unknown_keys = hash.keys.reject { |k| known_names_set.include?(k) || k == "type" }

                if unknown_keys.any?
                    CouchbaseOrm.logger.warn "Ignoring unknown attribute(s) for #{self.class.name}: #{unknown_keys.join(', ')}"
                end
                super(known_attrs)
            end
        end


# Updates the attributes of the model from the passed-in hash and saves the
Expand Down
65 changes: 65 additions & 0 deletions spec/base_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,12 @@ class BaseTestWithIgnoredProperties < CouchbaseOrm::Base
attribute :job, :string
end

class BaseTestWithUnknownAttributesAllowed < CouchbaseOrm::Base
self.raise_on_unknown_attributes = false
attribute :name, :string
attribute :job, :string
end

class BaseTestWithPropertiesAlwaysExistsInDocument < CouchbaseOrm::Base
self.properties_always_exists_in_document = true
attribute :name, :string
Expand Down Expand Up @@ -349,6 +355,65 @@ class InvalidNested < CouchbaseOrm::NestedDocument
end
end

describe 'handling unknown attributes' do
context 'when raise_on_unknown_attributes is set to false' do
it 'returns false when queried' do
expect(BaseTestWithUnknownAttributesAllowed.raise_on_unknown_attributes).to be(false)
end

it 'silently ignores unknown attributes in new' do
model = BaseTestWithUnknownAttributesAllowed.new(name: 'test', job: 'dev', unknown_attr: 'value')
expect(model.name).to eq('test')
expect(model.job).to eq('dev')
expect(model.respond_to?(:unknown_attr)).to be(false)
end

it 'silently ignores unknown attributes in assign_attributes' do
model = BaseTestWithUnknownAttributesAllowed.new(name: 'test')
expect {
model.assign_attributes(name: 'updated', job: 'engineer', foo: 'bar', baz: 'qux')
}.not_to raise_error
expect(model.name).to eq('updated')
expect(model.job).to eq('engineer')
expect(model.respond_to?(:foo)).to be(false)
expect(model.respond_to?(:baz)).to be(false)
end

it 'only stores known attributes' do
model = BaseTestWithUnknownAttributesAllowed.new(
name: 'Alice',
job: 'Developer',
unknown_field_1: 'value1',
unknown_field_2: 'value2'
)
# Only known attributes should be stored
expect(model.name).to eq('Alice')
expect(model.job).to eq('Developer')
expect(model.respond_to?(:unknown_field_1)).to be(false)
expect(model.respond_to?(:unknown_field_2)).to be(false)
end
end

context 'default behavior (raise_on_unknown_attributes = true)' do
it 'returns true by default' do
expect(BaseTest.raise_on_unknown_attributes).to be(true)
end

it 'raises ActiveModel::UnknownAttributeError on unknown attributes in new' do
expect {
BaseTest.new(name: 'bob', job: 'dev', foo: 'bar')
}.to raise_error(ActiveModel::UnknownAttributeError)
end

it 'raises ActiveModel::UnknownAttributeError on unknown attributes in assign_attributes' do
model = BaseTest.new(name: 'bob')
expect {
model.assign_attributes(job: 'dev', foo: 'bar')
}.to raise_error(ActiveModel::UnknownAttributeError)
end
end
end

describe '.properties_always_exists_in_document' do
it 'Uses NOT VALUED when properties_always_exists_in_document = false' do
where_clause = BaseTest.where(name: nil)
Expand Down
Loading