aboutsummaryrefslogtreecommitdiffstats
path: root/activerecord/lib/active_record/associations/has_many_through_association.rb
Commit message (Collapse)AuthorAgeFilesLines
* Merge pull request #35799 from leboshi/masterRyuta Kamizono2019-03-311-11/+4
|\ | | | | | | Fix callbacks on has_many :through associations
| * Fix callbacks on has_many :through associations (#33249)Ryan Kerr2019-03-301-10/+4
|/ | | | | | | | | | | 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.
* Ensure that `delete_all` on collection proxy returns affected countRyuta Kamizono2018-12-041-0/+2
| | | | | | | Unlike the `Relation#delete_all`, `delete_all` on collection proxy doesn't return affected count. Since the `CollectionProxy` is a subclass of the `Relation`, this inconsistency is probably not intended, so it should return the count consistently.
* Optimize difference and intersectionFlorian Ebeling2018-11-061-17/+9
|
* Rename union to intersectionFlorian Ebeling2018-11-061-1/+1
|
* Fix handling of duplicates for `replace` on has_many-throughFlorian Ebeling2018-11-061-0/+30
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | There was a bug in the handling of duplicates when assigning (replacing) associated records, which made the result dependent on whether a given record was associated already before being assigned anew. E.g. post.people = [person, person] post.people.count # => 2 while post.people = [person] post.people = [person, person] post.people.count # => 1 This change adds a test to provoke the former incorrect behavior, and fixes it. Cause of the bug was the handling of record collections as sets, and using `-` (difference) and `&` (union) operations on them indiscriminately. This temporary conversion to sets would eliminate duplicates. The fix is to decorate record collections for these operations, and only for the `has_many :through` case. It is done by counting occurrences, and use the record together with the occurrence number as element, in order to make them work well in sets. Given a, b = *Person.all then the collection used for finding the difference or union of records would be internally changed from [a, b, a] to [[a, 1], [b, 1], [a, 2]] for these operations. So a first occurrence and a second occurrence would be distinguishable, which is all that is necessary for this task. Fixes #33942.
* Don't expose internal methods in the associationsRyuta Kamizono2018-10-081-14/+14
|
* Initialization block is a part of `build_record`Ryuta Kamizono2018-06-041-1/+1
| | | | Should be done before `before_add` callbacks.
* Fix building has_one through recordRyuta Kamizono2018-01-231-8/+1
| | | | Fixes #31762.
* Don't update counter cache when through record was not destroyedEugene Kenny2018-01-141-1/+1
| | | | | | When removing a record from a has many through association, the counter cache was being updated even if the through record halted the callback chain and prevented itself from being destroyed.
* Merge pull request #23146 from piotrj/issue_18424Ryuta Kamizono2018-01-111-0/+1
|\ | | | | | | When deleting through records, take into account association conditions
| * When deleting through records, take into account association conditionsPiotr Jakubowski2016-05-041-8/+9
| | | | | | | | | | | | | | | | Fixes #18424. When deleting through records, it didn't take into account the conditions that may have been affecting join model table, but was defined in association definition.
* | Simply use `scope.delete_all` instead of constructing delete managerRyuta Kamizono2018-01-071-8/+1
| |
* | Fix broken doc for Active Record [ci skip]Yoshiyuki Hirano2017-08-251-1/+1
| |
* | Refactor Active Record to let Arel manage bind paramsSean Griffin2017-07-241-1/+1
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | 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.
* | Use frozen-string-literal in ActiveRecordKir Shatrov2017-07-191-0/+2
| |
* | Revert "Merge pull request #29540 from kirs/rubocop-frozen-string"Matthew Draper2017-07-021-1/+0
| | | | | | | | | | This reverts commit 3420a14590c0e6915d8b6c242887f74adb4120f9, reversing changes made to afb66a5a598ce4ac74ad84b125a5abf046dcf5aa.
* | Enforce frozen string in RubocopKir Shatrov2017-07-011-0/+1
| |
* | ActiveRecord: do not create "has many through" records that have been removedTobias Kraze2017-06-281-0/+5
| | | | | | | | | | If a record was built on a HasManyThroughAssociation, then removed, and then the record was saved, the removed record would be created anyways.
* | Prevent double firing the before save callback of new object when the parent ↵Ryuta Kamizono2017-04-211-5/+1
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | association saved in the callback Related #18155, #26661, 268a5bb, #27434, #27442, and #28599. Originally #18155 was introduced for preventing double insertion caused by the after save callback. But it was caused the before save issue (#26661). 268a5bb fixed #26661, but it was caused the performance regression (#27434). #27442 added new record to `target` before calling callbacks for fixing #27434. But it was caused double firing before save callback (#28599). We cannot add new object to `target` before saving the object. This is improving #18155 to only track callbacks after `save`. Fixes #28599.
* | Should not update children when the parent creation with no reasonRyuta Kamizono2016-12-291-4/+6
| | | | | | | | | | | | | | | | This issue was introduced with d849f42 to solve #19782. However, we can solve #19782 without causing the issue. It is enough to save only when necessary. Fixes #27338.
* | Add a record to target before any callbacks loads the recordRyuta Kamizono2016-12-231-4/+0
| | | | | | | | | | | | | | | | | | `append_record` was added at 15ddd51 for not double adding the record. But adding `append_record` (checking `@target.include?(record)`) caused performance regression #27434. Instead of checking not double adding the record, add a record to target before any callbacks loads the record. Fixes #27434.
* | Merge PR #19759Arthur Neves2016-10-281-1/+4
|\ \ | | | | | | | | | Fix for has_and_belongs_to_many & has_many_through associations
| * | Fix for has_and_belongs_to_many & has_many_through associations while ↵Mehmet Emin İNAÇ2016-02-131-1/+4
| |/ | | | | | | | | | | | | | | partial_writes is false This will fix #19663 Also with this fix, active record does not fire unnecassary update queries while partial_writes is true
* | Don't skip in-memory insertion of associations when loaded in validateSean Griffin2016-09-291-0/+4
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | This was caused by 6d0d83a33f59d9415685852cf77818c41e2e2700. While the bug it's trying to fix is handled if the association is loaded in an after_(create|save) callback, it doesn't handle any cases that load the association before the persistence takes place (validation, or before_* filters). Instead of caring about the timing of persistence, we can just ensure that we're not double adding the record instead. The test from that commit actually broke, but it was not because the bug has been re-introduced. It was because `Bulb` in our test suite is doing funky things that look like STI but isn't STI, so equality comparison didn't happen as the loaded model was of a different class. Fixes #26661.
* | modernizes hash syntax in activerecordXavier Noria2016-08-061-2/+2
| |
* | applies new string literal convention in activerecord/libXavier Noria2016-08-061-1/+1
| | | | | | | | | | The current code base is not uniform. After some discussion, we have chosen to go with double quotes by default.
* | Avoid duplicated `set_inverse_instance` for target scopeRyuta Kamizono2016-08-031-1/+1
|/ | | | | | | | | Because `scope` (`target_scope`) is a `AssociationRelation`. `AssociationRelation` handles `set_inverse_instance`. https://github.com/rails/rails/blob/v5.0.0/activerecord/lib/active_record/association_relation.rb#L31-L33 See also #26022.
* Add missing source_type if provided on hmt which belongs to an sti recordVipul A M2016-01-241-0/+5
| | | | Fixes #23209
* HasManyAssociation: moved half of counter cache code to reflectionBogdan Gusiev2015-09-031-1/+1
| | | | | | | | | | | | | | | | | | Current implementation has a lot of utility methods that accept reflection call a lot of methods on it and exit. E.g. has_counter_cache?(reflection) It causes confusion and inability to cache result of the method even through it always returns the same result for the same reflection object. It can be done easier without access to the association context by moving code into reflection itself. e.g. reflection.has_counter_cache? Reflection is less complex object than association so moving code there automatically makes it simplier to understand.
* Revert "Revert "Reduce allocations when running AR callbacks.""Guo Xiang Tan2015-07-161-1/+1
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | This reverts commit bdc1d329d4eea823d07cf010064bd19c07099ff3. Before: Calculating ------------------------------------- 22.000 i/100ms ------------------------------------------------- 229.700 (± 0.4%) i/s - 1.166k Total Allocated Object: 9939 After: Calculating ------------------------------------- 24.000 i/100ms ------------------------------------------------- 246.443 (± 0.8%) i/s - 1.248k Total Allocated Object: 7939 ``` begin require 'bundler/inline' rescue LoadError => e $stderr.puts 'Bundler version 1.10 or later is required. Please update your Bundler' raise e end gemfile(true) do source 'https://rubygems.org' # gem 'rails', github: 'rails/rails', ref: 'bdc1d329d4eea823d07cf010064bd19c07099ff3' gem 'rails', github: 'rails/rails', ref: 'd2876141d08341ec67cf6a11a073d1acfb920de7' gem 'arel', github: 'rails/arel' gem 'sqlite3' gem 'benchmark-ips' end require 'active_record' require 'benchmark/ips' ActiveRecord::Base.establish_connection('sqlite3::memory:') ActiveRecord::Migration.verbose = false ActiveRecord::Schema.define do create_table :users, force: true do |t| t.string :name, :email t.boolean :admin t.timestamps null: false end end class User < ActiveRecord::Base default_scope { where(admin: true) } end admin = true 1000.times do attributes = { name: "Lorem ipsum dolor sit amet, consectetur adipiscing elit.", email: "foobar@email.com", admin: admin } User.create!(attributes) admin = !admin end GC.disable Benchmark.ips(5, 3) do |x| x.report { User.all.to_a } end key = if RUBY_VERSION < '2.2' :total_allocated_object else :total_allocated_objects end before = GC.stat[key] User.all.to_a after = GC.stat[key] puts "Total Allocated Object: #{after - before}" ```
* Autosave existing records on HMT associations when the parent is newSean Griffin2015-04-181-6/+4
| | | | | | | | | | | | | | | To me it seems like this should only be the case if `autosave: true` is set on the association. However, when implemented that way, it caused issues with has many associations, where we have explicit tests stating that child records are updated when the parent is new, even if autosave is not set (presumably to update the parent id, but other changed attributes would be persisted as well). It's quirky, but at least we should be consistently quirky. This constitutes a minor but subtle change in behavior, and therefore should not be backported to 4.2 and earlier. Fixes #19782
* Revert "Reduce allocations when running AR callbacks."Guo Xiang Tan2015-03-221-1/+1
| | | | This reverts commit 796cab45561fce268aa74e6587cdb9cae3bb243e.
* Correct errors in counter cache updatingSean Griffin2015-02-031-2/+2
| | | | | | | | | | | The cache name should be converted to a string when given, not compared as a symbol. This edge case is already adequately covered by our tests, but was masked by another issue where we were incorrectly updating the counter cache twice. When paired with a bug where we didn't update the counter cache because we couldn't find a match with the name, this made it look like everything was working fine. Fixes #10865.
* Remove Relation#bind_paramsSean Griffin2015-01-271-1/+1
| | | | | | | | `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.
* Remove unneeded requiresRafael Mendonça França2015-01-041-2/+0
| | | | These requires were added only to change deprecation message
* Remove deprecated automatic counter caches on `has_many :through`Rafael Mendonça França2015-01-041-14/+0
|
* Remove unneeded special case to calculate size for has_many :throughBogdan Gusiev2014-12-231-15/+0
| | | | | All cases are properly handled in CollectionAssociation for all subclasses of this association
* Update Arel usage for rails/arel#98fc259Sean Griffin2014-11-291-1/+1
| | | | | `where_sql` now requires that we pass it an engine. None of the manager classes take an engine in their constructor.
* Pass symbol as an argument instead of a blockErik Michaels-Ober2014-11-291-3/+1
|
* Improve the performance of reading attributesSean Griffin2014-11-181-1/+1
| | | | | | | We added a comparison to "id", and call to `self.class.primary_key` a *lot*. We also have performance hits from `&block` all over the place. We skip the check in a new method, in order to avoid breaking the behavior of `read_attribute`
* Avoid unnecessary allocations/callsPablo Herrero2014-11-021-1/+1
|
* edit pass over all warningsXavier Noria2014-10-281-3/+3
| | | | | | | | | | | | | | | This patch uniformizes warning messages. I used the most common style already present in the code base: * Capitalize the first word. * End the message with a full stop. * "Rails 5" instead of "Rails 5.0". * Backticks for method names and inline code. Also, converted a few long strings into the new heredoc convention.
* let's warn with heredocsXavier Noria2014-10-281-5/+8
| | | | | | | | | | | | The current style for warning messages without newlines uses concatenation of string literals with manual trailing spaces where needed. Heredocs have better readability, and with `squish` we can still produce a single line. This is a similar use case to the one that motivated defining `strip_heredoc`, heredocs are super clean.
* Prefix internal method with _Rafael Mendonça França2014-10-251-1/+1
| | | | This will avoid naming clash with user defined methods
* Reduce allocations when running AR callbacks.Pete Higgins2014-09-281-1/+1
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Inspired by @tenderlove's work in c363fff29f060e6a2effe1e4bb2c4dd4cd805d6e, this reduces the number of strings allocated when running callbacks for ActiveRecord instances. I measured that using this script: ``` require 'objspace' require 'active_record' require 'allocation_tracer' ActiveRecord::Base.establish_connection adapter: "sqlite3", database: ":memory:" ActiveRecord::Base.connection.instance_eval do create_table(:articles) { |t| t.string :name } end class Article < ActiveRecord::Base; end a = Article.create name: "foo" a = Article.find a.id N = 10 result = ObjectSpace::AllocationTracer.trace do N.times { Article.find a.id } end result.sort.each do |k,v| p k => v end puts "total: #{result.values.map(&:first).inject(:+)}" ``` When I run this against master and this branch I get this output: ``` pete@balloon:~/projects/rails/activerecord$ git checkout master M Gemfile Switched to branch 'master' pete@balloon:~/projects/rails/activerecord$ bundle exec ruby benchmark_allocation_with_callback_send.rb > allocations_before pete@balloon:~/projects/rails/activerecord$ git checkout remove-dynamic-send-on-built-in-callbacks M Gemfile Switched to branch 'remove-dynamic-send-on-built-in-callbacks' pete@balloon:~/projects/rails/activerecord$ bundle exec ruby benchmark_allocation_with_callback_send.rb > allocations_after pete@balloon:~/projects/rails/activerecord$ diff allocations_before allocations_after 39d38 < {["/home/pete/projects/rails/activesupport/lib/active_support/callbacks.rb", 81]=>[40, 0, 0, 0, 0, 0]} 42c41 < total: 630 --- > total: 590 ``` In addition to this, there are two micro-optimizations present: * Using `block.call if block` vs `yield if block_given?` when the block was being captured already. ``` pete@balloon:~/projects$ cat benchmark_block_call_vs_yield.rb require 'benchmark/ips' def block_capture_with_yield &block yield if block_given? end def block_capture_with_call &block block.call if block end def no_block_capture yield if block_given? end Benchmark.ips do |b| b.report("block_capture_with_yield") { block_capture_with_yield } b.report("block_capture_with_call") { block_capture_with_call } b.report("no_block_capture") { no_block_capture } end pete@balloon:~/projects$ ruby benchmark_block_call_vs_yield.rb Calculating ------------------------------------- block_capture_with_yield 124979 i/100ms block_capture_with_call 138340 i/100ms no_block_capture 136827 i/100ms ------------------------------------------------- block_capture_with_yield 5703108.9 (±2.4%) i/s - 28495212 in 4.999368s block_capture_with_call 6840730.5 (±3.6%) i/s - 34169980 in 5.002649s no_block_capture 5821141.4 (±2.8%) i/s - 29144151 in 5.010580s ``` * Defining and calling methods instead of using send. ``` pete@balloon:~/projects$ cat benchmark_method_call_vs_send.rb require 'benchmark/ips' class Foo def tacos nil end end my_foo = Foo.new Benchmark.ips do |b| b.report('send') { my_foo.send('tacos') } b.report('call') { my_foo.tacos } end pete@balloon:~/projects$ ruby benchmark_method_call_vs_send.rb Calculating ------------------------------------- send 97736 i/100ms call 151142 i/100ms ------------------------------------------------- send 2683730.3 (±2.8%) i/s - 13487568 in 5.029763s call 8005963.9 (±2.7%) i/s - 40052630 in 5.006604s ``` The result of this is making typical ActiveRecord operations slightly faster: https://gist.github.com/phiggins/e46e51dcc7edb45b5f98
* Avoid using heredoc for user warningsGodfrey Chan2014-08-281-6/+6
| | | | | | | | | | Using heredoc would enforce line wrapping to whatever column width we decided to use in the code, making it difficult for the users to read on some consoles. This does make the source code read slightly worse and a bit more error-prone, but this seems like a fair price to pay since the primary purpose for these messages are for the users to read and the code will not stick around for too long.
* remove blank lines in the start of the ActiveRecord filesPonomarev Nikolay2014-07-291-1/+0
|
* Deprecate automatic counter caches on has_many :throughSean Griffin2014-06-261-0/+14
| | | | | | | | | | | Reliant on https://github.com/rails/rails/pull/15747 but pulled to a separate PR to reduce noise. `has_many :through` associations have the undocumented behavior of automatically detecting counter caches. However, the way in which it does so is inconsistent with counter caches everywhere else, and doesn't actually work consistently. As with normal `has_many` associations, the user should specify the counter cache on the `belongs_to`, if they'd like it updated.
* Merge pull request #15747 from sgrif/sg-trolololol-this-is-so-brokenRafael Mendonça França2014-06-191-1/+0
|\ | | | | Always update counter caches in memory when adding records
generated by cgit v1.2.3 (git 2.25.1) at 2024-06-28 15:34:35 +0000