🐛 Fix jsonable_encoder alters json_encoders of Pydantic v1 objects#4972
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
Clean PR |
| encoder = getattr(obj.__config__, "json_encoders", {}) | ||
| if custom_encoder: | ||
| encoder.update(custom_encoder) | ||
| encoder = {**encoder, **custom_encoder} |
There was a problem hiding this comment.
Can you explain, how does this fix the jsonable_encoder side effect?
There was a problem hiding this comment.
The following snippet from slackner illustrates the side effect in 2 cases:
- With
print(c): side effect fromb = jsonable_encoder(creds, custom_encoder=ENCODERS)when callingjsonable_encoderlater on the same instance but with a different custom_encoder - With
print(d): side effect fromb = jsonable_encoder(creds, custom_encoder=ENCODERS)when callingjsonable_encoderlater on another instance
from pydantic import BaseModel, SecretStr
from fastapi.encoders import jsonable_encoder
class Credentials(BaseModel):
password: SecretStr
ENCODERS = {SecretStr: lambda v: v.get_secret_value() if v is not None else None}
creds = Credentials(password="helloworld")
a = jsonable_encoder(creds)
print(a) # {'password': '**********'}, as expected
b = jsonable_encoder(creds, custom_encoder=ENCODERS)
print(b) # {'password': 'helloworld'}, as expected
c = jsonable_encoder(creds)
print(c) # gives {'password': 'helloworld'}, but should be {'password': '**********'}?
creds = Credentials(password="123456789")
d = jsonable_encoder(creds)
print(d) # gives {'password': '123456789'}, but should be {'password': '**********'}?Before this PR, calling jsonable_encoder with a custom_encoder updates the global "json_encoders" config of the model.
After this PR, calling jsonable_encoder with a custom_encoer argument only affects the current call
There was a problem hiding this comment.
Thank you very much, easy to understand
|
What's the next step on this ? @odiseo0 @nurettinabaci |
a57928d to
76cc8d7
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
Any plan to include this fix in a future release ? The bug seems to still be here on the master branch, it happens when you use multiple jsonable_encoders on the same model |
|
📝 Docs preview for commit 50aa786 at: https://6421d467c393971f9f7d59a6--fastapi.netlify.app |
7bb362f to
fa4b7c8
Compare
YuriiMotov
left a comment
There was a problem hiding this comment.
LGTM!
@aboubacs, good catch! Thank you!
There are several more occurrences of o.isoformat() in tests. I think it should be addressed in separate PR and in consistent way (all occurrences of the problem)
jsonable_encoder alters json_encoders of Pydantic objects
jsonable_encoder alters json_encoders of Pydantic objectsjsonable_encoder alters json_encoders of Pydantic v1 objects
This PR removes a side effect in the
jsonable_encoderfunction, which was modifying thejson_encodersconfiguration of pydantic models when any instance of that model was given to the function with a custom encoder, which fixes this issue: #4962Explanation: #4972 (comment)
UPD YuriiMotov: it also attempted to fix the following issue in tests, but I reverted this change since it should be done in separate PR and in consistent way:
o.isoformat()too.strftime("%H:%M:%S"), becauseisoformatis the already the default pydantic encoder for datetime. The test wasn't properly testing the feature because it could pass even without the custom_encoder