rails-on-services / apartment

Database multi-tenancy for Rack (and Rails) applications
388 stars 159 forks source link

Prevent Rails 7.1 create_schema from being added to db/schema.rb #276

Closed patbenatar closed 2 months ago

patbenatar commented 4 months ago

This seems like noise in the schema file for me and I can't see how it'd be t relevant to an Apartment project. In an Apartment project, schemas are managed by Apartment not ActiveRecord like they would be in a vanilla Rails setup.

mnovelo commented 4 months ago

Yes, we had to do something similar ourselves to stop the schemas from getting added

mnovelo commented 4 months ago

@patbenatar specs are failing with error

NameError:
  uninitialized constant ActiveRecord::ConnectionAdapters::SchemaDumper
  Did you mean?  ActiveRecord::SchemaDumper
kjakub commented 2 months ago

I managed to get this working with different approach - (loading issue is probably solvable), but with second look on this approach i asked myself why apartment gem should override another gem - which is active_record default ?

I believe this https://github.com/rails/rails/blob/main/activerecord/lib/active_record/connection_adapters/postgresql/schema_dumper.rb#L31 has a reason to be there. And one would be wondering in future shy create_schema are not appearing in schema.rb.

Imagine scenario where i would be using apartment but not schema/postgresql approach but still using schemas for different reasons.

I would leave this to user, his overrides.

For example in rails, can be initializers/override_schema_dumper.rb

module ActiveRecord
  module ConnectionAdapters
    module PostgreSQL
      class SchemaDumper
        private
        def schemas(stream)
        end
      end
    end
  end
end

And thank you for this gem

patbenatar commented 2 months ago

@mnovelo sorry for my delay on this. I'm just getting back to this PR now to see if I can get tests passing. Not sure what the error is all about as this code works in our app, but I'll get to the bottom of it.

@kjakub you bring up a good point that this create_schema suppression is only relevant if you're using schemas with Apartment. I'll see if I can adjust the code to make it only apply the patch when config.use_schemas is set to true.

patbenatar commented 2 months ago

@mnovelo ok I believe I've addressed both items in the latest commits. Please re-run CI and let's see if it passes now.

patbenatar commented 1 month ago

@mnovelo Will you please cut a new release?