Only set duration attribute to interval on Rails 6.1

[bdewater-thatch]

This causes ActiveRecord::ConnectionNotEstablished issues in 4.0.2 with Tapioca, this works around the problem by not setting it if not needed. I think it's a bug in Tapioca so I opened Shopify/tapioca#1950 but figured this might be a useful workaround in the meantime.

This line was originally shipped in #1408, 4.0.1 does not causes issues for me.

[Earlopain]

This seems fine, the line is only relevant on 6.1 anyways like you said. I don't use tapioca myself but I was curios and added it to one of my apps and encounter the same behaviour as you. I'm a bit surprised that it worked on 4.0.1 without the database being up at all.

I guess I'm surprised by rails needing an active connection for this call at all. For that reason alone this seems like a good change to do.

[bensheldon]

Thank you both. This makes sense to me:

• We only need the attribute configuration to quiet a Rails 6.1 deprecation warning, and the deprecation warning is only present in Rails 6.1
• Tapioca seems to break on Active Record attribute configuration on any version
• Therefore, this change minimizes the scope such that anyone will only experience the Tapioca breakage on Rails 6.1

I guess the alternative would be to remove the attribute configuration altogether from GoodJob and just accept the deprecation warning for Rails 6.1 in favor of not breaking Tapioca in Rails 6.1. And I think one could maybe patch their own application (though there isn't an easy load hook for that yet) and make that tradeoff themselves. e.g. from my poking at it, this sort of thing in an application initializer would quiet the warning (and wow, that's a big warning):

# this load hook doesn't exist yet
ActiveSupport.on_load(:good_job_execution) do
  attribute :duration, :interval
end

[Earlopain]

It should be set on 6.1 in my opinion. The moment you want to do more with the duration attribute you'd get some pretty unexpected behaviour (since it would be returned as a string on 6.1). It's only used in a single query right now so its whatever and tests would catch it most likely but still.

Alternativly, wait for someone to actually complain. I only changed this because I noticed the deprecation in CI output.

[bensheldon]

Those are good points 🤔 I feel like the balance here is like: (right now right now) a big annoyance versus breakage. And so if I think of like elevating a new user who is considering adopting GoodJob, I sort of imagine their appetite for debugging is lower than someone who is already using GoodJob 😦

I think I'm leaning toward just removing the attribute duration fix for the deprecation. It does seem like it's possible to patch in an application to quiet, and we'll just have to do some coercion in GoodJob while supporting Rails 6.1

[bensheldon]

I'll push that change to this branch, and land #1414 at the same time.

[bensheldon]

Welp, I guess I can change my mind in 2 minutes. Yes, let's do this as is and I'll wait to see if anyone opens an issue about Tapioca incompatibility.

[Earlopain]

@bdewater-thatch found a test (rails/rails#41302) which expects that this doesn't require a connection. I tried making a repro for this, thought perhaps it's pg interval specific but no luck, the test just passes:

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  # Activate the gem you are reporting the issue against.
  # gem "rails", github: "rails/rails", branch: "main"
  gem "rails", "7.1"
  gem "pg"
end

require "active_record/railtie"
require "minitest/autorun"
require "logger"

class TestApp < Rails::Application
  config.load_defaults 7.1
  config.eager_load = false
  config.root = __dir__

  config.logger = Logger.new($stdout)
  Rails.logger  = config.logger
end

ENV["DATABASE_URL"] = "postgresql://localhost:5432/foo"
Rails.application.initialize!

require "active_support/testing/method_call_assertions"

class BugTest < ActiveSupport::TestCase
  include ActiveSupport::Testing::MethodCallAssertions

  def test_not_called
    assert_not_called(ActiveRecord::Base, :connection) do
      Class.new(ActiveRecord::Base) do
        attribute :foo, :interval
      end
    end
  end
end

[bensheldon]

Released in https://github.com/bensheldon/good_job/releases/tag/v4.0.3

[bdewater-thatch]

Thanks @bensheldon!

accept the deprecation warning for Rails 6.1 in favor of not breaking Tapioca in Rails 6.1

Tapioca follows Rails' support policy. With 6.1 being EOL Tapioca support for it is on a best effort basis - see Shopify/tapioca#1913 (comment) - so unless someone comes up with an elegant fix to make it all work I think this the current state is fine.

[bdewater-thatch]

@Earlopain I found that PR too, thanks for trying to create a repro script. FWIW we do have some attribute calls inside our app's models (different data type) that don't cause issues, could it have to do with it coming from an engine?

[Earlopain]

Yeah, I guess it could be because of an engine. I'd have to do some more testing, at least I've hit the issue myself. If I manage to report upstream I'll link it here.

[Earlopain]

Should have just started with a fresh bug report template instead of mangling the last one I had still around. Here it is: rails/rails#52311