Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for multilevel many-to-many counters #367

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion lib/counter_culture/counter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,11 @@ def foreign_key_value(obj, relation, was = false)
value = value.send(relation.shift)
end

return value.try(relation_primary_key(original_relation, source: obj, was: was).try(:to_sym))
if value.is_a?(ActiveRecord::Associations::CollectionProxy)
value.map { |v| v.try(relation_primary_key(original_relation, source: obj, was: was).try(:to_sym)) }
else
value.try(relation_primary_key(original_relation, source: obj, was: was).try(:to_sym))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we pull this out into a helper method like

def relation_primary_key_from_model(model, original_relation, obj, was)
  model.try(relation_primary_key(original_relation, source: obj, was: was).try(:to_sym)
end

Not sure if that's worth it, but it makes it clearer that the same thing is happening in both cases, just mapping in one.

end
end

# gets the reflect object on the given relation
Expand Down
43 changes: 43 additions & 0 deletions spec/counter_culture_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,11 @@
require 'models/group'
require 'models/sub_group'
require 'models/group_item'
require 'models/many_to_many/article'
require 'models/many_to_many/author'
require 'models/many_to_many/reader'
require 'models/many_to_many/authors_article'
require 'models/many_to_many/readers_article'

if ENV['DB'] == 'postgresql'
require 'models/purchase_order'
Expand Down Expand Up @@ -2766,4 +2771,42 @@ def mess_up_counts
po.reload
expect(po.total_amount).to eq(0.0)
end

describe "many to many association counter caches" do
let!(:reader) { Reader.create! }
let!(:author) { Author.create! }
let(:article) { Article.new(readers: [reader], authors: [author]) }

context "when the relation is a single-level one" do
context "when creating a new record" do
it "updates the counter cache" do
expect { article.save }.to change { author.reload.articles_count }.by(1)
end
end

context "when deleting an existing record" do
before { article.save }

it "updates the counter cache" do
expect { article.destroy }.to change { author.reload.articles_count }.by(-1)
end
end
end

context "when the relation is a multi-level one" do
context "when creating a new record" do
it "updates the counter cache" do
expect { article.save }.to change { author.reload.readers_count }.by(1)
end
end

context "when deleting an existing record" do
before { article.save }

it "updates the counter cache" do
expect { article.destroy }.to change { author.reload.readers_count }.by(-1)
end
end
end
end
end
7 changes: 7 additions & 0 deletions spec/models/many_to_many/article.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
class Article < ActiveRecord::Base
has_many :readers_articles, dependent: :destroy
has_many :authors_articles, dependent: :destroy

has_many :readers, through: :readers_articles, dependent: :destroy
has_many :authors, through: :authors_articles, dependent: :destroy
end
4 changes: 4 additions & 0 deletions spec/models/many_to_many/author.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
class Author < ActiveRecord::Base
has_many :authors_articles, dependent: :destroy
has_many :articles, through: :authors_articles, dependent: :destroy
end
6 changes: 6 additions & 0 deletions spec/models/many_to_many/authors_article.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
class AuthorsArticle < ActiveRecord::Base
belongs_to :author
belongs_to :article

counter_culture :author, column_name: :articles_count
end
4 changes: 4 additions & 0 deletions spec/models/many_to_many/reader.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
class Reader < ActiveRecord::Base
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be reading this wrong, but it looks like the Reader class isn't actually required for the test scenario here, is it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, the problem with this scenario cannot be solved so easily, and the implementation is too naive. In the scenario, we have a ReadersArticle that updates the readers_count counter. Everything works fine until we delete the associated AuthorsArticle, which leads to the counter becoming out of sync.

Therefore, I don't see much benefit in this functionality. Unless leaving the technical possibility of specifying a multi-level path for the counter, and letting the implementation be up to a user, as it should be quite complex.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we close this then?

has_many :readers_articles, dependent: :destroy
has_many :articles, through: :readers_articles, dependent: :destroy
end
6 changes: 6 additions & 0 deletions spec/models/many_to_many/readers_article.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
class ReadersArticle < ActiveRecord::Base
belongs_to :reader
belongs_to :article

counter_culture [:article, :authors], column_name: :readers_count
end
25 changes: 25 additions & 0 deletions spec/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,31 @@
t.string "sub_group_uuid", :null => false
end

create_table "readers", :force => true do |t|
t.string "name"
end

create_table "articles", :force => true do |t|
t.string "title"
end

create_table "authors", :force => true do |t|
t.string "name"

t.integer "articles_count", default: 0, null: false
t.integer "readers_count", default: 0, null: false
end

create_table "readers_articles", :force => true do |t|
t.integer "reader_id", null: false
t.integer "article_id", null: false
end

create_table "authors_articles", :force => true do |t|
t.integer "author_id", null: false
t.integer "article_id", null: false
end

if ENV['DB'] == 'postgresql' && Gem::Version.new(Rails.version) >= Gem::Version.new('5.0')
create_table :purchase_orders, :force => true do |t|
t.money "total_amount", scale: 2, default: "0.0", null: false
Expand Down