We ran into a few cases at GitHub where we were using alias_attribute
incorrectly and the new behavior either didn't warn or raised an unclear
deprecation warning. This attempts to add clarity to the deprecation reason
when you try to alias something that isn't actually an attribute.
Previously, trying to alias a generated attribute method, such as `attribute_in_database`, would
still hit `define_proxy_call`, because we were only checking the owner of the target method.
In the future, we should probably raise if you try to use alias_attribute for a non-attribute.
Note that we don't raise the warning for abstract classes, because the attribute may be implemented
by a child class. We could potentially figure out a way to raise in these cases as well, but this
hopefully is good enough for now.
Finally, I also updated the way we're setting `local_alias_attributes` when `alias_attribute` is
first called. This was causing problems since we now use `alias_attribute` early in the
`load_schema!` call for some models: https://buildkite.com/rails/rails/builds/98910
This backfills a changelog entry for PR #48876. This is potentially something Rails users
should be aware of as in very specific situations it can be a change in behavior
This commit deprecates `read_attribute(:id)` returning the primary key
if the model's primary key is not the id column. Starting in Rails 7.2,
`read_attribute(:id)` will always return the value of the id column.
This commit also changes `read_attribute(:id)` for composite primary
key models to return the value of the id column, not the composite
primary key.
In `1818beb3a3ea5fdb498095d4885f8a7e512f24ca` Rails changed the target
where alias attribute methods are defined. It lead to
`undefine_attribute_methods` to clean alias attribute methods along with
the attribute methods. It was an intended behavior change but it wasn't
properly documented and tested. This commit clarifies the new behavior
in the Active Model changelog along with covering the behavior with tests.
When an abstract class inherited from an Active Record model
defines an alias attribute Rails should expect the original methods
of the aliased attribute to be defined in the parent class and avoid
raising deprecation warning.
This is similar to a [previous commit][1] which ensures that versioned
migrations always call `super` in `compatible_table_definition`. In this
case, these methods are being pulled up to `Current` so that all
subclasses will use a `TableDefinition` class and future developers do
not have to remember to add all of these methods to new versioned
classes when a new one is created.
[1]: 16f8bd79444a512dfebf2d77bd2fd3075041475b
While working on #48969, I found that some of the Compatibility test
cases were not working correctly. The tests removed in this commit were
never running the `change_table` migration and so were not actually
testing that `change_table` works correctly. The issue is that the two
migrations created in these tests both have `nil` versions, and so the
Migrator only runs the first one.
This commit refactors the tests so that its easier to test the behavior
of each Migration class version (and I think the rest of the tests
should be updated to use this strategy as well). Additionally, since the
tests are fixed it exposed that `t.change` in a `change_table` is not
behaving as expected so that is fixed as well.
This commit adds support for composite identifiers in `to_key`.
Rails 7.1 adds support for composite primary key which means that
composite primary key models' `#id` method returns an `Array` and
`to_key` needs to avoid double-wrapping the value.
This allows access to the raw id column value on records for which an
id column exists but is not the primary key. This is common amongst
models with composite primary keys.
rails/rails@a88f47d fixed an issue where subclasses were regenerating aliased attribute methods defined on parent classes. Part of the solution was to call #generate_alias_attributes recursively on a class's superclass to generate all the alias attributes in the inheritance hierarchy. However, the implementation relies on #base_class to determine if we should call #generate_alias_attributes on the superclass. Since all models that inherit from abstract classes are base classes, this means that #generate_alias_attributes will never be called on abstract classes, meaning no method will be generated for any alias attributes defined on them.
To fix this issue, we should always call #generate_alias_attributes on the superclass unless the superclass is ActiveRecord::Base.
I'm working on an enhancement to query constraints that will require me
to know when we have query constraints but not a composite key.
Currently if you have a composite key it will be included in the query
constraints list. There's not a way to differentiate between the two
which means that we're forced into setting the query constraints on the
associations for the primary key.
This change adds a `has_query_constraints?` method so we can check the
class for query constraints. The options still work as well but we can
be sure we're always picking up the query constraints when they're
present.
We originally implemented this project to use OR queries instead of IN
because it seemed like there was no way to do that without a row
constructor. While working on implementing this feature in our vitess
gem I noticed that we can actually get the queries we wanted, and the
new code is more performant than generating an OR.
SQL Before:
```sql
SELECT "sharded_comments".*
FROM "sharded_comments"
WHERE ("sharded_comments"."blog_id" = 969142904
AND ("sharded_comments"."blog_post_id" = 357271355
OR "sharded_comments"."blog_post_id" = 756811794)
OR "sharded_comments"."blog_id" = 308674288
AND "sharded_comments"."blog_post_id" = 1055755181)
```
SQL After:
```sql
SELECT "sharded_comments".*
FROM "sharded_comments"
WHERE "sharded_comments"."blog_id"
IN (969142904, 308674288)
AND "sharded_comments"."blog_post_id"
IN (357271355, 756811794, 1055755181)
```
Using one of the tests that utilizes this code path, I benchmarked the
queries. The new implementation is faster:
Before:
```
Warming up --------------------------------------
queries 147.000 i/100ms
Calculating -------------------------------------
queries 1.486k (± 3.2%) i/s - 7.497k in 5.050742s
```
After:
```
Warming up --------------------------------------
queries 179.000 i/100ms
Calculating -------------------------------------
queries 1.747k (± 4.5%) i/s - 8.771k in 5.031424s
```
We can probably improve this more but I think this query is more in line
with what we want and expect while also being more performant (without
having to build a new row constructor in arel).
Because we're using a class_attribute to track all of the attribute_aliases,
each subclass was regenerating the parent class methods, causing some unexpected
deprecation messages.
Instead, we can use a class instance variable to track the attributes aliased locally
in that particular subclass. We then walk up the chain and re-define the attribute methods
if they haven't been defined yet.
Pluses cannot be used to create code blocks when the content includes a
space.
Found using a regular expression:
```bash
$ rg '#\s[^+]*\+[^+]*\s[^+]*\S\+'
```
