Skip to content

Conversation

lilyminium
Copy link
Contributor

@lilyminium lilyminium commented Mar 26, 2022

#3935 contains a pin to jinja; unsure if I should wait on that or pin myself.

Change Summary

  • adds encode_as_json keyword to a bunch of serialisation functions to try getting nested JSON encoders
  • adds use_nested_encoders to BaseModel.json
  • use_nested_encoders does not pair with models_as_dict=False; in that case, only the global encoder is used

Related issue number

fix #2277

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

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.

I think this looks great, a few things to change and will need to wait for v1.10, but doesn't need to wait until v2.

Comment on lines 21 to 25
class Config:
json_encoders = {
timedelta: lambda v: v.total_seconds(),
WithCustomEncoders: lambda _: 'using parent encoder',
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this makes the example more complicated than it needs to be - I think it's sufficient just to show that ParentModel uses child.Config.json_encoders.

If there's a more subtle point to be made about how ParentModel interacts, either mention it in the code or add another example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this was highlighting that ParentModel doesn't use child.Config.json_encoders when models_as_dict=False, whether use_nested_encoders=True|False. That's mentioned in the code as a comment, should it be more explicit?

@lilyminium lilyminium marked this pull request as ready for review April 3, 2022 14:35
@samuelcolvin
Copy link
Member

Once this is ready to review, please use the special phrase in a comment so I know it's my responsibility.

https://github.com/samuelcolvin/pydantic/blob/8997cc5961139dd2695761a33c06a66adbf1430a/.github/PULL_REQUEST_TEMPLATE.md?plain=1#L21

@lilyminium
Copy link
Contributor Author

please review

@07pepa
Copy link
Contributor

07pepa commented Apr 24, 2022

@lilyminium i think this will satify this feature request #3910 👍 (i need to serialize date times time deltas and other python stuff)

also if you will add tests for negative time deltas there is small bug in it time #3909 so do not do it yet or do it after this fix is merge (but i think curent state will suffice. This is just heads up since i saw it in test used.)

@barringtonhaynes
Copy link

barringtonhaynes commented May 11, 2022

@lilyminium thanks for working on this, it's a feature I need. When I've tried running these tests using the above code though, if you define any primitive types, string for example it raises a TypeError: Object of type 'str' is not JSON serialisable. I've amended the test slightly to show this.

from datetime import datetime, timedelta
from pydantic import BaseModel
from pydantic.json import timedelta_isoformat


class CustomChildModel(BaseModel):
    dt: datetime
    diff: timedelta
    text: str

    class Config:
        json_encoders = {
            datetime: lambda v: v.timestamp(),
            timedelta: timedelta_isoformat,
        }

class ParentModel(BaseModel):
    diff: timedelta
    child: CustomChildModel

    class Config:
        json_encoders = {
            timedelta: lambda v: v.total_seconds(),
            CustomChildModel: lambda _: 'using parent encoder',
        }

child = CustomChildModel(dt=datetime(2032, 6, 1), diff=timedelta(hours=100), text='hello')
parent = ParentModel(diff=timedelta(hours=3), child=child)

# default encoder uses total_seconds() for diff
print(parent.json())

# nested encoder uses isoformat
print(parent.json(use_nested_encoders=True))

# turning off models_as_dict only uses the top-level formatter, however

print(parent.json(models_as_dict=False, use_nested_encoders=True))

print(parent.json(models_as_dict=False, use_nested_encoders=False))

Also, in the following example I couldn't get it to json encode the GrandChildModel using my custom encoder, but I could if it was a standard python data class.

from datetime import datetime, timedelta
from pydantic import BaseModel
from pydantic.json import timedelta_isoformat


class GrandChildModel(BaseModel):
    dt: datetime
    diff: timedelta

class CustomChildModel(BaseModel):
    dt: datetime
    diff: timedelta
    grand_child: GrandChildModel

    class Config:
        json_encoders = {
            datetime: lambda v: v.timestamp(),
            timedelta: timedelta_isoformat,
            GrandChildModel: lambda v: 'foobar',
        }


class ParentModel(BaseModel):
    diff: timedelta
    child: CustomChildModel

    class Config:
        json_encoders = {
            timedelta: lambda v: v.total_seconds(),
            CustomChildModel: lambda _: 'using parent encoder',
        }


grand_child = GrandChildModel(dt=datetime(2040, 7, 13), diff=timedelta(hours=10))
child = CustomChildModel(dt=datetime(2032, 6, 1), diff=timedelta(hours=100), grand_child=grand_child)
parent = ParentModel(diff=timedelta(hours=3), child=child)

# default encoder uses total_seconds() for diff
print(parent.json())

# nested encoder uses isoformat
print(parent.json(use_nested_encoders=True))

# turning off models_as_dict only uses the top-level formatter, however

print(parent.json(models_as_dict=False, use_nested_encoders=True))

print(parent.json(models_as_dict=False, use_nested_encoders=False))

@samuelcolvin samuelcolvin enabled auto-merge (squash) August 8, 2022 14:15
@samuelcolvin
Copy link
Member

thanks so much for this.

@samuelcolvin samuelcolvin merged commit b42fae0 into pydantic:master Aug 8, 2022
@barringtonhaynes
Copy link

@samuelcolvin did you see my notes above. Just noticed you merged this.

@samuelcolvin
Copy link
Member

So sorry @barringtonhaynes, I've been crashing through PRs and missed your comment.

I guess we should either revert this or work on a separate fix. Do you think you could submit a fix?

@samuelcolvin samuelcolvin mentioned this pull request Aug 11, 2022
14 tasks
@barringtonhaynes
Copy link

So sorry @barringtonhaynes, I've been crashing through PRs and missed your comment.

I guess we should either revert this or work on a separate fix. Do you think you could submit a fix?

I would love to but don't have any time at the moment due to other commitments, it's also possible that I missed something and those use cases do work. It's a super useful feature, but think it needs a little TLC. If it's still around in a month or so I'm happy to have a look.

@samuelcolvin
Copy link
Member

Okay, I'll look into it. Thanks for the heads up.

And thanks again to @lilyminium for the PR.

@samuelcolvin
Copy link
Member

Okay, having looked into it, this introduces a lot of problems, for example even in this very simple case the output is not valid JSON:

from pydantic import BaseModel

class Child(BaseModel):
    b: str
    c: int

    class Config:
        json_encoders = {
            str: lambda v: 'custom value!',
        }

class Parent(BaseModel):
    a: str
    child: Child

x = Parent(a='a', child=dict(b='b', c=1))
print(x.json(use_nested_encoders=True))

Outputs:

{"a": "\"a\"", "child": {"b": "\"b\"", "c": "1"}}

Note::

  • a is not valid JSON
  • child.b is not valid JSON
  • child.c is a string when it should be an int

I'll revert the PR, and if someone else wants to take a stab at this before V1.10 is locked next week, I'll try and review. But realistically, we'll probably need to wait for V2 when serialisation will be completely re-thought.

samuelcolvin added a commit that referenced this pull request Aug 12, 2022
samuelcolvin added a commit that referenced this pull request Aug 12, 2022
@lilyminium
Copy link
Contributor Author

Huge apologies for falling off the map -- Google was helpfully filtering out a substantial chunk of my GitHub notifications. Thanks for noticing the problems, @barringtonhaynes and sorry for any complications this introduced.

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

Successfully merging this pull request may close these issues.

Json Encoders are ignored in nested structures
7 participants