Skip to content

pymysql: fetch result type #5567

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

Closed
Akuli opened this issue Jun 3, 2021 · 4 comments · Fixed by #5652
Closed

pymysql: fetch result type #5567

Akuli opened this issue Jun 3, 2021 · 4 comments · Fixed by #5652

Comments

@Akuli
Copy link
Collaborator

Akuli commented Jun 3, 2021

import pymysql

connection = pymysql.connect(password='hunter2', cursorclass=pymysql.cursors.DictCursor)
cursor = connection.cursor()
reveal_type(cursor)  # pymysql.cursors.Cursor, but DictCursor at runtime
reveal_type(cursor.fetchone())  # Union[builtins.tuple[Any], builtins.dict[builtins.str, Any], None]

cursor.execute("select 1 as foo")
foo = cursor.fetchone()['foo']  # error: Value of type "Union[Tuple[Any, ...], Dict[str, Any], None]" is not indexable

Ideally cursor would have type pymysql.cursors.DictCursor, so that cursor.fetchone() would return Dict[str, Any] instead of an annoying union.

Because cursor.fetchone() can return None but often checking for it is annoying, it could instead return Union[Dict[str, Any], Any]. See #5528 for a similar situation.

We could make connection generic and overload connect() to figure out what type connection.cursor() returns. Something like this (omitting lots of irrelevant stuff):

_CursorT = TypeVar("_CursorT", bound=pymysql.cursors.Cursor)

class Connection(Generic[_CursorT]):
   def cursor(self) -> _CursorT: ...

@overload
def connect(*, cursorclass: Type[_CursorT]) -> Connection[_CursorT]: ...
@overload
def connect(*, cursorclass: Type[pymysql.cursors.Cursor] = ...) -> Connection[pymysql.cursors.Cursor]: ...

The downside is that now all other arguments of connect() have to be copy/pasted into two places, and there's many of them.

@srittau
Copy link
Collaborator

srittau commented Jun 3, 2021

What are the situations where you wouldn't want to check cursor.fetchone() for None? I guess when you are checking cursor.rowcount beforehand, but in general I feel that not checking the result of fetchone() for None is a mistake that the type checker should catch.

@Akuli
Copy link
Collaborator Author

Akuli commented Jun 3, 2021

Here's my actual code where I don't want to check for it:

        self._cursor.execute("select connection_id() as id")
        self._connection_id = self._cursor.fetchone()["id"]  # type: ignore

As far as I can tell, fetchone() will always succeed in this case, since the select always outputs a row. But I think this is somewhat rare and it might be better to just return Optional[Dict[str, Any]] (but still, not a Union containing tuples too).

@srittau
Copy link
Collaborator

srittau commented Jun 3, 2021

You are right, I actually have similar code, where I know the exact number of rows and columns returned (for example SELECT COUNT(*) FROM foo). But since this is a fairly sensitive I do prefer the Optional (or | None), despite the slight inconvenience, even though this is a violation of our "prefer false negatives over false positives" principle.

@srittau
Copy link
Collaborator

srittau commented Jun 3, 2021

Haha, I just opened my IDE and it automatically opened to this piece of code:

        query = (
            "SELECT "
            "COUNT(*), "
            "SUM(CASE WHEN benutzer.kuerzel LIKE 'B%' THEN 0 ELSE 1 END) "
            "FROM patienten LEFT OUTER JOIN benutzer "
            "ON patienten.behandlerid = benutzer.solid "
            "WHERE passiv = 0 AND behandelt_seit >= ? AND behandelt_seit < ?"
        )
        rows = store.execute_sql(query, [dt1, dt2])
        assert rows is not None
        assert len(rows) == 1
        return rows[0][0], rows[0][1] or 0

(Although it's using SQLalchemy, not pymysql directly.)

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 a pull request may close this issue.

2 participants