Rails 4.2 support

[ronen]

Now that rails 4.2 is out, get the final kinks out of the 4.2 branch.

Opening this issue since PRs #175 and #186 with discussion of 4.2 development are closed.

[ronen]

@donv, @lowjoel, @dimonzozo, @methodmissing, @tovodeverett, and anybody else out there in githubland :)

Question: What's do you think's the best path to getting 4.2 support going?

I can think of three options:

• [A]. Business as usual. Continue down the path that @methodmissing and @dimonzozo did in #175 and #186. It seems like it's getting pretty close: only 5 spec failures for mysql2 and 6 for postgresql, though sqlite3 doesn't work at all (see https://travis-ci.org/SchemaPlus/schema_plus/builds/43763732). This might be the most expedient and least disruptive.
• [B]. Rails 4 cleanup, as discussed in #186: Bump to version 2.0, dropping support for AR < 4.2, and ripping out things that are no longer needed because AR now has them natively. This seems like it'd be a bit more work than [A], but would result in a cleaner gem. Or would it be less work since there'd be no need to worry about backwards-compatibility?
• [C]. Do it as part of schema_plus refactor as per #168. This would of course be the most work. But promises to result in the cleanest gem (or rather family of gems).

Part of me wants to become draconic and say "let's stop committing anything until we do [C]" otherwise it might never happen. But that might take a while, stalling 4.2 support. So maybe [A] or [B] is a better choice? (and [C] might never happen...)

Any thoughts/suggestions? A big consideration of course is what any of you would like to do... since we're all doing this in our spare time, most progress will be made be somebody who gets motivated to dive in.

[donv]

Hi all!

For us option [B] sounds the best with a 1.x branch that supports AR <=4.1 for bug fixes.

I fully support option [C] for longer term, and option [A] if it is quicker.

I can contribute with testing for JRuby and ARJDBC.

[tovodeverett]

I'm still out here, but I have a lot less time for Ruby these days - I got a paying job and 98% of the coding I do for that is in T-SQL. That said, I'm still using schema_plus for a personal web application, so I do have an interest.

I suspect option B is easier in the short term (since we don't have to support multiple versions of Rails from a single code-base and can let them diverge). That said, the foreign key support built into 4.2 is not as full-featured as that in schema_plus (no support for deferrable, for instance), so the question becomes how do we maintain everything that schema_plus supports while letting AR take over the core responsibility, and that basically means monkey patching.

I think a full-blown refactor in the model of option C is likely to prove really difficult, and given the limited time available for all of us, I don't know if it's a good approach. The problem with option A is that it requires a lot more commitment from a development standpoint to support testing. The advantage of option B is that it allows us to clean up the code in each release to focus on that specific version of Rails. The downside is that as additional functionality moves into Rails, we may find that we have to repeat option B (i.e. what happens if Rails adds support for deferrable in 4.2.1).

Personally, I'd like to see as much functionality as possible move into Rails, even if it's not as pretty, so long as I don't lose functionality along the way.

[ronen]

@donv, @tovodeverett - thanks for the feedback.

I started going down the option [B] path -- I made a branch off the 4.2 branch and started ripping out any conditionals for Rails < 4.2. But I quickly found that it would be a whole lot easier to do that if I were starting with a baseline where rails 4.2 passes the specs.

So I think the best approach is to do option [A] for now, until 4.2 passes the tests; especially since it's so close. In fact, today I got 4.2 passing all tests for MRI & postgresql!

So once [A] is done then, switch to option [B] for future development. I.e. as @donv said make a 1.x branch for posterity/bug fixes, and switch master to 2.0, ripping out support for the older versions. From there can, in a leisurly manner, look into as @tovodeverett said letting AR take over the core reponsibility for the features that 4.2 supports.

and that basically means monkey patching.

Yeah. I'd be very excited if that monkey patching were bundled in a schema_monkey gem, which would typically do a monkey patch to include ActiveSupport::Callbacks. Then the main gem would register the callback.

Personally, I'd like to see as much functionality as possible move into Rails, even if it's not as pretty, so long as I don't lose functionality along the way.

Yeah again. I'd be thrilled if schema_plus would wither away to nothing. I'm only doing it because I want to have the features, not because I want to spend the time maintaining it :)

It could be that we'll quickly want or need to bump to 3.0 or beyond if we start dropping some schema_plus things that are now available in AR 4.2 albeit with different syntax.

I have a lot less time for Ruby these days - I got a paying job ... I'm still using schema_plus for a personal web application

Likewise. I've got a few paying jobs that don't currently use RoR, and one personal project that occupies what little spare time I have, which does use RoR. And so I have very little spare time for schema_plus.

[ronen]

OK, [A] is done -- I pushed through today and fixed the remaining problems; 4.2 now works. I've cut a new release 1.8.0 so people can start using it.

@donv I didn't attempt to get jruby working for 4.2, I'll leave that to you :) You can see the last build I did that included it: https://travis-ci.org/SchemaPlus/schema_plus/jobs/45496952 -- it looks like only a handful of errors (well, a lot of failures but a bunch of them seem like they're probably the same issue).

I do still believe that migrating to [B] and eventually to [C] is the way to go. Probably it would make sense to start [B] right now. I.e. make the 1.x branch and switch master to 2.0.0.rc0, dropping AR < 4.2 from the tests. Then gradually rip out the unneeded code. But it's past my bedtime...

Thanks again everyone for your contributions, past present and maybe future. And especially @methodmissing and @dimonzozo for getting the 4.2 momentum going.

[donv]

On 2014-12-31, at 00:16, ronen barzel notifications@github.com wrote:

OK, [A] is done -- I pushed through today and fixed the remaining problems; 4.2 now works. I've cut a new release 1.8.0 so people can start using it.

Woohoo! Well done! I’ll try it out immediately! Great job!

@donv I didn't attempt to get jruby working for 4.2, I'll leave that to you :) You can see the last build I did that included it: https://travis-ci.org/SchemaPlus/schema_plus/jobs/45496952 -- it looks like only a handful of errors (well, a lot of failures but a bunch of them seem like they're probably the same issue).

I’ll start right now.

I do still believe that migrating to [B] and eventually to [C] is the way to go. Probably it would make sense to start [B] right now. I.e. make the 1.x branch and switch master to 2.0.0.rc0, dropping AR < 4.2 from the tests. Then gradually rip out the unneeded code. But it's past my bedtime…

Sleep well :)

Uwe Kubosch uwe@kubosch.no http://kubosch.no/

[tovodeverett]

First off, let me second the woohoo! I've started trying it out, and everything seems to be working, but I've got one observation.

What I've done so far is update my app to schema_plus 1.8.0 and rails 4.2. I then did a db:schema:dump and noticed something. It seems like it dumps out a bunch of add_foreign_key statements at the top of schema.rb, before it even starts creating tables. I tried disabling schema_plus and then doing a dump, and when I do that all of the add_foreign_key statements are at the very end of schema.rb.

When schema_plus is enabled, the add_foreign_key statements are more "robust" (they have :on_update and :on_delete parameters), but they duplicate the t.foreign_key calls in the actual tables. Also, the t.foreign_key calls have the :deferrable parameter, whereas the add_foreign_key calls don't.

Thoughts?

[ronen]

Thoughts?

Sigh. That's what i get for trusting the specs and not doing real testing.

I guess in true TDD form, I'd say need to add a few more cases to schema_dumper_specs to check that the foreign_key statements generated exactly once, with the right args.

And for 4.2, it should dump t.foreign_key rather than add_foreign_key.

[ronen]

And for 4.2, it should dump t.foreign_key rather than add_foreign_key.

Oh I just took a quick look -- I forgot, schema_plus already dumps t.foreign_key, it only dumps add_foreign_key when there's a cyclic dependency; and those should be coming later on rather than at the top. If all those are coming out at the top, there's probably some bad interaction between schema_plus's dependency analysis and whatever AR 4.2 must be doing likewise.

[tovodeverett]

If I comment out those add_foreign_key statements, the schema loads properly, and it dumps out something that matches (albeit before the add_foreign_key statements were commented out).

Me, I'm just excited I managed to get the RSpec nullipotent_tests hack I wrote for 2.x working with 3.x (https://github.com/rspec/rspec-core/issues/756)!

[ronen]

turned out to be an easy fix. whew.

in case you care: schema_plus hijacks AR's dumper, capturing it output and mucking with it before writing it. AR 4.2's native dumper handles foreign keys by waiting until the end and dumping all constraints using add_foreign_key (this wouldn't work with sqlite3 AFAIK). But schema_plus's hijacking didn't capture that, so AR's add_foreign_key statements ended up at the top of the output. All I needed to do was suppress AR 4.2's dumping of foreign keys, since schema_plus handles them (inline using t.foreign_key, which aside from being cleaner also will work with sqlite3. plus supporting :on_update etc.).

just waiting for travis to report the all-clear and i'll release 1.8.1

[ronen]

OK, 1.8.1 is released. Hopefully you won't find any other new problems :)

Me, I'm just excited I managed to get the RSpec nullipotent_tests hack I wrote for 2.x working with 3.x (rspec/rspec-core#756)!

Cute! Have you seen https://github.com/jimweirich/rspec-given ? Lets you use Given/When/Then structure for your tests; and in that context has an "And" clause to chain together nullipotent tests that share a setup

[ronen]

I've gone ahead and made the 1.x branch and switched master to 2.0.0.alpha (not released).

In master I removed all conditional for AR < 4.2, and all vestigial code as determined by code coverage. (Actually there are still a few lines that aren't covered, but I wasn't certain whether that's just because the tests are inadequate or because they're really not needed with AR 4.2. Haven't investigated further.)

@donv I also removed the conditionals for jruby, since I wasn't sure whether the conditional code would be needed for AR 4.2 and figured best to start clean. Those were in a5ae8188b6e7063929971e925612d3432364e76f and 5ffa52987df5d72bccd593e0305dd420b2599898 if you want to revert them. Also I'd probably recommend getting jruby/AR4.2 working in 1.x branch first (since it was reasonably close to working), and we can push out a 1.9 with it. Then cherry-pick the jruby/AR4.2-specific stuff over to master for 2.0

Next step is to investigate eliminating bits of the code that are now redundant with AR4.2 core capability, and monkey patch instead. Dunno if/when I'll get a chance to dive into that. Anybody else who wants to, please step up!

And again ideally while at it gradually migrate over to [C], with monkey patching isolated in its own gem providing a clean extension API, allowing schema_plus to be refactored into separate smaller feature gems.

[tovodeverett]

That does the trick! I'll have to think about whether I think B is better than A given that implementing B is possibly more difficult. You would have to extend Rails 4.2's native support for foreign keys (i.e. to support deferrable, for instance), and that might mean more difficult than simply supplanting it.

I hadn't seen rspec-given, but one difference is that in my hacked up nullipotent stuff, each it { } clause continues to report separately. The downside is that you get two additional test reports from each nullipotent block (one for the setup and one for the teardown, to ensure that any failures in those blocks get reported properly).

[ronen]

@tovodeverett

whether I think B is better than A given that implementing B is possibly more difficult. You would have to extend Rails 4.2's native support for foreign keys (i.e. to support deferrable, for instance), and that might mean more difficult than simply supplanting it.

Yeah, could go either way. I haven't looked yet at what's under the hood in AR 4.2. schema_plus currently does a mix of both; things are extended/monkey patched when possible, but some things--most notabily creating/looking up indexes in postgresql--are supplanted. That supplanting does bother me though. AR may have added new features to postgreql indexes that schema_plus didn't, may need to go back merge those manually into our own copy.

But if nothing else, at least it's nice to have a clean branch that doesn't need to keep supporting old code.

I hadn't seen rspec-given, but one difference is that in my hacked up nullipotent stuff, each it { } clause continues to report separately.

True, rspec-given's Ands don't report properly.

[donv]

Getting JRuby to work with ActiveRecord 4.2 is a much bigger job than I thought. It will take a while, and could be tracked in a separate issue.

[ronen]

@donv

Getting JRuby to work with ActiveRecord 4.2 is a much bigger job than I thought.

Bummer, I hoped it was already pretty close in the 1.x branch (given the work done by @bacrossland in #173).

could be tracked in a separate issue.

sure, go ahead and open one!

[bacrossland]

That is a bummer. When last I dove into the 1.x branch, the tests for JRuby only failed on AR 4.0. AR 4.1 passed as did AR 3.2. I had changed nothing in my gem setup just switching between AR versions. Digging into that it turns out that the failure is due to changes in AR 4.0 that were fixed in AR 4.1. So the 1.x branch with the #173 fix supported JRuby in all AR versions except 4.0.

@donv I'm interested in seeing that issue opened and hearing about what you've already discovered.

[ronen]

In case any of you are interested, I had a few spare moments today so I cleaned up the master branch more. it's now at 100% code coverage.

[donv]

Great!

@ronen @bacrossland I started looking at getting ARJDBC working with AR4.2 without schema_plus, and I found and fixed some things, but it is a big gem to wade through, and I got discouraged and dropped it. I will pick it up at some point, I am sure, but for now I am working on other projects.

The issue to track ARJDBC+AR42 support is here: https://github.com/jruby/activerecord-jdbc-adapter/issues/599

Would be nice to make progress there.

[ronen]

started [C]. see the refactor branch

[donv]

Woohoo! Great!

[bacrossland]

Nice!

[ronen]

Closing this out to move discussion to #197. Feedback welcome!