Skip to content

Typecheck Json inner type#4332

Merged
samuelcolvin merged 8 commits intopydantic:masterfrom
Bobronium:typecheck-json-type
Aug 9, 2022
Merged

Typecheck Json inner type#4332
samuelcolvin merged 8 commits intopydantic:masterfrom
Bobronium:typecheck-json-type

Conversation

@Bobronium
Copy link
Copy Markdown
Contributor

@Bobronium Bobronium commented Aug 5, 2022 •
edited
Loading

Change Summary

Make Json a proper generic via Annotated. Typecheckers will now infer the type of field as inner_type of Json.
These changes are soft breaking, one should use Json[Any] instead of plain Json, as the later will result in error from mypy.
In runtime, all should remain the same, except Json[Any] is now allowed.

from typing import Any

from pydantic import BaseModel, Json, ValidationError


class AnyJsonModel(BaseModel):
    json_obj: Json[Any]


class ConstrainedJsonModel(BaseModel):
    json_obj: Json[list[int]]

reveal_type(ConstrainedJsonModel().json_obj)  # Revealed type is "list[int]"

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)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

@Bobronium Bobronium force-pushed the typecheck-json-type branch from f995915 to 5f226d2 Compare August 5, 2022 20:03
@Bobronium
Copy link
Copy Markdown
Contributor Author

Bobronium commented Aug 5, 2022

please review :)

@Bobronium Bobronium changed the title Typecheck Json type Typecheck Json inner type Aug 5, 2022
samuelcolvin
samuelcolvin reviewed Aug 9, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@samuelcolvin samuelcolvin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for this.

Two questions:

  • is this better than the status quo - I think "yes"
  • is this suitable for a patch release since some legitimate usage will "break" (at least for type checkers) - I think probably not

I would suggest we defer this to V2.

We need to think a lot about serialisation and in particular round-trip validation which affects Json, but perhaps not this.

Comment thread tests/mypy/modules/success.py
Comment on lines +233 to +234
my_json: Json[Dict[str, str]] = '{"hello": "world"}' # type: ignore
my_json_list: Json[List[str]] = '["hello", "world"]' # type: ignore
Copy link
Copy Markdown
Member

@samuelcolvin samuelcolvin Aug 9, 2022

Choose a reason for hiding this comment

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

Humm, problem is this makes the output type compatibility better, but makes the input type compatibility worse.

Hence the "type ignore" here.

Copy link
Copy Markdown
Contributor Author

@Bobronium Bobronium Aug 9, 2022

Choose a reason for hiding this comment

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

Yes, indeed. I thought that it's alright since it's kind of consistent throughout pydantic: you can even see same type: ignore's above for other types.

Copy link
Copy Markdown
Member

@samuelcolvin samuelcolvin Aug 9, 2022

Choose a reason for hiding this comment

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

Yes, agreed, also presumably it allows bytes.

Hence why I said this is better than the status quo on balance.

@samuelcolvin samuelcolvin added the deferred Deferred until future release or until something else gets done label Aug 9, 2022
@Bobronium
Copy link
Copy Markdown
Contributor Author

Bobronium commented Aug 9, 2022 •
edited
Loading

I think it certainly not suitable for a patch release.
As for minor release (still not V2), it's totally your call. I can imagine it breaks CI here and there, maybe prevent something from getting deployed. On the other hand, runtime behaviour should be intact, so one might vouch for slipping it under a minor.

I also don't have enough context about V2 release date.

As for changing inner logic, I'd like to see something entirely different in terms of inner types support in V2. Currently I have a case where I need to decode and validate JWS, would be nice to be able to do something like:

signed_payload: Annotated[PayloadModel, Field(decode=decode_jws_payload)

This would allow for usages like

T = TypeVar("T")
JWS = Annotated[T, Field(decode=decode_jws_payload)

signed_payload: JWS[PayloadModel]

Now I'm accomplishing same with custom generic type. Doable, but definitely much more verbose.

@samuelcolvin
Copy link
Copy Markdown
Member

samuelcolvin commented Aug 9, 2022

Ye, I see your point on runtime behaviour not changing.

Regarding Annotated[PayloadModel, Field(decode=decode_jws_payload), I think it should be possible. decode would probably be validator, which should then be a pre-function validator. Since all pydantic is doing in V2 is building a pydantic-core schema, allowing this via an argument to Field seems pretty simple (at least from here, when we haven't started implementing it 😉). Could you create an issue so we remember about this?

@samuelcolvin samuelcolvin merged commit 0500610 into pydantic:master Aug 9, 2022
@samuelcolvin
Copy link
Copy Markdown
Member

samuelcolvin commented Aug 9, 2022

thanks so much.

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

Reviewers

@samuelcolvin samuelcolvin samuelcolvin left review comments

Assignees

@hramezani hramezani

@samuelcolvin samuelcolvin

@PrettyWood PrettyWood

Labels

deferred Deferred until future release or until something else gets done ready for review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Bobronium @samuelcolvin @hramezani @PrettyWood