Remove ActiveRecord::Dirty usage #12
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)
pimpin
force-pushed
the
fix_save_return_for_nested
branch
from
f4ae61b to
3d71024
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- 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 <[email protected]>
|
Thanks to Claude, we get a green inplem according to CI. Let's check in Docto CI that this implem is not breaking smth. |
There was a problem hiding this comment.
Pull Request Overview
This PR removes ActiveRecord::Dirty usage to fix a bug where nested resource changes were being reset on save operations. The change replaces the ActiveRecord::Dirty dependency with a custom implementation to handle change tracking properly for nested resources.
- Removes
ActiveModel::Dirtyinclusion and replaces with customChangeablemodule - Modifies
changes_appliedmethod to prevent unwanted change resets - Adds test coverage for the nested resource change preservation bug
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| lib/couchbase-orm/base.rb | Removes ActiveModel::Dirty inclusion, keeping only custom Changeable module |
| lib/couchbase-orm/changeable.rb | Removes call to super in changes_applied to prevent change resets |
| lib/couchbase-orm/types/timestamp.rb | Adds .floor calls to timestamp casting for consistency |
| spec/type_spec.rb | Adds cast method to DateTimeWith3Decimal test class |
| spec/type_nested_spec.rb | Adds test cases to verify nested resource changes are preserved after save |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| 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 |
There was a problem hiding this comment.
The addition of .floor calls changes the behavior of timestamp casting by truncating subsecond precision. This could be a breaking change for consumers expecting microsecond precision in timestamps. Consider documenting this behavioral change or making it configurable.
| 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 | |
| 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 |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
We had a look at the doc of Time#floor and we thought it was ok ... but we must double check it.
WHY ?
To fix a bug, having nester resource change reset on
#save.HOW ?
changes_appliedprovided by AR::DirtyNB : in AR, the cast is done at assignment time. Lets try to do it.
TODO before undraft the PR :
TODO before merge the PR :