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

Counter on polymorphic relationship #4

Closed
jonas-jasas opened this issue Sep 13, 2012 · 25 comments
Closed

Counter on polymorphic relationship #4

jonas-jasas opened this issue Sep 13, 2012 · 25 comments

Comments

@jonas-jasas
Copy link

How to make counter on polymorphic relationship?

@magnusvk
Copy link
Owner

Mmm, that's a good question. I've never tested that so this might or might not work. Feel free to fork and pull-request any necessary changes to make that work.

@asantoya17
Copy link

can I use this with has_and_belong_to_many?

@magnusvk
Copy link
Owner

Again, I haven't tested that but I don't think so. Frankly, I wouldn't recommend has_and_belongs_to_many. It's usually better to just have a model sit in between. And then that way counter_culture also works fine.

@dorilla
Copy link

dorilla commented Feb 19, 2013

Hey guys, if anyone wants to use counter_culture for polymorphic relationships, check out my fork. I had to fix this for a project. I added some logic that detects if a relationship is polymorphic and traverses the respective models to find the caches.

Check out my fork: https://github.com/dorilla/counter_culture

I've only tested and confirmed the following:

  • Works with single level polymorphic associations
  • counter_culture_fix_counts works

The edits I had to do seems hacky to me, so I do plan of working on it some more when I have the time.

Hope it helps.

@aaronchi
Copy link

worked for me 👍

@magnusvk
Copy link
Owner

@dorilla -- thanks so much for sharing this and great to hear that it works for people. As you'd pointed out, the code is a little hack-ish right now so I wouldn't want to merge it back into the main gem yet. But it's definitely a great place to start.

I've also added a link to this issue in the documentation so people can find your fork more easily.

Thanks again for contributing!

@dopa
Copy link

dopa commented Jun 21, 2013

+1 - Would love to have this as a native feature of counter_culture

@mull
Copy link

mull commented Aug 15, 2013

@magnusvk is there any on-going work regarding this issue?

@magnusvk
Copy link
Owner

No, I'm not working on it and I'm not aware of anybody else. Unfortunately,
I'd love to add this as a feature but I don't need it myself so I'm not
finding the time to work on it.

On Thu, Aug 15, 2013 at 8:52 AM, Emil Ahlbäck [email protected]:

@magnusvk https://github.com/magnusvk is there any on-going work
regarding this issue?


Reply to this email directly or view it on GitHubhttps://github.com//issues/4#issuecomment-22700823
.

@mull
Copy link

mull commented Oct 4, 2013

@magnusvk Thanks for the reply. Could you change the link in the README so that it points to this repo instead of the old one? Will make it easier for people to find this issue.

@magnusvk
Copy link
Owner

magnusvk commented Oct 4, 2013

@mull Done -- thanks for pointing that out.

@dcoder2099
Copy link

@magnusvk As you mentioned 7 months ago, @dorilla's fork is "a little hackish" so you're not inclined to merge that work into your mainline.

Well, I am also in need of this polymorphic capability, but I'm also interested in seeing the work incorporated here, for benefit to all. 😄

Anyway, I'm willing to spend the time to make that work less "hackish" but I'd like to know what you consider less to be.

@magnusvk
Copy link
Owner

magnusvk commented Oct 7, 2013

Well, I'd love to find a solution that is not hackish. :)

Here's a couple of things:

  • solid test suite for polymorphic relationships, including all the different features we support (i.e. multi-level, delta-column, dynamic column, etc. etc)
  • tests where we chain multiple polymorphic relationships, as well as polymorphic with non-polymorphic ones
  • no code duplication; the fork right now copy & pastes a bunch of code, which I wouldn't want to do in master
  • some documentation in the README

This is probably not exhaustive but it's a solid amount of work to start.

I looked into implementing this myself last week, but backed away a bit after seeing the complexity involved, particularly with multi-level support. Of course all this is possible, but it's a major undertaking, and I don't want to destabilize the non-polymorphic support we have right now.

@dopa
Copy link

dopa commented Dec 10, 2013

First I was like, "I have a big need and a perfect use case for polymorphic counter_culture. It would be so nice! Ok, I guess I'll use dorilla's fork."

Then I was like, "Ok, I switched my Gemfile and did bundle update. I have the new fork. Wait, how do I even use the polymorphic capability. It's nowhere in the documentation."

Then I was like, "Let me look at the source code."

def all_polymorphic_types_currently(name)
  @poly_hash ||= {}.tap do |hash|
    Rails.application.eager_load! unless Rails.configuration.cache_classes
    ::ActiveRecord::Base.descendants.each do |reflection|
      klass = (reflection rescue nil)
      next if klass.nil? or !klass.ancestors.include?(::ActiveRecord::Base)
      klass.reflect_on_all_associations(:has_many).select{|r| r.options[:as] }.each do |reflection|
        (hash[reflection.options[:as]] ||= []) << klass
      end
    end
  end
  @poly_hash[name.to_sym]
end

Then I was like, "... ... ... Whoa. Whoa Whoa Whoa. I can't even read this, let alone figure out what it does."

Then I was like, "I wonder how out of date it is anyway."

Then I was like, "Oh. 8 Months. I wonder if, since @magnusvk and I both live in NYC, if there wouldn't be some Saturday we could pair and just knock this motherfucker out."

@magnusvk
Copy link
Owner

Justin -- maybe in the new year. :) Seriously, though, as I don't use
counter_culture on polymorphic relationships I'm not sure I'll find the
time for this. I had looked into this before, and it's not trivial to
implement. Personally, I wouldn't use that fork, as it *is *outdated and a
little hackish. YMMV. I'd be very happy to merge a fully tested pull
request though. ;)

@esbanarango
Copy link

Any work on this?

@deniskorobicyn
Copy link

@esbanarango, I have some code in my fork https://github.com/deniskorobicyn/counter_culture/tree/polymorphic-assosiations, but its not fully tested and not documented. If I'll have some time in next year, I'll finish this according with TODO posted below by @magnusvk. Also my code have some refactoring that needs more cleanup.

Some simple usage:

#community.rb
class Community
# should have users_count column
  has_many :users
end

#organization.rb
class Organization
# should have users_count column
  has_many :users
end

#user.rb
class User
 belongs_to :group, polymorphic: true
 counter_culture :group
end

# new feature count only certain classes (not fully tested)
class User
  belongs_to :group, polymorphic: true
  counter_culture :group, only: [[:community]] # double brackets are required by now 
end

You can find other examples in spec/models folder.

P.S.
Also, if it possible, it's better to _not_ use polymorphic associations, because its actually antipatern of database design, http://stackoverflow.com/questions/922184/why-can-you-not-have-a-foreign-key-in-a-polymorphic-association?answertab=active#tab-top

@esbanarango
Copy link

@deniskorobicyn thank you for this! 👍

@dmitry
Copy link

dmitry commented Feb 27, 2015

@deniskorobicyn thanks for the anti-pattern link. There are lot of useful information.

@vishaldeepak
Copy link

any updates on this, would love to have polymorphic capability, as i have used this extensively in my project.

@magnusvk
Copy link
Owner

magnusvk commented Feb 3, 2016

Nothing new from my end, unfortunately.

@danielbonnell
Copy link

Hi,

Are there any updates on this yet or newer forks with this capability? I used this gem extensively in a project and now need polymorphic support. Thanks!

@magnusvk
Copy link
Owner

magnusvk commented Nov 16, 2016 via email

@timdiggins
Copy link
Contributor

#154 is a new(ish) PR for single-level polymorphic counters (I think multi-level may be possible, I just haven't come up with a reasonable test case yet).

After a bit of too-ing and fro-ing I think this is working.

If anyone has a use-case for multi-level polymorphic counters can you add a simplified example here -- it would help motivate a subsequent multi-level polymorphic counter PR.

@magnusvk
Copy link
Owner

Closing this as one-level polymorphic relationships are supported now as of version 1.5.0. Thank you @timdiggins for making this happen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests