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

Root Errors and "__obj__" vs "__root__" #729

Closed
samuelcolvin opened this issue Aug 9, 2019 · 14 comments · Fixed by #817
Closed

Root Errors and "__obj__" vs "__root__" #729

samuelcolvin opened this issue Aug 9, 2019 · 14 comments · Fixed by #817

Comments

@samuelcolvin
Copy link
Owner

@samuelcolvin samuelcolvin commented Aug 9, 2019

Currently here (for invalid data to parse_obj) and here (in parse_raw) we use '__obj__' as the location for errors to indicate an error which is general to the model, not specific to a field.

We should change this in v1 to __root__ to better match other nomenclature, eg. custom root types, where we use __root__.

We should also implement at some point entire model validation, as discussed at #691 (comment). I guess to match the above naming, we should do this using a special value to a validator @validator('__root__'). If this raised an error it would end up in ValidationError details with 'location': '__root__', (Unless it's a "@validator('__root__')" validator for a submodel, when obviously it should have the location of that model.

Perhaps it would would be nice to have a special decorator @root_validator which is an alias of @validator('__root__'), we should support multiple root validators either as a result of inheritance or on a single class.

I guess the __obj__ > __root__ change must happen with v1 and the new root validator could go in a later release, but we should be sure we won't need other backwards incompatible changes to implement the root validator.

Thoughts?

@koxudaxi

This comment has been minimized.

Copy link
Contributor

@koxudaxi koxudaxi commented Aug 10, 2019

It seems so good to me.

We should change this in v1 to __root__ to better match other nomenclature, eg. custom root types, where we use __root__.

I know about custom root type. But, someone might not understand the __root__ clearly.

@samuelcolvin

This comment has been minimized.

Copy link
Owner Author

@samuelcolvin samuelcolvin commented Aug 10, 2019

agreed, documentation will need improving, but better to use one term everywhere.

I'm not wedded to the term "root" other offers considered...

@koxudaxi

This comment has been minimized.

Copy link
Contributor

@koxudaxi koxudaxi commented Aug 10, 2019

I agree.

My answer is __root__ for the title of the issue.

@dmontagu

This comment has been minimized.

Copy link
Collaborator

@dmontagu dmontagu commented Aug 10, 2019

We should also implement at some point entire model validation

For the sake of discussion, here's a somewhat radical proposal:

  1. If the list of fields passed to the validator is empty, it becomes a __root__/entire-model validator (depending on whether the model has a non-default __root__). (I see no reason to drop support for @validator('__root__'), but wouldn't be opposed to it for simplicity's sake.)
  2. The loc field of ErrorWrapper is changed from typically containing {field.name} to typically containing{Model.Config.title or Model.__name__}.{field.name}.
    • I recognize that right now, validation happens inside the Field, which is not aware of the containing model. I think this could be addressed by the existence of a function/method that returns a modified ErrorWrapper where loc includes the name of the model.
  3. For __root__ or entire-model validation, drop the .{field.name} from loc.

Technical implementation challenges aside, I think this might be a good way to achieve a unified interface for specifying whole-model and __root__ validators, and for achieving consistent error labeling.

I'll open a separate issue to discuss the implications of adding the model name to ErrorWrapper, but, any thoughts on this in the context of handling root errors?

@samuelcolvin

This comment has been minimized.

Copy link
Owner Author

@samuelcolvin samuelcolvin commented Aug 10, 2019

So you're basically suggesting prefixing loc with the model name?

The problem is that very often (in few types of case, but the large majority of cases) we're simply doing something like:

class LoginModel:
    email: EmailStr
    password: constr(max_length=100)

def login_view(request):
  m = LoginModel(**request.json())
  ...

The user (or frontend developer) has never heard of LoginModel, so an error at loc: ['LoginModel', 'email'] is a lot more confusing than at loc: ['email'].

We could just use the model name instead of __root__ for those errors?

@samuelcolvin

This comment has been minimized.

Copy link
Owner Author

@samuelcolvin samuelcolvin commented Aug 10, 2019

I know that in theory loc is designed to be processed by the middleware of using package and used to generate the final error format, but I for one quite often end up using it directly.

@dmontagu

This comment has been minimized.

Copy link
Collaborator

@dmontagu dmontagu commented Aug 10, 2019

@samuelcolvin I was coming from the context of FastAPI, and was imagining that the model name is externally available through the OpenAPI spec. But maybe that is a big assumption (especially if you want validation errors but don't want a public OpenAPI spec). I guess it would be nice if there were a config setting for include_model_name_in_error_loc (or something less verbose 😅). That would make it possible to optionally incorporate the change without many changes to code, and without needing to worry about causing conflicts with frameworks using pydantic (like FastAPI).

Either way, I think __root__ would be similarly confusing externally. But it would have the advantage of leaking less implementation information if you wanted to avoid that.

@samuelcolvin

This comment has been minimized.

Copy link
Owner Author

@samuelcolvin samuelcolvin commented Aug 10, 2019

I agree __root__ isn't much better. But at least it won't get added to the majority of errors which have a field.

What do others think?

Also what do other libraries do? Django (and presumably therefore DRF) uses __all__ which is no clearer than __root__.

@dmontagu

This comment has been minimized.

Copy link
Collaborator

@dmontagu dmontagu commented Aug 17, 2019

I've thought this through some more, and given ValidationError tracebacks now include the model name, I think it makes sense to just use __root__ everywhere (and prefer this to __all__). For the sake of debugging, I think it's fine to require access to the tracebacks to determine the model name. I'll take a shot at implementing.

@dmontagu

This comment has been minimized.

Copy link
Collaborator

@dmontagu dmontagu commented Aug 18, 2019

@samuelcolvin I tried taking a shot at this, but I think I got in over my head. Validation currently always happens in a field, and I couldn't find a way to add "root" validation for non-custom-root models without substantial and/or very-likely-controversial changes. Given the amount of changes that it seemed like I'd have to make, (not to mention the effort required to get it to actually work even if I was willing to make large changes), I'm going to leave this to you for now.

(If you have an approach in mind that you think should work, I would also be happy to put in some renewed effort given more guidance.)

@skewty

This comment has been minimized.

Copy link
Contributor

@skewty skewty commented Sep 3, 2019

@dmontagu do you have your unfinished code in a fork or branch somewhere where I can take a look at it?

@dmontagu

This comment has been minimized.

Copy link
Collaborator

@dmontagu dmontagu commented Sep 3, 2019

@skewty I'll look, but I'd probably advise against basing anything off of it, as I made a number of choices that backed me into a corner that I don't think had any hope of success. And I suspect any approach to this will have to get pretty deep into the internals, so it's probably better not to push your thinking down the same line that led me to problems.

I was thinking about this recently, and thought it might actually be easier to implement this somehow via something closer to the dataclass __post_init__ than to how validators currently work.

@samuelcolvin

This comment has been minimized.

Copy link
Owner Author

@samuelcolvin samuelcolvin commented Sep 3, 2019

I'm going to work on this soon, just haven't got a chance yet.

@samuelcolvin samuelcolvin mentioned this issue Sep 17, 2019
8 of 8 tasks complete
@samuelcolvin

This comment has been minimized.

Copy link
Owner Author

@samuelcolvin samuelcolvin commented Sep 18, 2019

root validators and renaming __obj__ -> __root__ is done in #817.

Please give feedback there. I don't promise to wait long for feedback as I'm pushing get v1 out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.