| Commit message (Collapse) | Author | Age | Files | Lines |
|
|
|
|
|
|
|
|
| |
`test_update_does_not_run_sql_if_record_has_not_changed` would pass
without #18501 since `assert_queries` ignores BEGIN/COMMIT unless
`ignore_none: true` is given.
Since #32647, empty BEGIN/COMMIT is ommited. So we no longer need to use
`assert_queries(0)` to ignore BEGIN/COMMIT in the queries.
|
|
|
|
| |
Since #29860, `travel_back` automatically called at the end of the test.
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
`changes_applied` should be called before continuing around callback
chain. Otherwise the mutation tracker returns old value for methods like
`changed`? or `id_in_database` in around callbacks. Also methods depend
on `id_in_database`, like `update_column`, are not working in
`around_create` callbacks.
```
class Foo < ActiveRecord::Base
around_create :around_create_callback
def around_create_callback
...
yield
p id_in_database # => nil
update_column(:generated_column, generate_value) # silently fails
end
...
end
```
|
|
|
|
|
| |
This autocorrects the violations after adding a custom cop in
3305c78dcd.
|
|
|
|
|
|
|
|
|
|
|
|
| |
When an array of hashes is added to a `HashWithIndifferentAccess`, the
hashes are replaced with HWIAs by mutating the array in place.
If an attribute's value is an array of hashes, `changes_to_save` will
convert it to an array of HWIAs as a side-effect of adding it to the
changes hash.
Using `merge!` instead of `[]=` fixes the problem, as `merge!` copies
any array values in the provided hash instead of mutating them.
|
|
|
|
| |
Related to 948b931925febac3c965ab13470065ced68f7b53
|
|
|
|
| |
See 948b931925febac3c965ab13470065ced68f7b53 for context
|
|
|
|
|
|
|
|
|
|
|
| |
Currently the place where we limit what gets sent to the database is in
the implementation for `partial_writes`. We should also be restricting
it to column names when partial writes are turned off.
Note that we're using `&` instead of just defaulting to
`self.class.column_names`, as the instance version of `attribute_names`
does not include attributes which are uninitialized (were not included
in the select clause)
|
| |
|
| |
|
|\
| |
| |
| | |
fix the dirty tracking code's save hook overwriting missing attribute…
|
| |
| |
| |
| | |
initialized-to-nil attributes. fixes #29017.
|
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
```
rails/activerecord$ bundle exec rake postgresql:test TESTOPTS="--seed=26536"
..(compressed)....F
Failure:
DirtyTest#test_save_should_not_save_serialized_attribute_with_partial_writes_if_not_present [/home/travis/build/rails/rails/activerecord/test/cases/dirty_test.rb:473]:
Expected nil to not be nil.
```
|
| | |
|
| | |
|
| | |
|
| | |
|
| |
| |
| |
| |
| | |
This reverts commit 3420a14590c0e6915d8b6c242887f74adb4120f9, reversing
changes made to afb66a5a598ce4ac74ad84b125a5abf046dcf5aa.
|
| | |
|
|\ \
| |/
|/| |
Extract `NumericData` model for tests
|
| |
| |
| |
| | |
Currently `NumericData` model is defined some places.
|
| |
| |
| |
| |
| |
| | |
We already have a _read_attribute method that can get the value we need
from the model. Lets define that method in AM::Dirty and use the
existing one from AR::Dirty rather than introducing a new method.
|
| | |
|
| | |
|
| |
| |
| |
| |
| |
| |
| | |
`supports_migrations?` was added at 4160b518 to determine if schema
statements (`create_table`, `drop_table`, etc) are implemented in the
adapter. But all tested databases has been supported migrations since
a4fc93c3 at least.
|
| |
| |
| |
| |
| | |
The column information for the testings table was being cached
so clear the cache in the test teardown.
|
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
This was never really intended to work (at least not without calling
`define_attribute_methods`, which is less common with Active Record). As
we move forward the intention is to require the use of `attribute` over
`attr_accessor` for more complex model behavior both on Active Record
and Active Model, so this behavior is deprecated.
Fixes #27956.
Close #27963.
[Alex Serban & Sean Griffin]
|
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
callbacks
We pretty frequently get bug reports that "dirty is broken inside of
after callbacks". Intuitively they are correct. You'd expect
`Model.after_save { puts changed? }; model.save` to do the same thing as
`model.save; puts model.changed?`, but it does not.
However, changing this goes much farther than just making the behavior
more intuitive. There are a _ton_ of places inside of AR that can be
drastically simplified with this change. Specifically, autosave
associations, timestamps, touch, counter cache, and just about anything
else in AR that works with callbacks have code to try to avoid "double
save" bugs which we will be able to flat out remove with this change.
We introduce two new sets of methods, both with names that are meant to
be more explicit than dirty. The first set maintains the old behavior,
and their names are meant to center that they are about changes that
occurred during the save that just happened. They are equivalent to
`previous_changes` when called outside of after callbacks, or once the
deprecation cycle moves.
The second set is the new behavior. Their names imply that they are
talking about changes from the database representation. The fact that
this is what we really care about became clear when looking at
`BelongsTo.touch_record` when tests were failing. I'm unsure that this
set of methods should be in the public API. Outside of after callbacks,
they are equivalent to the existing methods on dirty.
Dirty itself is not deprecated, nor are the methods inside of it. They
will only emit the warning when called inside of after callbacks. The
scope of this breakage is pretty large, but the migration path is
simple. Given how much this can improve our codebase, and considering
that it makes our API more intuitive, I think it's worth doing.
|
|/ |
|
|
|
|
|
|
| |
assert [1, 3].includes?(2) fails with unhelpful "Asserting failed" message
assert_includes [1, 3], 2 fails with "Expected [1, 3] to include 2" which makes it easier to debug and more obvious what went wrong
|
|
|
|
|
|
|
|
| |
Style/SpaceBeforeBlockBraces
Style/SpaceInsideBlockBraces
Style/SpaceInsideHashLiteralBraces
Fix all violations in the repository.
|
| |
|
| |
|
|
|
|
|
| |
The current code base is not uniform. After some discussion,
we have chosen to go with double quotes by default.
|
|
|
|
|
| |
Where appropriatei, prefer the more concise Regexp#match?,
String#include?, String#start_with?, or String#end_with?
|
|
|
|
|
| |
This code was added in 81286f858770e0b95e15af37f19156b044ec6a95, but was
not used by that commit and does not appear to have ever been used.
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
As part of refactoring mutation detection to be more performant, we
introduced the concept of `original_value` to `Attribute`. This was not
overridden in `Attribute::Uninitialized` however, so assigning ot an
uninitialized value and calling `.changed?` would raise
`NotImplementedError`.
We are using a sentinel value rather than checking the result of
`original_attribute.initialized?` in `changed?` because `original_value`
might go through more than one node in the tree.
Fixes #25228
|
| |
|
|
|
|
|
|
|
| |
This was passing prior to 20b177b78ef5d21c8cc255f0376f6b2e948de234,
because we were not properly applying our contract that
`model.attr == model.tap(&:save).reload.attr` for this case. Now that
that has been resolved, this test is invalid on some adapters
|
|
|
|
| |
They didn't help.
|
|
|
|
| |
Nobody can replicate locally and the failure makes no sense
|
|
|
|
|
|
| |
When I originally reviewed the #20317, I believe these changes were
present, but it appears that it was later updated so that they were
removed. Since Travis hadn't re-run the build, this slipped through.
|
|
|
|
|
|
|
|
|
|
|
| |
If a getter has side effects on the DB, `changes_applied` will be called
twice. The second time will try and remove the changed attributes cache,
and will crash because it's already been unset. This also demonstrates
that we shouldn't assume that calling getters won't change the value of
`changed_attributes`, and we need to clear the cache if an attribute is
modified.
Fixes #20531.
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
This is a usability change to fix a quirk from our definition of partial
writes. By default, we only persist changed attributes. When creating a
new record, this is assumed that the default values came from the
database. However, if the user provided a default, it will not be
persisted, since we didn't see it as "changed". Since this is a very
specific case, I wanted to isolate it with the other quirks that come
from user provided default values. The number of edge cases which are
presenting themselves are starting to make me wonder if we should just
remove the ability to assign a default, in favor of overriding
`initialize`. For the time being, this is required for the attributes
API to not have confusing behavior.
We had to delete one test, since this actually changes the meaning of
`.changed?` on Active Record models. It now specifically means
`changed_from_database?`. While I think this will make the attributes
API more ergonomic to use, it is a subtle change in definition (though
not a backwards incompatible one). We should probably figure out the
right place to document this. (Feel free to open a PR doing that if
you're reading this).
/cc @rafaelfranca @kirs @senny
This is an alternate implementation of #19921.
Close #19921.
[Sean Griffin & Kir Shatrov]
|
|
|
|
|
|
|
|
|
| |
This helper no longer makes sense as a separate method. Instead I'll
just have `deserialize` call `cast` by default. This led to a random
infinite loop in the `JSON` pg type, when it called `super` from
`deserialize`. Not really a great way to fix that other than not calling
super, or continuing to have the separate method, which makes the public
API differ from what we say it is.
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
The same is not true of `define_attribute`, which is meant to be the low
level no-magic API that sits underneath. The differences between the two
APIs are:
- `attribute`
- Lazy (the attribute will be defined after the schema has loaded)
- Allows either a type object or a symbol
- `define_attribute`
- Runs immediately (might get trampled by schema loading)
- Requires a type object
This was the last blocker in terms of public interface requirements
originally discussed for this feature back in May. All the
implementation blockers have been cleared, so this feature is probably
ready for release (pending one more look-over by me).
|
|
|
|
|
|
|
|
|
|
|
| |
When an attribute is assigned, we determine if it was already marked as
changed so we can determine if we need to clear the changes, or mark it
as changed. Since this only affects the `attributes_changed_by_setter`
hash, in-place changes are irrelevant to this process. Since calculating
in-place changes can be expensive, we can just skip it here.
I also added a test for the only edge case I could think of that would
be affected by this change.
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
This sets a precident for how we handle `attribute` calls, which aren't
backed by a database column. We should not take this as a conscious
decision on how to handle them, and this can change when we make
`attribute` public if we have better ideas in the future.
As the composed attributes API gets fleshed out, I expect the
`persistable_attributes` method to change to
`@attributes.select(&:persistable).keys`, or some more performant
variant there-of. This can probably go away completely once we fully
move dirty checking into the attribute objects once it gets moved up to
Active Model.
Fixes #18407
|
|
|
|
| |
`ActiveModel::Dirty#reset_changes`.
|