coleifer / peewee

a small, expressive orm -- supports postgresql, mysql, sqlite and cockroachdb
http://docs.peewee-orm.com/
MIT License
11.06k stars 1.37k forks source link

shortcuts.ReconnectMixin is not compatible anymore with playhouse.postgres_ext.PostgresqlExtDatabase in version 3.17.6 #2925

Closed Bahus closed 2 weeks ago

Bahus commented 2 weeks ago

Hello! You have added named_cursor argument to execute_sql method in latest release

https://github.com/coleifer/peewee/compare/3.17.5...3.17.6#diff-05c9e68bc3e35107ad643797f22b363e574bc5c3f0a8b13377eeb17d92e07325R503

Unfortunately this change brakes shortcuts.ReconnectMixin since no such argument accepted in

https://github.com/coleifer/peewee/blob/6d59dd6c438c2e86ad10e0203e1734e71dfd4e2e/playhouse/shortcuts.py#L253

Therefore the following error appears:

 TypeError: ReconnectMixin.execute_sql() got an unexpected keyword argument 'named_cursor'

Is this the right time to add a Database interface (abstract class)?

Bahus commented 2 weeks ago

Duplicate

coleifer commented 2 weeks ago

How many people are using this with Postgres ?!

Bahus commented 2 weeks ago

I could not find any indication that it can not be used with Postgres. Moreover - why not? It has a generic interface and can be easily extended. I use it like this:

class PostgresReconnectMixin(ReconnectMixin):
    """Mixin that allows to reconnect if connection in pool is stale."""

    reconnect_errors = (
        (peewee.InterfaceError, 'connection already closed'),
    )

So it was a surprise when a minor peewee release broke our CI/CD for a while.

Bahus commented 2 weeks ago

Moreover there is code example how to use it with postgres:

https://github.com/coleifer/peewee/blob/6d59dd6c438c2e86ad10e0203e1734e71dfd4e2e/playhouse/shortcuts.py#L238-L243

So I would consider this issue as a backward incompatible change.

coleifer commented 2 weeks ago

The more relevant lines are in the docstring:

https://github.com/coleifer/peewee/blob/6d59dd6c438c2e86ad10e0203e1734e71dfd4e2e/playhouse/shortcuts.py#L223-L226

Given the warning, I think it falls under caveat emptor.