Arjdbc greater 1.3.12 compatibility. Fix #214

[lfidnl]

Here is a fix for compatibility with > activerecord-jdbc-1.3.12. I hope this solution is normal, because I can't find more elegant way

[fj]

This:

• seems like it's likely to break a lot of things
• doesn't have any tests
• blows up if ArJdbc::VERSION is defined but the regular expression doesn't match:

module ArJdbc; VERSION = '1.124.456'; end
# => "1.124.456"

# kablammo!
ArJdbc::VERSION.match(/1.3.(\d*)/)[1].to_i > 12
# => NoMethodError: undefined method `[]' for nil:NilClass

• matches on ArJdbc::VERSIONs that it shouldn't match on, and is not strict enough:

# we don't want this to match...
module ArJdbc; VERSION = '1.1111111111111113.456'; end
# => "1.1111111111111113.456"

# but it does, because '.' in regexps matches any single character, not a literal '.'
ArJdbc::VERSION.match(/1.3.(\d*)/)[1].to_i > 12
# => true

I'll defer to @ronen here but this definitely needs tests and a cleaner approach, imo. Are there other examples that could be used as a reference for integrating?

[fj]

BTW, generally, instead of testing for versions (which is brittle), we want to be testing for capabilities or a distinguishing feature of some kind -- is there something distinguishing in #214 that could be used as an identifier?

For example, if the solution to #214 is:

The solution is to include SchemaPlus::ActiveRecord::ConnectionAdapters::PostgreSQLColumn after Arjdbc::ArJdbc::PostgreSQL

then could we just have an adapter that tests for the presence of ArJdbc and if it's not there, loads it, and then loads SchemaPlus's connection adapter?

Also, would a temporary fix be to just do this in the downstream application (that is, require 'arjdbc' before you require 'schema_plus')?

[lfidnl]

@fj, thanks for reply

This seems like it's likely to break a lot of things

It can break any functionality if only PostgreSQL class will have any peace of code.
Now there is no code except include ::ArJdbc::PostgreSQL::Column. I know the solution is not ideal, but I think it can be temporary

This doesn't have any tests

Sorry, I forgot about them

Is there something distinguishing in #214 that could be used as an identifier?

You are right, may be we should check ArJdbc::PostgreSQL::Column was included in ActiveRecord::ConnectionAdapters::PostgreSQLColumn,
instead of checking ArJdbc::VERSION

Could we just have an adapter that tests for the presence of ArJdbc and if it's not there, loads it, and then loads SchemaPlus's connection adapter?

In #214 I mean than we should include SchemaPlus after ArJdbc ancestors:

# Works (with fix)
[ActiveRecord::ConnectionAdapters::PostgreSQLColumn, ArJdbc::PostgreSQL::ColumnHelpers, ArJdbc::PostgreSQL::Column, SchemaPlus::ActiveRecord::ConnectionAdapters::PostgreSQLColumn, 

# Doesn't works (without fix)
[ActiveRecord::ConnectionAdapters::PostgreSQLColumn, SchemaPlus::ActiveRecord::ConnectionAdapters::PostgreSQLColumn, ArJdbc::PostgreSQL::ColumnHelpers, ArJdbc::PostgreSQL::Column

So first we must include SchemaPlus::ActiveRecord::ConnectionAdapters::PostgreSQLColumn and second include ArJdbc::PostgreSQL::Column

Are there other examples that could be used as a reference for integrating?

I didn't find

[ronen]

we should check ArJdbc::PostgreSQL::Column was included in ActiveRecord::ConnectionAdapters::PostgreSQLColumn, instead of checking ArJdbc::VERSION

I agree that would be a cleaner way to go -- and also a bit more self-documenting.

Regarding tests, I'm OK with just turning on jruby + postgresql in the travis matrix as you did. Especially since I'm not expecting a lot of ongoing development in this 1.* branch. (Do either of you want to try to get schema_plus 2.0 working with jruby....? I have no idea how easy or hard that would be.)

Thanks to you both