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

Support Rails 5 #9

Closed
ronen opened this issue Mar 5, 2016 · 12 comments
Closed

Support Rails 5 #9

ronen opened this issue Mar 5, 2016 · 12 comments

Comments

@ronen
Copy link
Member

ronen commented Mar 5, 2016

Creating the issue...

...if anybody out there in github-land is interested in working on Rails 5 support for SchemaPlus, please let me know. I myself am a bit swamped these days so would welcome a collaborator stepping in.
(I haven't even looked into whether this would be hard or trivial.)

Thanks!

@ronen
Copy link
Member Author

ronen commented Apr 19, 2016

Latest: I've made a branch "activerecord-5.0", which includes AR 5.0 in the test matrix, but aside from that haven't changed anything. And, unsurprisingly, it fails. See https://travis-ci.org/SchemaPlus/schema_plus_core/builds/124089884

@ronen
Copy link
Member Author

ronen commented Apr 19, 2016

...and again, if there's anybody out there who has the time to look into this, that'd be great. I'd be happy to help out of course.

@md5
Copy link
Contributor

md5 commented Jun 8, 2016

I've been looking in to using this with AR 5 and I've run into the following problems so far with a Postgres database:

  1. The internal prepare_column_options method no longer takes a second parameter for types (cf. no need to pass native_database_types around rails/rails#17576)
  2. The AR 5 dumper will now include indexes in the create_table statement with t.index if the adapter supports it (cf. Dump indexes in create_table instead of add_index rails/rails#19994 and Dump indexes in create_table for generates SQL in one query rails/rails#23557). This breaks the column dumping code here: https://github.com/SchemaPlus/schema_plus_core/blob/970bf76/lib/schema_plus/core/active_record/schema_dumper.rb#L70-L78

The first one is easy to fix by changing the signature of prepare_column_options from prepare_column_options(column, types) to prepare_column_options(column, *) to accommodate both AR 4 and AR 5. I can provide a PR for this change if you're interested.

The second one will require some additional coding to have a special case for t.index and handle it appropriately. This is going to be a bit more work to get right and potentially error prone.

@md5
Copy link
Contributor

md5 commented Jun 8, 2016

I've opened #12 for the first issue with prepare_column_options

@ronen
Copy link
Member Author

ronen commented Jun 9, 2016

@md5 Thanks -- you're a hero!

@md5
Copy link
Contributor

md5 commented Jun 9, 2016

As discussed above and briefly in #12 and in my earlier comment, AR5 now has an indexes_in_create method in ActiveRecord::SchemaDumper that adds each index inside the create_table statement as t.index .... It still has the indexes method as well, but that's only used for indexes that are not on normal tables (the comment in the source mentions materialized views).

I'm not sure what the right fix is for this, but this seems as good a place to discuss as any.

@ronen
Copy link
Member Author

ronen commented Jun 10, 2016

I took a look at indexes_in_create (activerecord/lib/active_record/schema_dumper.rb#L207).

I think the thing to do would be to provide an override similar to the override of indexes() at https://github.com/SchemaPlus/schema_plus_core/blob/970bf76/lib/schema_plus/core/active_record/schema_dumper.rb#L84. I think the override of indexes_in_create could just about be identical, except for tweaking the regex to match t.table syntax. Probably the env and Index objects should each get an extra boolean flag to indicate that the view was dumped inline.

Then schema_plus_core's (re)dumper at

statements.each do |statement|
could dump any indexes flagged as inline using t.index, and the code at would only dump those that are not flagged as inline.

Does that all seems sensible?

Speaking of materialized views, there's an outstanding issue #6 which should probably be dealt with; and we should see what happens with them and their indexes in schema_plus_core + AR 5.

@md5
Copy link
Contributor

md5 commented Jun 11, 2016

Yes, that seems sensible.

@md5
Copy link
Contributor

md5 commented Jun 11, 2016

BTW, the problem in #6 doesn't seem to be related to materialized views, but rather to query-defined tables (I'm not sure what they're officially called). The materialized view implementations I'm familiar with use CREATE MATERIALIZED VIEW, not CREATE TABLE.

@md5
Copy link
Contributor

md5 commented Jun 22, 2016

@ronen Just wanted to follow up on this and let you know that I haven't had time to look into this further. For the time being, I'm just using execute() to manipulate my enum types directly.

@rhymes
Copy link

rhymes commented Aug 31, 2016

Any news? I saw that #14 has been merged in schema_plus_core

@boazy
Copy link
Member

boazy commented Sep 13, 2016

@rhymes
schema_plus_core fully supports AR5 on the latest version, but not all other schema_plus gems have been updated to support it yet. Unfortunately, AR5 introduced many incompatible changes, so in most cases just changing the dependency version numbers is not enough.

This issue tracks all the currently updated gems:
SchemaPlus/schema_plus#229

@boazy boazy closed this as completed Sep 13, 2016
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

No branches or pull requests

4 participants