Jdbc mode

[iaddict]

• 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.

[iaddict]

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]

$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]

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]

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: https://github.com/jruby/jruby/issues/4705

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

[iaddict]

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

[metaskills]

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

[iaddict]

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 (https://github.com/jruby/jruby/commit/d335c0cbbc4fe44bda4e16f4f647ae9cc3ed4528) 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]

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

[seizmo]

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. https://github.com/jruby/activerecord-jdbc-adapter/issues/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]

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]

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]

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

[seizmo]

@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]

@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]

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

[coderjoe]

@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]

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

[rdubya]

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]

@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]

@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]

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]

@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.

[rdubya]

Hey all, glad we finally got a conversation started and can hopefully work towards something that is workable for everybody. At this point I think the best course of action may be for us to look into having the activerecord-jdbcmssql-adapter include this project as a dependency and try to keep any java specific changes in there. I think we can follow a similar model as we did for the other jdbc adapters in that case, in that most of the code will just be to override anywhere the drivers differ.

@coderjoe, @metaskills are you open to prs that separate driver dependencies so they can be more easily overridden?

If so, I'm willing to start hacking on the jdbc adapter to try to get it up and running. Unfortunately, I'm on a big project at work that will probably run through the end of the year so I won't have a lot of time to dedicate to it, but I'll pick at it and if anybody is interested in helping, I'll try to come up with some pieces that can be focused on to keep the project moving.

[coderjoe]

@rdubya I don't really understand the adapter enough to really grok the implications yet, but frankly I say go for it. Your suggestion seems like the merge will be easier, might help with a separation of responsibilities surrounding driver selection, might also resolve my concern about the gemspec not including critical dependencies. On the surface it sounds great.

Thank you in advance for you contribution and I look forward to reviewing it. 👍

[coderjoe]

@rdubya As a quick follow-up. Please mention this PR in your driver dependency PR so that we can track the effort in both and potentially close this as a duplicate later once it's resolved.

[jayjlawrence]

1. The TinyTDS dependency was removed from the standard MRI/Ruby gem platform target. I'm

Correct me if I'm wrong but there are alternatives to tiny_tds when running under MRI. Secondly, when building for different platforms you most likely have different dependencies. I've become well acquainted (not necessarily by choice) with bundler's handling of platform specific dependencies and it handles it properly. Within gemfiles I am less confident - I'd look to a gem like FFI that handles this type of issue. And this discussion speaks to the point.

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.

sqljdbc4 is a Jar from Microsoft. The reference here is just an interim handling of this artifact. I believe there's a 'modern way' to bundle Jar's these days.

5. Performance. What type of performance impact will this work have on the existing MRI gem? Negative impact is going to be a big problem.

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.

I am keenly interested in the advancement of JDBC support for MS SQL. It's very core to what I do and am prepared to put a few bucks on the table if required. I had tried to push a little bit on the JDBC gem, when I had the cycles. Then I learnt about this work.

Personally I think that this is the way to go. Addressing the MS SQL dialect is the big part of the task and this is being handled very well in the the activerecord-sqlserver-adapter gem. The transport, TDS, JDBC, etc., is critical, of course, but secondary.

@kares also raised a few other points in earlier correspondence. Following AR-JDBC would make sense if the transport was the big issue - but it isn't. Add that to some concerns that AR-JDBC is going to need a lot of LTC. It looks like this is the best route forward. Thanks @iaddict !

The fact that work has been stalled for a year is a concern. Significant effort has been invested and appears to be blocked waiting on any signal from the core team. That has to suck for @iaddict and likely a real demotivator for an enthusiastic contributor. We should work to prevent in the future. For me, personally, if I'm going to put much more effort on this matter, I'd want assurances that the work is going to receive some attention.

Perhaps what would help is some interactive conversation to determine how best to bring us JDBC types along without creating further maintenance work for the gem as a whole.

@rdubya and @iaddict happy to chat more if you guys have the time. @kares are you interested in joining?

Thanks for your attention on this matter @coderjoe ! Please feel free to direct us to any information that might help us align our thinking with the the plans for the gem as a whole.

[rdubya]

@jayjlawrence yeah I can chat at some point. I can see the concern with maintaining a platform that the maintainers don't use, because in the end, if we all stop using jruby/sqlserver for some reason, they'll be stuck with it.

[coderjoe]

1. The TinyTDS dependency was removed from the standard MRI/Ruby gem platform target. I'm

Correct me if I'm wrong but there are alternatives to tiny_tds when running under MRI. Secondly, when building for different platforms you most likely have different dependencies. I've become well acquainted (not necessarily by choice) with bundler's handling of platform specific dependencies and it handles it properly. Within gemfiles I am less confident - I'd look to a gem like FFI that handles this type of issue. And this discussion speaks to the point.

You're absolutely right! In fact the FFI gem is exactly the model I used in my suggestion. They do in fact bundle a different gemspec for java than they do for MRI.

Please see their Rakefile task gem:java as an example of producing a separate gemspec based on a different platform linked below.

https://github.com/ffi/ffi/blob/0ee4f7931bb56c7d0e11ef66c51711cb59a80bc4/Rakefile#L154

sqljdbc4 is a Jar from Microsoft. The reference here is just an interim handling of this artifact. I believe there's a 'modern way' to bundle Jar's these days.

That's good to know. I would expect to see that new modern way of bundling jar files in the eventual PR then. Either way if I were the maintainer I would never approve a private github gem implementation for a public gem even as a stop-gap that so many people are depending on. It just introduces way too much risk to the project.

I am keenly interested in the advancement of JDBC support for MS SQL. It's very core to what I do and am prepared to put a few bucks on the table if required. I had tried to push a little bit on the JDBC gem, when I had the cycles. Then I learnt about this work.

That's fine, and FWIW I believe that @metaskills has a Patreon listed in the project readme in order to fund his ability to increase the amount of time that goes into the project.

Quite frankly though as a volunteer contributor I'd much prefer your contribution to the project than your bucks as it will likely have a larger impact (unless you can fund actual engineering time).

The fact that work has been stalled for a year is a concern. Significant effort has been invested and appears to be blocked waiting on any signal from the core team.

It's worth noting that the build on this PR has also been broken for the entire year of review. Even if it was completed it wouldn't be fully reviewed or merged unless there is a working build and test cycle. I don't blame anybody for that. Slow review probably killed @iaddict's interest. But I personally wouldn't review without a successful build/test cycle except if the contributor is explicitly asking for help getting the build working. (Edited: I shouldn't speak for Metaskills)

Your concern is still valid though, but it's best to resolve that concern through direct contribution. As always, PRs welcome.

Perhaps what would help is some interactive conversation to ... [snip]

If the conversation may be valuable to @rdubya 's efforts, then lets please constrain that conversation to his eventual PR. I'd suggest given the importance of this to your project that we focus on "working" first and "java/jruby specific enhancements" second.

Thanks for your attention on this matter @coderjoe ! Please feel free to direct us to any information that might help us align our thinking with the the plans for the gem as a whole.

I'm just happy I was able to spawn a productive discussion and potentially avoid another ecosystem fork. I'll be even more excited to see @rdubya's contributions. 👍

[kares]

@rdubya and @iaddict happy to chat more if you guys have the time. @kares are you interested in joining?

sorry, not really into chatting more ... was interested before, as mentioned, to move AR-JDBC forward. atm I can mostly help out with setting up a standalone JDBC adapter gem depending on AR-JDBC, but I am not into any SQLServer work nor am passionate for MS products, so maybe ain't the best choice.

that being said I am happy to do paid oss work if no one else ends up picking up.

[JesseChavez]

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.