Dumper overrides change order of schema dump

[alyssais]

Since this gem overrides SchemaDumper, and generates table schemas in a different place to ActiveRecord, it interferes with other gems that hook into SchemaDumper, e.g. Scenic. All of e.g. Scenic's dump output will come first, and the normal ActiveRecord output after, which doesn't work if the e.g. Scenic output depends on the ActiveRecord tables existing first (which is almost always the case).

Now, since this gem is a dependency of schema_validations, it interferes with schema generation even when that's completely unrelated to what the user actually wants, and there's not even a good way to disable it. I'm having to do this to turn off the schema dump features:

module SchemaPlus::Core::ActiveRecord::SchemaDumper
  instance_methods.each do |name|
    define_method(name) { |*args| super *args }
  end
end

To fix this, the SchemaDumper should dump tables in the same place as ActiveRecord, rather than after the rest of the dump is done.

Thanks for an otherwise great library.

[alyssais]

An alternative would be just to provide some way to turn off the SchemaDumper overrides — as I said above, this library will often be included for users who have no interest in those overrides.

[boazy]

This is probably a SchemaPlus::Core issue. Almost all modules in the Schema*/SchemaPlus family ultimately depend on it, and this is the module which monkey patches both ActiveRecord::ConnectionAdapters and ActiveRecord::SchemaDumper. The patches for the connection adapters are generally unintrusive and just provide hooks for middleware, but SchemaDumper does way more.

I'm not sure how Scenic hooks into SchemaDumper. The ideal solution would allow SchemaDumper-dependant gems like Scenic to work even with SchemaDumper hooks enabled, so you could use gems like SchemaPlus::Views which actually use the dumper.

If that's not possible, we can still have a configuration option that disables schema dumper middleware hooks, while still registering the rest of the hooks by separating hooks into two different modules.

@ronen What do you think?

[ronen]

Probably SchemaPlus::Core could be split into two: SchemaPlus::Core that doesn't affect the dumper and SchemaPlus::Dumper that does, and gems that don't need to do any custom dumping, and only the gems that need to do custom dumping would depend on SchemaPlus::Dumper.

Still, at some point using multiple different gems that monkey-patch AR differently is likely to come into conflict. One solution is to try to convince authors of other gems to build on SchemaPlus::Core, since its purpose is to build an internal middleware API on which other gems can be built without each gem need to do its own monkey-patching.

Another solution is to stay within the SchemaPlus family. E.g. I'm not familiar with Scenic but it seems to support views -- could schema_plus_views work for you?

[alyssais]

I'm not sure how Scenic hooks into SchemaDumper.

It just overrides SchemaDumper#tables and inserts its own thing after the tables are generated.

Still, at some point using multiple different gems that monkey-patch AR differently is likely to come into conflict.

I think this should be mostly preventable by making sure that changes to ActiveRecord functionality are kept as minimal as possible. The problem here isn't intrinsic to two gems wanting to monkey-patch — it's that SchemaPlus changes how ActiveRecord works by moving the table dumping from SchemaDumper#tables to somewhere else. Of course, making this functionality opt-in (or at least configurable) rather than being enabled automatically by schema_validations would go a long way too.

[alyssais]

Another solution is to stay within the SchemaPlus family. E.g. I'm not familiar with Scenic but it seems to support views -- could schema_plus_views work for you?

It probably would, but I think I slightly prefer Scenic's approach, and I'd rather not feel like I'm locked into a whole ecosystem of gems just because I use one part of it.

[boazy]

I think separating into two gems alone, without allowing to disable SchemaDumper monkey-patching won't help in this particular case, since schema_validations relies on schema_plus_columns - which on turn relies on schema_plus_indexes - which requires the SchemaDumper middleware for some functionality. So even if we split schema_plus_core into two, some gems will still end up needing both, since they either rely on schema_plus_indexes directly or indirectly through schema_plus_columns.

The problem here is that in @alyssais case the SchemaDumper-dependant functionality part of SchemaPlus::Indexes is not required, the other parts are. We can introduce a configuration option that will prevent SchemaPlus::Core from registering the SchemaDumper middleware to solve that. This is not the most elegant solution, and it will silently break some behavior if you enable this option accidentally, but I don't see any other option except for breaking up SchemaPlus::Indexes (and perhaps some other gems) further. This doesn't require separating SchemaPlus::Core into two different gems, but we will need to have two different namespaces for SchemaMonkey.

[boazy]

I'm not sure how Scenic hooks into SchemaDumper.

It just overrides SchemaDumper#tables and inserts its own thing after the tables are generated.

For this particular case, I think it will be fairly easy to create a small shim that patches Scenic to nicely play with SchemaPlus::Core even with SchemaDumper activated. That might not be very useful for you, but it would be a better solution for someone who needs both Scenic and a SchenaDumper-dependent SchemaPlus gem. Still more work that a configuration switch though.

[alyssais]

For this particular case, I think it will be fairly easy to create a small shim that patches Scenic to nicely play with SchemaPlus::Core even with SchemaDumper activated.

I'm not familiar enough with SchemaPlus to know how difficult it would be, but it seems to me like it would be better to patch SchemaPlus than Scenic — SchemaPlus is the thing that's changing how ActiveRecord behaves, so you'd need to patch n gems that depend on ActiveRecord behaving a certain way as opposed to patching SchemaPlus once.

[boazy]

Unfortunately it won't, but for a good reason. SchemaPlus::Core parses the entire rails dump (header, tables, indexes, trailer) into SchemaDump and only when the state is complete and all other SchemaPlus gems have finished hooking into the middleware stack and modifying SchemaDump, it dumps everything at once. Changing that logic would not be simple, would possibly require modifying some other SchemaPlus gems and would eliminate what is essentially a useful feature.

There is a cleaner solution though: provide our own stream to super on SchemaDumper#dump and track writes to that stream that were made by other gems hooking into SchemaDumper. We can then tell SchemaDump#new (which generates the actual dump) to insert everything that was added by other gems in its proper place. In Scenic's case, that would be inserting Scenic's views after the tables. An even cleaner solution is to move all parsing logic to the stream object, and let it keep everything it doesn't recognize as is, so extensions by other gems will be left untouched, in their proper position.