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

Lots of failures with rolify gem after 4.1 -> 5.0 bump #203

Closed
geoffharcourt opened this issue Jun 8, 2023 · 12 comments
Closed

Lots of failures with rolify gem after 4.1 -> 5.0 bump #203

geoffharcourt opened this issue Jun 8, 2023 · 12 comments
Assignees
Labels

Comments

@geoffharcourt
Copy link

Hi,

We're still not 100% sure what the root cause is, but when we tried to upgrade to 5.0.0 after the recent release, nearly every spec that involved the use of functionality from the rolify gem (v6.0.1) started failing for one of two reasons.

The first and most common is that when we try to apply a role to a user we're now seeing:

NoMethodError:
  undefined method `define_dynamic_method' for AdminUser:Class

These specs do not fail in local testing. I am assuming from the rolify issues and the differences between CI and local development that this has something to do with loading order that's somehow different with CI doing eager loading and using Knapsack vs. not.

I can't tell from the code changes in the new release what exactly would cause this, but I wanted to file this issue in case anyone else sees similar behavior.

@ArturT ArturT added the bug label Jun 8, 2023
@ArturT
Copy link
Member

ArturT commented Jun 8, 2023

Hi @geoffharcourt

Do you use Queue Mode?
Do you use RSpec?

The only significant change I can think of is using by default RAILS_ENV=test/RACK_ENV=test
#199

List of changes for the knapsack_pro 5.0.0
https://github.com/KnapsackPro/knapsack_pro-ruby/blob/master/CHANGELOG.md#500

@geoffharcourt Is the bug happening also when you try to run knapsack_pro on your local machine or only in CI?
https://docs.knapsackpro.com/ruby/troubleshooting/#debug-knapsack-pro-on-your-development-environmentmachine

Can you share your Gemfile? Is rolify gem in a particular group?

Can you share what command do you use to run Knapsack Pro? Do you pass any options or use non standard prefix etc.

FYI @3v0k4 if you have any ideas what we could check to debug this feel free to chime in. Thanks.

@3v0k4
Copy link
Contributor

3v0k4 commented Jun 8, 2023

Thanks for the issue @geoffharcourt and sorry for the trouble.

I assume in local testing you are using the test runner directly without Knapsack, correct?

Could you please try to add RAILS_ENV=test RACK_ENV=test in front of the command you use for local testing and let me know if you see the same errors?

It would be super useful if you could try to run on CI the following and let me know if the problems go away (notice the _go part):

# pick the appropriate test runner

bundle exec rake knapsack_pro:queue:rspec_go
bundle exec rake knapsack_pro:queue:cucumber_go
bundle exec rake knapsack_pro:minitest_go

Also, the answers to @ArturT's question would be useful for me to investigate deeper.

@3v0k4 3v0k4 self-assigned this Jun 8, 2023
@geoffharcourt
Copy link
Author

Hi, we're using RSpec in Queue Mode.

I tried adding the RAILS_ENV=test RACK_ENV=test to our local executions and it didn't change thing the results. We're trying the _go runners now to see if this has any effect!

@geoffharcourt
Copy link
Author

rolify is in the general group for us. Here's our Gemfile:

source "https://rubygems.org"

source "https://enterprise.contribsys.com/" do
  gem "sidekiq-ent"
  gem "sidekiq-pro"
end

ruby "3.2.2"

gem "rails", "7.0.4.3"

gem "activeadmin", "~> 2.0"
gem "activeadmin_addons", "~> 1.5"
gem "active_kms"
gem "active_model_serializers", "~> 0.10.0"
gem "active_record_union"
gem "acts_as_paranoid", "~> 0.8.0"
gem "ahoy_matey"
gem "authtrail"
gem "aws-sdk-firehose"
gem "aws-sdk-polly", "~> 1.5"
gem "aws-sdk-s3", "~> 1.1"
gem "aws-sdk-secretsmanager", "~> 1.46"
gem "aws-sdk-translate", "~> 1.10"
gem "bootsnap", require: false
gem "breadcrumbs_on_rails"
gem "browser"
gem "cancancan"
gem "canonical-rails"
gem "caxlsx_rails", "~> 0.6.0"
gem "commonlit-clever-ruby", "~> 2.1.3", require: "clever-ruby"
gem "console1984"
gem "countries", "~> 5.0"
gem "dalli", "~> 3.2.0"
gem "ddtrace", require: "ddtrace/auto_instrument"
gem "descriptive_statistics", require: "descriptive_statistics/safe"
gem "devise", "~> 4.8"
gem "devise-two-factor", "~> 5.0"
gem "docraptor"
gem "dogstatsd-ruby"
gem "down"
gem "draftjs_exporter"
gem "draper"
gem "excon"
gem "factory_bot_rails", "~> 6.0"
gem "fast_blank"
gem "fuzzy_match"
gem "fx"
gem "google-apis-classroom_v1"
gem "google-apis-youtube_v3", "~> 0.1"
gem "hashids", "~> 1.0", ">= 1.0.4"
gem "http-2"
gem "http_accept_language"
gem "hubspot-api-client"
gem "hypershield"
gem "i18n-tasks"
gem "jwt"
gem "kt-paperclip", "~> 7.0"
gem "lograge"
gem "loofah-activerecord"
gem "mail", "~> 2.7"
gem "mobility", "~> 1.0"
gem "net-imap", require: false
gem "net-pop", require: false
gem "net-smtp", require: false
gem "oj", "~> 3.14.1"
gem "omniauth-google-oauth2", "~> 1.0"
gem "omniauth-rails_csrf_protection"
gem "opensearch-ruby"
gem "paul_revere", "~> 3.3.0"
gem "pg", "~> 1.5.3"
gem "pgsync", "~> 0.7.0"
gem "postmark-rails"
gem "prometheus_exporter"
gem "puma", "~> 6.0"
gem "rack-attack"
gem "rack-cors", require: "rack/cors"
gem "rack-dev-mark", "~> 0.7.9"
gem "rails-i18n", "~> 7.0"
gem "rb-readline"
gem "react_on_rails", "~> 12.1"
gem "redis", "~> 4.0"
gem "rolify"
gem "rqrcode"
gem "rubyzip", "~> 2.0"
gem "scenic"
gem "searchkick", "~> 5.2"
gem "secure_headers"
gem "sentry-rails", "~> 5.0"
gem "sentry-ruby", "~> 5.0"
gem "sentry-sidekiq", "~> 5.0"
gem "sidekiq"
gem "sidekiq-postpone"
gem "silencer", require: false
gem "sitemap_generator"
gem "slack-notifier"
gem "splitclient-rb", "~> 8.1.0"
gem "sprockets", "< 4.0.0"
gem "sprockets-rails"
gem "strong_migrations"
gem "translate_enum"
gem "uglifier", ">= 1.3.0"
gem "validates_overlap"
gem "valid_email2"
gem "zxcvbn-ruby", require: "zxcvbn"

group :development do
  gem "better_errors", "~> 2.8"
  gem "binding_of_caller"
  gem "letter_opener"
  gem "rubocop", require: false
  gem "rubocop-performance", require: false
  gem "rubocop-rails", require: false
  gem "rubocop-rspec", require: false
  gem "solargraph", require: false
end

group :development, :test do
  gem "brakeman"
  gem "dotenv-rails", "~> 2.2"
  gem "isolator", require: false
  gem "localhost"
  gem "pry"
  gem "pry-byebug"
  gem "super_diff"
end

group :test do
  gem "capybara", "3.38.0"
  gem "capybara_discoball"
  gem "capybara-select-2"
  gem "capybara-slow_finder_errors"
  gem "climate_control"
  gem "cuprite"
  gem "db-query-matchers"
  gem "email_spec"
  gem "knapsack_pro"
  gem "percy-capybara", ">= 5.0.0"
  gem "rack_session_access"
  gem "rails-controller-testing"
  gem "roo", "~> 2.8"
  gem "rspec-buildkite"
  gem "rspec-rails", "~> 6.0"
  gem "rspec-retry"
  gem "rspec-sidekiq"
  gem "shoulda-matchers"
  gem "sinatra", "~> 3.0", require: false
  gem "sniffer", require: ["all_prepend", "sniffer"]
  gem "test-prof"
  gem "timecop"
  gem "vcr", "~> 6.1.0", require: false
  gem "webmock", require: false
end

group :production do
  gem "cloudflare-rails", "~> 3.0"
  gem "rack-timeout", "~> 0.5"
  gem "slowpoke", "~> 0.4.0"
end

The stacktrace for these failures almost always looks like this:


Failure/Error: admin_user.add_role(:content_editor)
--
  |  
  | NoMethodError:
  | undefined method `define_dynamic_method' for AdminUser:Class
  | # /bundle/ruby/3.2.0/gems/rolify-6.0.1/lib/rolify/role.rb:18:in `add_role'
  | # ./spec/factories/admin_users.rb:39:in `block (4 levels) in <main>'
  | # /bundle/ruby/3.2.0/gems/factory_bot-6.2.1/lib/factory_bot/callback.rb:12:in `instance_exec'
  | # /bundle/ruby/3.2.0/gems/factory_bot-6.2.1/lib/factory_bot/callback.rb:12:in `run'
  | # /bundle/ruby/3.2.0/gems/factory_bot-6.2.1/lib/factory_bot/callbacks_observer.rb:11:in `block in update'
  | # /bundle/ruby/3.2.0/gems/factory_bot-6.2.1/lib/factory_bot/callbacks_observer.rb:10:in `each'
  | # /bundle/ruby/3.2.0/gems/factory_bot-6.2.1/lib/factory_bot/callbacks_observer.rb:10:in `update'
  | # /bundle/ruby/3.2.0/gems/factory_bot-6.2.1/lib/factory_bot/evaluation.rb:24:in `notify'
  | # /bundle/ruby/3.2.0/gems/factory_bot-6.2.1/lib/factory_bot/strategy/create.rb:13:in `block in result'
  | # /bundle/ruby/3.2.0/gems/factory_bot-6.2.1/lib/factory_bot/strategy/create.rb:9:in `result'
  | # /bundle/ruby/3.2.0/gems/factory_bot-6.2.1/lib/factory_bot/factory.rb:43:in `run'
  | # /bundle/ruby/3.2.0/gems/factory_bot-6.2.1/lib/factory_bot/factory_runner.rb:29:in `block in run'
  | # /bundle/ruby/3.2.0/gems/factory_bot-6.2.1/lib/factory_bot/factory_runner.rb:28:in `run'
  | # /bundle/ruby/3.2.0/gems/factory_bot-6.2.1/lib/factory_bot/strategy_syntax_method_registrar.rb:28:in `block in define_singular_strategy_method'
  | # ./spec/requests/admin/activities_controller_spec.rb:12:in `block (4 levels) in <main>'

So FactoryBot has a callback, and that method's missing on the model in CI but not in other environments as far as we can tell. The only change that causes this failure to start appearing is bumping knapsack_pro from 4.1.0 -> 5.0.0 (there are no other changes in Gemfile.lock).

@geoffharcourt
Copy link
Author

@3v0k4 running with the rspec_go runner for Knapsack avoided all the exceptions! Is that OK for us to start using, or is that something we should just hold on 4.1.x for now on?

@3v0k4
Copy link
Contributor

3v0k4 commented Jun 9, 2023

@geoffharcourt thanks for all the info, please hold on 4.1.x for now.

@3v0k4
Copy link
Contributor

3v0k4 commented Jun 9, 2023

@geoffharcourt

The difference between knapsack_pro:queue:rspec and knapsack_pro:queue:rspec_go is the following:

task :rspec, [:rspec_args] do |_, args|
      Kernel.system("RAILS_ENV=test RACK_ENV=test #{$PROGRAM_NAME} 'knapsack_pro:queue:rspec_go[#{args[:rspec_args]}]'")
      Kernel.exit($?.exitstatus)
end

task :rspec_go, [:rspec_args] do |_, args|
      KnapsackPro::Runners::Queue::RSpecRunner.run(args[:rspec_args])
end

In other words, rspec calls rspec_go prepending RAILS_ENV=test RACK_ENV=test.

If you use RAILS_ENV=test RACK_ENV=test bundle exec rake knapsack_pro:queue:rspec with 4.1.x on CI you should see the same failures as bundle exec rake knapsack_pro:queue:rspec with 5.x.x on CI.

Is it the case?

Also, can you please paste your CI config here? (or send via email at support (at) knapsackpro.com).


I assume you call config.use_dynamic_shortcuts in config/initializers/rolify.rb and nowhere in the codebase dynamic_shortcuts is set with something like:

Rolify.dynamic_shortcuts = false
Rolify.dynamic_shortcuts = true

When rolify is called in AdminUser, it executes:

include Role
extend Dynamic if Rolify.dynamic_shortcuts

where include Role adds the add_role method and extend Dynamic adds the define_dynamic_method method.

In your stacktrace, I see role.rb:18 is called:

/bundle/ruby/3.2.0/gems/rolify-6.0.1/lib/rolify/role.rb:18:in `add_role'

which is

self.class.define_dynamic_method(role_name, resource) if Rolify.dynamic_shortcuts

which is the line that triggers the error:

NoMethodError:
  | undefined method `define_dynamic_method' for AdminUser:Class

But define_dynamic_method should have been defined by extend Dynamic if Rolify.dynamic_shortcuts when rolify was called in the model assuming dynamic_shortcuts was true.

RolifyCommunity/rolify#455 describes a similar problem. Though, I cannot see why upgrading knapsack_pro from 4.1 to 5.x would trigger it (given that we identified the issue in knapsack_pro:queue:rspec vs knapsack_pro:queue:rspec_go).


Do you see the same failures if you run Knapsack 5.x locally?

@3v0k4
Copy link
Contributor

3v0k4 commented Jun 12, 2023

@geoffharcourt

I noticed a couple of things that probably won't fix this issue but you may want to consider:

  • knapsack_pro is in the test group only while we suggest using development, test because some features like encryption require it
  • some customers reported problems in the past that went away by using the binstubs (bundle exec rake knapsack_pro:queue:rspec vs bin/rake knapsack_pro:queue:rspec)

I try to create a new Rails app from scratch with RSpec, FactoryBot, and Rolify, however I cannot reproduce this issue. I know it's a big ask, but is there any chance you could share a reproducible example.

If not, I'll try to dig deeper as soon as you have a chance to take a look at my last comment.

Thanks for your help, Geoff! 🙏

@geoffharcourt
Copy link
Author

@3v0k4 I'm deeply appreciative of the time you've already spent here. I'm working tracking some of the possible variables contributing to our case here. One thing we're doing that might be relevant is that we're calling add_role in an after(:create) hook in FactoryBot.

@geoffharcourt
Copy link
Author

geoffharcourt commented Jun 12, 2023

OK, I tried moving knapsack_pro to :development, :test and ensured that RACK_ENV=test was explicitly set (RAILS_ENV=test is already set in our environments) and I'm still seeing the issues. Going to try using binstubs over bundle exec next.

@geoffharcourt
Copy link
Author

An update: we saw that the #add_role method for us was also failing in production! We never use it, but it as a tipoff that something odd was happening with rolify. I'm still not 100% sure what is/was happening here other than that it had something to do with eager loading, but we've removed the offending code from our app and upgraded Knapsack Pro Ruby. I don't think this is worth any more of your time, but I want to thank you for your advice and ideas here!

@3v0k4
Copy link
Contributor

3v0k4 commented Jun 13, 2023

@geoffharcourt you're welcome. This actually taught me a few things, so thank you!

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

No branches or pull requests

3 participants