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

Typecheck Json inner type #4332

Merged
merged 8 commits into from Aug 9, 2022

Conversation

Bobronium
Copy link
Contributor

@Bobronium Bobronium commented Aug 5, 2022

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
Copy link
Contributor Author

please review :)

@Bobronium Bobronium changed the title Typecheck Json type Typecheck Json inner type Aug 5, 2022
Copy link
Member

@samuelcolvin samuelcolvin left a comment

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 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
Member

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
Contributor Author

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
Member

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
Contributor Author

Bobronium commented Aug 9, 2022

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
Member

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
Member

thanks so much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deferred deferred until future release or until something else gets done ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants