Skip to content
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.

Sign up for GitHub

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

Jump to bottom

TypeDecorator.process_bind_param can return more than Optional[Text] #205

Open
huonw opened this issue Jan 25, 2021 · 2 comments · May be fixed by #206
Open

TypeDecorator.process_bind_param can return more than Optional[Text] #205

huonw opened this issue Jan 25, 2021 · 2 comments · May be fixed by #206

Comments

@huonw
Copy link

huonw commented Jan 25, 2021

Currently, the process_bind_param method on TypeDecorator[_T] has signature:

sqlalchemy-stubs/sqlalchemy-stubs/sql/type_api.pyi

Line 95 in 8495c22

def process_bind_param(self, value: Optional[_T], dialect: Dialect) -> Optional[typing_Text]: ...

Unfortunately, it looks like this is incorrect: I believe it can return anything that the underlying impl can accept. For instance, in SQLAlchemy's tests there's type decorators that return ints:

class MyNewIntType(types.TypeDecorator):
    impl = Integer

    def process_bind_param(self, value, dialect):
        return value * 10

    def process_result_value(self, value, dialect):
        return value * 10

    def copy(self):
        return MyNewIntType()

The process_bind_param return value should probably be loosened to match the Optional[Any] of its inverse operation process_result_value.

sqlalchemy-stubs/sqlalchemy-stubs/sql/type_api.pyi

Line 96 in 8495c22

def process_result_value(self, value: Optional[Any], dialect: Dialect) -> Optional[_T]: ...

(This probably applies to process_literal_param too.)
huonw added a commit to huonw/sqlalchemy-stubs that referenced this issue Jan 25, 2021
@huonw
Relax TypeDecorator.process_*_param to return Optional[Any]
1226825 
The process_bind_param and process_literal_param methods are called to
do decorator-specific conversion of values, before deferring to the
underlying .impl's conversion methods. This means they can return any
value accepted by those methods, not just str (or typing.Text). These
are related to the process_result_value method, which is effectively
the inverse, doing decorator-specific conversion of the output of the
underlying .impl, and indeed this method accepts value: Optional[Any],
representing the unknown type of the output of the underlying .impl.

Unfortunately, modelling this accurately is likely to be
impossible (or at least, much more difficult), because, for instance,
the `impl` property itself can only be typed as `Any`, let alone the
input/output type it uses.

Fixes dropbox#205
huonw added a commit to huonw/sqlalchemy-stubs that referenced this issue Jan 25, 2021
@huonw
Relax TypeDecorator.process_*_param to return Optional[Any]
9dd367b 
The process_bind_param and process_literal_param methods are called to
do decorator-specific conversion of values, before deferring to the
underlying .impl's conversion methods. This means they can return any
value accepted by those methods, not just str (or typing.Text). These
are related to the process_result_value method, which is effectively
the inverse, doing decorator-specific conversion of the output of the
underlying .impl, and indeed this method accepts value: Optional[Any],
representing the unknown type of the output of the underlying .impl.

Unfortunately, modelling this accurately is likely to be
impossible (or at least, much more difficult), because, for instance,
the `impl` property itself can only be typed as `Any`, let alone the
input/output type it uses.

Fixes dropbox#205
@huonw huonw linked a pull request Jan 25, 2021 that will close this issue
Open
@watterso
Copy link

I've run into this as well. My use case is a TypeDecorator that allows for storing unix timestamp as a float in the database and making it available in python as a utc datetime. This is done by making a TypeDecorator that takes a datetime in process_bind_param and returns a float with the situation flipped in process_result_value (it accepts and float and returns a datetime):

class FloatDateTime(TypeDecorator):
    impl = Float

    def process_bind_param(
        self, value: Optional[datetime.datetime], _
    ) -> Optional[float]:
        if value is None:
            return None
        return value.timestamp()

    def process_result_value(
        self, value: Optional[float], _
    ) -> Optional[datetime.datetime]:
        if value is None:
            return None

        return datetime.datetime.fromtimestamp(value, tz=datetime.timezone.utc)

I think the biggest downside to your proposed solution is that introducing Any into the mix adds ambiguity, I'm not expert enough in expressing python types to submit a code change (like you did in #206 ), but I think we want something would work like this:

  • class MyNewIntType(types.TypeDecorator[Integer]): ... - your use case where process_bind_param and process_result_value have the same type for input and outputs
  • class FloatDateTime(TypeDecorator[Float, datetime.datetime]): ... my use case where the db-side type is not the same as the python-side type

@huonw
Copy link
Author

huonw commented Apr 26, 2021

@watterso I tried to do something like that, but I couldn't work out how express it on the TypeDecorator superclass. It'll take someone more experience with mypy/type hints than me to get it to work!

I think Any is a strict improvement over the current situation of str, because these functions are generally small and not reused, meaning the Any-ness doesn't propagate very fa (that is, there's a relatively small scope for mistakes).

This was referenced Apr 26, 2021
Open
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Relax TypeDecorator.process_*_param to return Optional[Any] huonw/sqlalchemy-stubs
2 participants
@huonw @watterso