Add raise_on_unknown_attributes configuration option#22
Add raise_on_unknown_attributes configuration option#22JulienBURNET-FAUCHE wants to merge 17 commits intomasterfrom
Conversation
NB : in AR, the cast is done at assignment time. Lets try to do it.
TODO : fix move casting stuff from serialize and place them in cast obviously
And keep unchanged the serialize_value needed for queries (ie find_by_some_time)
- Remove double casting in create_setters to avoid type inconsistencies - Fix Timestamp type precision by applying floor() during cast - Fix DateTimeWith3Decimal precision handling in both cast and serialize 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…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 introduces a significant enhancement to how CouchbaseOrm models handle attributes that are not explicitly defined in their schema. By adding a new 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 handling document attributes. The implementation is solid and is accompanied by a good set of tests. However, I've identified a few areas for improvement. There's a potential issue with the module inclusion order in base.rb that could affect dirty attribute tracking. Additionally, the override for assign_attributes should be moved to ensure consistent behavior across all document types. Finally, there's an unrelated and undocumented breaking change in the Timestamp type that truncates timestamps, which should be reverted and addressed in a separate pull request.
| include Changeable | ||
| include ::ActiveModel::Attributes |
There was a problem hiding this comment.
The change in module inclusion order appears to introduce a bug. ActiveModel::Attributes includes ActiveModel::Dirty. By including Changeable before ActiveModel::Attributes, the methods in ActiveModel::Dirty (like changes_applied) will override the ones in Changeable. The original code and its comment indicated that Changeable is intended to override ActiveModel::Dirty's behavior. To restore the correct override behavior, Changeable should be included after ActiveModel::Attributes.
include ::ActiveModel::Attributes
include Changeable| def cast(value) | ||
| return nil if value.nil? | ||
| return Time.at(value) if value.is_a?(Integer) || value.is_a?(Float) | ||
| return Time.at(value.to_i) if value.is_a?(String) && value =~ /^[0-9]+$/ | ||
| return value.utc if value.is_a?(Time) | ||
| super(value).utc | ||
| return Time.at(value).floor if value.is_a?(Integer) || value.is_a?(Float) | ||
| return Time.at(value.to_i).floor if value.is_a?(String) && value =~ /^[0-9]+$/ | ||
| return value.utc.floor if value.is_a?(Time) | ||
| super(value).utc.floor | ||
| end |
There was a problem hiding this comment.
The addition of .floor to the return values in the cast method truncates timestamps to the nearest second, effectively removing fractional seconds. This is a significant, backward-incompatible change that is not documented in the pull request description and is unrelated to the primary goal of this PR. This could lead to data loss for applications that rely on sub-second precision. It's recommended to revert this change and submit it as a separate pull request with a clear explanation of its impact and motivation.
def cast(value)
return nil if value.nil?
return Time.at(value) if value.is_a?(Integer) || value.is_a?(Float)
return Time.at(value.to_i) if value.is_a?(String) && value =~ /^[0-9]+$/
return value.utc if value.is_a?(Time)
super(value).utc
end| 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 assign_attributes override is located in the Persistence module, which is only included in CouchbaseOrm::Base. This means it won't apply to CouchbaseOrm::NestedDocument, leading to inconsistent behavior for handling unknown attributes between different document types. This method should be moved to the CouchbaseOrm::Document class in lib/couchbase-orm/base.rb to ensure it applies to all document types inheriting from it.
|
Closing this PR as it was created from the wrong branch. Opening a new PR from master. |
JulienBURNET-FAUCHE
deleted the
feature/add-raise-on-unknown-attributes
branch
2 participants
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)