polyfactory icon indicating copy to clipboard operation
polyfactory copied to clipboard

Bug: Optional constrained types do not work

Open jmspereira opened this issue 1 year ago • 7 comments

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.
Fund with Polar

jmspereira avatar Jan 22 '24 11:01 jmspereira

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

albertferras-vrf avatar Jan 23 '24 09:01 albertferras-vrf

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

vkcku avatar Jan 23 '24 14:01 vkcku

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

vkcku avatar Jan 23 '24 15:01 vkcku

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

jmspereira avatar Jan 23 '24 16:01 jmspereira

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 :)

vkcku avatar Jan 25 '24 01:01 vkcku

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

alessio-locatelli avatar Jan 25 '24 11:01 alessio-locatelli

@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?

vkcku avatar Jan 26 '24 06:01 vkcku

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

vkcku avatar Feb 07 '24 13:02 vkcku