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

Fix User-Defined Column Cache Override #911

Merged
merged 2 commits into from
Aug 7, 2018

Conversation

tonyta
Copy link
Contributor

@tonyta tonyta commented Jul 24, 2018

Problem

We ran into a problem where this gem was unexpectedly overriding our defined columns method. This was because this gem uses instance_eval which defines methods directly on the class.

Before

Given the following schema and model, the columns method should reject the ignored_column:

create_table :columns_override_models, force: true do |t|
  t.column :name, :string
  t.column :type, :string
  t.column :ignored_column, :string
end
class ColumnsOverrideModel < ActiveRecord::Base
  def self.columns
    super.reject { |c| c.name == 'ignored_column' }
  end
end

ColumnsOverrideModel.columns.map(&:name)
# => ["id", "name", "type"]

Unfortunately, invoking acts_as_taggable on the model after the user has defined columns will override the user's method using instance_eval.

class ColumnsOverrideModel < ActiveRecord::Base
  def self.columns
    super.reject { |c| c.name == 'ignored_column' }
  end
end

ColumnsOverrideModel.columns.map(&:name)
# => ["id", "name", "type", "ignored_column"] 😳

Solution

The solution in this PR is to user extend instead of instance_eval, which will not override the user-defined method, and will define the intercept method as part of the inheritance ancestor walk-up.

class ColumnsOverrideModel < ActiveRecord::Base
  def self.columns
    super.reject { |c| c.name == 'ignored_column' }
  end
end

ColumnsOverrideModel.columns.map(&:name)
# => ["id", "name", "type"] 😊

In addition, since I was confused about where to defined my test models, this PR also cleans up the definition and loading of those models. Hope this is helpful! 💜❤️🧡💛💚💙

@tonyta tonyta changed the title Column cache override Fix User-Defined Column Cache Override Jul 24, 2018
@seuros
Copy link
Collaborator

seuros commented Jul 28, 2018

Thank you.

Can you add a changelog entry ? i will merge and release a version

@tonyta tonyta force-pushed the column-cache-override branch from c0ec0f9 to 564909a Compare August 7, 2018 03:06
@tonyta
Copy link
Contributor Author

tonyta commented Aug 7, 2018

Thanks for your patience, @seuros. I've added the changelog entry and rebased. 🤘

@tonyta tonyta mentioned this pull request Aug 7, 2018
tonyta added 2 commits August 6, 2018 22:14
Previously, ActsAsTaggableOn::Taggable::Cache intercepts calls to
columns by using instance_eval. Unfortunately, if the user had defined
columns before invoking acts-as-taggable-on, their method would be
overridden.

This commit ensures the intercept happens as part of the inheritance
ancestor walk-up, before any user-defined methods.
@tonyta tonyta force-pushed the column-cache-override branch from 564909a to dfb34e0 Compare August 7, 2018 05:14
@seuros seuros merged commit df16530 into mbleigh:master Aug 7, 2018
sanchojaf pushed a commit to PulseSoftwareInc/acts-as-taggable-on that referenced this pull request Jan 9, 2019
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

Successfully merging this pull request may close these issues.

2 participants