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

Bug: Optional constrained types do not work #493

Closed
1 of 4 tasks
jmspereira opened this issue Jan 22, 2024 · 8 comments · Fixed by #499
Closed
1 of 4 tasks

Bug: Optional constrained types do not work #493

jmspereira opened this issue Jan 22, 2024 · 8 comments · Fixed by #499
Assignees
Labels
bug Something isn't working

Comments

@jmspereira
Copy link

jmspereira commented Jan 22, 2024

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

  • 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 jmspereira added the bug Something isn't working label Jan 22, 2024
@albertferras-vrf
Copy link

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

@guacs
Copy link
Member

guacs commented Jan 23, 2024

@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
Copy link
Member

guacs commented Jan 23, 2024

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
Copy link
Author

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

@guacs
Copy link
Member

guacs commented Jan 25, 2024

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

@guacs guacs self-assigned this Jan 25, 2024
@guacs guacs changed the title Bug: Handle unions with conlist Bug: Optional constrained types do not work Jan 25, 2024
@alessio-locatelli
Copy link

alessio-locatelli commented Jan 25, 2024

@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
Copy link
Member

guacs commented Jan 26, 2024

@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
Copy link
Member

guacs commented Feb 7, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants