| Commit message (Collapse) | Author | Age | Files | Lines |
|---|
| |
|
|
|
|
| |
Follow up to 5b14129d8d4ad302b4e11df6bd5c7891b75f393c.
http://edgeapi.rubyonrails.org/classes/ActiveRecord/Attribute.html
|
| | |
|
| | |
|
| |
|
|
| |
For reduce instantiating `Type::Value`.
|
| |
|
|
| |
Fixes #26122
|
| | |
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Previously we had primarily tested the behavior of these attributes by
calling `.new`, allowing this to slip through the cracks. There were a
few ways in which they were behaving incorrectly.
The biggest issue was that attempting to read the attribute would
through a `MissingAttribute` error. We've corrected this by returning
the default value when the attribute isn't backed by a database column.
This is super special cased, but I don't see a way to avoid this
conditional. I had considered handling this higher up in
`define_default_attribute`, but we don't have the relevant information
there as users can provide new defaults for database columns as well.
Once I corrected this, I had noticed that the attributes were always
being marked as changed. This is because the behavior of
`define_default_attribute` was treating them as assigned from
`Attribute::Null`.
Finally, with our new implementation, `LazyAttributeHash` could no
longer be marshalled, as it holds onto a proc. This has been corrected
as well. I've not handled YAML in that class, as we do additional work
higher up to avoid YAML dumping it at all.
Fixes #25787
Close #25841
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
| |
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
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Currently CI is broken due to 56a61e0 and c4cb686. This occurred because
the failures are not present on SQLite which is what I normally run
locally before pushing.
The optimizations to our YAML size were dropping mutations, as
`with_type` didn't set the previous value if it'd already been read
(that method was never really designed to be used with values on
individual objects, it was previously only used for defaults). I'm
questioning whether there's a better place to be handling the exclusion
of the type, but this will fix the failing build.
Additionally, there was a bug in `remove_foreign_key` if you passed it
an options hash containing `to_table`. This now occurs whenever removing
a reference, as we always normalize to a hash.
[Sean Griffin & Ryuta Kamizono]
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
This reduces the size of a YAML encoded Active Record object by ~80%
depending on the number of columns. There were a number of wasteful
things that occurred when we encoded the objects before that have
resulted in numerous wins
- We were emitting the result of `attributes_before_type_cast` as a hack
to work around some laziness issues
- The name of an attribute was emitted multiple times, since the
attribute objects were in a hash keyed by the name. We now store them
in an array instead, and reconstruct the hash using the name
- The types were included for every attribute. This would use backrefs
if multiple objects were encoded, but really we don't need to include
it at all unless it differs from the type at the class level. (The
only time that will occur is if the field is the result of a custom
select clause)
- `original_attribute:` was included over and over and over again since
the ivar is almost always `nil`. We've added a custom implementation
of `encode_with` on the attribute objects to ensure we don't write the
key when the field is `nil`.
This isn't without a cost though. Since we're no longer including the
types, an object can find itself in an invalid state if the type changes
on the class after serialization. This is the same as 4.1 and earlier,
but I think it's worth noting.
I was worried that I'd introduce some new state bugs as a result of
doing this, so I've added an additional test that asserts mutation not
being lost as the result of YAML round tripping.
Fixes #25145
|
| |
|
|
|
|
|
|
|
|
|
|
| |
Using ActiveRecord::Base.attribute to declare an attribute with a default value on a model where the attribute is not backed by the database would raise a NotImplementedError when model.save is called.
The error originates from https://github.com/rails/rails/blob/59d252196b36f6afaafd231756d69ea21537cf5d/activerecord/lib/active_record/attribute.rb#L84.
This is called from https://github.com/rails/rails/blob/59d252196b36f6afaafd231756d69ea21537cf5d/activerecord/lib/active_record/attribute.rb#L46 on an ActiveRecord::Attribute::Null object.
This commit corrects the behavior by implementing ActiveRecord::Attribute::Null#type_cast.
With ActiveRecord::Attribute::Null#type_cast defined, ActiveRecord::Attribute::Null#value (https://github.com/rails/rails/blob/59d252196b36f6afaafd231756d69ea21537cf5d/activerecord/lib/active_record/attribute.rb#L173..L175) can be replaced with its super method (https://github.com/rails/rails/blob/59d252196b36f6afaafd231756d69ea21537cf5d/activerecord/lib/active_record/attribute.rb#L36..L40).
fixes #24979
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
We can skip the allocation of a full `AttributeSet` by changing the
semantics of how we structure things. Instead of comparing two separate
`AttributeSet` objects, and `Attribute` is now a singly linked list of
every change that has happened to it. Since the attribute objects are
immutable, to apply the changes we simply need to copy the head of the
list.
It's worth noting that this causes one subtle change in the behavior of
AR. When a record is saved successfully, the `before_type_cast` version
of everything will be what was sent to the database. I honestly think
these semantics make more sense, as we could have just as easily had the
DB do `RETURNING *` and updated the record with those if we had things
like timestamps implemented at the DB layer.
This brings our performance closer to 4.2, but we're still not quite
there.
|
| |
|
|
|
|
| |
The external uses of this method have been removed, and I'd like to
internally re-use that name, as I'm planning to encapsulate `changed?`
into the attribute object itself.
|
| |
|
|
|
|
|
|
|
|
|
|
|
| |
This moves a bit more of the logic required for dirty checking into the
attribute objects. I had hoped to remove the `with_value_from_database`
stuff, but unfortunately just calling `dup` on the attribute objects
isn't enough, since the values might contain deeply nested data
structures. I think this can be cleaned up further.
This makes most dirty checking become lazy, and reduces the number of
object allocations and amount of CPU time when assigning a value. This
opens the door (but doesn't quite finish) to improving the performance
of writes to a place comparable to 4.1
|
| | |
|
| | |
|
| | |
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
It's finally finished!!!!!!! The reason the Attributes API was kept
private in 4.2 was due to some publicly visible implementation details.
It was previously implemented by overloading `columns` and
`columns_hash`, to make them return column objects which were modified
with the attribute information.
This meant that those methods LIED! We didn't change the database
schema. We changed the attribute information on the class. That is
wrong! It should be the other way around, where schema loading just
calls the attributes API for you. And now it does!
Yes, this means that there is nothing that happens in automatic schema
loading that you couldn't manually do yourself. (There's still some
funky cases where we hit the connection adapter that I need to handle,
before we can turn off automatic schema detection entirely.)
There were a few weird test failures caused by this that had to be
fixed. The main source came from the fact that the attribute methods are
now defined in terms of `attribute_names`, which has a clause like
`return [] unless table_exists?`. I don't *think* this is an issue,
since the only place this caused failures were in a fake adapter which
didn't override `table_exists?`.
Additionally, there were a few cases where tests were failing because a
migration was run, but the model was not reloaded. I'm not sure why
these started failing from this change, I might need to clear an
additional cache in `reload_schema_from_cache`. Again, since this is not
normal usage, and it's expected that `reset_column_information` will be
called after the table is modified, I don't think it's a problem.
Still, test failures that were unrelated to the change are worrying, and
I need to dig into them further.
Finally, I spent a lot of time debugging issues with the mutex used in
`define_attribute_methods`. I think we can just remove that method
entirely, and define the attribute methods *manually* in the call to
`define_attribute`, which would simplify the code *tremendously*.
Ok. now to make this damn thing public, and work on moving it up to
Active Model.
|
| |
|
|
|
|
|
|
| |
`bound_attributes` is now used universally across the board, removing
the need for the conversion layer. These changes are mostly mechanical,
with the exception of the log subscriber. Additional, we had to
implement `hash` on the attribute objects, so they could be used as a
key for query caching.
|
| | |
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
This method can be used to see all of the fields on a model which have
been read. This can be useful during development mode to quickly find
out which fields need to be selected. For performance critical pages, if
you are not using all of the fields of a database, an easy performance
win is only selecting the fields which you need. By calling this method
at the end of a controller action, it's easy to determine which fields
need to be selected.
While writing this, I also noticed a place for an easy performance win
internally which I had been wanting to introduce. You cannot mutate a
field which you have not read. Therefore, we can skip the calculation of
in place changes if we have never read from the field. This can
significantly speed up methods like `#changed?` if any of the fields
have an expensive mutable type (like `serialize`)
```
Calculating -------------------------------------
#changed? with serialized column (before)
391.000 i/100ms
#changed? with serialized column (after)
1.514k i/100ms
-------------------------------------------------
#changed? with serialized column (before)
4.243k (± 3.7%) i/s - 21.505k
#changed? with serialized column (after)
16.789k (± 3.2%) i/s - 84.784k
```
|
| |
|
|
|
|
| |
While we don't want to change the form input when validations fail,
blindly using `_before_type_cast` will cause the input to display the
wrong data for any type which does additional work on database values.
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
In the case of serialized columns, we would expect the unserialized
value as input, not the serialized value. The original issue which made
this distinction, #14163, introduced a bug. If you passed serialized
input to the method, it would double serialize when it was sent to the
database. You would see the wrong input upon reloading, or get an error
if you had a specific type on the serialized column.
To put it another way, `update_column` is a special case of
`update_all`, which would take `['a']` and not `['a'].to_yaml`, but you
would not pass data from `params` to it.
Fixes #18037
|
| | |
|
| |
|
|
|
| |
This was a small self contained piece of the refactoring that I am
working on, which required these objects to be comparable.
|
| |
|
|
|
|
| |
Making this change revealed several subtle bugs related to models with
no primary key, and anonymous classes. These have been fixed as well,
with regression tests added.
|
| |
|
|
|
| |
This allows using polymorphism for the uninitialized attributes raising
an exception behavior.
|
| |
|
|
|
|
|
|
| |
This will make it less painful to add additional properties, which
should persist across writes, such as `name`.
Conflicts:
activerecord/lib/active_record/attribute_set.rb
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Moved `Builder` to its own file, as it started looking very weird once I
added private methods to the `AttributeSet` class and the `Builder`
class started to grow.
Would like to refactor `fetch_value` to change to
```ruby
self[name].value(&block)
```
But that requires the attributes to know about their name, which they
currently do not.
|
| |
|
|
|
|
|
|
|
|
| |
Adding `# :nodoc:` to the parent `class` / `module` is not going
to ignore nested classes or modules.
There is a modifier `# :nodoc: all` but sadly the containing class
or module will continue to be in the docs.
/cc @sgrif
|
| | |
|
|
|
There's a lot more that can be moved to these, but this felt like a good
place to introduce the object. Plans are:
- Remove all knowledge of type casting from the columns, beyond a
reference to the cast_type
- Move type_cast_for_database to these objects
- Potentially make them mutable, introduce a state machine, and have
dirty checking handled here as well
- Move `attribute`, `decorate_attribute`, and anything else that
modifies types to mess with this object, not the columns hash
- Introduce a collection object to manage these, reduce allocations, and
not require serializing the types
|
