laymonage / django-jsonfield-backport

Backport of the cross-DB JSONField model and form fields from Django 3.1.
https://pypi.org/project/django-jsonfield-backport
BSD 3-Clause "New" or "Revised" License
42 stars 6 forks source link

Add support for simplejson #27

Closed wrvdklooster closed 3 years ago

wrvdklooster commented 3 years ago

We are using Decimal fields in json which is supported in simplejson but not in standard json.

When using django-jsonfield-backport we run into an issue as this uses the standard json library:

  File "python3.8/site-packages/django_jsonfield_backport/models.py", line 149, in get_prep_value
    return json.dumps(value, cls=self.encoder)
  File "python3.8/json/__init__.py", line 231, in dumps
    return _default_encoder.encode(obj)
  File "python3.8/json/encoder.py", line 199, in encode
    chunks = self.iterencode(o, _one_shot=True)
  File "python3.8/json/encoder.py", line 257, in iterencode
    return _iterencode(o, 0)
  File "python3.8/json/encoder.py", line 179, in default
    raise TypeError(f'Object of type {o.__class__.__name__} '
TypeError: Object of type Decimal is not JSON serializable

Would it be possible to add support for both json and simplejson? Maybe add something like this:

try:
    import simplejson as json
except ImportError:
    import json
laymonage commented 3 years ago

Hi @wrvdklooster,

I don't see the advantage of using simplejson here. You can already add support for Decimal using the encoder option with an encoder class that supports Decimal, e.g. DjangoJSONEncoder. If you also need the value to be Decimal on retrieval, you can create a custom decoder and also use that in the JSONField.

wrvdklooster commented 3 years ago

Hi @laymonage,

Thanks for the fast reply. I agree there are other solutions. It is just that simplejson adds some things out of the box and many packages added support for it (e.g. the requests library).

By adding support for simplejson integrating django-jsonfield-backport will be just a matter of adding it to the project without the need for other code changes.

Would you be open for a pull request for this? Or do you prefer not to add it?

laymonage commented 3 years ago

I personally have never used simplejson, but from the looks of it, I don't see much value over the built-in json library. This package aims to be as compatible as possible with the built-in JSONField (both from contrib.postgres and db.models), so upgrading to the built-in JSONField won't be too painful. Using simplejson introduces different behaviors in how JSON is encoded and decoded. It would also be a backward-incompatible release, as some people who have simplejson installed may not expect it to be automatically used, given the previous releases.

Also, if you want to leverage the features from simplejson, doing

try:
    import simplejson as json
except ImportError:
    import json

may not be enough, because some features of simplejson needs to be explicitly used through parameters. That would require edits to the json.dumps() and json.loads() calls throughout the package.

In addition, people are actually moving away from simplejson, even requests: https://github.com/psf/requests/pull/3056

Therefore, I'm sorry, but I don't think I would add it to this package. If you really need it, you can always fork the repository. I hope you understand my decision :pray:

wrvdklooster commented 3 years ago

I understand, not an issue. Thanks for creating package in the first place. I'll fix this one our side.

Thanks for the detailed explanation of your decision.