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

Remove non-ActiveRecord::Base backed association #892

Merged
merged 1 commit into from
Mar 16, 2023

Conversation

rrunyon
Copy link
Contributor

@rrunyon rrunyon commented Mar 16, 2023

I have a project using GoodJob that I attempted to autogenerate Sorbet types for today. During bin/tapioca dsl I was running into this error:

/Users/rick/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/activerecord-7.0.4.2/lib/active_record/reflection.rb:436:in `compute_class': ArgumentError: Rails couldn't find a valid model for GoodJob::Batch association. Please provide the :class_name option on the association declaration. If :class_name is already provided, make sure it's an ActiveRecord::Base subclass. (Parallel::UndumpableException)
	from /Users/rick/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/activerecord-7.0.4.2/lib/active_record/reflection.rb:382:in `klass'

I traced the breaking association to GoodJob::Execution#batch_callback. From what I can tell, this association is not referenced anywhere in the code, nor is it valid because it relies on a PORO as its backing class. I was able to reproduce the issue by following the association on one of my execution records:

Loading development environment (Rails 7.0.4.2)
irb(main):001:0> GoodJob::Execution.first.batch_callback
  GoodJob::Execution Load (2.0ms)  SELECT "good_jobs".* FROM "good_jobs" ORDER BY "good_jobs"."id" ASC LIMIT $1  [["LIMIT", 1]]
/Users/rick/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/activerecord-7.0.4.2/lib/active_record/reflection.rb:436:in `compute_class': Rails couldn't find a valid model for GoodJob::Batch association. Please provide the :class_name option on the association declaration. If :class_name is already provided, make sure it's an ActiveRecord::Base subclass. (ArgumentError)

Seems to me like removing it is probably the best thing to do.

Thanks for all the hard work!

@rrunyon rrunyon changed the title Remove non-ActiveRecord:Base backed association Remove non-ActiveRecord::Base backed association Mar 16, 2023
@bensheldon
Copy link
Owner

@rrunyon 😓 Nice find! Yay Sorbet (and you!) being helpful here. I'll merge this.

I'm curious, did Tapioca/Sorbet complain about the models otherwise? I've struggled with with Sorbet'ing this repository because of this line:

class BaseRecord < Object.const_get(GoodJob.active_record_parent_class)

...or maybe you'll find that next after we solve this exception 😬

@rrunyon
Copy link
Contributor Author

rrunyon commented Mar 16, 2023

I'm curious, did Tapioca/Sorbet complain about the models otherwise? I've struggled with with Sorbet'ing this repository because of this line:

class BaseRecord < Object.const_get(GoodJob.active_record_parent_class)

...or maybe you'll find that next after we solve this exception 😬

The typegen for DSLs crashed on the above error so I wasn't able to see if there were more issues downstream. I'm happy to follow up after this is merged, and if I run into more issues maybe I'll work off of a fork until I can power through compilation and contribute back what I find.

@bensheldon bensheldon merged commit cd1c550 into bensheldon:main Mar 16, 2023
@bensheldon
Copy link
Owner

@rrunyon I just pushed a release to RubyGems if you can try that next 🤞🏻 https://github.com/bensheldon/good_job/releases/tag/v3.14.2

@rrunyon rrunyon deleted the remove-non-ar-assocation branch March 16, 2023 16:37
@rrunyon
Copy link
Contributor Author

rrunyon commented Mar 16, 2023

@bensheldon that did the trick! I was able to generate .rbi files for my project including these files from GoodJob:

      create  sorbet/rbi/dsl/good_job/active_job_extensions/concurrency.rbi
      create  sorbet/rbi/dsl/good_job/active_job_extensions/notify_options.rbi
      create  sorbet/rbi/dsl/good_job/application_controller.rbi
      create  sorbet/rbi/dsl/good_job/active_job_job.rbi
      create  sorbet/rbi/dsl/good_job/batches_controller.rbi
      create  sorbet/rbi/dsl/good_job/cron_entries_controller.rbi
      create  sorbet/rbi/dsl/good_job/batch_record.rbi
      create  sorbet/rbi/dsl/good_job/frontends_controller.rbi
      create  sorbet/rbi/dsl/good_job/execution.rbi
      create  sorbet/rbi/dsl/good_job/jobs_controller.rbi
      create  sorbet/rbi/dsl/good_job/lockable.rbi
      create  sorbet/rbi/dsl/good_job/job.rbi
      create  sorbet/rbi/dsl/good_job/processes_controller.rbi
      create  sorbet/rbi/dsl/good_job/process.rbi
      create  sorbet/rbi/dsl/good_job/setting.rbi

This list is missing base_record, I'm still learning the Sorbet ecosystem but I think Tapioca is deciding what to generate based on what constants my application references at runtime. If I have time in the future I'd be happy to try to set up Sorbet for GoodJob. What's the current state of #760?

@bensheldon
Copy link
Owner

@rrunyon I'm a bit stalled on #760. I don't think there's anything stopping me from merging that, but I keep running into issues actually getting any file type checked.

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