| Commit message (Collapse) | Author | Age | Files | Lines |
|
|
|
|
|
|
| |
`joins_values.partition` will break joins values order. It should be
kept as user intended order.
Fixes #15488.
|
|
|
|
|
|
|
| |
It is only used `primary_key` and `connection` in the internal, so it is
not needed to delegate others to `klass` explicitly.
This doesn't change public behavior because `relation` will delegate
missing method to `klass`.
|
|
|
|
|
|
|
|
|
| |
Currently `Relation#merge` will almost fill `values` with empty values
(e.g. `other.order_values` is always true, it should be
`other.order_values.any?`). This means that `Relation#merge` always
changes `values` even if actually `values` is nothing changed. This
behavior will makes `Relation#empty_scope?` fragile. So `Relation#merge`
should avoid unnecessary changes.
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
A common source of bugs and code bloat within Active Record has been the
need for us to maintain the list of bind values separately from the AST
they're associated with. This makes any sort of AST manipulation
incredibly difficult, as any time we want to potentially insert or
remove an AST node, we need to traverse the entire tree to find where
the associated bind parameters are.
With this change, the bind parameters now live on the AST directly.
Active Record does not need to know or care about them until the final
AST traversal for SQL construction. Rather than returning just the SQL,
the Arel collector will now return both the SQL and the bind parameters.
At this point the connection adapter will have all the values that it
had before.
A bit of this code is janky and something I'd like to refactor later. In
particular, I don't like how we're handling associations in the
predicate builder, the special casing of `StatementCache::Substitute` in
`QueryAttribute`, or generally how we're handling bind value replacement
in the statement cache when prepared statements are disabled.
This also mostly reverts #26378, as it moved all the code into a
location that I wanted to delete.
/cc @metaskills @yahonda, this change will affect the adapters
Fixes #29766.
Fixes #29804.
Fixes #26541.
Close #28539.
Close #24769.
Close #26468.
Close #26202.
There are probably other issues/PRs that can be closed because of this
commit, but that's all I could find on the first few pages.
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
I investigated where `scope_for_create` is reused in tests with the
following code:
```diff
--- a/activerecord/lib/active_record/relation.rb
+++ b/activerecord/lib/active_record/relation.rb
@@ -590,6 +590,10 @@ def where_values_hash(relation_table_name = table_name)
end
def scope_for_create
+ if defined?(@scope_for_create) && @scope_for_create
+ puts caller
+ puts "defined"
+ end
@scope_for_create ||= where_values_hash.merge!(create_with_value.stringify_keys)
end
```
It was hit only `test_scope_for_create_is_cached`. This means that
`scope_for_create` will not be reused in normal use cases. So we can
remove caching `scope_for_create` to respect changing `where_clause` and
`create_with_value`.
|
|
|
|
|
|
|
| |
This is related with #27680.
Since `where_values_hash` keys constructed by `where` are string, so we
need `stringify_keys` to `create_with_value` before merging it.
|
|
|
|
|
| |
Because Arel is a private API and to describe `where_values_hash` keys
constructed by `where` are string.
|
|
|
|
|
| |
`FakeKlass` in `relation_test.rb` and `relation/mutation_test.rb` are
almost the same.
|
|
|
|
|
| |
This reverts commit 3420a14590c0e6915d8b6c242887f74adb4120f9, reversing
changes made to afb66a5a598ce4ac74ad84b125a5abf046dcf5aa.
|
| |
|
|
|
|
|
|
|
|
| |
This fixes the following warnings:
```
activerecord/test/cases/relation_test.rb:231: warning: assigned but unused variable - authors_with_commented_posts
```
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Doing `Author.joins(:posts).merge(Post.joins(:comments))` does this
`SELECT ... INNER JOIN posts ON... LEFT OUTER JOIN comments ON...`
instead of doing
`SELECT ... INNER JOIN posts ON... INNER JOIN comments ON...`.
This behavior is unexpected and makes little sense as, basically, doing
`Post.joins(:comments)` means I want posts that have comments. Turning
it to a LEFT JOIN means I want posts and join the comments data, if
any.
We can see this problem directly in the existing tests.
The test_relation_merging_with_merged_joins_as_symbols only does joins
from posts to comments to ratings while the ratings fixture isn't
loaded, but the count is non-zero.
|
|
|
|
|
| |
This change reverted in eac6f369 but it is needed for data integrity.
See #25328.
|
|
|
|
|
|
|
|
|
| |
mtsmfm/disable-referential-integrity-without-superuser-privilege-take-2"
This reverts commit c1faca6333abe4b938b98fedc8d1f47b88209ecf, reversing
changes made to 8c658a0ecc7f2b5fc015d424baf9edf6f3eb2b0b.
See https://github.com/rails/rails/pull/27636#issuecomment-297534129
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
privileges (take 2)
Re-create https://github.com/rails/rails/pull/21233
eeac6151a5 was reverted (127509c071b4) because it breaks tests.
----------------
ref: 72c1557254
- We must use `authors` fixture with `author_addresses` because of its foreign key constraint.
- Tests require PostgreSQL >= 9.4.2 because it had a bug about `ALTER CONSTRAINTS` and fixed in 9.4.2.
|
|
|
|
| |
because Struct.new returns a Class, we just can give it a name and use it directly without inheriting from it
|
|
|
|
|
|
|
|
|
|
| |
mtsmfm/disable-referential-integrity-without-superuser-privileges"
This reverts commit eeac6151a55cb7d5f799e1ae33aa64a839cbc3aa, reversing
changes made to 5c40239d3104543e70508360d27584a3e4dc5baf.
Reason: Broke the isolated tests.
https://travis-ci.org/rails/rails/builds/188721346
|
|
|
|
|
|
|
|
|
| |
privileges
ref: 72c1557254
- We must use `authors` fixture with `author_addresses` because of its foreign key constraint.
- Tests require PostgreSQL >= 9.4.2 because it had a bug about `ALTER CONSTRAINTS` and fixed in 9.4.2.
|
| |
|
|
|
|
|
|
| |
All indentation was normalized by rubocop auto-correct at 80e66cc4d90bf8c15d1a5f6e3152e90147f00772.
But heredocs was still kept absolute position. This commit aligns
heredocs indentation for consistency.
|
|\
| |
| | |
Remove over meta programming in AR::Relation
|
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
Introduced low level methods #set_value and #get_value for setting query attributes:
relation.set_value(:where, {id: 1})
relation.get_value(:includes)
Used those internally when working with relation's attributes
at the abstract level
|
|/
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
The current behaviour of checking if there is a LEFT OUTER JOIN arel
node to detect if we are doing eager_loading is wrong. This problem
wasn't frequent before as only some pretty specific cases would add
a LEFT OUTER JOIN arel node. However, the recent new feature
left_outer_joins also add this node and made this problem happen
frequently.
Since in the perform_calculation function, we don't have access to
eager_loading information, I had to extract the logic for the distinct
out to the calculate method.
As I was in the file for left_outer_join tests, I fixed a few that had
bugs and I replaced some that were really weak with something that
will catch more issues.
In relation tests, the first test I changed would have failed if it
had validated the hash returned by count instead of just checking how
many pairs were in it. This is because this merge of join currently
transforms the join node into an outer join node, which then made
count do a distinct. So before this change, the return was
{1=>1, 4=>1, 5=>1}.
|
|
|
|
|
|
|
|
| |
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.
|
| |
|
|
|
|
|
| |
The `Arel::Nodes::Quoted` was removed already.
Follow up to f916aa247bddba0c58c50822886bc29e8556df76.
|
|
|
|
|
|
| |
In b71e08f we started raising when nil or false was passed to merge to
fix #12264, however we should also do this for truthy values that are
invalid like true.
|
|
|
|
|
|
|
|
|
|
| |
From Ruby ( 2.3.0dev trunk 52520), `Hash#to_proc` is defined
(https://github.com/ruby/ruby/commit/fbe967ec02cb65a7efa3fb8f3d747cf6f620dde1),
and many tests have been failed with
`ArgumentError: wrong number of arguments (given 0, expected 1)`.
Because we call `Hash#to_proc` with no args in `#merge!`.
This commit changes order of conditionals to not call `Hash#to_proc`.
|
|
|
|
| |
This commit follows up of 6a6dbb4c51fb0c58ba1a810eaa552774167b758a.
|
|
|
|
|
|
|
|
| |
This commit follow up of 4d8f62d.
The difference from 4d8f62d are below:
* Change `WhereClauseFactory` to accept `Arel::Nodes::Node`
* Change test cases of `relation_test.rb`
|
|
|
|
| |
See 7dcfc25e7c52682a4343c2ba7188a69e7c06c936 for more details
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Our general contract in Active Record is that strings are assumed to be
SQL literals, and symbols are assumed to reference a column. If a from
clause is given, we shouldn't include the table name, but we should
still quote the value as if it were a column.
Upon fixing this, the tests were still failing on SQLite. This was
because the column name being returned by the query was `"\"join\""`
instead of `"join"`. This is actually a bug in SQLite that was fixed a
long time ago, but I was using the version of SQLite included by OS X
which has this bug. Since I'm guessing this will be a common case for
contributors, I also added an explicit check with a more helpful error
message.
Fixes #20360
|
| |
|
| |
|
|
|
|
|
|
| |
nil or false should not be valid argument to the merge method.
Closes #12264
|
|
|
|
|
|
|
|
|
|
|
| |
The only place it was accessed was in tests. Many of them have another
way that they can test their behavior, that doesn't involve reaching
into internals as far as they did. `AssociationScopeTest` is testing a
situation where the where clause would have one bind param per
predicate, so it can just ignore the predicates entirely. The where
chain test was primarly duplicating the logic tested on `WhereClause`
directly, so I instead just make sure it calls the appropriate method
which is fully tested in isolation.
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
The way that bind values are currently stored on Relation is a mess.
They can come from `having`, `where`, or `join`. I'm almost certain that
`having` is actually broken, and calling `where` followed by `having`
followed by `where` will completely scramble the binds.
Joins don't actually add the bind parameters to the relation itself, but
instead add it onto an accessor on the arel AST which is undocumented,
and unused in Arel itself. This means that the bind values must always
be accessed as `relation.arel.bind_values + relation.bind_values`.
Anything that doesn't is likely broken (and tons of bugs have come up
for exactly that reason)
The result is that everything dealing with `Relation` instances has to
know far too much about the internals. The binds are split, combined,
and re-stored in non-obvious ways that makes it difficult to change
anything about the internal representation of `bind_values`, and is
extremely prone to bugs.
So the goal is to move a lot of logic off of `Relation`, and into
separate objects. This is not the same as what is currently done with
`JoinDependency`, as `Relation` knows far too much about its internals,
and vice versa. Instead these objects need to be black boxes that can
have their implementations swapped easily.
The end result will be two classes, `WhereClause` and `JoinClause`
(`having` will just re-use `WhereClause`), and there will be a single
method to access the bind values of a `Relation` which will be
implemented as
```
join_clause.binds + where_clause.binds + having_clause.binds
```
This is the first step towards that refactoring, with the internal
representation of where changed, and an intermediate representation of
`where_values` and `bind_values` to let the refactoring take small
steps. These will be removed shortly.
|
|
|
|
|
|
|
|
|
|
| |
This is no longer required now that we are injecting a type caster
object into the Arel table, with the exception of uniqueness
validations. Since it calls `ConnectionAdapter#type_cast`, the value has
already been cast for the database. We don't want Arel to attempt to
cast it further, so we need to continue wrapping it in a quoted node.
This can potentially go away when this validator is refactored to make
better use of `where` or the predicate builder.
|
|
|
|
|
|
|
| |
Part of the larger refactoring to remove type casting from Arel. We can
inform it that we already have the right type by wrapping the value in
an `Arel::Nodes::Quoted`. This commit can be reverted when we have
removed type casting from Arel in Rail 5.1
|
|
|
|
|
|
|
| |
Construction of relations can be a hotspot, we don't want to create one
of these in the constructor. This also allows us to do more expensive
things in the predicate builder's constructor, since it's created once
per AR::Base subclass
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
We have several test cases on "tricky" types that are essentially
testing that `update_all` goes through the same type casting behavior as
a normal assignment + save. We recently had another case to add this
test for another type in https://github.com/rails/rails/pull/12742.
Rather than testing this separately for every type which is "tricky"
when round tripping, let's instead have a fairly exhaustive test that
ensures we're getting the correct values at every step for `update_all`.
Given the structure of the code now, we can be confident that if the
type is correct, and `update_all` is type casting correctly, we're going
to get the right behavior for all types.
|
|\
| |
| |
| |
| |
| |
| | |
Fix insertion of records for hmt association with scope
Conflicts:
activerecord/CHANGELOG.md
|
| | |
|
| | |
|