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

Fix SystemStackError in schema_dumper.rb in combination with activerecord-multi-tenant #270

Closed
wants to merge 2 commits into from

Conversation

Laykou
Copy link

@Laykou Laykou commented Aug 28, 2024

When using https://github.com/citusdata/activerecord-multi-tenant gem, it also overrides the SchemaDumper. This gem uses alias method instead of super.

Reference:
https://github.com/citusdata/activerecord-multi-tenant/blob/0ac43aa98334a29ed3e5b33e0bcc5ee281f97254/lib/activerecord-multi-tenant/migrations.rb#L86

strong_migrations in combination with this gem created following issue:

SystemStackError: stack level too deep (SystemStackError)
.../gems/strong_migrations-1.8.0/lib/strong_migrations/schema_dumper.rb:6:in `new'
.../gems/strong_migrations-1.8.0/lib/strong_migrations/schema_dumper.rb:6:in `initialize'
.../gems/activerecord-multi-tenant-2.4.0/lib/activerecord-multi-tenant/migrations.rb:93:in `initialize'
.../gems/strong_migrations-1.8.0/lib/strong_migrations/schema_dumper.rb:6:in `initialize'
.../gems/activerecord-multi-tenant-2.4.0/lib/activerecord-multi-tenant/migrations.rb:93:in `initialize'
.../gems/strong_migrations-1.8.0/lib/strong_migrations/schema_dumper.rb:6:in `initialize'

I figured out using the prepended with the alias method instead avoided the issue in overriding the initialize method by multiple gems

Laykou added 2 commits August 28, 2024 13:49
…cord-multi-tenant

When using https://github.com/citusdata/activerecord-multi-tenant gem, it also overrides the SchemaDumper. This gem uses alias method instead of `super`.

Reference:
https://github.com/citusdata/activerecord-multi-tenant/blob/0ac43aa98334a29ed3e5b33e0bcc5ee281f97254/lib/activerecord-multi-tenant/migrations.rb#L86

strong_migrations in combination with this gem created following issue:

```
SystemStackError: stack level too deep (SystemStackError)
.../gems/strong_migrations-1.8.0/lib/strong_migrations/schema_dumper.rb:6:in `new'
.../gems/strong_migrations-1.8.0/lib/strong_migrations/schema_dumper.rb:6:in `initialize'
.../gems/activerecord-multi-tenant-2.4.0/lib/activerecord-multi-tenant/migrations.rb:93:in `initialize'
.../gems/strong_migrations-1.8.0/lib/strong_migrations/schema_dumper.rb:6:in `initialize'
.../gems/activerecord-multi-tenant-2.4.0/lib/activerecord-multi-tenant/migrations.rb:93:in `initialize'
.../gems/strong_migrations-1.8.0/lib/strong_migrations/schema_dumper.rb:6:in `initialize'
```

I figured out using the `prepended` with the `alias` method instead avoided the issue in overriding the `initialize` method by multiple gems
@ankane
Copy link
Owner

ankane commented Aug 28, 2024

Hi @Laykou, thanks for the PR, but I think it's better for conflicting gems to switch to Module#prepend (#129).

@ankane ankane closed this Aug 28, 2024
@Laykou
Copy link
Author

Laykou commented Aug 28, 2024

@ankane Thank you, I may not be that fluent in this "Ruby overriding" world. So you're saying using:

ActiveRecord::SchemaDumper.prepend(StrongMigrations::SchemaDumper)

like you do in this gem is preferred/better option rather than monkey pathing:

module ActiveRecord
  class SchemaDumper
    alias initialize_without_citus initialize

    def initialize(connection, options = {})
      # ... calling 
    end
  end
end

that is done in the other gem? And the root cause of the issue is this monkey patching.
Calling super when using prepend is the correct way, right?

Thank you for your insights

@ankane
Copy link
Owner

ankane commented Aug 28, 2024

Yes, I think Module#prepend is the better pattern.

Laykou added a commit to Laykou/activerecord-multi-tenant that referenced this pull request Aug 29, 2024
…igrations

When using https://github.com/ankane/strong_migrations gem, it also overrides the SchemaDumper. 

Reference:
https://github.com/Laykou/strong_migrations/blob/master/lib/strong_migrations/schema_dumper.rb

strong_migrations in combination with this gem created following issue:

```
SystemStackError: stack level too deep (SystemStackError)
.../gems/strong_migrations-1.8.0/lib/strong_migrations/schema_dumper.rb:6:in `new'
.../gems/strong_migrations-1.8.0/lib/strong_migrations/schema_dumper.rb:6:in `initialize'
.../gems/activerecord-multi-tenant-2.4.0/lib/activerecord-multi-tenant/migrations.rb:93:in `initialize'
.../gems/strong_migrations-1.8.0/lib/strong_migrations/schema_dumper.rb:6:in `initialize'
.../gems/activerecord-multi-tenant-2.4.0/lib/activerecord-multi-tenant/migrations.rb:93:in `initialize'
.../gems/strong_migrations-1.8.0/lib/strong_migrations/schema_dumper.rb:6:in `initialize'
```

`Module#prepend` is recommended approach to be used here instead of monkey patching as advised here ankane/strong_migrations#270 (comment)
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