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

Make schema work for Literal and NewType #649

Merged
merged 8 commits into from Jul 15, 2019

Conversation

@dmontagu
Copy link
Collaborator

commented Jul 10, 2019

Change Summary

BaseModel.schema() produces reasonable output for both Literal and NewType (both of which previously generated errors).

Related issue number

#646

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • HISTORY.rst has been updated
    • if this is the first change since a release, please add a new section
    • include the issue number or this pull request number #<number>
    • include your github username @<whomever>
'properties': {
'a': {'title': 'A', 'type': 'integer'},
'b': {'title': 'B', 'type': 'string'},
'c': {'anyOf': [{'type': 'integer'}, {'type': 'string'}], 'title': 'C'},

This comment has been minimized.

Copy link
@dmontagu

dmontagu Jul 10, 2019

Author Collaborator

It might be preferable for this to look more like the schema for an Enum, but I'm not sure what is right; I'm not super knowledgeable about JSON schemas. Let me know if something else is preferred.

@layday I suppose you might have an opinion given you reported the issue?

This comment has been minimized.

Copy link
@layday

layday Jul 10, 2019

Contributor

This should be enum:

Suggested change
'c': {'anyOf': [{'type': 'integer'}, {'type': 'string'}], 'title': 'C'},
'c': {'enum': ['a', 1], 'title': 'C'},

This comment has been minimized.

Copy link
@dmontagu

dmontagu Jul 10, 2019

Author Collaborator

@samuelcolvin would you agree on this point? If so, I'll modify to ensure it gets 'enum' here

This comment has been minimized.

Copy link
@layday

layday Jul 10, 2019

Contributor
  1. 'anyOf' is wrong because in JSON Schema parlance 'anyOf' means it should validate against one or more subschemas in the array. You want to use 'oneOf'.
  2. 'type' is wrong because it should match one of the values in the array exactly, meaning you'd have to use 'const'.
  3. 'oneOf': [{'const': 'a'}, {'const': 1}] is a roundabout way to say 'enum': ['a', 1].

This comment has been minimized.

Copy link
@samuelcolvin

samuelcolvin Jul 10, 2019

Owner

I'm not a JSONSchema expert either, @tiangolo what do you think?

This comment has been minimized.

Copy link
@tiangolo

tiangolo Jul 13, 2019

Collaborator

I would suggest oneOf instead of enum.

That would allow having extra data in the schema. For example, if you use a NewType based on EmailStr, the type would be string with format: "email". But if it's an enum, it won't be able to have the extra format: "email".

Also, const can go along with type. type could be used by clients (e.g. frontends) to display the value in a certain way, even if it can only be a specific value.

E.g.:

'oneOf': [{'type': 'string', 'const': 'a'}, {'type': 'integer', 'const': 1}]

This comment has been minimized.

Copy link
@layday

layday Jul 13, 2019

Contributor

Literals can only contain primitive literals - not NewType - so the situation you describe does not arise. But if you think including the type can be useful let's go with oneOf.

This comment has been minimized.

Copy link
@dmontagu

dmontagu Jul 13, 2019

Author Collaborator

I have updated this to use 'const', but I refactored to use Union to handle multiple values. This is consistent with the semantics of Literal, (i.e., Literal[1, 2, 3] is the same as Union[Literal[1], Literal[2], Literal[3]]), and means that we can leverage the Union-handling logic to generate a multi-value schema.

Currently, this results in anyOf rather than oneOf. In the case of Literal, it is impossible to match on multiple, so anyOf is essentially equivalent to oneOf. I figured this was preferable to implementing custom handling of the multi-value case, but could rework to explicitly use oneOf if there is a good reason.

Alternatively, #656 might result in Union using oneOf anyway, in which case the point would be moot anyway.

This comment has been minimized.

Copy link
@tiangolo

tiangolo Jul 14, 2019

Collaborator

Great! Agree.

And actually, after thinking about it, I guess anyOf makes more sense, as I guess there might be cases where even with Literal, a single value could be valid with more than one schema. Not sure, but in any case, it is not enforcing that a value not valid against the other schemas, so, it's not really enforcing a oneOf.

This comment has been minimized.

Copy link
@tiangolo

tiangolo Jul 14, 2019

Collaborator

In short, agreed.

@codecov

This comment has been minimized.

Copy link

commented Jul 10, 2019

Codecov Report

Merging #649 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #649   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          15     15           
  Lines        2651   2671   +20     
  Branches      524    529    +5     
=====================================
+ Hits         2651   2671   +20

@dmontagu dmontagu force-pushed the dmontagu:literal_newtype_schema branch from 2fb06d5 to a6533a9 Jul 13, 2019

@dmontagu

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 15, 2019

@samuelcolvin I think the discussion of the proper schema to use for Literal has been resolved.

I'm keen to get this merged because, while I have a workaround I'm happy with for now for non-UUID types, the workaround for uuid.UUID-based NewTypes is currently a little more painful (I describe this in more detail below).

If it would make it easier for you to review, I would be happy to split this into two pull requests (one for NewType and one for Literal). I would just like to get the NewType fix into a release as soon as possible, as it will allow me to remove a decent amount of cruft from my code.


For most types involved with NewType (e.g., str, int, etc.), type(type(x)) == type(x). Because of this, I can use the following work around:

if TYPE_CHECKING:
    MyLabel = NewType("MyLabel", str)
else:
    MyLabel = str

This way, type checking works properly, and if I use the "NewType" as a field label, it is interpreted correctly by pydantic. This results in a little more runtime overhead than NewType, but at least everything works properly.

Unfortunately, uuid.UUID(uuid.UUID('00000000-0000-0000-0000-000000000000')) raises an exception. As a result, if I try to cast a UUID to a specific NewType for type hinting, it causes runtime errors. In order to deal with this, I need to create an assignment-type-ignored local variable every time I want to use one:

if TYPE_CHECKING:
    MyID = NewType("MyID", uuid.UUID)
else:
    MyID = uuid.UUID

random_id: MyID = uuid.UUID(uuid4_str())  # type: ignore

However, needing to create a local variable for this every time is starting to pollute my code, and worse, mypy can't notify me if I forget to avoid casting uuids to the typed variant (leading to annoying runtime errors).

@samuelcolvin

This comment has been minimized.

Copy link
Owner

commented Jul 15, 2019

I'm happy with this.

Sorry I've been absent in this conversation, I was at pycon europe, then at a friends wedding the sorting the rest of my life out.

I'll try to put some time into pydantic later in the week to get this and other PRs merged and released and work out what to do about v1.

@samuelcolvin

This comment has been minimized.

Copy link
Owner

commented Jul 15, 2019

@dmontagu please could you rebase and update HISTORY, as per #664 I've just made a release so you'll need a new section.

@dmontagu dmontagu force-pushed the dmontagu:literal_newtype_schema branch from f8aecd0 to f65a50e Jul 15, 2019

David Montague
@dmontagu

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 15, 2019

@samuelcolvin No worries! Thanks for being so consistently responsive. The history should be fixed.

@samuelcolvin samuelcolvin merged commit 18d4b2b into samuelcolvin:master Jul 15, 2019

6 of 9 checks passed

Header rules No header rules processed
Details
Pages changed 1 new file uploaded
Details
Redirect rules No redirect rules processed
Details
Mixed content No mixed content detected
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
samuelcolvin.pydantic Build #20190715.6 succeeded
Details
samuelcolvin.pydantic (Job Python36) Job Python36 succeeded
Details
samuelcolvin.pydantic (Job Python37) Job Python37 succeeded
Details
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.