search

mkleehammer / pyodbc

Python ODBC bridge
https://github.com/mkleehammer/pyodbc/wiki
MIT No Attribution
2.88k stars 562 forks source link

Allow python exception handling for empty iterator #1207

Closed ndmlny-qs closed 9 months ago

ndmlny-qs commented 1 year ago

This commit allows Python to handle empty iterators given to a pyodbc connection instead of issuing a segfault. See the below code for an example of what would normally result in a segfault.

import collections
import pyodbc

class MySequence(collections.abc.Sequence):
    def __getitem__(self, index):
        ...

    def __len__(self):
        return 1

connection = pyodbc.connect(...)
connection.execute("SELECT ?, ?", 123, MySequence()).fetchone()
mkleehammer commented 1 year ago

Well, we certainly don't want any segfaults. Appveyor builds failed for some reason. It looks like the new exception is being raised in all of the SQL Server tests or something.

Once that is worked out, I'm wondering if ValueError would be more appropriate.

ndmlny-qs commented 1 year ago

I see where the tests need to be updated, and will have to look at them later next week for a fix.

The tests do show a ValueError being raised, https://ci.appveyor.com/project/mkleehammer/pyodbc/builds/46853185/job/tkldqoprtygxr1vp#L702, unless I'm reading that incorrectly. I'm new to the Python ABI so if I should have chosen a different way to raise the exception, then I'm happy to switch it. Just let me know what you would like changed and I'll fix it.

ndmlny-qs commented 11 months ago

I modified the segfault so that the return propagates back to the Python code that calls it. We have GitHub mimic Appveyor tests here https://github.com/Quansight/pyodbc/actions/runs/5592899278 and they all were green, except for Python 2.7, as it is deprecated. I'm not sure why the tests here do not pass. If you have any ideas, I'm happy to spend time investigating and implementing any fixes you would like done.

ndmlny-qs commented 9 months ago

closing so I can use the 5.0 version of pyodbc for this update