sqlalchemy-stubs icon indicating copy to clipboard operation
sqlalchemy-stubs copied to clipboard

TypeDecorator.process_bind_param can return more than Optional[Text]

Open huonw opened this issue 4 years ago • 2 comments

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

https://github.com/dropbox/sqlalchemy-stubs/blob/8495c229cf8a31d75fcea36090e07c0e4b867605/sqlalchemy-stubs/sql/type_api.pyi#L95

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.

https://github.com/dropbox/sqlalchemy-stubs/blob/8495c229cf8a31d75fcea36090e07c0e4b867605/sqlalchemy-stubs/sql/type_api.pyi#L96

(This probably applies to process_literal_param too.)

huonw avatar Jan 25 '21 05:01 huonw

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

watterso avatar Feb 28 '21 00:02 watterso

@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).

huonw avatar Apr 26 '21 07:04 huonw