cTDS incorrectly fails to raise exception?

[amachanic]

According to the documentation, cTDS will not raise an exception for (as I understand it) a statement-terminating error on the SQL Server side (e.g. RAISERROR, or some other error that occurs prior to the end of the batch). However, in practice it seems that something is not working properly:

>>> conn = ctds.connect(...)
>>> cur = conn.cursor()
>>> cur.execute("select convert(int, 'abcd')")
<stdin>:1: Warning: Conversion failed when converting the varchar value 'abcd' to data type int.
>>> cur.execute("select 1/0")
<stdin>:1: Warning: Divide by zero error encountered.

The first error is batch-terminating even on the SQL Server side, so I'd argue that it should always raise a full exception, not just a warning. The second error is statement-terminating on the SQL Server side, but in this case there are no subsequent statements in the batch, so per what's documented this should also raise an exception on the Python side.

This behavior came to my attention because for weeks we've been seeing odd issues where one of our queries seemed to randomly get zero rows back from the database. We discovered yesterday that what was actually happening was that it was the victim in a deadlock - and yet again, cTDS simply raised a warning. That's not a good default behavior at all in my opinion.

I've enabled the fix mentioned in the docs, using the warnings module, and it corrects all of these problems. But I think that it should not be necessary, and that any of these scenarios should produce a proper exception on the Python side, by default. I also think that the conversion of non-batch-terminating warnings to full errors should be internalized, perhaps exposed as a connection-level option, so that users don't have to import a new module to get this behavior. This would also serve to better highlight this behavior, which I'd guess is not at all expected by the vast majority of users.

[joshuahlang]

This looks to be related to how FreeTDS handles error responses from SQL Server. It's been a while since I've gone deep into FreeTDS, but basically the code is currently relying on FreeTDS to indicate whether some request to the DB server failed via a FAIL code returned from the FreeTDS/DB-Lib API ctds relies on. In some cases, such as the ones you mention, that error code is never returned. As such any messages sent back from the server are assumed to be warnings. This is desired for things like PRINT statements, but unexpected in this case as you point out.

At first glance this seems like a FreeTDS issue and the API ctds relies on should return an error result for these statements you mention. I'll dig into this some more, but in the meantime I've started a branch which simply treats these messages as errors instead of warnings: https://github.com/zillow/ctds/tree/issue-84

[amachanic]

I just found another thing worth mentioning here. The workaround using the warnings module has a rather nasty side-effect. Try the following query:

SELECT SUM(x)
FROM (VALUES(NULL),(123)) AS y(x)

In addition to the output, it returns: Warning: Null value is eliminated by an aggregate or other SET operation.

And if the workaround is enabled, that becomes an exception! Does your new branch do that too? I'm going to try turning off ANSI_WARNINGS and see if I can get some balance here, but I'm concerned that there might be some warnings that don't fall into that bucket or whatever. Seems very tough to do an all-or-nothing approach.

[joshuahlang]

The fix referenced above should treat that as a warning because it is severity 10 according to the MSDN docs and only severities > 10 are turned into Python exceptions. The lower severities are kept as Warnings in python, but you shouldn't need the warnings module to treat them as errors once the more severe errors are turned into proper python exceptions.

[amachanic]

Awesome. Any way I can help you test this or otherwise push the change forward?

[joshuahlang]

Yeah, let me get a new version published to test.pypi.org for you to try

[joshuahlang]

I've got a version up on test.pypi.org with this change if you'd like to try it:

pip install -i https://test.pypi.org/simple ctds==1.14.0

[amachanic]

I tested this against my app's workload as well as a few error cases and it seems to be functioning perfectly.

[joshuahlang]

Fixed in 1.14.0