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.

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

Unintentional type casting with union types when creating instances #379

Closed
david-wahlstedt opened this issue Dec 14, 2022 · 11 comments
Closed

Comments

@david-wahlstedt
Copy link

Hi, I encountered some problems with how union types are handled. If I have a field x : Union[int, float] and set it to 1.9 inside the constructor, it gets truncated into 1: int. But if I assign the field x=1.9 after instance creation, it is preserved a a float with correct value. The same the other way around: if I have y : Union[float, int] and assign it to 2 inside the constructor, it gets casted into 2.0: float. But if I do x=2 after instance creation, it is preserved as an int. I also tried to put Optional inside or outside the union, and it changed the behavior as well. Furthermore I have noticed that if you create an instance with _auto_sync on (default), it is not saved in Redis until you assign some field to it after creation. I don't know if this might be a pydantic issue rather than a pydantic-aioredis one. But I start here ans see what you think. I have written a pytest script that reproduces the issue, and I tried to make it small to isolate the problem. Here it is:

from typing import Union

from pydantic_aioredis import Model, RedisConfig, Store
from syncer import sync # used to save from writing some asyncio code

"""Notes:

pip package dependencies: syncer, pytest

Run with

pytest test_typing.py -v -s
(the -s is to have stdout visible during test, and -v makes it more verbose)

The test has no cleanup of  Redis keys, sorry!
Keys affected (hashes):
  "floatinttest:__index"
  "floatinttest:test_float_int"
  "floatinttest:test_float_int_assign_after"
  "intfloattest:__index"
  "intfloattest:test_int_float_assign_after"
  "intfloattest:test_int_float_assign_inside"
"""


redis_config = RedisConfig()

store = Store(
    name="test_typing",
    redis_config=redis_config,
)


class FloatIntTest(Model):
    _primary_key_field: str = "key"

    key: str
    float_int: Union[float, int]  # fails
    # float_int: Optional[Union[float,int]] # passes
    # float_int: Union[Optional[float],Optional[int]] # fails


class IntFloatTest(Model):
    _primary_key_field: str = "key"

    key: str
    int_float: Union[int, float]  # fails
    # int_float: Optional[Union[int,float]] # fails
    # int_float: Union[Optional[int],Optional[float]] # fails


store.register_model(FloatIntTest)
store.register_model(IntFloatTest)

"""Integers gets cast into floats unintentionally"""


def test_float_int_assign_inside():
    key = "test_float_int"
    instance = FloatIntTest(
        key=key,
        float_int=2,  # gets cast to 2.0
    )
    sync(instance.save())  # this operation doesn't affect the test outcome
    # Fails !
    assert isinstance(instance.float_int, int)


def test_float_int_assign_after():
    key = "test_float_int_assign_after"
    instance = FloatIntTest(
        key=key,
        float_int=2,  # gets cast to 2.0
    )
    print(f"{instance.float_int=}")  # is float here
    instance.float_int = 1
    # Passes !
    assert isinstance(instance.float_int, int)


"""Floats gets cast (and truncated) into integers unintentionally"""


def test_int_float_assign_inside():
    key = "test_int_float_assign_inside"
    instance = IntFloatTest(
        key=key,
        int_float=2.9,  # gets cast into and truncated to 2
    )
    sync(instance.save())  # this operation doesn't affect the test outcome
    # Fails !
    assert isinstance(instance.int_float, float)


def test_int_float_assign_after():
    key = "test_int_float_assign_after"
    instance = IntFloatTest(
        key=key,
        int_float=2.9,
    )
    print(f"{instance.int_float=}")
    instance.int_float = 1.9
    # Passes !
    assert isinstance(instance.int_float, float)

@david-wahlstedt
Copy link
Author

This issue seems related to this one in pydantic.
So maybe it belongs there. But I still wonder why there is a difference when you assign the field in the constructor or afterwards.

@andrewthetechie
Copy link
Owner

Hi David,

Thanks for creating such a detailed issue with tests to replicate it!

I'm going to split this into two issues - one for the Union typing issue and one for the _auto_sync issue.

@andrewthetechie
Copy link
Owner

#380 for the _auto_sync concerns you mentioned. I've added a comment there, tl;dr that behavior is expected.

@andrewthetechie
Copy link
Owner

I reviewed your tests (thanks again!) and added them as a test case to the pydantic_aioredis coverage here https://github.com/andrewthetechie/pydantic-aioredis/blob/main/test/test_union_typing.py

This is an issue in pydantic. To validate that, I wrote two test cases that replicate your issue, but using Pydantic instead of pydantic_aioredis models

https://github.com/andrewthetechie/pydantic-aioredis/blob/main/test/test_union_typing.py#L48-L61
https://github.com/andrewthetechie/pydantic-aioredis/blob/main/test/test_union_typing.py#L99-L112

These tests fail just like yours did when running pydantic==1.10.2

Reading through pydantic/pydantic#4519, it doesn't seem like this is something easy to fix.

@andrewthetechie
Copy link
Owner

@david-wahlstedt I wrote up some docs to cover this case and PR'd it here #383

If you've got a moment, I'd really appreciate if you review it and any feedback you have.

@andrewthetechie
Copy link
Owner

@all-contributors add @david-wahlstedt for test

@allcontributors
Copy link

@andrewthetechie

I've put up a pull request to add @david-wahlstedt! 🎉

@david-wahlstedt
Copy link
Author

@david-wahlstedt I wrote up some docs to cover this case and PR'd it here #383

If you've got a moment, I'd really appreciate if you review it and any feedback you have.

Hi, when I wrote my tests I ran them in another directory where I had pydanti-aioredis installed, and I used Redis locally on my machine. Now I tried running pytest in the root directory of the repo, but it feild with the following message:

pytest -s -v test/test_union_typing.py 
ERROR: usage: pytest [options] [file_or_dir] [file_or_dir] [...]
pytest: error: unrecognized arguments: -n --cov=pydantic_aioredis --cov-report=term-missing --cov-fail-under 97 test/test_union_typing.py
  inifile: /home/davidw/python_repos/pydantic-aioredis/pyproject.toml
  rootdir: /home/davidw/python_repos/pydantic-aioredis

I got the same error when i just ran pytest without arguments.
How am I supposed to run the tests? Sorry for my ignorance!

@andrewthetechie
Copy link
Owner

You need to ensure you have all the dev requirements installed to run the testing. Here's the docs on how to setup a dev environment https://pydantic-aioredis.readthedocs.io/en/latest/development.html#environment-setup

tl;dr is

clone the repo
setup a virtualenv (I use 3.10, but should work with anything 3.7 or higher)
make install

Then you can run the tests directly with pytest or with nox

# this will use the mark on the tests
pytest -m 'union_tests'
# or by file
pytest tests/test_union_typing.py

With nox you can run the full test suite, but you'll need to have python 3.7, 3.8, 3.9, 3.10, and 3.11 available:

nox -s tests

andrewthetechie added a commit that referenced this issue Dec 17, 2022
* docs: add docs about Union types and casting

#379

* docs: add additional example
@andrewthetechie
Copy link
Owner

#383 merged and should show up in read the docs soon. Ty for the PR review

@sveinugu
Copy link

sveinugu commented Jul 1, 2023

Hi, just discovered this issue as it linked to my pydantic issue #4519. Just wanted to inform that these has nothing to do with each other and the seemingly faulty functionality of the current issue is really about configuring pydantic correctly for your use case. E.g.:

# Mini-version of your issue

from typing import Union
from pydantic import BaseModel

class FloatInt(BaseModel):
    a: Union[float, int]

float_int = FloatInt(a=2)
print(float_int.a)  # returns: 2.0
float_int.a = 2
print(float_int.a)  # returns: 2


# Solution

from typing import Union
from pydantic import BaseModel

class FloatInt(BaseModel):
    a: Union[float, int]
    class Config:
        validate_assignment = True

float_int = FloatInt(a=2)
print(float_int.a)  # returns: 2.0
float_int.a = 2
print(float_int.a)  # returns: 2.0

I assume the default configuration is set for performance issues. This is for pydantic v1. v2 arrived yesterday and I haven't checked if this changes anything.

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

No branches or pull requests

3 participants