Added support for class specific custom conversion through to_json/from_json methods #819
Conversation
…s. Note this still doesn't support fields of generic types.
mfateev
changed the title
There was a problem hiding this comment.
LGTM, only a couple of comments. @dandavison - any opinions here?
temporalio/converter.py
Outdated
| @@ -489,6 +489,43 @@ class AdvancedJSONEncoder(json.JSONEncoder): | |||
|
|
|||
| This encoder supports dataclasses and all iterables as lists. | |||
|
|
|||
| A class can implement to_json and from_json methods to support custom conversion logic. | |||
There was a problem hiding this comment.
I would move this to JSONPlainPayloadConverter instead of here since this class is only for to_json. May also be worth a tiny statement about to_json/from_json support in the "Data Conversion" section of the README.
| self.field1 = field1 | ||
| self.field2 = "foo" | ||
|
|
||
| @classmethod |
There was a problem hiding this comment.
Can we add a test for @staticmethod too just in case?
temporalio/converter.py
Outdated
| @@ -499,6 +536,15 @@ def default(self, o: Any) -> Any: | |||
|
|
|||
| See :py:meth:`json.JSONEncoder.default`. | |||
| """ | |||
| # Custom encoding and decoding through to_json and from_json | |||
| # to_json should be an instance method with only self argument | |||
| to_json = "to_json" | |||
There was a problem hiding this comment.
I too could not find this method used anywhere (Pydantic V1 and V2 have similar but different names). However, I am concerned our users may have it defined on a model and this code may start to break on upgrade. I wonder if we need some kind of global opt-out or something just to be safe. One option could just be module level const TO_JSON_METHOD_NAME = "to_json" (and from equivalent) so they could change it . Thoughts?
There was a problem hiding this comment.
I'm also concerned that the to_json name might be in use by users with different semantics (after all, to_json should really return bytes). How about calling it "to_jsonable_python"? This is the name that pydantic have chosen for this functionality: https://docs.pydantic.dev/latest/api/pydantic_core/#pydantic_core.to_jsonable_python
It's pretty likely that anyone who already has a method with that name has the expected semantics.
There was a problem hiding this comment.
Also, in some cases pydantic's to_jsonable_python will be a suitable implementation of this method, in which case the name will be helpfully suggestive.
There was a problem hiding this comment.
I think we need to keep it Temporal specific, I think @mfateev just did some searching and saw no library was expecting to_json/from_json on models (I can't find any either) so he used it. If we are concerned about clashing, which I admittedly am, I would prefer temporal_to_json and temporal_from_json so it's not accidentally used for any other purpose.
There was a problem hiding this comment.
IMO the name should not be to_json for at least two reasons:
-
A function named
to_jsonshould return valid JSON bytes, not Python objects. -
Pydantic uses the name
to_jsonto return valid JSON bytes, not Python objects.
The Python objects wouldn't even conform to JSON, e.g. they could have non-string keys.
As they say on the pydantic-ai GitHub page, Pydantic is very dominant in the Python AI-oriented ecosystem
the validation layer of the OpenAI SDK, the Anthropic SDK, LangChain, LlamaIndex, AutoGPT, Transformers, CrewAI, Instructor and many more
so, in addition to it making sense from a typing POV, it also makes sense for our conventions to be consistent with those of Pydantic.
Calling the method to_jsonable_python would be consistent with Pydantic and would almost never clash with a method that wasn't suitable for purpose. But we could prefix with temporal_, or introduce a TO_JSONABLE_PYTHON_METHOD_NAME escape hatch.
Although other libraries have in the past used "to_json" to mean "return jsonable python", it's clear that Pydantic has taken a stance against that in favor of using names that are more correct from a type POV.
There was a problem hiding this comment.
I'd rather not be consistent with Pydantic or be consistent with any library. I think these methods should be Temporal-SDK-only methods that have no other purpose or expectations to match any other use but Temporal's. I think to_temporal_json and from_temporal_json (or switch verb and temporal around there) is ideal to make sure there is no confusion about what they are for or their very limited use.
What was changed
Added ability to customize a class conversion by adding to_json and from_json methods.
Why?
Some classes, for example, a reference to a large value stored in an external database, require custom conversion logic.