Fixes bug? datetime and time types should still use nil adapter

[jNicker]

I believe that you still want to keep the adapter as nil, otherwise you fall back to needing a database.

[jrochkind]

Thanks! I actually am not following this and don't understand the difference, I think you've gone somewhere I haven't explored in the attribute adapters.

Is it feasible to provide a test that fails without this change? That would be best, to prevent any regression too.

Alternately, if that's not feasible, if you could just describe more the situation where this makes a difference, I would find that helpful! I'm not following what "fall back to needing a database" means (in what circumstances?). Thank you for your contribution!

[jNicker]

The change is ensuring lookup_kwargs always still includes { adapter: nil }, so that #41 doesn't happen.

This is setting up the options hash called lookup_kwargs that will be passed to the ActiveRecord::Type.lookup

Because ActiveRecord::Type.lookup may/will try to connect to a database in some situations.
See #41

So... { adapter: nil } needs to be passed as one of the options to the lookup.

And we can see in the code that the option is indeed set.... but that it gets fully overwritten if the attribute type is :datetime or :time.

[jrochkind]

Aha, thanks! I had forgotten about that bug, and plus I see we thought we had fixed it!

Maybe it came back, or just for datetime, because we lacked a test for it!

I am not sure if there is a good way to make a test, can you think of any?

Either way, thank you!

[jrochkind]

Oh aha, maybe just need to add datetime to the test that i put in when i fixed this before, but didn't realize should test multiple types.

b426b5d

OK, i'll get on it, thanks!

[jNicker]

Yes... just for :datetime and :time

Ill think about how to write a test....
not sure yet. I have not dug in very deep into this project to see how to form it yet or if it would be possible feasible.

[jNicker]

yah... that should trigger it.

[jNicker]

I added to the testcase.
However I was unable to make the test fail in either case without also checking if
ActiveRecord::Type.adapter_name_from was called.
I'm not sure this is the best way to check this or not.... but on my end it now fails on the old code, or when { adapter: nil } is not passed to any type... and passes on the new code.
...

[jrochkind]

Interesting, thanks! Definitely kind of mysterious.

Have you encountered the actual real failure in the real world, where you are getting an exception when the DB isn't yet available?

If so, can you share the stack trace from that actual error?

[jrochkind]

Still failing your new test in Rails 6.0, which is weird. And I don't know we really need to keep supporting Rails 6, but I try not to drop rails versions without a major version release.

I can work on trying to figure out the right way to add these tests when I have time, but might take me a bit. Is this problem urgent for you?

[jNicker]

No. This is not urgent. I can work around it. I was just trying to be helpful and squash a bug.
No rush.
Ill continue to work on this and see if I can figure it out across the test matrix. I'm sure we can get it figured out eventually.

Ill put up a stack trace for it later as well.

I appreciate your time.

[jrochkind]

Oh no worries, you have indeed been super helpful! Thank you for reporting this with the fix!

I was just asking if it was urgent because I felt bad and would have tried to get this merged sooner if it were!

I've got a few balls in the air at the moment, but you are welcome to do as much work on it as you want, but no pressure I also don't expect you to, and I promise I will circle back to it later when I have a moment!

I just ideally want to understand a bit more about what's going on before I merge it, but I appreciate your helpfulness, thank you!

[jNicker]

So I have figured out this is specific to rails 6.0 & 6.1
Calling ActiveRecord::Type.lookup in 7.X and above doesn't trigger a database connection in my testing.

Im thinking changing the test to something like

if ActiveRecord.version < Gem::Version.new('7.2.0')
  ActiveRecord::Base.clear_all_connections!
else
  ActiveRecord::Base.connection_handler.clear_all_connections!
end
expect(ActiveRecord::Base.connected?).to eq(false)

Class.new(ActiveRecord::Base) do
  include AttrJson::Record

  self.table_name = "products"
  attr_json :value, :string
  attr_json :datetime_value, :datetime
  attr_json :time_value, :time
end

expect(ActiveRecord::Base.connected?).to eq(false)

may work better....
let me know what you think.

[jrochkind]

Interesting! But...

Calling ActiveRecord::Type.lookup in 7.X and above doesn't trigger a database connection in my testing.

Does this imply that the problem we are trying to solve doesn't exist in 7.0 and above? (7.0 and below are all EOL from Rails).

Still nothing wrong with cleaning this up and putting in a test for regression, but curious.... you ran into this problem yourself @jNicker? Was it on a Rails prior to 7.0?

[jNicker]

Yes. I am currently upgrading a clients application from Rails 5.X to latest...
and experienced the issue in Rails 6.1

So yes... I think it only exists in EOL Rails at this point.

[jrochkind]

Oh nice! Still no reason not to fix it though! I'll try to get it merged early next week! Thank you!

[jrochkind]

So I have figured out this is specific to rails 6.0 & 6.1 Calling ActiveRecord::Type.lookup in 7.X and above doesn't trigger a database connection in my testing.

Im thinking changing the test to something like

Oh yeah, that's a great way to do it! Actually check for connected, nice!

I'll put that in tomorrow prob, and give you credit, or feel free if you want to.

[jNicker]

Updated.