H2 support #109

Merged
merged 3 commits into from Mar 7, 2013

Conversation

Projects
None yet
2 participants
Contributor

maxsz commented Feb 27, 2013

This pull request adds support for the h2 database (http://www.h2database.com/) with JRuby. Furthermore it improves compatibility with other active record drivers as it replaces change_column_null, which isn't officially part of the API, with the corresponding change_column call.

Owner

ileitch commented Feb 27, 2013

Did you see the build failures under 1.8?

@ileitch ileitch commented on an outdated diff Feb 27, 2013

lib/rapns/daemon/database_reconnectable.rb
@@ -1,4 +1,7 @@
class PGError < StandardError; end if !defined?(PGError)
+if not defined?(SQLite3).nil?
@ileitch

ileitch Feb 27, 2013

Owner

Why is SQLite3 being referenced? Is H2 based on SQLite?

Owner

ileitch commented Feb 27, 2013

What do you mean by change_column_null not being official API? Does the H2 adapter not implement it?

All core AR adapters implement change_column_null, also notice that change_column is not implemented on AbstractAdapter, it is implemented on db specific adapters just as change_column_null.

I'm nervous about switching to change_column as it has the potential to change unintended attributes.

Contributor

maxsz commented Feb 27, 2013

Nope, I don't have any build failures on 1.8, can you be more specific?

change_column_null isn't documented API (see http://api.rubyonrails.org/classes/ActiveRecord/Migration.html). The H2 adapter does (correctly) implement it as a private method.

Owner

ileitch commented Feb 27, 2013

See the Travis CI build: https://travis-ci.org/ileitch/rapns/builds/5100539

I think that doc is incomplete. See the auto generated docs here: http://api.rubyonrails.org/classes/ActiveRecord/ConnectionAdapters/AbstractMysqlAdapter.html#method-i-change_column_null

It sounds like the H2 adapter has marked it as private by mistake. Other JDBC adapters make it public. change_column also doesn't delegate to change_column_null so it'd never get used as a private method - what else would call it other than user migrations?

Either way, I'd like to stick with change_column_null to be on the safe side. Can you monkey-patch the H2 adapter to make change_column_null public?

Contributor

maxsz commented Feb 27, 2013

Sorry, I cannot reproduce these build failures, I also can't see why this would be caused by this code change. The output reads:

442 Finished in 2 minutes 43.02 seconds
443 315 examples, 0 failures
444 invalid option: --backtrace
445 Test::Unit automatic runner.
446 Usage: /home/travis/.rvm/gems/jruby-1.7.2-d18/bin/rspec [options] [-- untouched arguments]

Maybe you can give me a hint to why this might happen. Looks as if there's some configuration error/incompatibility with the --backtrace option (which I didn't add here).

I know that the Mysql adapter implements the change_column_null method and any auto generated doc would include this method, but it's still not documented and not part of the active record API (see the abstract adapter http://api.rubyonrails.org/classes/ActiveRecord/ConnectionAdapters/AbstractAdapter.html and the included schema statements http://api.rubyonrails.org/classes/ActiveRecord/ConnectionAdapters/SchemaStatements.html#method-i-change_column). At first I considered patching the H2 driver, but after inspecting the active record documentation, I came to the conclusion that using documented API is preferred over monkey-patching a (correctly implemented) driver. (Oh, and the H2 change_column method delegates to change_column_null, I guess that's why it's still included although being private. Sorry for the confusion on this one.)

Owner

ileitch commented Mar 4, 2013

The error does indeed appear unrelated to your changes and is a Travis issue,

Can you address my question about SQLite3 and then i'll be happy to merge this.

Contributor

maxsz commented Mar 4, 2013

I removed the SQLite3 commit, as I did not intend to include it anyway.

@ileitch ileitch added a commit that referenced this pull request Mar 7, 2013

@ileitch ileitch Merge pull request #109 from equinux/h2_support
H2 support
65c479c

@ileitch ileitch merged commit 65c479c into ileitch:master Mar 7, 2013

1 check failed

default The Travis build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment