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

[REQ][python-nextgen] Use Annotated with constrained types #15464

Closed
robertschweizer opened this issue May 10, 2023 · 14 comments
Closed

[REQ][python-nextgen] Use Annotated with constrained types #15464

robertschweizer opened this issue May 10, 2023 · 14 comments

Comments

@robertschweizer
Copy link
Contributor

robertschweizer commented May 10, 2023

Is your feature request related to a problem? Please describe.

python-nextgen uses pydantic constrained types like conlist, conint etc. to reflect type constraints from the OpenAPI schema in Python.

Constrained types are not directly supported by static analysis tools like pylint and mypy, but can be made understandable for these tools with the Annotated type annotation (pydantic/pydantic#975).

Describe the solution you'd like

Use constructs like Annotated[List[int], conlist(min_items=2)] to tell pydantic about type constraints and let static type checkers know that it's a list in the end (suggested in pydantic/pydantic#975). Note that this does not work for nested lists in pydantic<2 (pydantic/pydantic#4333).

Maybe it is possible to unify this between request parameters and model properties: They could both use Annotated[<type>, Field(<default>, <constraints>)] whenever there is no nested list whose inner type needs to be validated. I believe inner list type constraints can only be specified e.g. as conlist(conint(ge=2)).

Describe alternatives you've considered

In a lot of cases, type constraints can be specified as Field() customization, so standard type annotations can be used. This also allows replicating the title and description. But this does not allow setting constraints for arrays' elements (e.g. conlist(conint(ge=2))).

Pydantic v2's support for annotated-types might make the final type annotations prettier.

@Jay-Madden
Copy link

This is much needed to take full advantage of the nextgen generator. Im working on a combo of PRs to allow for mypy to be used on a generated async kubernetes client. I am willing to add this into my work as its needed.

@Yasu-umi
Copy link

Yasu-umi commented Jun 12, 2023

I have same issue when use stubgen generate-package. (Will stubgen be unnecessary once py.typed issue resolved?)
in this simple case,

{"components":{"schemas":{"Parent":{"properties":{"children":{"items":{"$ref":"#/components/schemas/Child"},"type":"array","minItems":1,"uniqueItems":true}},"required":["children"],"title":"Parent","type":"object"},"Child":{"properties":{"age":{"type":"integer"}},"required":["age"],"title":"Child","type":"object"}}},"info":{"title":"openapi test","version":"0.1"},"openapi":"3.0.2","paths":{"/parents":{"get":{"responses":{"200":{"content":{"application/json":{"schema":{"items":{"$ref":"#/components/schemas/Parent"},"title":"Get Parents","type":"array"}}},"description":"Successful Response"}}}}}}

v6.6 openapi-generator generates

class Parent(BaseModel):
  children: conlist(Child, min_items=1, unique_itmes=True) = Field(...)

but, just can be this, and maybe mypy-friendly

class Parent(BaseModel):
  children: list(Child) = Field(..., min_items=1, unique_itmes=True)

and, I find building "conlist" lines in this repo.

@Jay-Madden
Copy link

Jay-Madden commented Jun 12, 2023

@Yasu-umi I believe it needs to be

class Parent(BaseModel):
  children: List[Child] = Field(..., min_items=1, unique_items=True)

list -> List for py3.7-3.9 compatibility

@Yasu-umi
Copy link

I tested little changes, generated sample by generate-sample.sh is good for me.

But, I don't understand

this does not allow setting constraints for arrays'.

@robertschweizer Could you elaborate?

@robertschweizer
Copy link
Contributor Author

@Yasu-umi I think I meant there is no way to specify constraints on arrays' elements without conlist. I updated the issue description now and added the example conlist(conint(ge=2)).

@Yasu-umi
Copy link

@robertschweizer I checked it and it seems ok!

>>> from pydantic import conint, BaseModel, Field
>>> class Test(BaseModel):
...     ints: list[conint(ge=0)] = Field(..., min_items=1)
...
>>> Test(ints=[1,2])
Test(ints=[1, 2])
>>> Test(ints=[-1, 1])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "pydantic/main.py", line 341, in pydantic.main.BaseModel.__init__
pydantic.error_wrappers.ValidationError: 1 validation error for Test
ints -> 0
  ensure this value is greater than or equal to 0 (type=value_error.number.not_ge; limit_value=0)

@robertschweizer
Copy link
Contributor Author

@robertschweizer I checked it and it seems ok!

But you're still using conint(), because the inner constraint cannot be set with Field() here. I think using Annotated[] is the only option to make these inner constraints work in a way that static type checkers like mypy understand.

@Jay-Madden
Copy link

I have a POC branch in the works for attempting to switch everything to Annotated.

@robertschweizer
Copy link
Contributor Author

That sounds awesome, thanks!

How did you handle nested lists, which can lead to wrong validation errors with Annotated in pydantic<2 (pydantic/pydantic#4333)?

Looking forward to a PR!

@Jay-Madden
Copy link

Jay-Madden commented Jun 14, 2023

@robertschweizer Currently leveraging typing.TYPE_CHECKING as the mechanism for differing typecheck to runtime behaviors for pydantic<1 compatibility

Python 3.11.3 
Type "help", "copyright", "credits" or "license" for more information.
>>> import typing
>>> from pydantic import BaseModel, conlist, conint, Field
>>>
>>>
>>> class Bar(BaseModel):
...     if typing.TYPE_CHECKING:
...         x: list[int]
...     else:
...         x: conlist(conint(lt=10), min_items=1, unique_items=True) = Field(...)
...
>>> a = Bar(x=[1]) # Mypy passes, runtime passes
>>>
>>> b = Bar(x=[])  # Mypy passes, runtime fails
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "pydantic/main.py", line 341, in pydantic.main.BaseModel.__init__
pydantic.error_wrappers.ValidationError: 1 validation error for Bar
x
  ensure this value has at least 1 items (type=value_error.list.min_items; limit_value=1)
>>>
>>> c = Bar(x=['hi']) # Mypy fails, runtime fails
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "pydantic/main.py", line 341, in pydantic.main.BaseModel.__init__
pydantic.error_wrappers.ValidationError: 1 validation error for Bar
x -> 0
  value is not a valid integer (type=type_error.integer)
>>> d = Bar(x=[1, 1]) # Mypy passes, runtime fails
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "pydantic/main.py", line 341, in pydantic.main.BaseModel.__init__
pydantic.error_wrappers.ValidationError: 1 validation error for Bar
x
  the list has duplicated items (type=value_error.list.unique_items)
>>> f = Bar(x=[11]) # Mypy passes, runtime fails
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "pydantic/main.py", line 341, in pydantic.main.BaseModel.__init__
pydantic.error_wrappers.ValidationError: 1 validation error for Bar
x -> 0
  ensure this value is less than 10 (type=value_error.number.not_lt; limit_value=10)
>>>
[I] ➜ mypy -c 'import typing
from pydantic import BaseModel, conlist, conint, Field


class Bar(BaseModel):
    if typing.TYPE_CHECKING:
        x: list[int]
    else:
        x: conlist(conint(lt=10), min_items=1, unique_items=True) = Field(...)

a = Bar(x=[1]) # Mypy passes, runtime passes
b = Bar(x=[])  # Mypy passes, runtime fails
c = Bar(x=["hi"]) # Mypy fails, runtime fails
d = Bar(x=[1, 1]) # Mypy passes, runtime fails
f = Bar(x=[11]) # Mypy passes, runtime fails
'
<string>:13: error: List item 0 has incompatible type "str"; expected "int"  [list-item]
Found 1 error in 1 file (checked 1 source file)

Note: this certainly makes the generated code far uglier, but it does allow for correct behavior with static linters. Im also evaluating a simple .pyi approach which might win out over this. Both are on the table

@Jay-Madden
Copy link

Draft PR here: #15878

@multani
Copy link
Contributor

multani commented Sep 19, 2023

@robertschweizer If you are still interested, could you take a look at the new samples produced by #16624 if let me know if you think this is going in the right direction?

There are still several improvements I'd like to do but hopefully this should solve already some of the issues you mentioned here!

@multani
Copy link
Contributor

multani commented Sep 30, 2023

@robertschweizer I think this one can be closed: #16624 replaced all the previous incompatible types with fully annotated types.

There are still many typing errors, but I will try to address them one after each other (I started to make some propositions already in #16691 and #16692)

If you see further improvements that could be done, please let me know!

@robertschweizer
Copy link
Contributor Author

I agree, #16624 closes this. Thanks a lot @multani!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants