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

Json field support #20

Merged
merged 5 commits into from
Feb 8, 2024
Merged

Json field support #20

merged 5 commits into from
Feb 8, 2024

Conversation

djbios
Copy link
Contributor

@djbios djbios commented Jan 31, 2024

Added serializers.JSONField support (pydantic's dict)

Copy link

codecov bot commented Jan 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (fe4ff95) 100.00% compared to head (3cfda19) 98.73%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##              main      #20      +/-   ##
===========================================
- Coverage   100.00%   98.73%   -1.27%     
===========================================
  Files            5        5              
  Lines          154      158       +4     
===========================================
+ Hits           154      156       +2     
- Misses           0        2       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@georgebv
Copy link
Owner

@djbios dict field is already supported here

elif type_.__origin__ is dict:
# Enforced by pydantic, check just in case
assert len(type_.__args__) == 2
if type_.__args__[0] is not str:
raise FieldConversionError(
f"{type_} is not a supported dict type. "
f"Dict annotation must look like dict[str, <annotation>]."
)
return serializers.DictField(
child=_convert_type(type_.__args__[1]),
allow_empty=True,
**kwargs,
)

We cannot convert dict to JSON field because JSON can be either a dict or a list, and those are already implemented separately. Is there a specific example of a pydantic model whose field could be converted to JSON field?

@djbios
Copy link
Contributor Author

djbios commented Feb 1, 2024

@georgebv What I meant is scalar dict, not composite one. The use-case is when you have unstructured data in an API call, that you don't want to define by a model and process as it is. In DB it could be stored as JSONField (or not stored at all, cache info, pass-through gateway, etc.).
So on a pydantic level, you have just dict in that case, without details:

class A(BaseModel):
  unstructured_data: dict = {}

@georgebv
Copy link
Owner

georgebv commented Feb 1, 2024

@djbios can you please elaborate more on the intended relation between pydantic and DRF fields here. My intention is:

  • dict -> DictField() - This is your example
  • dict[str, int] -> DictField(child=IntegerField())
  • list -> ListField()
  • list[int] -> ListField(child=IntegerField())

What type would correspond to a JSONField? My understanding that it's more like dict | list | int | float | bool | str | None or str | binary represented JSON string. First scenario doesn't work well without additional annotation metadata because there would be collision with other types and with second we would have no way to know if we are speaking about str by itself (naked string is a valid JSON in itself), str as StringField, or if str represents some encoded JSON structure.

My solution for situations like this is to allow manually specifying fields in annotation in scenarios where default transformation doesn't work. This would work like:

class MyModel(BaseModel):
    json_value: Annotated[dict, CustomField(JSONField(binary=False)]

This is currently work in progress here: #19

This particular scenario would still be problematic because now JSONField implies that the value can be list, dict, int, float, ... (any valid JSON) but pydantic allows only dict.

@djbios
Copy link
Contributor Author

djbios commented Feb 2, 2024

@georgebv Hm, I see your point now, thanks for the explanation.
You are right - any "jsonable" type could be valid JSONField input, I was misled by having the best practice in my head of always having an object at the root of json in REST API communication.

So, we have to be specific that we expect JSON value, I've pushed an update:

  • Using pydantic's JsonValue alias to indicate json field
  • Fixed some parse logic (ensured that its a class before calling issubclass)
  • Not related to the issue but I think useful: searching for match with explicit mapping through all MRO instead of just first base

PS Having the ability to force using a particular drf field is a very cool feature, looking forward to it!

src/drf_pydantic/parse.py Show resolved Hide resolved
src/drf_pydantic/parse.py Show resolved Hide resolved
src/drf_pydantic/parse.py Show resolved Hide resolved
src/drf_pydantic/parse.py Show resolved Hide resolved
@georgebv georgebv merged commit a147403 into georgebv:main Feb 8, 2024
10 of 11 checks passed
@djbios
Copy link
Contributor Author

djbios commented Feb 12, 2024

@georgebv seems like the new Pypi package was not released with this change. What is the publishing strategy of the package and when json field support could be expected can be expected?

@georgebv
Copy link
Owner

@djbios I'm going to release it later today, wanted to merge this PR first #19

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

Successfully merging this pull request may close these issues.

2 participants