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

Add ODBC support for SQL Server Adapter v7.0 #17

Merged
merged 14 commits into from
Oct 23, 2023

Conversation

lavika
Copy link

@lavika lavika commented Oct 10, 2023

This pull request brings back ODBC support to activerecord-sqlserver-adapter gem which was revoked sometimes back,

With every rails upgrade, we also try to upgrade activerecord-sqlserver-adapter and bring ODBC support back to the repo.

This pull request contains total 13 commits out of which initial 9 commits are responsible for bring back ODBC and are being cherry-picked for every other upgrade as well.
The other four commits are specific to version 7 and needs specific review

Note - CI/CD checks are breaking here as there is a trouble installing ruby-odbc gem. The same has been happening in earlier releases as well. We have tested the changes internally and everything looks good. So, CI/CD checks can be ignored.

Lalitha and others added 13 commits October 5, 2023 16:27
- use SET NOCOUNT ON so that the number of rows affected by INSERT
  statement is returned instead of the number of rows affected by the trigger
- Refer https://techcommunity.microsoft.com/t5/SQL-Server/UPDATE-with-OUTPUT-clause-8211-Triggers-8211-and-SQLMoreResults/ba-p/383457
- TinyTDS gem for windows is compiled with OpenSSL with a version that has vulnerabilities
- Updating to use ruby-odbc as dependency, as this forked repository is for connecting to SQLServer in ODBC mode
…ed as binary / ASCII-8Bit

- Added a check to make sure the object is String. ActiveRecord tests passes objects which are other than String.
- Added a check to make sure we are not modifying the frozen object. There are tests which passes frozen objects.
- Error found: ActiveRecord::ValueTooLong: ODBC::Error: 22001 (8152)[Microsoft][ODBC Driver 17 for SQL Server][SQL Server]String or binary data would be truncated.
- Require odbc_utf8 so that  UTF-8 variant of the module is in use, and string data is automatically converted to/from Unicode
- Error found: MigrationTestSQLServer::For changing column#test_0002_not drop the default contraint if just renaming: TypeError: no implicit conversion of String into Integer
- array needs to be flattened before calling select method
- Error found: SerializedAttributeTest#test_json_read_legacy_null: ActiveRecord::RecordNotFound: Couldn't find Topic without an ID
- updated sql_for_insert method to use OUTPUT clause to get the ID of the last inserted record and with a single sql statement
…he Ruby::ODBC module

- The UTF8 version does not appear to support multiple statements in a single query. So things like "query; SELECT @@rowcount" do not work
ActiveRecord SQLServer Adapter defaults to TinyTDS. And the support for
ODBC has been removed. This is the effort to support Adapter with ODBC
Driver again.

- ruby-odbc is now pointed to https://github.com/cloudvolumes/ruby-odbc
- restricted sqlite3 version to < 1.4
Copy link

@zeeamber zeeamber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lavika lavika changed the title Bring ODBC back for SQL Server Adapter Add ODBC support for SQL Server Adapter v7.0 Oct 20, 2023
@sukeerthiadiga
Copy link

Looks good to me.

@raj-sharan
Copy link

I think version should be 7.0.4.0.odbc in VERSION file

@raj-sharan
Copy link

Have you added this or automatically created in CHANGELOG.md file. I guess it is created with using a gem, not sure, but I see pull request link "#17" is missing in this file.

v7.0.4.0.odbc

Added

  • ODBC restoration.

@lavika lavika merged commit c171ca8 into cloudvolumes:7-0-stable-with-odbc Oct 23, 2023
0 of 3 checks passed
@smlsml
Copy link

smlsml commented Oct 23, 2023

Good work all, you should try to get the tests working and showcase this to the community. Obviously it is still needed, and with a little extra effort, it can be brought back to the gem officially!

@tojified
Copy link

Good work all, you should try to get the tests working and showcase this to the community…

Good to hear from you! -toj

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.

6 participants