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

Jdbc mode #599

Open
wants to merge 70 commits into
base: master
from
Open

Jdbc mode #599

wants to merge 70 commits into from

Conversation

@iaddict
Copy link

iaddict commented May 31, 2017

  • Drop strict gem dependency on tiny_tds.
  • Add gem dependency for tests on new (un-published) gem sqljdbc4 which bundles the MS jdbc driver.
  • Implement a thin ruby jdbc wrapper that handles queries, stored procedures and type conversion to ruby.
  • Use jdbc wrapper methods where required in a conditional manner.

This pull request resembles the discussion from #579.

Copy link
Contributor

metaskills left a comment

Thanks! I got plenty of time now so I should be much more responsive. Can you rebase to latest master and take a look at my other notes? Looking forward to this in 5.1.

when :jdbc
result = begin
@connection.fetch(sql).all
rescue => e

This comment has been minimized.

@metaskills

metaskills Jun 16, 2017 Contributor

This rescue feels risky. Why would an empty result set to fetch raise an exception? Is it due to the .all method call? If so, can we avoid the rescue trap?

This comment has been minimized.

@iaddict

iaddict Jun 22, 2017 Author

The problem is, that I can't detect if the sql statement returns a result set or not.
#fetch uses jdbcs executeQuery (https://docs.oracle.com/javase/7/docs/api/java/sql/Statement.html#executeQuery(java.lang.String)) which throws an exception when the statement does not yield a result (like ddl statements, in which case the execute(sql) method should be used).

The statement is executed none the less.

This comment has been minimized.

@coderjoe

coderjoe Oct 31, 2018 Contributor

I have a similar concern to @metaskills. Is there a reason we're rescuing all exceptions as "statement invalid", instead of just the ones the method throws that we know to mean "statement invalid"?

results =
begin
handle.all(query_options)
rescue => e

This comment has been minimized.

@metaskills

metaskills Jun 16, 2017 Contributor

Same trap on rescue, would you mind letting me know why no results returns an error. Can that be avoided? I'd really like to know why the JDBC connection wold do that.

This comment has been minimized.

@iaddict

iaddict Jun 22, 2017 Author

See above.

@@ -20,11 +20,11 @@ def current_database
end

def charset
select_value "SELECT DATABASEPROPERTYEX(DB_NAME(), 'SqlCharSetName')"
select_value "SELECT CAST(DATABASEPROPERTYEX(DB_NAME(), 'SqlCharSetName') AS NVARCHAR(128))"

This comment has been minimized.

@metaskills

metaskills Jun 16, 2017 Contributor

Nice change. But I think we need to update sql_counter_sqlserver.rb with the ignored SQL format too right?

This comment has been minimized.

@iaddict

iaddict Jun 22, 2017 Author

Not sure why this needs to be done. Isn't this statement execute in isolation all the time and never with count?

@@ -194,7 +196,7 @@ def reset!

def tables_with_referential_integrity
schemas_and_tables = select_rows <<-SQL.strip_heredoc
SELECT s.name, o.name
SELECT s.name sc, o.name tab

This comment has been minimized.

@metaskills

metaskills Jun 16, 2017 Contributor

Another nice change. Did JDBC throw a warning here or did you notice the ambiguity?

This comment has been minimized.

@iaddict

iaddict Jun 22, 2017 Author

The method handle.all which is used to retrieve the result within DatabaseStatements#handle_to_names_and_values_jdbc returns a hash with {column: value}.
Without the aliases there would only be one column.

end
end

module ActiveRecordSqlServerAdapter

This comment has been minimized.

@metaskills

metaskills Jun 16, 2017 Contributor

We have made a concerted effort to not add more/abstract things to the top level namespace. Could you please change ActiveRecordSqlServerAdapter::Jdbc to ActiveRecord::ConnectionAdapters::SQLServer::Jdbc namespace with a file in the correct spot at lib/active_record/connection_adapters/sqlserver/jdbc.rb. This may seem persnickety, but it is import to use the namespace we setup and file convention.

This comment has been minimized.

@iaddict

iaddict Jun 22, 2017 Author

Yes. This place looks much better.


def synchronize(&block)
@cache_mutex.synchronize(&block)
end

This comment has been minimized.

@metaskills

metaskills Jun 16, 2017 Contributor

I have concerns on adding stuff like this and why it is needed so I would first like to know the full details on what this is used for? Is the JDBC connection not thread safe? I though most of Rails v5.1 and upward are leveraging concurrent-ruby now too? But first, I'd just like to know more about this.

This comment has been minimized.

@iaddict

iaddict Jun 22, 2017 Author

The jdbc connection should be thread save, but our ruby jdbc wrapper is not.

One example is the implementation of Database#execute which does executes two statements in one after another for inserts on the same connection. If this connection would be used in between these statements the result of the second statement would be probably wrong.

Maybe this is not necessary because AR does pooling anyways, but I'm not sure.

This comment has been minimized.

@metaskills

metaskills Jun 22, 2017 Contributor

but our ruby jdbc wrapper is not.
Maybe this is not necessary because AR does pooling anyways, but I'm not sure.

Exactly! We don't have to do this for them. It is code Sequl has to do, but again, not us.

This comment has been minimized.

@iaddict

iaddict Jun 27, 2017 Author

Ok. I did remove the synchronize stuff for Jdbc::Database.
I think the cache still needs it.

# as access to the cache is not thread safe without a mutex if other
# threads can reference the dataset. Symbol keys prefixed with an
# underscore are reserved for internal use.
attr_reader :cache

This comment has been minimized.

@metaskills

metaskills Jun 16, 2017 Contributor

Should we even use the cache? How much of this Sequel code can we get rid of?

This comment has been minimized.

@iaddict

iaddict Jun 22, 2017 Author

The cache is hit frequently. It saves one query each time which is needed to fetch the column information. The column information is used for the type conversion from java to ruby types and for the column names of the results dataset.

Maybe we can streamline the code none the less. But first I would like to get the test suite completely green.

@@ -0,0 +1,96 @@
module ActiveRecordSqlServerAdapter

This comment has been minimized.

@metaskills

metaskills Jun 16, 2017 Contributor

Reminder to make a jdbc folder in lib/active_record/connection_adapters/sqlserver/jdbc and put all these files in there with the other namespace.

This comment has been minimized.

@iaddict iaddict force-pushed the iaddict:jdbc-mode branch from d997833 to b18454a Jun 22, 2017
@iaddict
Copy link
Author

iaddict commented Jun 22, 2017

The rebase is done. The test suite is not yet totaly green:

$ ONLY_SQLSERVER=1 jruby -S bundle exec rake test:jdbc
#=> 218 runs, 1349 assertions, 1 failures, 4 errors, 5 skips

$ jruby -S bundle exec rake test:jdbc
#=> 5012 runs, 14857 assertions, 2 failures, 16 errors, 10 skips

Especially the uuid identity columns do not work for insert. It looks like the 'select identity' statements do not yield the generated ids.

Do you have a hint how to get these? I didn't find any code the handles this for tiny_tds which confuses me because this seems to be an issue with SQL-Server in general.

@iaddict
Copy link
Author

iaddict commented Jun 27, 2017

$ONLY_SQLSERVER=1 jruby -S bundle exec rake test:jdbc

The tests now look even better. Two tests still fail, but the errors stem from the same issue.
I have a problem to get the last inserted id for uuid columns. @metaskills Do you have a hint how to get the generated uuid on insert?

The 'select identity' statements do not yield the generated ids.

$ ONLY_SQLSERVER=1 jruby -S bundle exec rake test:jdbc

  1) Error:
SQLServerUuidTest#test_0003_can create other uuid column on reload:
ActiveRecord::RecordNotFound: Couldn't find SSTestUuid without an ID
    .../activerecord/lib/active_record/relation/finder_methods.rb:448:in `find_with_ids'
    .../activerecord/lib/active_record/relation/finder_methods.rb:66:in `find'
    .../activerecord/lib/active_record/querying.rb:3:in `find'
    .../activerecord/lib/active_record/core.rb:171:in `find'
    .../activerecord/lib/active_record/persistence.rb:462:in `block in reload'
    .../activerecord/lib/active_record/scoping/default.rb:35:in `block in unscoped'
    .../activerecord/lib/active_record/relation.rb:336:in `scoping'
    .../activerecord/lib/active_record/scoping/default.rb:35:in `unscoped'
    .../activerecord/lib/active_record/persistence.rb:462:in `reload'
    .../activerecord/lib/active_record/attribute_methods/dirty.rb:50:in `reload'
    .../activerecord/lib/active_record/associations.rb:283:in `reload'
    .../activerecord/lib/active_record/autosave_association.rb:236:in `reload'
    .../activerecord/lib/active_record/aggregations.rb:13:in `reload'
    .../activerecord-sqlserver-adapter/test/cases/uuid_test_sqlserver.rb:21:in `block in test_0003_can create other uuid column on reload'


  2) Failure:
SQLServerUuidTest#test_0002_can create with a new pk [.../activerecord-sqlserver-adapter/test/cases/uuid_test_sqlserver.rb:15]:
Expected nil to be present?.

218 runs, 1352 assertions, 1 failures, 1 errors, 5 skips
@iaddict
Copy link
Author

iaddict commented Jun 29, 2017

Ok. I fixed the uuid issue. The ONLY_SQLSERVER tests are now all green!

$ ONLY_SQLSERVER=1 jruby -S bundle exec rake test:jdbc 

Finished in 36.358056s, 5.9959 runs/s, 37.2957 assertions/s.

218 runs, 1356 assertions, 0 failures, 0 errors, 5 skips
@iaddict
Copy link
Author

iaddict commented Jul 3, 2017

Hi. I worked some more on the jdbc mode. Travisci now also runs with jruby.
Only one AR test case is still failing, the rest is green. 🎉

$ jruby -S bundle exec rake test

  1) Failure:
TransactionTest#test_rollback_when_thread_killed [.../jruby-9.1.12.0/lib/ruby/gems/shared/bundler/gems/rails-8e7fff4c2122/activerecord/test/cases/transactions_test.rb:541]:
First shouldn't have been approved

5031 runs, 14918 assertions, 1 failures, 0 errors, 10 skips

This test fails due to a bug in jruby, see: jruby/jruby#4705

Do you like what you see so far? Please lets talk about the next steps.

@iaddict
Copy link
Author

iaddict commented Sep 6, 2017

Hi @metaskills
Did you already find some time to check this pull request?

@metaskills
Copy link
Contributor

metaskills commented Sep 6, 2017

I'll be spending this week climbing into all PRs and issues.

@iaddict
Copy link
Author

iaddict commented Nov 20, 2017

Hi @metaskills.
I did some investigation and found the cause for the above mentioned issue on jruby with killed threads and rollbacks. This has been fixed on jruby v9.1.14 (jruby/jruby@d335c0c) and thus almost all tests are green on rails 5.1.4 (with your ONE_AS_ONE patch applied 👍). Only one rails AR-test fails (test_serialized_attribute_works_under_concurrent_initial_access).
Maybe this one has already been fixed with your further development on the adapter.
On rails 5.1.0-5.1.2 all tests are green.

I would merge my branch with master and see how it works, but I would first like to hear your plans concerning this PR.

@iaddict
Copy link
Author

iaddict commented Mar 29, 2018

Hi @metaskills. New year, new chance ;)
Have you already had time to look into this PR?

@seizmo
Copy link

seizmo commented May 24, 2018

Hi @iaddict and @metaskills, great job in the project! Since the MS SQL Support of activerecord-jdbc-adapter has been in decline for a while (cf. jruby/activerecord-jdbc-adapter#784) and I don't think that will change anytime soon, having activerecord-sqlserver-adapter as a high quality option for JRuby users would be great!

@rdubya
Copy link

rdubya commented Jul 23, 2018

Hey all, has there been any more movement on this? We're stuck at Rails 4.2 until there is a viable MSSQL adapter. So, if there's anything I can do to help move it along, I'd be more than happy to put some time into it.

@iaddict
Copy link
Author

iaddict commented Aug 2, 2018

I would like to see this pull request also to make it into the adapter.
I already tested it with a medium sized rails 4.2 project which I did upgrade to rails 5.1 and it worked fine.

What can we do to make this a reality?

@neojin
Copy link

neojin commented Sep 18, 2018

@iaddict Is it possible to use the jdbc-mode branch in our Gemfiles while waiting for this to get merged?

@seizmo
Copy link

seizmo commented Sep 18, 2018

@iaddict Would you consider to fork activerecord-sqlserver-adapter with you jdbc changes and upload your own gem? The maintainer seems to be MIA and there are lots of people waiting for it to be easily integratable in their projects (esp. to be able to upgrade to Rails 5.x).

@jayjlawrence
Copy link

jayjlawrence commented Oct 26, 2018

@seizmo agree 100%. This work is FAR too important to neglect like this. I for one will chip in any way I can to help move this along. The lack of 5.x Rails support for Jruby has been a really major concern for us. Our code is fully invested in Jruby and SQL Server right now - we need this to keep up with the rest of the Rails world.

I say fork - make it clear why and that we want to rejoin ASAP. And make it really clear what are the implications (if any) for any user of this forked code.

@coderjoe coderjoe closed this Oct 26, 2018
@coderjoe coderjoe reopened this Oct 26, 2018
@coderjoe
Copy link
Contributor

coderjoe commented Oct 26, 2018

Leave it up to me to fat-finger a close request while trying to type a response...

@coderjoe
Copy link
Contributor

coderjoe commented Oct 26, 2018

@jayjlawrence @seizmo and @iaddict

You are, of course, welcome to fork the code and create whatever you need to in order to move your work forward. However before forking I recommend getting the code into a build-able state with passing tests and requesting further review. Maybe even start a conversation about taking management of review and support of mssql jdbc changes?

If the speed of the review has not matched your expectations there's also a third option. Perhaps consider contributing to the jruby/activerecord-jdbc-adapter project? It looks like they have been looking to support MSSQL for quite a while and would likely welcome contribution as well.

I feel your pain with respect to the delay in review, but I wouldn't give up yet.

@coderjoe
Copy link
Contributor

coderjoe commented Oct 26, 2018

In the interim for your existing projects, might the existing activerecord-jdbcmssql-adapter from the jdbc/activerecord-jdbc get you up-and-running?

@rdubya
Copy link

rdubya commented Oct 27, 2018

Hi @coderjoe, I'm a contributor to the activerecord-jdbc family of gems and the activerecord-jdbcmssql-adapter gem is no where near rails 5 support. We were actually hoping that this project could add the jdbc driver support so that we could kill off that portion of the code.

My employer uses jruby and has a legacy system in mssql that we will need to interface with for at least the next few years, so I would be more than happy to help maintain it (or a fork of it if that is the ultimate resolution.)

Another option would be to make some changes to enable the drivers to not be as tightly integrated and then have the new mssql jdbc driver all live in a separate gem that has this gem as a dependency. Then when the new gem is loaded, it can load the driver agnostic pieces and only has to implement the pieces that touch the driver (this is the approach we took with the other activerecord-jdbc* gems). If that route is preferable, it may be worth the effort to get the activerecord-jdbcmssql-adapter gem configured so that it could plug in like that and build on top of the work that has been done in both projects.

@coderjoe
Copy link
Contributor

coderjoe commented Oct 27, 2018

@rdubya thanks for chiming in! Do you mean that the activerecord-jdbc-adapter devs want to completely abandon just the activerecord-jdbcmssql-adapter, just the jdbc-jtds portion, or both?

Ultimately forking or not will depend on @metaskills' goals for the project and whatever development direction he wants jdbc support to take. Either way I'm hoping that your offer to help with review and maintenance can grease the wheels a bit and help avoid a fork unless absolutely necessary.

@rdubya
Copy link

rdubya commented Oct 27, 2018

@coderjoe There hasn't been a lot of support for the activerecord-jdbc* gems so I think the main goal of the project now is just to maintain the main 3 dbs (sqlite,postgres,mysql). There hasn't been any work done (that I know of) on updating any of the other gems associated with that project to work with rails 5. @enebo, @kares can either of you add any insight here?

@kares
Copy link

kares commented Oct 27, 2018

for context, AR-JDBC has been quite under utilized - 3.x/4.x support was pretty much a one man show with all adapters (MySQL, SQLite, Postgre, DB2, Oracle, SQLServer, HSQLDB+H2, Derby)
... if you add up all the Rails versions it might quickly get being too much, if you're not employed (or compensated) to do the maintenance, either directly or at least indirectly.

the 5.x work was more in line of a community effort, with folks such as @rdubya taking action and doing great work - really turned out great. Rob pretty much adopted the Postgres adapter with contributions towards the base part and performance improvements that might be a gain for all adapters.

I myself did hope for some "commercial" support to somehow 'lead' the 5.x way, but it did not materialize much. have talked with 2-3 folks (actually all where MS-SQL) that wanted to provide backing, but none of them actually did - for various reasons.

Do you mean that the activerecord-jdbc-adapter devs want to completely abandon just the activerecord-jdbcmssql-adapter, just the jdbc-jtds portion, or both?

just the adapter gem, the jdbc-jtds is just a slim wrapper for the driver, altough jtds seemed a dead project for years. the adapter was tested (at least I recall testing it) with the official driver but AR-JDBC does not maintain that driver wrapper gem.

all being said, I believe AR-JDBC could provide a base 'JDBC' adapter but atm we can not support more adapters esp. note "heavy" commercial ones such as Oracle or SQLServer (mean it would be great but why should anyone expect it from people that do not use or need it on a regular basis).
having a solid abstraction such as JDBC helps lower the cost, but there's still a lot of customizations esp. with adapters such as MS-SQL e.g. query rewriting exercised is just "crazy".

have also seen trade-offs with MS-SQL parts in AR-JDBC, at some point I tried following MRI adapter but some 'compromises' I did not like (believe the 2 projects failed some custom limit-offset queries differently) so I ended up complicating things by keeping the existing hacks instead of porting over more from mssql-adapter gem.

believe there's 2 (obvious) options left on the table - have it here along side native or have a jdbc gem for JRuby depending on AR-JDBC.

@coderjoe
Copy link
Contributor

coderjoe commented Oct 27, 2018

@rdubya and @kares thanks for the insight, I feel like I understand the availability of support a bit better now. Since nothing works, it sounds like anything that does work is the priority.

That said, taking a cursory glance at the gem, the following things worry me:

  1. The TinyTDS dependency was removed from the standard MRI/Ruby gem platform target. I'm not sure this is a good idea. I'd prefer to see a separate gemspec and a separate gem built targeting the java platform and explicitly declaring its java dependencies.
  2. It's based on a private sqljdbc4 wrapper gem, and there is no publicly maintained alternative. I wouldn't want to support that within this project if I were the maintainer, and I really wouldn't want to depend on it unless it's public, published, and has a declared maintainer.
  3. The code is based off of very old mainline and as of last comments still has broken tests. I'd like to see a successful build/test.
  4. Which means the tests need to be enhanced to actually test on jruby versions that plan to be supported.

If we can find someone interested in resolving this problems we should have a much cleaner potential merge. That would probably make it more likely to get reviewed and would also make it easier to fork into a jdbc version if the outcome isn't what you guys want (or takes too long).

Now I don't have any personal interest or time to spend writing or maintaining this stuff. I have no projects today that depend on the activerecord-sqlserver-adapter and I'm not using jruby so all that is left is to wait for contributions.

iaddict added 27 commits May 3, 2017
`ActiveRecordSqlServerAdapter::Jdbc` contains all code that is needed
for the jdbc mode. It does connection and result handling.
Jdbc result values are converted to the corresponding ruby types.

With these changes only the sqljdbc4-java gem has to be installed or
the sqljdbc42.jar has to be included in the java CLASSPATH.
So the jdbc adapter also fetches the identity.
And also rename to exec_jdbc_dml because its not ddl.
Especially remove Jdbc#synchronize because AR already does connection pooling and threadsafety stuff.
Guess from string encoding if this should by handled as a varbinary.
Fixes tests for varbinary column inserts.
This needs to be changed as soon as there is a build on gemserver.
This should make bundler work on tracisci.
@iaddict iaddict force-pushed the iaddict:jdbc-mode branch from dc71b44 to 9845c34 Feb 24, 2019
@JesseChavez
Copy link

JesseChavez commented Sep 2, 2019

If there is anyone still interested on an Active Record SQL Server adapter the works with JRuby

The is a fork of the activerecord-jdbc-adapter with Rails 5.0, 5.1, 5.2, and 6.0 support:

https://rubygems.org/gems/activerecord-jdbc-alt-adapter

It is strongly advised to read the README file of the forked repository for more information and differences compared to activerecord-sqlserver-adapter or activerecord-jdbc-adapter adapters.

platforms :jruby do
  gem 'activerecord-jdbc-alt-adapter', '~> 60.0.0'
  gem 'jdbc-mssql', '~> 0.6.0'
end

Some of our apps are already running in production and one of them is a huge ancient finance Rails app originally created with Rails 2.0 and JRuby.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants
You can’t perform that action at this time.