rails-sqlserver / activerecord-sqlserver-adapter

SQL Server Adapter For Rails
MIT License
972 stars 557 forks source link

Still getting "COMMIT TRANSACTION request has no corresponding BEGIN TRANSACTION" #766

Open glennfu opened 4 years ago

glennfu commented 4 years ago

I've been using this library for a long time on a database with compatibility level 100, and I just recently connected to another one with compatibility level 140. It's this one that's giving me trouble:

FactoryBot.create(:calendar)

  AppointmentStatus Load (0.6ms)  EXEC sp_executesql N'SELECT TOP 1  [appointment_status].* FROM [appointment_status] WHERE [appointment_status].[name] = @0  ORDER BY [appointment_status].[id] ASC ', N'@0 varchar(35), @1 int', @0 = 'Open', @1 = 1  [["name", "Open"], ["LIMIT", 1]]
  SQL (0.3ms)  BEGIN TRANSACTION
  SQL (0.5ms)  select newid()
   (1.2ms)  select newid()
Calendar Exists (0.5ms)  EXEC sp_executesql N'SELECT TOP 1  1 AS one FROM [calendar] WHERE [calendar].[id] = @0  ORDER BY [calendar].[id] ASC ', N'@0 uniqueidentifier, @1 int', @0 = '5B0B8280-6237-4791-A023-A7989E9634DF', @1 = 1  [["id", "5B0B8280-6237-4791-A023-A7989E9634DF"], ["LIMIT", 1]]
  SQL (1.9ms)  EXEC sp_executesql N'INSERT INTO [calendar] ([id], [start_date], [stop_date], [short_notes], [subject], [appointmentstatusid], [date_modified], [staff]) VALUES (@0, @1, @2, @3, @4, @5, @6, @7)', N'@0 uniqueidentifier, @1 datetime2(7), @2 datetime2(7), @3 varchar(1000), @4 varchar(255), @5 uniqueidentifier, @6 datetime2(7), @7 varchar(max)', @0 = '5B0B8280-6237-4791-A023-A7989E9634DF', @1 = '04-09-2020 15:37:39.0', @2 = '04-09-2020 12:22:39.0', @3 = 'Optio dolores ab. Exercitationem eos veniam. Maiores nostrum voluptate.', @4 = 'Subject', @5 = 'D0BF3A9C-BA9C-4A45-8B30-A98500A81501', @6 = '04-09-2020 19:54:39.4235502', @7 = '8CFFF4E7-CBEE-4E44-8431-61170E4CD108'  [["id", "5B0B8280-6237-4791-A023-A7989E9634DF"], ["start_date", "04-09-2020 15:37:39.0"], ["stop_date", "04-09-2020 12:22:39.0"], ["short_notes", "Optio dolores ab. Exercitationem eos veniam. Maiores nostrum voluptate."], ["subject", "Subject"], ["appointmentstatusid", "D0BF3A9C-BA9C-4A45-8B30-A98500A81501"], ["date_modified", "04-09-2020 19:54:39.4235502"], ["staff", "8CFFF4E7-CBEE-4E44-8431-61170E4CD108"]]
  SQL (0.7ms)  COMMIT TRANSACTION

ActiveRecord::StatementInvalid (TinyTds::Error: The COMMIT TRANSACTION request has no corresponding BEGIN TRANSACTION.: COMMIT TRANSACTION)

The sql looks fine to me, so I'm not sure what to think. I'm using gem version 5.0.8 because due to other limitations I can't upgrade this project beyond Rails 5.0. I tried ignoring that and upgrading to Rails 5.1 and upgrading this gem to the equivalent 5.1.# version but I still get the same error.

I actually have my test database in a container on dockerhub, but I'm not sure the best way to create a test app to reproduce this for you. Is there a base you all prefer to build off of for repro tests?

glennfu commented 4 years ago

I'm wondering if the insert is actually raising an error, but the adapter isn't trying to grab or report on the error. Googling for "sp_executesql" "COMMIT TRANSACTION request has no corresponding BEGIN TRANSACTION" I see a lot of posts about error handling, but they all vary wildly in how they address the issue. I'm not sure what to suggest but I do suspect I have a hidden error in here that I don't know how to access.

glennfu commented 4 years ago

Running directly in SqlServer I see:

EXEC sp_executesql N'INSERT INTO [calendar] ([subject]) VALUES (@0)', N'@0 varchar(255)', @0 = 'test'

The XML parse error 0xc00ce558 occurred on line number 0, near the XML text "".
Msg 6602, Level 16, State 2, Procedure sp_xml_preparedocument, Line 1 [Batch Start Line 0]
The error description is 'XML document must have a top level element.'.
The statement has been terminated.

Completion time: 2020-04-10T10:36:59.0913476-04:00

Looks like I have some other issues to figure out (I'm a Rails guy not a sqlserver guy!), but it does seem like this library could be catching and reporting these errors.

glennfu commented 4 years ago

Just in case anyone is wondering, my error was that the column resourceid, type varchar(max) is supposed to be xml and there's an OnCalendarInsert Trigger trying to parse that.

So for this Issue, I think it would be great if there were a way to rescue and report this error in the same way the SqlServer admin was able to when I ran the query, vs simply showing the misleading message about the COMMIT vs BEGIN transaction lines.

aidanharan commented 4 years ago

@glennfu In order to investigate this it would be best if you could submit a PR with a failing test that demonstrates the issue you are having.

glennfu commented 4 years ago

I think I could do that, however I'm not sure I have permission to share the affected DB. Do you have a way of setting up a DB for the purpose of this test? There's a table with an OnCalendarInsert Trigger that's raising an exception.

The problematic part is this:

--Create an internal representation of XML
EXEC sp_xml_preparedocument @idoc OUTPUT, @resourceid; 

Insert into appointment_staff(staffid, calendarid)
SELECT staffid, @calendarid AS calendarid
FROM openxml(@idoc, '/ResourceIds/ResourceId', 0) with (staffid UniqueIdentifier '@Value');

--Remove internal representation of XML
EXEC sp_xml_removedocument  @idoc

So if you pass nil or a blank string the XML fails to parse and it raises an exception. From the Rails side, all I'm doing is a Calendar.create and it goes boom.

If you can give me some guidance on how to set up a test with that trigger in place, or any trigger that raises an exception when called, then I think I could do the rest.

aidanharan commented 4 years ago

There are details on how to run the adapters unit tests at https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/blob/master/RUNNING_UNIT_TESTS.md

You could add a test that:

  1. Creates a trigger that tries to parse XML in a varchar column.
  2. Adds a record with an invalid XML value
  3. Asserts that you are expecting a specific exception.
glennfu commented 4 years ago

The unit test instructions assume I have sqlserver running locally, but I don't since I'm on a Mac. I actually run my stuff locally similar to the way your ci config is setup, where it connects to a docker image. I tried to leverage what you have there but I get errors like ERROR: Service 'ci' failed to build: manifest for railssqlserver/activerecord-sqlserver-adapter:5.0.8 not found: manifest unknown: manifest unknown for every version I could think to try. I guess that dockerhub account isn't public?

aidanharan commented 4 years ago

See https://github.com/aidanharan/activerecord-sqlserver-adapter-dev-box

wpolicarpo commented 4 years ago

No need to create a PR (unless you have a solution). You can create a minimal reproducible script following the script used in #616.

I will add an issue template so people can follow this pattern.

glennfu commented 4 years ago

So I have vagrant installed now and I installed that dev box which I can ssh into. However when looking at the instructions for setting up the user/database, I don't know how to get into sqlserver to execute those commands.

I think this would all be a lot easier if I could just run the CI commands since it already is defined to connect to a sqlserver base. The only hangup seems to be that I can't build the Dockerfile.ci because FROM railssqlserver/activerecord-sqlserver-adapter:5.0.8 can't be found.

If that can be made public, then anyone could simply pull this repo, and run the docker-compose command to build and run specs right away with no setup besides having docker-compose installed.

aidanharan commented 4 years ago

Agree with @wpolicarpo that creating a script like the one used in https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/issues/616 might be best.

Just so you know the user/databases were setup as part of the vagrant bootstrap in https://github.com/aidanharan/activerecord-sqlserver-adapter-dev-box/blob/master/bootstrap_sqlserver.sh#L37 You can connect to the SQL Server database on that vagrant box using localhost:1433.

wpolicarpo commented 4 years ago

@glennfu the Docker image is per ruby version not the SQL server adapter version. What ruby version are you running?

You might not find an image for all rubies because those images are something new I created after Rails 5.2.x to fix intermittent CI failures because FreeTDS failed to install due to network issues.

Anyway, I created an issue template for future references and also a wiki entry explaining how to create a minimal reproducible script. See How to report a bug.