Skip to content

Conversation

maximberg
Copy link
Contributor

@maximberg maximberg commented Feb 17, 2021

Change Summary

Continuing #2262, I started to develop the use of generics as field types in models.
My initial implementation of this functionality was rather naive. I made a revision.
In particular, I changed how modify_schema is called for generics and how the nested type schema is generated.

For example, you are using a generic type MyGeneric[MyCustomType, str].
You expect the data to be passed to validation as a Tuple, and a schema must be defined for each nested type and for the entire field.

The sequence of forming the scheme should be, as it seems to me, like this:

  1. Collect all nested type schemas into one allOf schema.
  2. Call modify_schema of the generic MyGeneric, to where we pass the schema collected at the previous stage. There it can be processed.
  3. Update the schema described in the model itself with the data collected in the previous stage.

Moreover, there are two options for nested types - if one type is specified or if several are specified.

For one type, the schema will look like this:
MyGeneric[MyCustomType] -> {"allOf": [{"$ref": "#/definitions/MyCustomType"}]}

For two nested types like this:
MyGeneric[MyCustomType, str] -> {"allOf": [{"type": "array", "items": [{"$ref": "#/definitions/CustomType"}, {"type": "string "}]}]}

Related issue number

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)

@codecov
Copy link

codecov bot commented Feb 17, 2021

Codecov Report

Merging #2375 (c514310) into master (7b7e705) will not change coverage.
The diff coverage is 100.00%.

❗ Current head c514310 differs from pull request most recent head fe101bb. Consider uploading reports for the commit fe101bb to get more accurate results

@@            Coverage Diff            @@
##            master     #2375   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           25        25           
  Lines         5109      5088   -21     
  Branches      1050      1044    -6     
=========================================
- Hits          5109      5088   -21     
Impacted Files Coverage Δ
pydantic/schema.py 100.00% <100.00%> (ø)
pydantic/json.py 100.00% <0.00%> (ø)
pydantic/main.py 100.00% <0.00%> (ø)
pydantic/types.py 100.00% <0.00%> (ø)
pydantic/fields.py 100.00% <0.00%> (ø)
pydantic/generics.py 100.00% <0.00%> (ø)
pydantic/_hypothesis_plugin.py 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f49887c...fe101bb. Read the comment docs.

@maximberg
Copy link
Contributor Author

@Mazyod What do you think? It may be useful to you.

@Mazyod
Copy link
Contributor

Mazyod commented Feb 17, 2021

Looks like a great addition! (although the details of schema generation as per openAPI standard is beyond me 🙇‍♂️)

@maximberg maximberg changed the title Advanced use of generic fields Advanced use of generic fields in schema generation Feb 19, 2021
@samuelcolvin
Copy link
Member

conflicts.

But honestly, this is all beyond me. Can you give the simplest possible example of how this might be used IRL?

@samuelcolvin samuelcolvin added the awaiting author revision awaiting changes from the PR author label Feb 25, 2021
@maximberg
Copy link
Contributor Author

@samuelcolvin, thanks for your comment.
First of all, we are not talking about data validation, but about the schema that describes our model. That is, we need to describe as accurately as possible what data must be transferred in order to successfully pass the validation. Validation for generics is a developer's job, as it seems to me.
In my project, I actively use schema to automatically generate documentation for the API, for example.
Here is a synthetic example, but similar to what I am working with in my project.
For example, the model has a parameter user: User which has a title. We pass a role and name as a tuple for validation. The role is the enum.
When forming the schema, we want to display that data have to be passed as a tuple. We will also describe our enum separately - as it has a title and description. And the generic has an example of use in the description of the scheme.
All this should be collected in the final scheme.
And now for some reason we need to teach model to validate both - one user and several users - in the same parameter. We will rewrite the validator in the generic User, description of it and also modify the schema modifier to show that an array of tuples can be passed too.
Hope my example explains the idea. If not - let me know, please.

@maximberg
Copy link
Contributor Author

@samuelcolvin If you are going to issue new version, please take my PR. I'm living on my fork in my project waiting for it. :) Thanks!

@PrettyWood PrettyWood removed the awaiting author revision awaiting changes from the PR author label Apr 5, 2021
@samuelcolvin
Copy link
Member

samuelcolvin commented May 9, 2021

I'm sorry this is still open and I've just got around to looking at it properly.

There are some trivial conflicts again, but I have some more fundamental questions here:

  1. Does this change the current schema for tuples in any scenarios? If so it's a problem as the schema shouldn't change in minor versions
  2. My small brain still doesn't understand exactly what case this is modifying for generics. Please could you create a self contained code example that shows what the current behaviour is and what the new behaviour is.

I'm worried this might need to wait for v2, unless either:

  • this doesn't change current schema by default
  • The current behaviour is obviously illogical or directly contradicts JSONSchema

https://m.xkcd.com/1172/

please update

@github-actions github-actions bot added awaiting author revision awaiting changes from the PR author and removed ready for review labels May 9, 2021
@github-actions github-actions bot assigned maximberg and unassigned samuelcolvin and PrettyWood May 9, 2021
� Conflicts:
�	tests/test_schema.py
@maximberg
Copy link
Contributor Author

Thanks for you comment.

  1. My change will not affect the logic for converting tuples to JSONSchema. Moreover, I partly used tuple logic to convert generics to schema.
  2. Below I have given an example in which I compared the previous version (which I made in Added schema generation for Generic fields #2262) and the new one that I propose in Modelling question - Feed #2275.
  3. New code for nested generic models from master changes my code logic. I need some time to merge.
from enum import IntEnum
from typing import TypeVar, Any, Generic, Optional

from pydantic import BaseModel, UUID4
from pydantic.fields import ModelField

T = TypeVar('T')
S = TypeVar('S')


class MyGeneric(Generic[T, S]):

    @classmethod
    def __modify_schema__(cls, field_schema):
        the_type = field_schema.pop('type', None)
        if not the_type:
            the_type = field_schema.pop('allOf', 'string')
        if isinstance(the_type, str):
            the_type = {'type': the_type}
        elif isinstance(the_type, list):
            the_type = the_type[0]
        field_schema.update(
            anyOf=[the_type, {'type': 'array', 'items': the_type}]
        )

    @classmethod
    def __get_validators__(cls):
        yield cls.validate

    @classmethod
    def validate(cls, v: Any, field: ModelField) -> 'MyGeneric':
        ...


class Status(IntEnum):
    new = 0
    active = 1
    closed = 2

    @classmethod
    def __modify_schema__(cls, field_schema):
        field_schema.update(dict(
            type='string',
            enum=[x.name for x in cls]
        ))


class MyModel(BaseModel):
    link_to_id: Optional[MyGeneric[UUID4, str]]
    status: MyGeneric[Status, str]


pprint.pp(MyModel.schema(), width=100)

#2262 produces:

{'title': 'MyModel',
 'type': 'object',
 'properties': {'link_to_id': {'title': 'Link To Id',
                               'anyOf': [{'type': 'string'},
                                         {'type': 'array', 'items': {'type': 'string'}}]},
                'status': {'title': 'Status',
                           'anyOf': [{'type': 'string'},
                                     {'type': 'array', 'items': {'type': 'string'}}]}},
 'required': ['status'],
 'definitions': {'Status': {'title': 'Status',
                            'description': 'An enumeration.',
                            'enum': ['new', 'active', 'closed'],
                            'type': 'string'}}}

#2275 produces:

{'title': 'MyModel',
'type': 'object',
'properties': {'link_to_id': {'title': 'Link To Id',
                              'anyOf': [{'type': 'array',
                                         'items': [{'type': 'string', 'format': 'uuid4'},
                                                   {'type': 'string'}]},
                                        {'type': 'array',
                                         'items': {'type': 'array',
                                                   'items': [{'type': 'string', 'format': 'uuid4'},
                                                             {'type': 'string'}]}}]},
               'status': {'title': 'Status',
                          'anyOf': [{'type': 'array',
                                     'items': [{'$ref': '#/definitions/Status'},
                                               {'type': 'string'}]},
                                    {'type': 'array',
                                     'items': {'type': 'array',
                                               'items': [{'$ref': '#/definitions/Status'},
                                                         {'type': 'string'}]}}]}},
'required': ['status'],
'definitions': {'Status': {'title': 'Status',
                           'description': 'An enumeration.',
                           'enum': ['new', 'active', 'closed'],
                           'type': 'string'}}}

@maximberg
Copy link
Contributor Author

@samuelcolvin Looks like I managed to merge.

@maximberg
Copy link
Contributor Author

@samuelcolvin Should I do something else?

Copy link
Collaborator

@PrettyWood PrettyWood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@PrettyWood PrettyWood removed the awaiting author revision awaiting changes from the PR author label Sep 4, 2021
@PrettyWood PrettyWood merged commit 93aed51 into pydantic:master Sep 4, 2021
jpribyl pushed a commit to liquet-ai/pydantic that referenced this pull request Oct 7, 2021
* Added schema generation for Generic fields with tests.
Fixed test_assert_raises_validation_error.

* Added schema generation for Generic fields with tests.
Fixed test_assert_raises_validation_error.

* tested on python 3.6, 3.7, 3.8, 3.9

* tested on python 3.6, 3.7, 3.8, 3.9

* restore formatting

* fix mistakes

* formatting

* formatting

* formatting

* fixed coverage

* changes file

* changes file

* remove redundant len

* Update pydantic/schema.py

Co-authored-by: Maz Jaleel <mazjaleel@gmail.com>

* Generic fields with params

* Generic fields with params

* Fix my previous test

* formatting

* merging with nested models-generics

* formatting

* formatting

* rewording

Co-authored-by: Maxim Berg <berg@petrostyle.com>
Co-authored-by: Maz Jaleel <mazjaleel@gmail.com>
Co-authored-by: PrettyWood <em.jolibois@gmail.com>
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

Successfully merging this pull request may close these issues.

4 participants