Commit Graph

1468 Commits

Author SHA1 Message Date
Ryuta Kamizono
783cafee4f Maintain eager loading joining order as before
If a relation has eager_load and string joins only, string joins will be
regarded as leading joins unlike before, due to #36805.

To maintain that joining order as before, check eager loading join
first before string joins.

Fixes #37133.
2019-09-18 17:42:15 +09:00
Ryuta Kamizono
e6f953fc39 Deduplicate joins values
#36805 have one possible regression that failing deduplication if
`joins_values` have complex order (e.g. `joins_values = [join_node_a,
:comments, :tags, join_node_a]`).

This fixes the deduplication to take it in the first phase before
grouping.
2019-08-02 03:35:44 +09:00
Kasper Timm Hansen
40fc31c103
Merge pull request #36708 from rails/has-one-polymorphic-touch-dont-cache-association-result-inside-create-transaction
Polymorphic has_one touch: Don't cache association result inside crea…
2019-07-31 06:31:50 +02:00
Kasper Timm Hansen
cea392eb96
Polymorphic has_one touch: Reset association cache result after create transaction
In case of a polymorphic association there's no automatic inverse_of to assign the
inverse record. So to get the record there needs to be a query executed,
however, if the query fires within the transaction that's trying to create
the associated record, no record can be found. And worse, the nil result is cached
on the association so after the transaction commits the record can't be found.

That's what happens if touch is enabled on a polymorphic has_one association.

Consider a Comment with a commentable association that needs to be touched.

For `Comment.create(commentable: Post.new)`, the existing code essentially
does `commentable.send(:comment)` within the create transaction for the comment
and thus not finding the comment.

Now we're purposefully clearing the cache in case we've tried accessing
the association within the transaction and found no object.

Before:

```
kaspth-imac 2.6.3 ~/code/rails/activerecord master *= ARCONN=postgresql bin/test test/cases/associations/has_one_associations_test.rb -n /commit/
Using postgresql
Run options: -n /commit/ --seed 46022

D, [2019-07-19T03:30:37.864537 #96022] DEBUG -- :   Chef Load (0.2ms)  SELECT "chefs".* FROM "chefs" WHERE "chefs"."employable_id" = $1 AND "chefs"."employable_type" = $2 LIMIT $3  [["employable_id", 1], ["employable_type", "DrinkDesignerWithPolymorphicTouchChef"], ["LIMIT", 1]]
D, [2019-07-19T03:30:37.865013 #96022] DEBUG -- :   Chef Create (0.2ms)  INSERT INTO "chefs" ("employable_id", "employable_type") VALUES ($1, $2) RETURNING "id"  [["employable_id", 1], ["employable_type", "DrinkDesignerWithPolymorphicTouchChef"]]
D, [2019-07-19T03:30:37.865201 #96022] DEBUG -- :   TRANSACTION (0.1ms)  RELEASE SAVEPOINT active_record_1
D, [2019-07-19T03:30:37.874136 #96022] DEBUG -- :   TRANSACTION (0.1ms)  ROLLBACK
D, [2019-07-19T03:30:37.874323 #96022] DEBUG -- :   TRANSACTION (0.1ms)  ROLLBACK
F

Failure:
HasOneAssociationsTest#test_polymorphic_has_one_with_touch_option_on_create_wont_cache_assocation_so_fetching_after_transaction_commit_works [/Users/kaspth/code/rails/activerecord/test/cases/associations/has_one_associations_test.rb:716]:
--- expected
+++ actual
@@ -1 +1 @@
-#<Chef id: 1, employable_id: 1, employable_type: "DrinkDesignerWithPolymorphicTouchChef", department_id: nil, employable_list_type: nil, employable_list_id: nil>
+nil
```

After:

```
kaspth-imac 2.6.3 ~/code/rails/activerecord master *= ARCONN=postgresql bin/test test/cases/associations/has_one_associations_test.rb -n /commit/
Using postgresql
Run options: -n /commit/ --seed 46022

D, [2019-07-19T03:30:22.479387 #95973] DEBUG -- :   Chef Create (0.3ms)  INSERT INTO "chefs" ("employable_id", "employable_type") VALUES ($1, $2) RETURNING "id"  [["employable_id", 1], ["employable_type", "DrinkDesignerWithPolymorphicTouchChef"]]
D, [2019-07-19T03:30:22.479574 #95973] DEBUG -- :   TRANSACTION (0.1ms)  RELEASE SAVEPOINT active_record_1
D, [2019-07-19T03:30:22.482051 #95973] DEBUG -- :   Chef Load (0.1ms)  SELECT "chefs".* FROM "chefs" WHERE "chefs"."employable_id" = $1 AND "chefs"."employable_type" = $2 LIMIT $3  [["employable_id", 1], ["employable_type", "DrinkDesignerWithPolymorphicTouchChef"], ["LIMIT", 1]]
D, [2019-07-19T03:30:22.482317 #95973] DEBUG -- :   TRANSACTION (0.1ms)  ROLLBACK
D, [2019-07-19T03:30:22.482437 #95973] DEBUG -- :   TRANSACTION (0.1ms)  ROLLBACK
.

Finished in 0.088498s, 11.2997 runs/s, 22.5994 assertions/s.
1 runs, 2 assertions, 0 failures, 0 errors, 0 skips
```

Notice the select now fires after the commit.
2019-07-31 05:59:23 +02:00
Ryuta Kamizono
b4478ae8bc Preserve user supplied joins order as much as possible
Currently, string joins are always applied as last joins part, and Arel
join nodes are always applied as leading joins part (since #36304), it
makes people struggled to preserve user supplied joins order.

To mitigate this problem, preserve the order of string joins and Arel
join nodes either before or after of association joins.

Fixes #36761.
Fixes #34328.
Fixes #24281.
Fixes #12953.
2019-07-30 04:02:58 +09:00
st0012
6fbf52d580 Use capture_sql helper method in tests 2019-07-28 14:47:57 +08:00
Akira Matsuda
d1ffe59ab5 Use match? where we don't need MatchData
We're already running Performance/RegexpMatch cop, but it seems like the cop is not always =~ justice
2019-07-27 13:06:49 +09:00
Takayuki Nakata
285f081e1d Fix join middle table alias when using HABTM
In using HABTM, join middle table alias is combined with the associated
models name without sort, while middle table name is combined with those
models name with sort.

Fixes #36742.
2019-07-26 23:12:44 +09:00
Ryuta Kamizono
c81af6ae72 Enable Layout/EmptyLinesAroundAccessModifier cop
We sometimes say "✂️ newline after `private`" in a code review (e.g.
https://github.com/rails/rails/pull/18546#discussion_r23188776,
https://github.com/rails/rails/pull/34832#discussion_r244847195).

Now `Layout/EmptyLinesAroundAccessModifier` cop have new enforced style
`EnforcedStyle: only_before` (https://github.com/rubocop-hq/rubocop/pull/7059).

That cop and enforced style will reduce the our code review cost.
2019-06-13 12:00:45 +09:00
Ryuta Kamizono
64d8c54e16 Allow column name with function (e.g. length(title)) as safe SQL string
Currently, almost all "Dangerous query method" warnings are false alarm.
As long as almost all the warnings are false alarm, developers think
"Let's ignore the warnings by using `Arel.sql()`, it actually is false
alarm in practice.", so I think we should effort to reduce false alarm
in order to make the warnings valuable.

This allows column name with function (e.g. `length(title)`) as safe SQL
string, which is very common false alarm pattern, even in the our
codebase.

Related 6c82b6c99, 6607ecb2a, #36420.

Fixes #32995.
2019-06-10 07:36:58 +09:00
Ryuta Kamizono
e3c1f42b31
Merge pull request #36429 from bogdan/fix-preloading-duplicate-records
Fix preloading on AR::Relation where records are duplicated by a join
2019-06-07 07:38:27 +09:00
Bogdan Gusiev
30c1999df1 Fix preloading on AR::Relation where records are duplicated by a join 2019-06-06 15:14:40 +03:00
Ryuta Kamizono
648144649a
Merge pull request #36426 from abhaynikam/bump-codeclimate-rubocop-version
Bump rubocop to 0.71
2019-06-06 20:40:20 +09:00
Abhay Nikam
00b3b68602 Bump rubocop to 0.71 2019-06-06 15:34:50 +05:30
Ryuta Kamizono
7696f44f6f Allow quoted identifier string as safe SQL string
Currently `posts.title` is regarded as a safe SQL string, but
`"posts"."title"` (it is a result of `quote_table_name("posts.title")`)
is regarded as an unsafe SQL string even though a result of
`quote_table_name` should obviously be regarded as a safe SQL string,
since the column name matcher doesn't respect quotation, it is a little
annoying.

This changes the column name matcher to allow quoted identifiers as safe
SQL string, now all results of the `quote_table_name` are regarded as
safe SQL string.
2019-06-06 03:57:24 +09:00
Ryuta Kamizono
f0445213d8 Address intermittent CI failure due to non-determined sort order
https://buildkite.com/rails/rails/builds/61384#ad441461-87d8-4bdc-a71f-61921fe2df2e/993-1004
2019-05-30 00:01:04 +09:00
Ryuta Kamizono
f47be7bd96 Address intermittent CI failure in cascaded_eager_loading_test.rb
https://buildkite.com/rails/rails/builds/61362#99165d42-172d-4ad5-bf72-b29d8cd44f3e/995-1006
2019-05-29 03:52:48 +09:00
Ryuta Kamizono
2f96cfbb54 Address intermittent CI failure due to unfilled schema columns cache
https://buildkite.com/rails/rails/builds/61358#a78ee50e-30b5-48a2-858f-63eba287d919/1290-1298
2019-05-29 00:32:41 +09:00
Ryuta Kamizono
3453c5d4c0 Give up filling schema cache before assert_no_queries
This reverts commit a1ee4a9ff9d4a3cb255365310ead0dc7b739c6be.

Even if a1ee4a9 is applied, CI is still flakiness.

https://buildkite.com/rails/rails/builds/61252#2c090afa-aa84-4a2b-8b81-9f09219222c6/994-1005
https://buildkite.com/rails/rails/builds/61252#2e55bf83-1bde-44a2-a4f1-b5c3f6820fb4/929-938

Failing tests by whether schema cache is filled or not, it actually
means that whether SCHEMA SQLs are executed or not is not target for the
tests.

So I've reverted commit a1ee4a9 which filling schema cache before
`assert_no_queries`, and replace `assert_no_queries` to
`assert_queries(0)`.
2019-05-22 01:23:58 +09:00
Ryuta Kamizono
7412b7f8a6 Implicit through table joins should be appeared before user supplied joins
#36293 was an issue for through association with `joins` for a long
time, but since #35864 through association with `left_joins` would also
be affected by the issue.

Implicit through table joins should be appeared before user supplied
joins, otherwise loading through association with joins will cause a
statement invalid error.

Fixes #36293.

```
% ARCONN=postgresql bundle exec ruby -w -Itest test/cases/associations/has_many_through_associations_test
.rb -n test_through_association_with_joins
Using postgresql
Run options: -n test_through_association_with_joins --seed 7116

# Running:

E

Error:
HasManyThroughAssociationsTest#test_through_association_with_joins:
ActiveRecord::StatementInvalid: PG::UndefinedTable: ERROR:  missing FROM-clause entry for table "posts"
LINE 1: ... "comments_posts" ON "comments_posts"."post_id" = "posts"."i...
                                                             ^
: SELECT "comments".* FROM "comments" INNER JOIN "comments" "comments_posts" ON "comments_posts"."post_id" = "posts"."id" INNER JOIN "posts" ON "comments"."post_id" = "posts"."id" WHERE "posts"."author_id" = $1

rails test test/cases/associations/has_many_through_associations_test.rb:61

Finished in 0.388657s, 2.5730 runs/s, 0.0000 assertions/s.
1 runs, 0 assertions, 0 failures, 1 errors, 0 skips
```
2019-05-19 19:43:25 +09:00
Ryuta Kamizono
13d6aa3a7b
Merge pull request #36284 from kamipo/fix_eager_loading_with_string_joins
Fix eager loading associations with string joins not to raise NoMethodError
2019-05-15 22:24:57 +09:00
Ryuta Kamizono
ec6dfdc8d2 Fix eager loading associations with string joins not to raise NoMethodError
Fixes #34456.
2019-05-15 22:00:01 +09:00
Ryuta Kamizono
dcb825902d Don't track implicit touch mutation
This partly reverts the effect of d1107f4d.

d1107f4d makes `touch` tracks the mutation whether the `touch` is
occurred by explicit or not.

Existing apps expects that the previous changes tracks only the changes
which is explicit action by users.

I'd revert the implicit `touch` mutation tracking since I'd not like to
break existing apps.

Fixes #36219.
2019-05-13 23:30:05 +09:00
Jean Boussier
cce26c36ca Namespace association extension modules under the owner model 2019-05-02 13:25:18 +02:00
Rob Trame
f4fd7d1d20 Make scope arity check consistent (#36134)
* Make scope arity check consistent

* Add test for arity change

[Rob Trame + Rafael Mendonça França]
2019-05-01 15:53:35 -05:00
Ryuta Kamizono
20ede2e2e6 Fix merging left_joins to maintain its own join_type context
This fixes a regression for #35864.

Usually, stashed joins (mainly eager loading) are performed as LEFT
JOINs.
But the case of merging joins/left_joins of different class, that
(stashed) joins are performed as the same `join_type` as the parent
context for now.
Since #35864, both (joins/left_joins) stashed joins might be contained
in `joins_values`, so each stashed joins should maintain its own
`join_type` context.

Fixes #36103.
2019-04-27 21:43:57 +09:00
Rafael França
d4d145a679
Merge pull request #32313 from lulalala/model_error_as_object
Model error as object
2019-04-24 16:16:00 -04:00
Ryuta Kamizono
186458753f Merge pull request #35869 from abhaynikam/35866-add-touch-option-for-has-one-association
Adds missing touch option to has_one association
2019-04-25 03:32:36 +09:00
Abhay Nikam
3fe83d1dd9 Adds touch option to has_one association 2019-04-25 03:31:34 +09:00
Ryuta Kamizono
5d858b5d3e Fix sliced IN clauses to be grouped
Follow up of #35838.

And also this refactors `in_clause_length` handling is entirely
integrated in Arel visitor.
2019-04-24 13:35:42 +09:00
Ryuta Kamizono
fc35da76e9 Make association builder methods private 2019-04-24 02:36:26 +09:00
Ryuta Kamizono
fa8c525d20 Fix automatic_inverse_of not to be disabled if extension block is given
If an association has a scope, `automatic_inverse_of` is to be disabled.
But extension block is obviously not a scope. It should not be regarded
as a scope.

Fixes #28806.
2019-04-12 18:01:57 +09:00
Ryuta Kamizono
faf07d1468 Merge pull request #28155 from lcreid/belongs_to
Fix "autosave: true" on belongs_to of join model causes invalid records to be saved
2019-04-10 16:42:16 +09:00
Ryuta Kamizono
17f2f3054c Association loading isn't to be affected by scoping consistently
Follow-up of 5c71000, #29834, and #30271.

Currently, preloading and eager loading are not to be affected by
scoping, with the exception of `unscoped`.

But non eager loaded association access is still affected by scoping.

Although this is a breaking change, the association loading will work
consistently whether preloaded / eager loaded or not.

Before:

```ruby
Post.where("1=0").scoping do
  Comment.find(1).post                   # => nil
  Comment.preload(:post).find(1).post    # => #<Post id: 1, ...>
  Comment.eager_load(:post).find(1).post # => #<Post id: 1, ...>
end
```

After:

```ruby
Post.where("1=0").scoping do
  Comment.find(1).post                   # => #<Post id: 1, ...>
  Comment.preload(:post).find(1).post    # => #<Post id: 1, ...>
  Comment.eager_load(:post).find(1).post # => #<Post id: 1, ...>
end
```

Fixes #34638.
Fixes #35398.
2019-04-05 13:21:50 +09:00
Ryuta Kamizono
8f05035b7e Stash left_joins into joins to deduplicate redundant LEFT JOIN
Originally the `JoinDependency` has the deduplication for eager loading
(LEFT JOIN). This re-uses that deduplication for `left_joins`.

And also, This makes left join order into part of joins, i.e.:

Before:

```
association joins -> stash joins (eager loading, etc) -> string joins -> left joins
```

After:

```
association joins -> stash joins (eager loading, left joins, etc) -> string joins
```

Now string joins are able to refer left joins.

Fixes #34325.
Fixes #34332.
Fixes #34536.
2019-04-05 06:40:53 +09:00
lulalala
abee034368 Raise deprecation for calling [:f] = 'b' or [:f] << 'b'
Revert some tests to ensure back compatibility
2019-03-31 22:59:12 +08:00
Ryan Kerr
2e3bba3e3a Fix callbacks on has_many :through associations (#33249)
When adding a child record via a has_many :through association,
build_through_record would previously build the join record, and then
assign the child record and source_type option to it.  Because the
before_add and after_add callbacks are called as part of build, however,
this caused the callbacks to receive incomplete records, specifically
without the other end of the has_many :through association.  Collecting
all attributes before building the join record ensures the callbacks
receive the fully constructed record.
2019-03-30 00:37:08 -04:00
Ryuta Kamizono
3a0929901f
Merge pull request #35496 from bogdan/right-preloading
Fix preloader to never reset associations in case they are already loaded
2019-03-28 04:42:38 +09:00
Ryuta Kamizono
b2b559c75c Fix CI failure due to remaining tagging records
`TRUNCATE TABLE posts` also resets `AUTO_INCREMENT`. If newly created a
post, it is wrongly associated with remaining tagging records.
To un-associate remaining tagging record, use `post.create_tagging!`
instead.

Fixes #35751.
2019-03-26 12:59:16 +09:00
Yasuo Honda
4dd3b2bd9a Use assert_queries(0) instead of assert_no_queries to ignore metadata queries
Fix #35665

```ruby
$ ARCONN=mysql2 bin/test test/cases/scoping/named_scoping_test.rb test/cases/tasks/database_tasks_test.rb test/cases/associations/cascaded_eager_loading_test.rb test/cases/associations/eager_singularization_test.rb -n "/^(?:NamedScopingTest#(?:test_many_should_not_fire_query_if_scope_loaded)|ActiveRecord::DatabaseTasksDumpSchemaCacheTest#(?:test_dump_schema_cache)|CascadedEagerLoadingTest#(?:test_eager_association_loading_with_has_many_sti_and_subclasses)|EagerSingularizationTest#(?:test_eager_no_extra_singularization_has_many_through_belongs_to))$/" --seed 16818
Using mysql2
Run options: -n "/^(?:NamedScopingTest#(?:test_many_should_not_fire_query_if_scope_loaded)|ActiveRecord::DatabaseTasksDumpSchemaCacheTest#(?:test_dump_schema_cache)|CascadedEagerLoadingTest#(?:test_eager_association_loading_with_has_many_sti_and_subclasses)|EagerSingularizationTest#(?:test_eager_no_extra_singularization_has_many_through_belongs_to))$/" --seed 16818

...F

Failure:
CascadedEagerLoadingTest#test_eager_association_loading_with_has_many_sti_and_subclasses [/home/yahonda/git/rails/activerecord/test/cases/associations/cascaded_eager_loading_test.rb:124]:
1 instead of 0 queries were executed.
Queries:
SHOW FULL FIELDS FROM `topics`.
Expected: 0
  Actual: 1

bin/test test/cases/associations/cascaded_eager_loading_test.rb:119

Finished in 6.894609s, 0.5802 runs/s, 1.0153 assertions/s.
4 runs, 7 assertions, 1 failures, 0 errors, 0 skips
$
```
2019-03-26 02:51:03 +00:00
Bogdan Gusiev
2847653869 Fix preloader to never reset associations in case they are already loaded
This patch fixes the issue when association is preloaded with a custom
preload scope which disposes the already preloaded target of the
association by reseting it.

When custom preload scope is used, the preloading is now performed into
a separated Hash - #records_by_owner instead of the association.
It removes the necessaty the reset the association after the preloading
is complete so that reset of the preloaded association never happens.

Preloading is still happening to the association when the preload scope
is empty.
2019-03-07 16:48:46 +02:00
Yasuo Honda
d7b6139840 Remove unnecessary current_adapter?(:OracleAdapter) for index length
Follow up #35455, there are two more test cases unnecessary `if current_adapter?(:OracleAdapter)`

```ruby
$ ARCONN=oracle bin/test test/cases/associations/eager_test.rb -n test_include_has_many_using_primary_key
Using oracle
Run options: -n test_include_has_many_using_primary_key --seed 62842

.

Finished in 50.280024s, 0.0199 runs/s, 0.0398 assertions/s.
1 runs, 2 assertions, 0 failures, 0 errors, 0 skips
$
```

```
$ ARCONN=oracle bin/test test/cases/migration/index_test.rb -n test_add_index
Using oracle
Run options: -n test_add_index --seed 52034

.

Finished in 13.152620s, 0.0760 runs/s, 0.0000 assertions/s.
1 runs, 0 assertions, 0 failures, 0 errors, 0 skips
$
```
2019-03-03 13:48:23 +00:00
Rafael Mendonça França
1059dede4c
Fix test that was broken by adding a default scope to an existing model 2019-02-26 21:58:42 -05:00
Rafael Mendonça França
572dcdd7e8
Fix preload with nested associations
When the middle association doesn't have any records and the inner
association is not an empty scope the owner will be `nil` so we can't
try to reset the inverse association.
2019-02-26 18:07:13 -05:00
Ryuta Kamizono
df2ebf9b59
Merge pull request #35247 from bogdan/fix-source-reflection-reset-code
Fix reset of the source association when through association is loaded
2019-02-20 21:24:38 +09:00
Bogdan Gusiev
bd4eff2f99 Fix reset of the source association when through association is loaded
The special case happens when through association has a custom scope
that is applied to the source association when loading.
In this case, the soucre association would need to be reset after
main association is loaded. See tests.

The special case exists when a through association has
2019-02-20 13:48:51 +02:00
Ryuta Kamizono
49bcb008cb Fix eager loading polymorphic association with mixed table conditions
This fixes a bug that the `foreign_key` and the `foreign_type` are
separated as different table conditions if a polymorphic association has
a scope that joins another tables.
2019-02-18 00:41:43 +09:00
Ryuta Kamizono
47e3bbeb90 Revert "Merge pull request #35127 from bogdan/counter-cache-loading"
This reverts commit eec3e28a1abf75676dcee58308ee5721bb53c325, reversing
changes made to 5588fb4802328a2183f4a55c36d6703ee435f85c.

Reason: Marking as loaded without actual loading is too greedy optimization.

See more context #35239.

Closes #35239.

[Edouard CHIN & Ryuta Kamizono]
2019-02-13 22:45:35 +09:00
Ryuta Kamizono
0ee96d13de Fix pluck and select with custom attributes
Currently custom attributes are always qualified by the table name in
the generated SQL wrongly even if the table doesn't have the named
column, it would cause an invalid SQL error.

Custom attributes should only be qualified if the table has the same
named column.
2019-02-13 02:47:46 +09:00
Ryuta Kamizono
5e493c3b83 Fix random CI failure due to non-deterministic sorting order
https://travis-ci.org/rails/rails/jobs/491045821#L1528-L1531
2019-02-10 09:04:10 +09:00