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

Custom Root Types - validate(), dict() not as expected #1193

Closed
peku33 opened this issue Jan 28, 2020 · 13 comments · Fixed by #2238
Closed

Custom Root Types - validate(), dict() not as expected #1193

peku33 opened this issue Jan 28, 2020 · 13 comments · Fixed by #2238
Labels
Change Suggested alteration to pydantic, not a new feature nor a bug

Comments

@peku33
Copy link

peku33 commented Jan 28, 2020

             pydantic version: 1.4
            pydantic compiled: False
                 install path: /home/rtbs106/rtb-test/pydantic/pydantic
               python version: 3.7.5 (default, Nov 20 2019, 09:21:52)  [GCC 9.2.1 20191008]
                     platform: Linux-5.3.0-26-generic-x86_64-with-Ubuntu-19.10-eoan
     optional deps. installed: ['typing-extensions', 'email-validator', 'devtools']

pydantic offers a feature called "Custom Root Type": https://pydantic-docs.helpmanual.io/usage/models/#custom-root-types

For now it looks like there is general poor support for such objects across available BaseModel methods.

validate() function used by fastapi does not consider this setting at all:

from pydantic import BaseModel
from typing import List

class SubModel(BaseModel):
    name: str

class ListModel(BaseModel):
    __root__: List[SubModel]

def test_validate():
    list_model = ListModel.validate([{"name": "foo"}])
    assert isinstance(list_model, ListModel)

This should pass, however pydantic.errors.DictError: value is not a valid dict is raised instead.
validate() function also isn't documented at all.
While such thing is implemented by parse_obj() it does not implement other features that validate() has, for example cls.__config__.orm_mode.
Also these two functions looks pretty the same, what are the differences between them?

dict() method used by fastapi returns value other than expected

While this is documented, and probably dict() should not return anything other than dict at the moment there is no function opposite to parse_obj(), eg. returning what dict() returns for normal models, and direct __root__ value for custom root type objects. Maybe serialize_obj() should be added?

Custom Root Type could probably use single __init__ parameter.

This would allow to completly hide root argument

Custom Root Type models should be a separate classes?

As handling them is a separate thing, and putting if __custom_root_type__ everywhere does not seem to be reasonable.

@phy25
Copy link

phy25 commented Jan 29, 2020

Reference: #730 (.json() behavior is changed to unwrap __root__)

@peku33
Copy link
Author

peku33 commented Jan 29, 2020

Yes, .json() works as expected, however for now there is no way to get actual value not as a string

@samuelcolvin samuelcolvin removed the bug V1 Bug related to Pydantic V1.X label Jan 30, 2020
@samuelcolvin
Copy link
Member

dict() behaviour will change in v2 to default to returning the root object, but that's backwards incompatible so can't happen now. It's also not a bug.

Regarding validate(), it's used internally when validating sub-models, it should probably be explicitly private in v2.

If there are problems with fastAPI, please create an issue there.

@samuelcolvin samuelcolvin added the Change Suggested alteration to pydantic, not a new feature nor a bug label Jan 30, 2020
@samuelcolvin samuelcolvin added this to the Version 2 milestone Jan 30, 2020
@anentropic
Copy link
Contributor

anentropic commented Apr 1, 2020

I think this may be related, apologies if not...

But I would really love to be able to instantiate a model that uses __root__ without having to use __root__ as a key in the input data.

This is particularly in the case where the rooted model is a child attr of the parent model you are instantiating.

I realise there is one case which works, you can:

class RootedModel(BaseModel):
    __root__: Dict[str, str]

RootedModel.parse_obj({"dynamic_field": "some value"})

But this fails as soon as you want to instantiate the parent:

>>> class ParentModel(BaseModel):
>>>     rooted: RootedModel
>>>
>>> ParentModel.parse_obj({"rooted": {"dynamic_field": "some value"}})

ValidationError: 1 validation error for ParentModel
rooted -> __root__
  field required (type=value_error.missing)

I don't really like that the __root__ "internal special name" is exposed to the public data model at all.

It would be great if the existing special case, for parse_obj, was consistently used everywhere so that instantiation of sub-objects in the same fashion can succeed.

@anentropic
Copy link
Contributor

anentropic commented Apr 1, 2020

Well, it seems that an easy workaround in this example is to eliminate RootedModel and just:

class ParentModel(BaseModel):
    rooted: Dict[str, str]

ParentModel.parse_obj({"rooted": {"dynamic_field": "some value"}})

...in which case I am not really sure what the point of __root__ is

Well, on large models it allows to separate the validation etc more logically. Or maybe you want to reuse the definition of RootedModel across multiple parents. So yes, it would be nice if it was more usable.

@AAlon
Copy link

AAlon commented Apr 21, 2020

Weird, I had the opposite issue: able to instantiate via a parent class but not directly. To overcome this, I'm now detecting direct instantiations and fixing the parameters passed to BaseModel, explicitly setting the __root__ keyword arg:

class MovieGenre(BaseModel):
    class MovieGenreEnum(str, Enum):
        Action = "Action"
        Drama = "Drama"
    __root__: MovieGenreEnum
    
    def __init__(self, *args, **kwargs):
        if len(args) == 1 and type(args[0]) == str:
            # a genre was passed as a parameter - this is a direct instantiation 
            genre = args[0]
            super().__init__(__root__=MovieGenreEnum(genre), *args[1:], **kwargs)
        else:
            super().__init__(*args, **kwargs)

A similar approach might solve your issue too. I wonder if there's a more elegant solution though.

@patrickkwang
Copy link

patrickkwang commented Jun 9, 2020

Regarding validation, validate_model() appears to be considered public.

from typing import Dict
from pydantic import BaseModel, validate_model

class StrDict(BaseModel):
    __root__: Dict[str, str]

value, fields_set, error = validate_model(StrDict, {'foo': 'bar'})
print(error)

yields

ValidationError(model='StrDict', errors=[{'loc': ('__root__',), 'msg': 'field required', 'type': 'value_error.missing'}])

Is this the intended behavior?

@samuelcolvin
Copy link
Member

yes, with validate_model you should use validate_model(StrDict, {'__root__': {'foo': 'bar'}})

@patrickkwang
Copy link

Okay. The feeling I'm getting is that the output of dict() should validate, i.e.

foo: BaseModel = ...
value, fields_set, error = validate_model(foo.__class__, foo.dict())
assert error is None

for any foo that is an instance of a subclass of BaseModel.

This seems to be true currently, and if it is meant to be true generally, this indicates a validation bug that mirrors the dict() bug described in #1414.

from typing import Dict
from pydantic import BaseModel, validate_model

class StrDict(BaseModel):
    __root__: Dict[str, str]

class NestedDict(BaseModel):
    v: StrDict

value, fields_set, error = validate_model(NestedDict, {'v': {'foo': 'bar'}})
print(error)

@PrettyWood
Copy link
Member

PrettyWood commented Jan 3, 2021

Hi!
I guess we can just change the order in BaseModel.validate

     @classmethod
     def validate(cls: Type['Model'], value: Any) -> 'Model':
-        if isinstance(value, dict):
+        if cls.__custom_root_type__:
+            return cls.parse_obj(value)
+        elif isinstance(value, dict):
             return cls(**value)
         elif isinstance(value, cls):
             return value.copy()
         elif cls.__config__.orm_mode:
             return cls.from_orm(value)
-        elif cls.__custom_root_type__:
-            return cls.parse_obj(value)
         else:
             try:
                 value_as_dict = dict(value)

@levrik
Copy link

levrik commented Mar 9, 2021

@PrettyWood This doesn't seem to be fixed at all. dict() for example still returns Root as key and then the actual value instead of putting the value there directly and a call to json() just gives me KeyError: '__root__'.

Edit: I've just realized that the issue with json() happens due to the alias_generator we're using. Maybe __root__ shouldn't be passed to that one because internals seem to rely on that name. Should I file another issue for that?
But this doesn't fix the dict() case as it still returns __root__ then which this ticket was about besides validate().

As a workaround I've created a sub class now to change the behavior and be in sync with json().

class BaseModel(PydanticBaseModel):
    def dict(self, *args, **kwargs):
        data = super().dict(*args, **kwargs)
        return data[ROOT_KEY] if self.__custom_root_type__ else data

@PrettyWood
Copy link
Member

PrettyWood commented Mar 9, 2021

Hi @levrik
Yes I solved the validate() and the nested parse_obj() issues...
But the dict() method was not changed because we don't agree on the default behaviour dict() should have.
Maybe what you want is related to #1409

@AlexanderFarkas
Copy link

Pydantic v2

class IdsOrAll(RootModel):
    root: Union[list[int], Literal["all"]
    
class DTO(BaseModel):
    applicable_to_location_ids: IdsOrAll
    
    
dto = DTO(applicable_to_location_ids=IdsOrAll(root="all"))
dto.model_dump() # produces {"applicable_to_location_ids": "all"}, should be {"applicable_to_location_ids": IdsOrAll}

Is that expected?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Change Suggested alteration to pydantic, not a new feature nor a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants