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

DataclassJSONField implementation #77

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

ghost
Copy link

@ghost ghost commented Nov 7, 2022

as discussed in #75

WIP WIP WIP

Example

@dataclass
class MyDataclass(JsonSchemaMixin):
    id: int
    whatever: str


class MyModel(TimestampedModel):
    single = DataclassJSONField(MyDataclass, null=True)
    many = DataclassJSONField(MyDataclass, many=True, blank=True, default=list)

image

In [1]: m = MyModel.objects.first()

In [2]: m.single
Out[2]: MyDataclass(id=123, whatever='123')

In [3]: m.many
Out[3]: [MyDataclass(id=123, whatever='123'), MyDataclass(id=456, whatever='456')]

In [4]: m.single.whatever='hello'

In [5]: m.many[0].whatever='world'

In [6]: m.many.pop()
Out[6]: MyDataclass(id=456, whatever='456')

In [7]: m.save()

In [8]: m = MyModel.objects.first()

In [9]: m.single
Out[9]: MyDataclass(id=123, whatever='hello')

In [10]: m.many
Out[10]: [MyDataclass(id=123, whatever='world')]

@@ -24,6 +24,8 @@ class DjangoArrayField:


class JSONField(DjangoJSONField):
form_class = JSONFormField
Copy link
Author

Choose a reason for hiding this comment

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

so it's overridable

self,
dataclass_cls: Type[DataclassJsonSchema],
*,
many: bool = False,
Copy link
Author

Choose a reason for hiding this comment

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

This many convention comes from Django Rest Framework

But we can consider making this a DataclassArrayJSONField instead

Copy link
Owner

Choose a reason for hiding this comment

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

I'm going to leave this decision up to you. I'm not familiar with the dataclasses-jsonschema library. So, whatever you think works best, we can go with that.

@@ -0,0 +1,3 @@
# `django_jsonform.contrib.dataclasses`

**Status:** experimental
Copy link
Author

Choose a reason for hiding this comment

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

TODO

import dataclasses
from typing import Type

from dataclasses_jsonschema import JsonSchemaMixin
Copy link
Author

Choose a reason for hiding this comment

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

I don't think we should add this as a dependency to this project.

Need to research how to make this contrib module optional.

Maybe removing the contrib/__init__,py would work?

Copy link
Owner

Choose a reason for hiding this comment

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

This is okay.

To make it an optional dependency, please modify the setup.cfg file and add this at the end:

+ 
+ [options.extras_require]
+ contrib-dataclasses = dataclasses-jsonschema

Users who wish to use this feature can install the dependencies by:

pip install "django-jsonform[contrib-dataclasses]"

@ghost ghost marked this pull request as ready for review November 7, 2022 22:42
@ghost
Copy link
Author

ghost commented Nov 8, 2022

@bhch , I'm unsure if this should become it's own package django-dataclass-field. Thoughts?

Copy link
Owner

@bhch bhch left a comment

Choose a reason for hiding this comment

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

I'm unsure if this should become it's own package django-dataclass-field. Thoughts?

Maybe.

If the goal is to have a dataclass interface to work with json data, why can't we write a function (or a method on JSONField) to convert the json data to a dataclass?

def json_to_dataclass(json_data, dataclass):
    return dataclass_instance

We can also accept a dataclass for the schema argument. And then do the conversions internally.


Anyway, I've reviewed the changes and left some comments.

import dataclasses
from typing import Type

from dataclasses_jsonschema import JsonSchemaMixin
Copy link
Owner

Choose a reason for hiding this comment

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

This is okay.

To make it an optional dependency, please modify the setup.cfg file and add this at the end:

+ 
+ [options.extras_require]
+ contrib-dataclasses = dataclasses-jsonschema

Users who wish to use this feature can install the dependencies by:

pip install "django-jsonform[contrib-dataclasses]"


schema["type"] = "array"
schema["description"] = f"An array of {dataclass_cls.__name__} objects."
del schema["required"]
Copy link
Owner

Choose a reason for hiding this comment

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

The code example in your comment above gives me KeyError: 'required'. Perhaps use this instead:

Suggested change
del schema["required"]
schema.pop("required", None)

schema["type"] = "array"
schema["description"] = f"An array of {dataclass_cls.__name__} objects."
del schema["required"]
schema["items"] = schema["properties"]["item"]
Copy link
Owner

@bhch bhch Nov 8, 2022

Choose a reason for hiding this comment

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

Line 21 also results in a KeyError: 'item'.

Copy link
Author

Choose a reason for hiding this comment

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

Hmm.. This shouldn't be happening.

What Python and dataclasses-jsonschema versions are you using?

Copy link
Owner

Choose a reason for hiding this comment

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

Python: 3.8

dataclasses-jsonschema: 2.16.0

Copy link
Author

Choose a reason for hiding this comment

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

I'm using dataclasses-jsonschema[fast-validation]==2.16.0 and Python 3.10

Will have to try 3.8 later (or make a separate package and drop support for older versions)

I noticed you're supporting a lot of older Django versions too. I only tested with Django==4.1.3

Copy link
Owner

Choose a reason for hiding this comment

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

Just tested with Python 3.10 and Django 4.1.3. Everything works fine this time.

self,
dataclass_cls: Type[DataclassJsonSchema],
*,
many: bool = False,
Copy link
Owner

Choose a reason for hiding this comment

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

I'm going to leave this decision up to you. I'm not familiar with the dataclasses-jsonschema library. So, whatever you think works best, we can go with that.

@dongyuzheng
Copy link

dongyuzheng commented Nov 11, 2022

FYI: I am @ghost. I had to delete my other account for logistics reasons.

I'm starting to lean towards making a separate package because I don't have time to investigate and support older Python/Django versions.

My next step is to fix the "required" fields, because on Admin, I can't submit empty for fields like x: int | None

@dongyuzheng
Copy link

dongyuzheng commented Nov 11, 2022

@bhch does your library support json schema with anyOf? https://stackoverflow.com/questions/35450405/how-to-represent-sum-union-types-in-in-json-schema

Edit:

Looks like No. It fails here

image

@bhch
Copy link
Owner

bhch commented Nov 11, 2022

Currently, anyOf doesn't work but it will be added (#39). I can't seem to come up with a good approach to create forms for anyOf/allOf/oneOf.

I'm starting to lean towards making a separate package because I don't have time to investigate and support older Python/Django versions.

That's understandable. I'd like to avoid breaking changes until the next major release (v3).

@dongyuzheng
Copy link

Another question @bhch , I noticed that the Django admin form sends Empty String for empty integer field, rather than null

I know this is probably because it's hard to tell the difference between null and no input and empty string.

One thing Swagger UI does is they have a tick box [] Send empty value to indicate sending a null.

Thoughts on adding this to the Form?

@bhch
Copy link
Owner

bhch commented Nov 11, 2022

@dongyuzheng Okay, I'll add something similar to that. I've just created an issue for this: #78

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.

None yet

2 participants