While working on updating Paper Trail for 5.1 compatibility, I noticed
that I was required to pass a second argument to `attribute`. I didn't
intend for this to be the case, as `attribute :foo` is totally
reasonable shorthand for "I want `attr_accessor :foo`, but also have it
work with things like `.attributes` and `ActiveRecord::Dirty`"
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
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#25787Close#25841
Ruby 2.4 unifies Fixnum and Bignum into Integer: https://bugs.ruby-lang.org/issues/12005
* Forward compat with new unified Integer class in Ruby 2.4+.
* Backward compat with separate Fixnum/Bignum in Ruby 2.2 & 2.3.
* Drops needless Fixnum distinction in docs, preferring Integer.
When a proc is given as a default value, the form builder ends up
displaying `Proc#to_s` when the default is used. That's because we
didn't handle the proc until type casting. This issue technically can
occur any time that a proc is the value before type casting, but in
reality the only place that will occur is when a proc default is
provided through the attributes API, so the best place to handle this
edge case is there.
I've opted to memoize instead of just moving the `Proc#call` up, as this
made me realize that it could potentially interact very poorly with
dirty checking.
The code here is a little redundant, but I don't want to rely on how
`value_before_type_cast` is implemented in the super class, even if it's
just an `attr_reader`.
Fixes#24249Close#24306
I've added a redundant test for this under the attributes API as well,
as that also causes this bug to manifest through public API (and
demonstrates that calling `reset_column_information` on the child
classes would be insufficient)
Since children of a class should always share a table with their parent,
just reloading the schema from the cache should be sufficient here.
`reload_schema_from_cache` should probably become public and
`# :nodoc:`, but I'd rather avoid the git churn here.
Fixes#22057
As described in the "Follow Coding Conventions" section in our
contribution guide (http://edgeguides.rubyonrails.org/contributing_to_ruby_on_rails.html#follow-the-coding-conventions)
we favor `assert_not` over `refute`.
While we don't usually make stylistic changes on it's own I opted to do
it in this case. The reason being that test cases are usually copied as
a starting point for new tests. This results in a spread of `refute` in
files that have been using it already.
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 is a variant implementation of the changes proposed in #19914.
Unlike that PR, the change in behavior is isolated in its own class.
This is to prevent wonky behavior if a Proc is assigned outside of the
default, and it is a natural place to place the behavior required by #19921
as well.
Close#19914.
[Sean Griffin & Kir Shatrov]
The name `ActiveModel::AttributeAssignment::UnknownAttributeError` is
too implementation specific so let's move the constant directly under
the ActiveModel namespace.
Also since this constant used to be under the ActiveRecord namespace, to
make the upgrade path easier, let's avoid raising the former constant
when we deal with this error on the Active Record side.
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).
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.
