Bug: Optional constrained types do not work

jmspereira commented 7 months ago

Description

Hey, I have a model that has a field that is a conlist or a None, in the last version of polyfactories, that stopped working because it is generating the wrong data (it generates a list with 3 lists inside instead of a list with 3 elements).

URL to code causing the issue

No response

MCVE

class B(BaseModel):
   field_a: conlist(float, min_length=3, max_length=3) | None 

class A(BaseModel):
   class_b: B

Steps to reproduce

No response

Screenshots

No response

Logs

No response

Release Version

2.14.1

Platform

[X] Linux
[ ] Mac
[ ] Windows
[ ] Other (Please specify in the description above)

[!NOTE]
While we are open for sponsoring on GitHub Sponsors and OpenCollective, we also utilize Polar.sh to engage in pledge-based sponsorship.

Check out all issues funded or available for funding on our Polar.sh dashboard

If you would like to see an issue prioritized, make a pledge towards it!

We receive the pledge once the issue is completed & verified

This, along with engagement in the community, helps us know which features are a priority to our users.

albertferras-vrf commented 7 months ago

I've also encountered this issue. Downgrading to 2.14.0 fixes it

guacs commented 7 months ago

@jmspereira thanks for reporting this!

I just wanted to confirm something. This only happens when it's an optional type right? That is, conlist(int) | None is the only time I'm able to reproduce this. If the annotation is conlist(int) | conlist(str) then there are no validation errors from pydantic.

guacs commented 7 months ago

Some observations from when I was reproducing this:

this only happens for optional types
- Annotated[list[int], MinLen(10)] | None will cause a validation error
- Annotated[list[int], MinLen(10)] | Annotated[list[str], MinLen(10)] will NOT cause a validation error
the issue arises when the optional type is a union with at least two of the union types having constraints
- Annotated[list[int], MinLen(10)] | Annotated[list[str], MinLen(10)] | None will cause a validation error
- Annotated[list[int], MinLen(10)] | list[str] | None will NOT cause a validation error even if the created list is a list of integers

jmspereira commented 7 months ago

Hey @guacs, yes it only happens with a union with optional types!

guacs commented 7 months ago

Okay, so the reason for this was that the constraints for union types weren't being properly parsed after the changes in #491. I have a fix for this, and it should be included in the next release :)

alessio-locatelli commented 7 months ago

@guacs In my opinion, it will be respectful to other developers to yank the 2.14.1 release on PyPI and release a new bug-free version (2.14.2) when it is ready. I hope you have the required permissions on PyPI. I think this is a good practice in some cases because now the released version only fails CI tests in other projects. Thank you for your work on this.

Another one example (perhaps already covered by the cases above):

from typing import Annotated
from annotated_types import MinLen
from pydantic import BaseModel
from polyfactory.factories.pydantic_factory import ModelFactory

class Person(BaseModel):
   pets: dict[int, Annotated[list[Annotated[str, MinLen(1)]], MinLen(1)]]

class PersonFactory(ModelFactory[Person]): ...

person_instance = PersonFactory.build()

guacs commented 7 months ago

@guacs In my opinion, it will be respectful to other developers to yank the 2.14.1 release on PyPI and release a new bug-free version (2.14.2) when it is ready. I hope you have the required permissions on PyPI. I think this is a good practice in some cases because now the released version only fails CI tests in other projects. Thank you for your work on this.

Another one example (perhaps already covered by the cases above):
from typing import Annotated
from annotated_types import MinLen
from pydantic import BaseModel
from polyfactory.factories.pydantic_factory import ModelFactory

class Person(BaseModel):
   pets: dict[int, Annotated[list[Annotated[str, MinLen(1)]], MinLen(1)]]

class PersonFactory(ModelFactory[Person]): ...

person_instance = PersonFactory.build()

Yeah I think that'd be a good idea actually. Especially since the broken release only has the faulty PR as well.

@provinzkraut what do you think? I think you have the permission to yank right?

guacs commented 7 months ago

If anyone of y'all (@jmspereira, @albertferras-vrf or @olk-m) could test #499, that'd be great. I've added the tests for the cases covered here, but I may have missed some edge case (or an obvious one 😄).

litestar-org / polyfactory