Improved performance with counter_cache for tags (tag.taggings_count)

[dgilperez]

See #126 for reference.

[bf4]

Can you rebase against master and force push?

[dgilperez]

I will not be able to do it in the following days, but if you can wait a little bit I will do it.

[bf4]

Great!

[bf4]

Rebase and remove whitespace?

[dgilperez]

Done! Tests are green, though I've no time atm to manually check everything is working as expected.

[seuros]

@dgilperez could you revert your change to db/migrate/1_acts_as_taggable_on_migration.rb and create another migration that will add the counter_cache ?

PS: how did you add a commit 2 years ago ? :)

[bf4]

@seuros My guess is @dgilperez cherry-picked an older commit from a previous PR when making this one... :)

[seuros]

or he is a time traveler !

I tested this PR, everything looks fine.

[dgilperez]

;)

Good guess, I cherry-picked that commit from @lichtamberg like two years ago.

I rebased again, added the migration code in a new migration file and merged the commits into a single one. Please tell me if you need anything else done.

Thanks!

[bf4]

Rather than using xit to temporarily disable the test, maybe just set it as pending 'until we figure out how to test this'

[dgilperez]

Done!

[bf4]

Looks good to merge. Wanna update the CHANGELOG? (It's new)

[bf4]

Would you mind rebasing / squashing some of these commits, e.g. remove whitespace added in earlier commit?

[dgilperez]

@bf4 rebased again and squashed!

About the CHANGELOG: What would you say this is for a change? A new Feature or a Misc? Also, I'm not sure how to deal with the fact that users are to be requested to run migrations again (using rake railties:install:migrations FROM=acts_as_taggable_on_engine db:migrate??) ... a warning in the bundle install log? Is this a breaking change?

Thanks !!!

[seuros]

I think you should expand the Misc by added "Performance" to it. Place the others in the 'Unsorted'
You should also place it in the breaking change to warn for the migration requirement.

Also please look at #449 for the migration syntax.

[dgilperez]

@seuros is this what you mean? 4917274

[bf4]

@seuros Performance is a good category. (This is a format I'm working on. Originally, I wanted to translate semver into English/devglish for the changelog. I should update notes in the rubygems style guide)

I don't consider a migration, on its own, a breaking change. Breaking change is w/r/t a change in expected behavior (e.g. returns different type) or the way to achieve it (e.g. method signature) from previous releases.

[seuros]

@dgilperez 👍

@bf4 While some migration can be ignored ( add_index, add comment, default value), some migrations like #390 are not optional, updating the gem and not applying the migration will cause the immediate malfunction of the application .

If the database user is not a super user or has no right to alter the table. Updating the gem will cause the application to crash after the next deploy. So the notice is useful to warn for the maintenance that should be done ((Turning off/Reconfiguring) database watchers/Alter user rights, ect)

[bf4]

@seuros so another major version bump? ugh. Maybe we can check columns for :taggings_count and only add the counter cache if present add_counter_cache_callbacks(reflection)?

[seuros]

@bf4 the check will be perfect, then we can remove #390 from the breaking change section.
the check can be done in-line.

@dgilperez Can you implement this check ?

[bf4]

You may want to reuse this pattern from cache https://github.com/mbleigh/acts-as-taggable-on/blob/master/lib/acts_as_taggable_on/acts_as_taggable_on/cache.rb#L25

[dgilperez]

@bf4 @seuros I'm not quite sure on how to implement that. Maybe something in this line, from the suggested pattern? (quick & dirty).

# acts-as-taggable-on / lib / acts_as_taggable_on / acts_as_taggable_on / cache.rb
module Cache
  def self.included(base)
    base.instance_eval do
      # @private
      def _has_tags_counter_cache_columns?(db_columns)
        db_column_names = db_columns.map(&:name)
        db_column_names.include?("taggings_count")
      end

      # @private
      def _add_tags_counter_cache_methods
        add_counter_cache_callbacks(???)
      end

      def columns
        @acts_as_taggable_on_columns ||= begin
          db_columns = super
          if _has_tags_cache_columns?(db_columns)
            _add_tags_caching_methods
          end
          if _has_tags_counter_cache_columns?(db_columns)
            _add_counter_cache_callbacks
          end
          db_columns
        end
      end

Please help!

[bf4]

First off @acts_as_taggable_on_columns has to be a different name or will conflict with the one on cache. (feel free to change it there)

[bf4]

(See https://github.com/rails/rails/blob/master/activerecord/lib/active_record/associations/builder/association.rb#L30 and https://github.com/rails/rails/blob/master/activerecord/lib/active_record/associations/builder/belongs_to.rb#L17 )

[dgilperez]

Sorry guys, this is a bit ahead of my capabilities ... and worse than that, I'm leaving tomorrow for some days off and won't have much time in the next few weeks ... please feel free to step ahead, otherwise I'll come over as soon as I can. Thanks for your directions and help on this in any case !!!

[bf4]

Well, I put it in a local branch so someone (not necessarily me) can finish it https://github.com/mbleigh/acts-as-taggable-on/tree/dgilperez-counter_cache

[bf4]

I'm a sucker for this stuff sometimes. Let's see if Travis passes. Please also test yourself, both before and after running the migration, if possible. https://travis-ci.org/mbleigh/acts-as-taggable-on/builds/16298201

[bf4]

Merged in manually f891175 and #390 (reference)