[WIP] Proper AR5 Support

[boazy]

PostgreSQL tests still fails.

The main issue is that the old connection.views used to filter out internal Postgres views ("pg_*"), and AR5 is not doing that.

We can easily monkey patch that to conform, but seeing the equivalent behavior of connection.tables, I think we should just add user_views_only (just as we have user_tables_only) to schema_plus_compatibility.

Besides that, there's another breaking change I didn't realize we're going to have before: SchemaPlus::Views patches connection.tables (using middleware) to filter out views. This would be the normal behavior of table in the future, but right now we have connection.tables_only for that, so I rather keep in line with ActiveRecord here.

@ronen, what do you think?

[coveralls]

Coverage remained the same at 100.0% when pulling 5245c35 on AR5.0-support into 792351d on master.

[coveralls]

Coverage remained the same at 100.0% when pulling 5245c35 on AR5.0-support into 792351d on master.

[boazy]

What's the deal with comparing the schemaname and ignoring PostGIS?
I'm not really familiar with PostGIS, but I guess it does something strange here?
@ronen do you have any suggestions what to do with that?

[ronen]

Here's the thread and PR that introduced it: SchemaPlus/schema_plus#94. I guess it's analogous to suppressing the pr_* tables. I'm not familiar with PostGIS either but over the years we've had occasional people raise issues/submit PRs to deal with it. Possibly this too could be maintained via middleware if it's still an issue?

[ronen]

I think schema_plus_compatibility should remain only for the purpose of helping to develop gems, not to provide a user-level API (you yourself didn't even want to document it!)

One of the features of SchemaPlus has long been to normalize inconsistent/inconvenient behavior of AR to make it more usable. So I think it's OK to continue doing that. In particular:

filter out internal Postgres views ("pg_*")

That's a documented feature of schema_plus_views, and I think we should keep it: The reason for supporting create_view, views etc is to make it easy to for AR users to work with views, and exposing postgres's internal views would just make that harder without providing any benefit.

(Given the existence of AR5's views now it could be done by middleware instead of in the adapter)

connection.tables (using middleware) to filter out views. This would be the normal behavior of table in the future, but right now we have connection.tables_only for that, so I rather keep in line with ActiveRecord here.

Here too, this is for usability. If somebody is including schema_plus_views, that means they work with views and expect to have views in their db. So it makes sense to fix tables to ensure it doesn't return views . And especially since AR5 is going to do that anyway, we may as well continue to support it now and let AR5 catch up. (And we don't want to encourage people to use tables_only from schema_plus_compatibility since that method makes little sense from a user perspective)

The only time I really want to keep in line with ActiveRecord is when AR introduces new features that "catch up" to features SchemaPlus was already supporting, but uses different syntax; then we should wean our users off SchemaPlus and onto the native AR functionality, by providing shims and deprecating them, and eventually removing them. I'd be happy for SchemaPlus to one day wither away entirely if AR does everything!

Speaking of which, what's up with view_definition? Does AR5 have an analogous method?

[boazy]

That's a documented feature of schema_plus_views, and I think we should keep it: The reason for supporting create_view, views etc is to make it easy to for AR users to work with views, and exposing postgres's internal views would just make that harder without providing any benefit.

I see. But shouldn't we filter out internal tables as well then? Well, to tell the truth, DB-internal tables (such as the SCHEMA table) are already filtered, it's the new ar_internal_metadata table that isn't. So we could say hiding DB level views is consistent with the way tables work.

(Given the existence of AR5's views now it could be done by middleware instead of in the adapter)
But we already override the adapter to insert the middleware hook. Or do you want to make this optional?
Or perhaps we should add the middleware hook in schema_plus_core now. But filtering through middleware means we don't use SQL to filter anymore.

Here too, this is for usability. If somebody is including schema_plus_views, that means they work with views and expect to have views in their db. So it makes sense to fix tables to ensure it doesn't return views . And especially since AR5 is going to do that anyway, we may as well continue to support it now and let AR5 catch up. (And we don't want to encourage people to use tables_only from schema_plus_compatibility since that method makes little sense from a user perspective)

Right now there's no Tables middleware, since it's implementation (and entire signature) changed, but perhaps there could be a flag to re-enable it, which would also force tables to use AR 5.1(?) behavior and show only tables.

Or we can go the simpler way and just override tables with schema_monkey, but when AR 5.1 comes out it would make sense to reintroduce the middleware anyway.

[boazy]

The only time I really want to keep in line with ActiveRecord is when AR introduces new features that "catch up" to features SchemaPlus was already supporting, but uses different syntax; then we should wean our users off SchemaPlus and onto the native AR functionality, by providing shims and deprecating them, and eventually removing them. I'd be happy for SchemaPlus to one day wither away entirely if AR does everything!

That would be a wonderful day indeed. :D

Speaking of which, what's up with view_definition? Does AR5 have an analogous method?

No, no such luck. AR5 provides no support of views besides listing them separately from tables.

[ronen]

Hi, sorry for the delay.

But shouldn't we filter out internal tables as well then? Well, to tell the truth, DB-internal tables (such as the SCHEMA table) are already filtered, it's the new ar_internal_metadata table that isn't. So we could say hiding DB level views is consistent with the way tables work.

Right now there's no Tables middleware, since it's implementation (and entire signature) changed, but perhaps there could be a flag to re-enable it, which would also force tables to use AR 5.1(?) behavior and show only tables.

Yes, you're right...

I think the way to go is to put that functionality in schema_plus_tables (which right now does very little). I guess in 5.0 it would provide the Tables middleware, and in 5.1 the Middleware could go back to schema_plus_core.

Then other gems could have a dependency on schema_plus_tables as needed.

(I think we'd still want to keep the table helpers in schema_plus_compatibility though, for lower-level testing.)

[boazy]

Sorry for the delay on my side this time. :)
Yes, that sounds consistent with the way schema_plus has been going until now: don't change view behavior until the user requires "schema_plus_views" and don't change table behavior until the user requires "schema_plus_tables".

schema_plus_core would introduce the Views middleware, without filtering or modifying the views and schema_plus_views would hook into the Views and DataSources middlewares and filter the views.

In schema_plus_tables, I'll basically throw away everything it's doing now (since the cascade option has been deprecated for years now, and if_exists is already supported by AR5) and hook into the data_sources middleware to filter the views.

Since Postgres needs SQL to do the filtering (it's not enough to just filter out specific names in the middleware later) we'll set a flag on the middleware env in schema_plus_core that enables this filtering and the middleware hooks would just enable that flag.

users_views_only and users_tables_only will remain in schema_plus_compatibility and could be used by anyone.

I agree that is a good idea to provide the Tables middleware in schema_plus_tables now, until AR5.1 to avoid compatibility mess.

[ronen]

Sorry for the delay on my side this time. :)

Between, us, it's shambling forward slowly... But let me again take the opportunity to repeat how much I appreciate your doing all this! No way I'd be able to do it myself.

Yes, that sounds consistent with the way schema_plus has been going until now: don't change view behavior until the user requires "schema_plus_views" and don't change table behavior until the user requires "schema_plus_tables".

schema_plus_core would introduce the Views middleware, without filtering or modifying the views and schema_plus_views would hook into the Views and DataSources middlewares and filter the views.

In schema_plus_tables, I'll basically throw away everything it's doing now (since the cascade option has been deprecated for years now, and if_exists is already supported by AR5) and hook into the data_sources middleware to filter the views.

users_views_only and users_tables_only will remain in schema_plus_compatibility and could be used by anyone.

I agree that is a good idea to provide the Tables middleware in schema_plus_tables now, until AR5.1 to avoid compatibility mess.

All sounds good

Since Postgres needs SQL to do the filtering (it's not enough to just filter out specific names in the middleware later) we'll set a flag on the middleware env in schema_plus_core that enables this filtering and the middleware hooks would just enable that flag.

This is the only thing I'm wary of. I think schema_plus_core shouldn't be doing any SQL or implementation of features for other gems. Instead the middleware for Postgresql can define the #implement method and write the base method that replaces AR's SQL to do the filtering. Or maybe you can do it by running extra SQL in an #after method to determine what to filter out at the cost of having two queries instead of one. Though maybe the results of that can be cached since internal tables don't change?

[boazy]

Between, us, it's shambling forward slowly... But let me again take the opportunity to repeat how much I appreciate your doing all this! No way I'd be able to do it myself.

Thanks. I'm really appreciate your help reviewing my ideas and making sure we get the best implementation.

This is the only thing I'm wary of. I think schema_plus_core shouldn't be doing any SQL or implementation of features for other gems. Instead the middleware for Postgresql can define the #implement method and write the base method that replaces AR's SQL to do the filtering. Or maybe you can do it by running extra SQL in an #after method to determine what to filter out at the cost of having two queries instead of one. Though maybe the results of that can be cached since internal tables don't change?

I actually wasn't aware that #implement will use the last middleware in the stack. That seems like the best option for me.

[boazy]

I actually have an idea I like more now:

Add a new value to env called env.extra_sql or env.sql_where_constraints or something along these lines to the DataSources and Views middleware (as well as the Tables middleware in SP::Tables). SP::Core would use the normal AR implementation if this value is empty, but would add extra SQL WHERE constraints (which can be chained by middleware) otherwise.

This means SP::Core won't modify any behavior by default unless at least one of the middlewares tell it to do so, but middlewares can still be chained together without overriding each other.

[boazy]

Started work here:
https://github.com/SchemaPlus/schema_plus_compatibility/tree/user_views_only-dev

Next we need middleware support for filtering in DataSources Views (in SP::Core) and Tables (SP::Tables, will be moved to SP::Core later).

[boazy]

@ronen The pull requests now passes all tests (with gemfile.local :))
I'm still not sure about whether there should be yet another gem for the SQL constraints.

[boazy]

@ronen Can you give me permission to add new repositories to the schema_plus project?
In addition, what we should do about gem releases? We can automate that from Travis CI.

[ckornaros]

Hi, @ronen and @boazy will you release soon AR5 Support ? Can I help in anything about this issue ?

[MattFenelon]

Has development of AR5 support stalled?

[boazy]

A little bit :(
I'll try to push the PR through this week.

[ronen]

@boazy let me know if there's something else i should look at or can help with. (given my still limited time :( )

[boazy]

@ronen I just wish I have more time myself. I didn't have time to update SP::Core and SP::Tables with the new middleware.

Since Rails master is already fully differentiating data_sources, views and tables, I think we can just put separate middleware stacks for Tables, Views and DataSources directly there. If anyone relied on SP::Core 1.x Tables middleware directly (highly unlikely) they would have migrated by now, and we've released a new major version anyway.

I still need to create a separate gem, probably called schema_plus_sql_constraints or just schema_plus_sql that will let Tables/Views/DataSources Middleware add extra SQL constraints to the native queries. It can also have this functionality for TableExists and other middleware.

[joxxoxo]

any news here?

[ronen]

@joxxoxo Sorry, my status re all of schema_plus remains unfortunately that I'm not currently using rails in any of my day jobs, and unfortunately I don't have enough spare time to do any serious work with it. So best I'm able to do is (often belatedly) view and merge PRs, and occasionally offer some advice.

I'd be thrilled if you (or anybody) were able to take a more active role.

[ronen]

@boazy how goes... still interested in this? my availability status hasn't changed. at this point i'd say we can skip AR 5.0 and 5.1 and switch directly to 5.2 :)

[boazy]

@ronen Wow, I'll have to come back and check in again.
Unfortunately my own availability status is only getting worse every year. I never worked on RoR on any day job, but I really like the super-DRY ORM you can get with SchemaPlus. I wish I had more time to maintain this. I'll try to get back to fixing this PR if I get some time. This time directly to be compatible with 5.2 of course.

[ronen]

If you can that'd be great. Seems like there'll be others out there appreciative of the work! (#18)

[ronen]

BTW schema_plus_core is now on AR5.2, so that's not something to have to worry about.

[georf]

Do you have a plan to merge this into master?

[georf]

I got the same failure as in the tests when I use this branch:

SchemaMonkey::MiddlewareError: SchemaPlus::Views::Middleware::Schema::Postgresql::Views: No stack SchemaMonkey::Middleware::Schema::Views
/home/georf/.rvm/gems/ruby-2.5.5@m3/gems/schema_monkey-2.1.5/lib/schema_monkey/stack.rb:9:in `rescue in insert'
/home/georf/.rvm/gems/ruby-2.5.5@m3/gems/schema_monkey-2.1.5/lib/schema_monkey/stack.rb:3:in `insert'
/home/georf/.rvm/gems/ruby-2.5.5@m3/gems/schema_monkey-2.1.5/lib/schema_monkey/client.rb:29:in `block in insert_middleware'
/home/georf/.rvm/gems/ruby-2.5.5@m3/gems/schema_monkey-2.1.5/lib/schema_monkey/client.rb:26:in `each'
/home/georf/.rvm/gems/ruby-2.5.5@m3/gems/schema_monkey-2.1.5/lib/schema_monkey/client.rb:26:in `insert_middleware'
/home/georf/.rvm/gems/ruby-2.5.5@m3/gems/schema_monkey-2.1.5/lib/schema_monkey/client.rb:11:in `insert'
/home/georf/.rvm/gems/ruby-2.5.5@m3/gems/schema_monkey-2.1.5/lib/schema_monkey/monkey.rb:24:in `block in insert'
/home/georf/.rvm/gems/ruby-2.5.5@m3/gems/schema_monkey-2.1.5/lib/schema_monkey/monkey.rb:24:in `each'
/home/georf/.rvm/gems/ruby-2.5.5@m3/gems/schema_monkey-2.1.5/lib/schema_monkey/monkey.rb:24:in `insert'
/home/georf/.rvm/gems/ruby-2.5.5@m3/gems/schema_monkey-2.1.5/lib/schema_monkey.rb:33:in `insert'
/home/georf/.rvm/gems/ruby-2.5.5@m3/gems/schema_monkey-2.1.5/lib/schema_monkey/active_record.rb:11:in `initialize'
/home/georf/.rvm/gems/ruby-2.5.5@m3/gems/activerecord-5.0.7.2/lib/active_record/connection_adapters/postgresql_adapter.rb:209:in `initialize'

Do I need a special configuration to use the postgres adapter?

[urkle]

Closing in favor of #21