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

Skip connection.setSendTimeAsDatetime when not available. #4

Open
wants to merge 1 commit into
base: 5-1-stable-jdbc
Choose a base branch
from

Conversation

HarlemSquirrel
Copy link

@HarlemSquirrel HarlemSquirrel commented May 4, 2020

Was getting the following with Rails 5.0 and 5.1

NoMethodError (undefined method `setSendTimeAsDatetime' for #<Java::ComMicrosoftSqlserverJdbc::SQLServerConnection:0x70ee1963>)
Did you mean?  sendTimeAsDatetime

Running SELECT @@VERSION against the db server returned
Microsoft SQL Server 2016 (SP2-CU7-GDR) (KB4505222) - 13.0.5366.0 (X64)

This guard clause fixed it for me.

Was getting the following with Rails 5.0 and 5.1 

```
NoMethodError (undefined method `setSendTimeAsDatetime' for #<Java::ComMicrosoftSqlserverJdbc::SQLServerConnection:0x70ee1963>)
Did you mean?  sendTimeAsDatetime
```

This guard clause fixed it for me.
@rdubya
Copy link
Collaborator

rdubya commented Jul 29, 2020

Can you create a test that shows this failure? Based on this test run: https://travis-ci.org/github/jruby/activerecord-jdbcsqlserver-adapter/jobs/712951989 it doesn't look like this change is needed.

@HarlemSquirrel
Copy link
Author

What version of mssql is in metaskills/mssql-server-linux-rails image?

@rdubya
Copy link
Collaborator

rdubya commented Jul 29, 2020

It looks like it is based on microsoft's image that is tagged as 2017. I don't think that should matter though since the issue is with the driver not having a defined method. Maybe they are doing something interesting in their driver, but I'm not sure why the db version would determine whether a method was defined or not. Can you see what version of the jdbc-mssql gem you have installed?

@rdubya
Copy link
Collaborator

rdubya commented Jul 29, 2020

Another question, which version of JRuby are you using and how are you including the library? Wondering if maybe something like that is making it so it doesn't exist. From the mssql documentation, it seems like that method should still be there.

@HarlemSquirrel
Copy link
Author

jdbc-mssql (0.8.0-java)
jruby 9.2.12.0

@rdubya
Copy link
Collaborator

rdubya commented Jul 30, 2020

That should be basically the same as the tests. The main thing I'm concerned about is whether this change is going to break date handling for other people if they run into a similar issue. I'd like to see some form of broken test to know that it is a gem issue and not something on your system specifically, or some combination of things that needs to have a different fix for the datetime handling.

@HarlemSquirrel
Copy link
Author

Is there a Docker container for mssql 2016?

@rdubya
Copy link
Collaborator

rdubya commented Jul 31, 2020

It doesn't look like it.

@HarlemSquirrel
Copy link
Author

What version of Java are these specs running on? I see three three different drivers in https://github.com/JesseChavez/jdbc-mssql/tree/v0.8.0/lib

@rdubya
Copy link
Collaborator

rdubya commented Jul 31, 2020

It looks like its using Java 1.8 and JRuby 9.2.7.0

@iaddict
Copy link

iaddict commented Dec 4, 2020

I think I've found the root cause for this issue. I have a project which used the SQLServer adapter for AR 3.2. To make this old adapter work, I manually required an old jdbc driver (sqljdbc4-4.0.2206.jar). When updating to this AR 5.1 adapter I overlooked this and thus it yielded the exact same error. After removing this outdated jdbc driver everything worked flawlessly.

Once the connection is working, you can check the jdbc driver version:

ActiveRecord::Base.connection.jdbc_connection.meta_data.driver_name
# => "Microsoft JDBC Driver 8.4 for SQL Server"
ActiveRecord::Base.connection.jdbc_connection.meta_data.driver_version
# => "8.4.1.0"

@rdubya
Copy link
Collaborator

rdubya commented Dec 4, 2020

@iaddict interesting, thanks for the info. I wonder if there is any way to check the driver version being used when the gem is loaded so we can throw an exception or something to prevent it from running with an unsupported version.

@iaddict
Copy link

iaddict commented Dec 8, 2020

@rdubya I just tried to get the version of the jdbc driver and actually it is quite easy.

Java::ComMicrosoftSqlserverJdbc::SQLServerDriver.new.major_version
# => 8

If you like, I can make a pull request that checks the major version right after the require of the mssql jdbc driver.

@rdubya
Copy link
Collaborator

rdubya commented Dec 8, 2020

Awesome. Yeah that would be great if you have the time. If you want to do it off of 5-2-stable-jdbc, I'll take care of getting it into the other branches.

@iaddict
Copy link

iaddict commented Dec 9, 2020

@rdubya Just one more thought. Maybe it it would be better to check if a jdbc driver is already loaded in the jdbc-mssql gem (https://github.com/JesseChavez/jdbc-mssql/blob/master/lib/jdbc/mssql.rb).
Or we check if a driver is already loaded, warn about it and then do not require the jdbc-mssql gem. This would make it possible to test with more recent drivers without an external dependency.
What do you think?

@dub357
Copy link

dub357 commented Apr 19, 2022

I'd like to revive this pull request. When this driver is used and the connection is JNDI, the application server can effectively wrap the connection (as is the case with Tomcat's connection pools - DBCP). In these cases, the connection is not a "com.microsoft.sqlserver.jdbc.SQLServerDriver" but rather a wrapper and the setSendTimeAsDatetime method is not available so errors are thrown:

undefined method 'setSendTimeAsDatetime' activerecord-jdbcsqlserver-adapter-52.0.0/lib/active_record/connection_adapters/sqlserver/jdbc_overrides.rb:103:in 'configure_connection'

@rdubya
Copy link
Collaborator

rdubya commented Apr 19, 2022

I haven't tried to set this up with a JNDI connection, if you want to put together a test case for it I can look at it. I haven't dug into this project in a long time, but I'm fairly certain that some date types will not function correctly without this being called.

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

Successfully merging this pull request may close these issues.

4 participants