Add raise_on_unknown_attributes configuration option #23
Conversation
…missing - Add class_attribute :raise_on_unknown_attributes with default: true - Implement attribute_writer_missing to handle unknown attributes - When set to false, logs warning instead of raising exception
…attributes is false - Modify assign_attributes to check raise_on_unknown_attributes setting - Filter out unknown attributes before calling super when disabled - Log warning for ignored attributes
- Add BaseTestWithUnknownAttributesAllowed test class - Test behavior when raise_on_unknown_attributes is false - Test default behavior (true) raises UnknownAttributeError - Cover new, assign_attributes, and attribute storage scenarios
Summary of ChangesHello @JulienBURNET-FAUCHE, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable configuration option, raise_on_unknown_attributes, to provide more flexibility when dealing with evolving schemas. The implementation is solid and the tests are comprehensive. I have a couple of suggestions to improve consistency and performance. My main feedback is to move the assign_attributes override to the Document class to ensure consistent behavior across all model types, including NestedDocument. I've also suggested a minor refactoring for better performance and a style improvement for an unused variable. Overall, great work on this feature.
lib/couchbase-orm/persistence.rb
Outdated
| 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 |
There was a problem hiding this comment.
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:
-
Consistency: This method is defined in the
Persistencemodule, which is included inCouchbaseOrm::Basebut not inCouchbaseOrm::NestedDocument. This leads to inconsistent behavior, asNestedDocumentwill fall back to the default implementation and log unknown attributes one by one viaattribute_writer_missing. To ensure consistent behavior for all models, I recommend moving this method override to theCouchbaseOrm::Documentclass inlib/couchbase-orm/base.rb. -
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| 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 |
There was a problem hiding this comment.
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- Move assign_attributes override from Persistence to Document class - Ensures consistent behavior for both Document and NestedDocument - Use Set for efficient unknown attribute lookup (performance optimization) - Add require 'set' to base.rb - Remove duplicate implementation from persistence.rb This addresses code review feedback about consistency and performance.
pimpin
left a comment
pimpin
left a comment
There was a problem hiding this comment.
Seems quite good. There are some missing coverage around nested docs.
I'd rather have implemented it by rescuing the error instead of monkey patching the implementation of AR internals.
…cument - Add test classes: NestedDocWithUnknownAttributesAllowed and NestedDocWithDefaultBehavior - Add parent document test classes to test nested behavior - Test NestedDocument with raise_on_unknown_attributes = false - Test NestedDocument with default behavior (true) - Test nested documents within parent documents - Test assign_attributes on NestedDocument - All 14 tests pass (7 for base documents + 7 for nested documents)
- Add attribute_names_set class method with memoization - Use cached Set in assign_attributes instead of converting on each call - Eliminates O(m) Set conversion overhead, where m = number of attributes - Maintains O(1) lookup performance for unknown attribute detection - All 14 tests pass
Summary
Adds a new
raise_on_unknown_attributesconfiguration option to control behavior when assigning unknown attributes to models.Changes
class_attribute :raise_on_unknown_attributes(default: true) to Document classattribute_writer_missingmethod to handle unknown attributesassign_attributesin Persistence to filter unknown attributes when disabledUsage
Motivation
Useful for:
Tests
All tests pass (7 new examples, 0 failures)