ilyakatz/data-migrate

NoMethodError (undefined method `exists?' for DataMigrate::DataSchemaMigration:Class)

rocket-turtle opened this issue · 9 comments

rocket-turtle commented

Hi there,

this commit broke our code:
9dda061

We use DataMigrate::DataSchemaMigration.exists? to check if any data_migration allready ran.
It's true if we set up a new system. With the commit above we can use DataMigrate::DataSchemaMigration.instance.exists? for the check. Is it the correct way to do this or should we use a different technique?

Thank you for your help and time providing this gem.

> require('data_migrate/version')
 => true
> DataMigrate::VERSION
 => "8.1.1"
> DataMigrate::DataSchemaMigration.exists?
  DataMigrate::DataSchemaMigration Exists? (0.1ms)  SELECT 1 AS one FROM "data_migrations" LIMIT ?  [["LIMIT", 1]]
 => true
> DataMigrate::DataSchemaMigration.method(:exists?)
 => #<Method: DataMigrate::DataSchemaMigration(version: string).exists?(...) /Users/jonas.schoeler/.rvm/gems/ruby-2.7.6@mhmr/gems/activerecord-6.0.6/lib/active_record/querying.rb:21>

> DataMigrate::VERSION
 => "8.2.0"
> DataMigrate::DataSchemaMigration.exists?
Traceback (most recent call last):
        1: from (irb):1
NoMethodError (undefined method `exists?' for DataMigrate::DataSchemaMigration:Class)
> DataMigrate::DataSchemaMigration.instance.exists?
   Exists? (0.1ms)  SELECT 1 AS one FROM "data_migrations" LIMIT ?  [["LIMIT", 1]]
 => true
ilyakatz commented

@defsprite do you have any recommendations on how to go about this?

elik-ru commented

Same with any ActiveRecords methods not explicitly delegated (all, delete_all etc).

I had to nail down version 8.1.1 to get around this problem.

defsprite commented

@ilyakatz I'm not sure whether DataMigrate::DataSchemaMigration was meant to be a "public" API, but folks seem to be actively using it.

If that is the case, we may want to reconsider the delegation approach (i.e revert the change) and look into restructuring how the gem is loaded in https://github.com/ilyakatz/data-migrate/blob/master/lib/data_migrate.rb in order to avoid eagerly making database connections.

What do you think?

rocket-turtle commented

We changed our code to DataMigrate::DataSchemaMigration.instance.exists?

I hope instance is "public".

Issue can be closed if @elik-ru can do the same "fix".

rocket-turtle commented

DataMigrate::DataSchemaMigration changed back to original implementation.

In this commit DataMigrate::DataSchemaMigration delegate to an anonymous subclass of AR::SchemaMigration
9dda061

In this commit kind of reverts the change:
efc9891

@defsprite, @wildmaples I'm a little bit confused, which way is better? And what were the intentions behind the changes.

defsprite commented

@rocket-turtle The commit was meant as a fix for #222 - because just loading the gem forces an active database connection to be present. However, a lot of (or very vocal) people seem to be relying on the fact that DataMigrate::DataSchemaMigration inherits from AR::SchemaMigration already, so this needs to be done either in a major breaking release or we'd need to refactor the way the gem is loaded instead (i.e. deferring requiring the migrations in a config.to_initialize block or similar).

rocket-turtle commented

Thank you for your reply.

If I understand it correct the ActiveRecord in DataSchemaMigration < ::ActiveRecord::SchemaMigration was the "problem".

So with the switch back to "inherit from ActiveRecord::SchemaMigration" the gem loads AR to early?
Can you confirm, that "your" problem is back?
Do you know why the switch back happend? I only see #275 which reads more like an cleanup not an intentional switch back.

Is there a way to navigate to the pull request of an commit. For example from 9dda061 to #229

defsprite commented

Yes, I can verify this problem exists in version 9.2.0 using the bug app provided. DataSchemaMigration is an AR model and eagerly requiring it in lib/data_migrate.rb forces the connection to be present.

I think there are two ways to go about this:

  • Use the delegation approach (which was accidentally removed)
  • Fix the eager requiring behaviour by using e.g. the mechanics provided by Rails::Railtie

I'd probably tend to the latter solution, because the delegation approach is confusing and unobvious (as it looks to me it was just accidentally reverted).

The problem described (forcing a db connection to be present) is also a widespread issue in many gems, so you'll end up up using a nulldb adapter approach anyway at some point. We may as well just close the issue #222 as wontfix.

Apologies to the community for creating breaking API changes for a rather minor issue that cause extra work dealing with it (for the second time now)

TLDR: I'd recommend keeping the inheritance approach and fix the gem loading eventually.

  • 👍1
ngan commented

FWIW..

Rails 7.1 makes ActiveRecord::SchemaMigration not an actual model (as in, not inherit from ActiveRecord::Base):
https://github.com/rails/rails/blob/main/activerecord/lib/active_record/schema_migration.rb#L8