Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

fix the (for some adapter gems) broken **1.2.4** release #284

Merged
merged 1 commit into from

8 participants

@kares
Collaborator

fixes #279

@arunagw
Collaborator

Should I revert this one #278 ?

@kares
Collaborator

actually, it seems to make sense let me check the history of why there's no explicit require for loading the driver gem ...
I'll update this PR with the fixed commit added at #278

@arunagw
Collaborator

Thank you very much! Really appreciated.

@kares
Collaborator

except for the jTDS fix you have at 7e4f2fb it all seems fine to me ...
I'm not sure why there wasn't a jdbc/... require in the first place, we might improve things by checking if the driver (Java) class can be loaded and thus not loading it if it's already on CP but that might change the class-loader hierarchy a bit thus I'm not sure if we should do that (it has nothing to do with the fix - just thinking about the driver loading code) ...

@arunagw
Collaborator

We need to fix this.

Sent you an email displayed on GitHub. Do you use different email address?

@kares
Collaborator

@arunagw I did reply in a few minutes, although from a different @gmail.com address so you can find me on gtalk
maybe check you spam folder, anyways I'll be offline for a few days now that it might take a while for me to reply ...

@kares
Collaborator

also the build is fine, if it worries you - it's just the occasional travis-ci failure ... hope you can do a release soon

@atambo
Collaborator

It's good that you yanked the broken activerecord-jdbc-adapter gem but now anyone updating to the newer jdbc-* gems (jdbc-mysql, jdbc-postgres, etc) while still using the old activerecord-jdbc-adapter gem will get the following error:

driver encountered an unknown error: cannot load Java class com.mysql.jdbc.Driver

Not sure if there's anything that can be done about the incompatibility until the new version of activerecord-jdbc-adapter comes out and even then just hope everyone updates both gems at the same time.

@kares
Collaborator

@atambo right, once again I'm sorry for not thinking this thru ... a defined? ArJdbc::Version::VERSION + version check might be performed when loading a jdbc- gem doing a (backwards compatible) driver auto-load if the version is < 1.2.3 - although that would need driver gem releases yanked and re-performed

@san

I was getting the exact same error mentioned by @atambo with jdbc-mysql and activerecord-jdbc-adapter. After reading these comments, reverted jdbc-mysql back to version 5.1.13 (from 5.1.22) in my Gemfile and its all good now.
Thanks.

@atambo
Collaborator

I think it'd probably be smart to yank the newer jdbc-* gems at least until the fixed activerecord-jdbc-adapter is out.

@san

+1 on @atambo 's comment. This incompatibility can potentially cause a lot of people a lot of trouble trying to figure out what they did to break what was working just fine.

@PriteshJain

I too got the same error
I was too eager to get it work and found a workaround

I copied the code from activerecord-jdbc-adapter / activerecord-jdbcmysql-adapter / lib / active_record / connection_adapters /jdbcmysql_adapter.rb

pasted it in application.rb. and it worked

if defined?(JRUBY_VERSION)
require 'jdbc/mysql'
Jdbc::MySQL.load_driver(:require) if Jdbc::MySQL.respond_to?(:load_driver)
end

@kares
Collaborator

not sure what else issues are there - but putting the AR-JDBC gems out ASAP with the "fixes" proposed here should (at least) make it pretty stable when using the AR-JDBC gems with any jdbc- gems ... the longer this gets fixed the longer issues mentioned by @atambo become real since users will update only the jdbc- gems instead of updating the activerecord-jdbc-adapter as well ...

@PriteshJain

Just in case this can help I have done a fresh install of gems and got these gem version.

activerecord-jdbc-adapter (1.2.2.1)
activerecord-jdbcmysql-adapter (1.2.2.1)
jdbc-mysql (5.1.22)

but this always threw error

driver encountered an unknown error: cannot load Java class com.mysql.jdbc.Driver

so used this in application.rb

if defined?(JRUBY_VERSION)
  require 'jdbc/mysql'
  Jdbc::MySQL.load_driver(:require) if Jdbc::MySQL.respond_to?(:load_driver)
end
@arunagw arunagw merged commit d3c5eb3 into jruby:master
@bbrowning

Since ar-jdbc 1.2.5 came out we've had a lot of TorqueBox users hit errors related to not being able to find JDBC driver classes - things like "java.lang.ClassNotFoundException: org.postgresql.Driver" and "(LoadError) no such file to load -- active_record/connection_adapters/jdbcsqlite3_adapter". Here's a gist with one user's stack trace - https://gist.github.com/4446375

Is there some known issue with ar-jdbc 1.2.5 and driver loading? It looks like from the comments on this issue that these kinds of errors were expected to be fixed.

@arunagw
Collaborator

Hey @bbrowning I think we have left with one issue with loading.

Can you give a try with this PR #289 if possible and let us know the result. ?

--Arun

@arunagw
Collaborator

And now you can test with head of master. I merged that PR.

Let me know of still fails. If all good we will do a release ASAP.

Cheers,
Arun

@ajuckel
Collaborator

I tested with a sample rails project deployed to torquebox this morning, and the recent changes don't appear to have resolved the torquebox-specific issue, which I could replicate when using either postgresql or jdbcpostgresql as the adapter in database.yml. I'll look into it further this weekend.

@ajuckel
Collaborator

Yeah, this still will not work without patching torquebox. This weekend I'll get it all sorted so that a require of the jdbc-* gems will again load the embedded jar. Perhaps we'll add a flag to disable the auto-load (in case a user is expecting the class to be loaded from elsewhere), but we can't break compatibility in the 1.2 series.

@kares
Collaborator

@ajuckel so it's TB specific than ? could we maybe see what's happening there ?
we did try before the 1.2.5 release Trinidad (JRuby-Rack) as well as plain WEBRick where fine (tested mysql/sqlite3)
also it should now support the case when the jdbc- gem is not available and driver.jar is expected on the CP (not completely handled previously) kares@5b29f4b besides backwards compat with the old jdbc- gem versions - we only expected issues with new jdbc- gems + AR-JDBC 1.2.2

@ajuckel
Collaborator

It is TB specific. I may submit a patch to them (once I better understand why they're doing it this way) but it's unreasonable for us to ask someone to update their app server due to a bug-fix release in our library. We need to maintain compatibility, and I think we can do it while still allowing for what you were trying to accomplish in 13fccb8. Speaking of which, do you have a test for the issue you're trying to avoid? I'm assuming something like using a DataSource provided via JNDI (where the Driver is loaded in a parent classloader).

We used to load the embedded jar as soon as the jdbc-* gem was loaded. What we're doing now is loading the jar when a connection is made. TB is (as you can see here) attempting to load the Driver class manually as soon as the jdbc-* gem is loaded.

@bbrowning

I can confirm this is TorqueBox specific and limited to our distributed transaction support, which users can disable. We made the assumption that once the appropriate jdbc/* library was required that the actual database driver would be available on the classpath. We have a new release coming out in the next few weeks and I can add the backwards compatibility Jdbc::Foo.load_driver call for that release.

It does mean that TorqueBox users prior to the upcoming 2.3.0 release won't be able to connect to a database out of the box until they either disable our distributed transaction support or downgrade to ar-jdbc < 1.2.5.

@kares
Collaborator

@ajuckel agreed, I thought about doing it "correctly" but by that time there was already a 1.2.x release, also it's quite a hustle since you need to assume everyone is expecting the jar to be loaded on a require (due backwards compat) but that contradicts the use-case I was refactoring this in the first place (e.g. from a custom pool - non AR-JDBC code - I'd like to require and load the jar later or add it to the web application's CP "manually").

can't have a test-case really since that would require running the tests with an updated CP and/or without any driver requires. I know it's pretty bad to break things this way in a "bug-fix" release (to be fair 2.3 support did degrade a bit as well - maybe tagging as 1.3.0 would've been the ultimate solution :)) and it certainly was not my intention - it seemed like the jdbc- gems are used only by AR-JDBC.

anyways I'd like to at least avoid having the sqlite.jar on the CP in a mysql application (as it happened previously since mysql/sqlite3/postgres support is getting pre-loaded in arjdbc) when refactoring all this back ... since I had issues with the driver leak prevention on Tomcat in a specific case.

@ajuckel
Collaborator

@kares Oh, I'm not trying to throw blame around, and you've definitely done a lot of good work. We all made some assumptions about how safe it would be to change this around. I just think we were a bit off in what was safe, so should try to fix it.

I'm glad you mentioned the auto-load case. I'd completely forgotten about that, and it does make the quick idea I had for fixing this insufficient. I may have to go back to the horrible idea of adding an environment variable to turn off the auto-load behavior in the 1.2.x series.

@guilleiguaran
Collaborator

Release 1.3.0 doesn't looks like a bad idea...

@ajuckel
Collaborator

I'm more inclined to release a fixed 1.2.6 (which auto-loads driver jars again), then merge the AR-4.0 stuff in and start 2.0.0 prereleases (with auto-loading of driver jars disabled by default).

@guilleiguaran
Collaborator

@ajuckel ah right, that sounds better

@kares
Collaborator

@ajuckel no hard feelings - I should have tried fixing it in the first place but instead assumed driver gems follow the jar version number and thus it's fine for them to break backwards compat ... seems really stupid now !

I already rebased AR-4.0.0 support locally - there's not that much change (no incompatibilities really) as to call it 2.0.0 in the end but if there's more to come (or even if not) it's probably fine.
I wanted to get Rails master compatibility out for a long time now thus I don't care how it's done as long as it's (finally) done :) - I shall open an updated PR since there are since some issues and tests did not run with master on first hit ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
2  lib/arjdbc/derby/connection_methods.rb
@@ -5,7 +5,7 @@ def derby_connection(config)
require 'active_record/connection_adapters/jdbcderby_adapter'
config[:url] ||= "jdbc:derby:#{config[:database]};create=true"
- config[:driver] ||= ::Jdbc::Derby.driver_name # org.apache.derby.jdbc.EmbeddedDriver
+ config[:driver] ||= defined?(::Jdbc::Derby.driver_name) ? ::Jdbc::Derby.driver_name : 'org.apache.derby.jdbc.EmbeddedDriver'
config[:adapter_spec] = ::ArJdbc::Derby
conn = embedded_driver(config)
md = conn.jdbc_connection.meta_data
View
2  lib/arjdbc/h2/connection_methods.rb
@@ -5,7 +5,7 @@ def h2_connection(config)
require 'active_record/connection_adapters/jdbch2_adapter'
config[:url] ||= "jdbc:h2:#{config[:database]}"
- config[:driver] ||= ::Jdbc::H2.driver_name # org.h2.Driver
+ config[:driver] ||= defined?(::Jdbc::H2.driver_name) ? ::Jdbc::H2.driver_name : 'org.h2.Driver'
config[:adapter_spec] = ::ArJdbc::H2
embedded_driver(config)
end
View
2  lib/arjdbc/hsqldb/connection_methods.rb
@@ -5,7 +5,7 @@ def hsqldb_connection(config)
require 'active_record/connection_adapters/jdbchsqldb_adapter'
config[:url] ||= "jdbc:hsqldb:#{config[:database]}"
- config[:driver] ||= ::Jdbc::HSQLDB.driver_name # org.hsqldb.jdbcDriver
+ config[:driver] ||= defined?(::Jdbc::HSQLDB.driver_name) ? ::Jdbc::HSQLDB.driver_name : 'org.hsqldb.jdbcDriver'
config[:adapter_spec] = ::ArJdbc::HSQLDB
embedded_driver(config)
end
View
2  lib/arjdbc/mssql/connection_methods.rb
@@ -5,7 +5,7 @@ def mssql_connection(config)
config[:host] ||= "localhost"
config[:port] ||= 1433
- config[:driver] ||= ::Jdbc::JTDS.driver_name # net.sourceforge.jtds.jdbc.Driver
+ config[:driver] ||= defined?(::Jdbc::JTDS.driver_name) ? ::Jdbc::JTDS.driver_name : 'net.sourceforge.jtds.jdbc.Driver'
config[:adapter_spec] = ::ArJdbc::MsSQL
config[:url] ||= begin
View
2  lib/arjdbc/mysql/connection_methods.rb
@@ -14,7 +14,7 @@ def mysql_connection(config)
options['useUnicode'] ||= 'true'
options['characterEncoding'] = config[:encoding] || 'utf8'
config[:url] ||= "jdbc:mysql://#{config[:host]}:#{config[:port]}/#{config[:database]}"
- config[:driver] ||= ::Jdbc::MySQL.driver_name # com.mysql.jdbc.Driver
+ config[:driver] ||= defined?(::Jdbc::MySQL.driver_name) ? ::Jdbc::MySQL.driver_name : 'com.mysql.jdbc.Driver'
config[:adapter_class] = ActiveRecord::ConnectionAdapters::MysqlAdapter
config[:adapter_spec] = ::ArJdbc::MySQL
connection = jdbc_connection(config)
View
2  lib/arjdbc/postgresql/connection_methods.rb
@@ -10,7 +10,7 @@ def postgresql_connection(config)
config[:port] ||= 5432
config[:url] ||= "jdbc:postgresql://#{config[:host]}:#{config[:port]}/#{config[:database]}"
config[:url] << config[:pg_params] if config[:pg_params]
- config[:driver] ||= ::Jdbc::Postgres.driver_name # org.postgresql.Driver
+ config[:driver] ||= defined?(::Jdbc::Postgres.driver_name) ? ::Jdbc::Postgres.driver_name : 'org.postgresql.Driver'
config[:adapter_class] = ActiveRecord::ConnectionAdapters::PostgreSQLAdapter
config[:adapter_spec] = ::ArJdbc::PostgreSQL
conn = jdbc_connection(config)
View
2  lib/arjdbc/sqlite3/connection_methods.rb
@@ -11,7 +11,7 @@ def sqlite3_connection(config)
database = config[:database]
database = '' if database == ':memory:'
config[:url] ||= "jdbc:sqlite:#{database}"
- config[:driver] ||= ::Jdbc::SQLite3.driver_name # org.sqlite.JDBC
+ config[:driver] ||= defined?(::Jdbc::SQLite3.driver_name) ? ::Jdbc::SQLite3.driver_name : 'org.sqlite.JDBC'
config[:adapter_class] = ActiveRecord::ConnectionAdapters::SQLite3Adapter
config[:adapter_spec] = ::ArJdbc::SQLite3
jdbc_connection(config)
Something went wrong with that request. Please try again.