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

JSONfield compatability #449

Closed
colin-byrne-1 opened this issue Jan 12, 2020 · 15 comments
Closed

JSONfield compatability #449

colin-byrne-1 opened this issue Jan 12, 2020 · 15 comments

Comments

@colin-byrne-1
Copy link

colin-byrne-1 commented Jan 12, 2020

This package is currently importing JSONfield from either django-jsonfield + django-jsonfield-compat or django_mysql, however the django-jsonfield/django-jsonfield-compat combination only supports up to django 1.9.

I can make a PR for importing from the current preferred location, django.contrib.postgres.fields, but I just need to know if you'd also want to continue support for django =< 1.9 and therefore allow import from the existing packages.

I am not a mysql user, but it appears no change is needed there, and importing from django_mysql is still the preferred method.

@yangpten
Copy link

I for one, would like this to happen

@cb109
Copy link
Contributor

cb109 commented Feb 19, 2020

@cobyrne09 Thanks for mentioning this, did you run into actual problems trying to get actstream to work in your project?

  • If yes: Please post the full traceback so someone can have a look.

  • If no: Looking at the test matrix (https://travis-ci.org/justquick/django-activity-stream) we currently test for Django 2.2 and 3.0 (support for older versions seems to have been dropped looking at recent commits) in combination MySQL and Postgres and all those are green. While it is not recommended to usedjango-jsonfield for newer Django versions, it still seems to do its job, maybe because it's coupled with django-jsonfield-compat. So no immediate action must be taken.

    I agree though, that we may want to update the current setup and use the native postgres field. This would however give us only two databases to work with, Postgres and MySQL. I would personally still want to have a TextField fallback for any other database, e.g. SQLite. django-jsonfield seems to have its own JSONField that uses some Postgres native features, but is not the same field that now comes with Django natively. Ideally, we'd want this:

    • Using Postgres: Use django.contrib.postgres.fields
    • Using MySQL: Use django_mysql.models.JSONField
    • Using any other database: Fallback to jsonfield.JSONField which uses a TextField underneath

    The current logic is very close to that already (https://github.com/justquick/django-activity-stream/blob/master/actstream/jsonfield.py), but based on which packages are installed, not which database type is being used, which may be a better indicator.

To summarize: Having a fresh look at which types of JSONFields are supported and how we pick one is welcome. django-jsonfield is not really there to support older Django versions, but more to allow different databases to use the feature. We should really use the native Postgres field though, if possible.

Pinging @auvipy for his input 👋

@cb109
Copy link
Contributor

cb109 commented Feb 19, 2020

Hoping for this one in the future to ease the struggle: django/django#12392

@travijuu
Copy link

please remove django-jsonfield-compat dependency, nobody takes care of this package.

@cb109
Copy link
Contributor

cb109 commented Mar 20, 2020

@travijuu is there an actual problem you are running into right now because of the mentioned package or just fear it may happen in the future?

@travijuu
Copy link

travijuu commented Mar 20, 2020

@cb109 When I try to use django-activity-stream with Django 3.0, I got error below

django.core.exceptions.ImproperlyConfigured: You must either install django-jsonfield + django-jsonfield-compat, or django-mysql as an alternative, if you wish to use a JSONField on your actions

Note: both django-jsonfield and django-jsonfield-compat already installed.

When I dig into error, I see invalid import statement of six in this link.

from django.utils import six

after I replaced this line with import six then Django 3.0 started working.

@dragonpaw
Copy link

dragonpaw commented Jul 8, 2020

FWIW: I did run into a peculiar crash related to mixing django-jsonfield-compat with django_schemas. It seems like somewhere between the two, they get into a circular dependency. The only way I could get around this was disable USE_JSONFIELD for actstream and tear out django-jsonfield-compat.

Traceback (most recent call last):
  File "/Users/ash/.pyenv/versions/django/bin/pytest", line 8, in <module>
    sys.exit(main())
  File "/Users/ash/.pyenv/versions/3.8.2/envs/django/lib/python3.8/site-packages/_pytest/config/__init__.py", line 73, in main
    config = _prepareconfig(args, plugins)
  File "/Users/ash/.pyenv/versions/3.8.2/envs/django/lib/python3.8/site-packages/_pytest/config/__init__.py", line 223, in _prepareconfig
    return pluginmanager.hook.pytest_cmdline_parse(
  File "/Users/ash/.pyenv/versions/3.8.2/envs/django/lib/python3.8/site-packages/pluggy/hooks.py", line 286, in __call__
    return self._hookexec(self, self.get_hookimpls(), kwargs)
  File "/Users/ash/.pyenv/versions/3.8.2/envs/django/lib/python3.8/site-packages/pluggy/manager.py", line 93, in _hookexec
    return self._inner_hookexec(hook, methods, kwargs)
  File "/Users/ash/.pyenv/versions/3.8.2/envs/django/lib/python3.8/site-packages/pluggy/manager.py", line 84, in <lambda>
    self._inner_hookexec = lambda hook, methods, kwargs: hook.multicall(
  File "/Users/ash/.pyenv/versions/3.8.2/envs/django/lib/python3.8/site-packages/pluggy/callers.py", line 203, in _multicall
    gen.send(outcome)
  File "/Users/ash/.pyenv/versions/3.8.2/envs/django/lib/python3.8/site-packages/_pytest/helpconfig.py", line 89, in pytest_cmdline_parse
    config = outcome.get_result()
  File "/Users/ash/.pyenv/versions/3.8.2/envs/django/lib/python3.8/site-packages/pluggy/callers.py", line 80, in get_result
    raise ex[1].with_traceback(ex[2])
  File "/Users/ash/.pyenv/versions/3.8.2/envs/django/lib/python3.8/site-packages/pluggy/callers.py", line 187, in _multicall
    res = hook_impl.function(*args)
  File "/Users/ash/.pyenv/versions/3.8.2/envs/django/lib/python3.8/site-packages/_pytest/config/__init__.py", line 794, in pytest_cmdline_parse
    self.parse(args)
  File "/Users/ash/.pyenv/versions/3.8.2/envs/django/lib/python3.8/site-packages/_pytest/config/__init__.py", line 1000, in parse
    self._preparse(args, addopts=addopts)
  File "/Users/ash/.pyenv/versions/3.8.2/envs/django/lib/python3.8/site-packages/_pytest/config/__init__.py", line 957, in _preparse
    self.hook.pytest_load_initial_conftests(
  File "/Users/ash/.pyenv/versions/3.8.2/envs/django/lib/python3.8/site-packages/pluggy/hooks.py", line 286, in __call__
    return self._hookexec(self, self.get_hookimpls(), kwargs)
  File "/Users/ash/.pyenv/versions/3.8.2/envs/django/lib/python3.8/site-packages/pluggy/manager.py", line 93, in _hookexec
    return self._inner_hookexec(hook, methods, kwargs)
  File "/Users/ash/.pyenv/versions/3.8.2/envs/django/lib/python3.8/site-packages/pluggy/manager.py", line 84, in <lambda>
    self._inner_hookexec = lambda hook, methods, kwargs: hook.multicall(
  File "/Users/ash/.pyenv/versions/3.8.2/envs/django/lib/python3.8/site-packages/pluggy/callers.py", line 208, in _multicall
    return outcome.get_result()
  File "/Users/ash/.pyenv/versions/3.8.2/envs/django/lib/python3.8/site-packages/pluggy/callers.py", line 80, in get_result
    raise ex[1].with_traceback(ex[2])
  File "/Users/ash/.pyenv/versions/3.8.2/envs/django/lib/python3.8/site-packages/pluggy/callers.py", line 187, in _multicall
    res = hook_impl.function(*args)
  File "/Users/ash/.pyenv/versions/django/lib/python3.8/site-packages/pytest_django/plugin.py", line 325, in pytest_load_initial_conftests
    _setup_django()
  File "/Users/ash/.pyenv/versions/django/lib/python3.8/site-packages/pytest_django/plugin.py", line 227, in _setup_django
    django.setup()
  File "/Users/ash/.pyenv/versions/3.8.2/envs/django/lib/python3.8/site-packages/django/__init__.py", line 24, in setup
    apps.populate(settings.INSTALLED_APPS)
  File "/Users/ash/.pyenv/versions/3.8.2/envs/django/lib/python3.8/site-packages/django/apps/registry.py", line 91, in populate
    app_config = AppConfig.create(entry)
  File "/Users/ash/.pyenv/versions/3.8.2/envs/django/lib/python3.8/site-packages/django/apps/config.py", line 90, in create
    module = import_module(entry)
  File "/Users/ash/.pyenv/versions/3.8.2/lib/python3.8/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1014, in _gcd_import
  File "<frozen importlib._bootstrap>", line 991, in _find_and_load
  File "<frozen importlib._bootstrap>", line 975, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 671, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 783, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "/Users/ash/.pyenv/versions/3.8.2/envs/django/lib/python3.8/site-packages/jsonfield_compat/__init__.py", line 1, in <module>
    from jsonfield_compat.fields import JSONField
  File "/Users/ash/.pyenv/versions/3.8.2/envs/django/lib/python3.8/site-packages/jsonfield_compat/fields.py", line 3, in <module>
    if use_native_jsonfield():
  File "/Users/ash/.pyenv/versions/3.8.2/envs/django/lib/python3.8/site-packages/jsonfield_compat/util.py", line 17, in use_native_jsonfield
    return django_supports_native_jsonfield() and is_db_postgresql() and \
  File "/Users/ash/.pyenv/versions/3.8.2/envs/django/lib/python3.8/site-packages/jsonfield_compat/util.py", line 9, in is_db_postgresql
    return connection.vendor == 'postgresql'
  File "/Users/ash/.pyenv/versions/3.8.2/envs/django/lib/python3.8/site-packages/django/db/__init__.py", line 28, in __getattr__
    return getattr(connections[DEFAULT_DB_ALIAS], item)
  File "/Users/ash/.pyenv/versions/3.8.2/envs/django/lib/python3.8/site-packages/django/db/utils.py", line 201, in __getitem__
    backend = load_backend(db['ENGINE'])
  File "/Users/ash/.pyenv/versions/3.8.2/envs/django/lib/python3.8/site-packages/django/db/utils.py", line 110, in load_backend
    return import_module('%s.base' % backend_name)
  File "/Users/ash/.pyenv/versions/3.8.2/lib/python3.8/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "/Users/ash/.pyenv/versions/3.8.2/envs/django/lib/python3.8/site-packages/django_tenants/postgresql_backend/base.py", line 8, in <module>
    from django.contrib.contenttypes.models import ContentType
  File "/Users/ash/.pyenv/versions/3.8.2/envs/django/lib/python3.8/site-packages/django/contrib/contenttypes/models.py", line 133, in <module>
    class ContentType(models.Model):
  File "/Users/ash/.pyenv/versions/3.8.2/envs/django/lib/python3.8/site-packages/django/db/models/base.py", line 103, in __new__
    app_config = apps.get_containing_app_config(module)
  File "/Users/ash/.pyenv/versions/3.8.2/envs/django/lib/python3.8/site-packages/django/apps/registry.py", line 252, in get_containing_app_config
    self.check_apps_ready()
  File "/Users/ash/.pyenv/versions/3.8.2/envs/django/lib/python3.8/site-packages/django/apps/registry.py", line 135, in check_apps_ready
    raise AppRegistryNotReady("Apps aren't loaded yet.")
django.core.exceptions.AppRegistryNotReady: Apps aren't loaded yet.

(Admittedly, not actstream's fault, but related to the use of the old jsonfield packages.)

@jayvdb
Copy link

jayvdb commented Sep 13, 2020

Just noting, the dj31 jsonfield is also available as https://github.com/laymonage/django-jsonfield-backport , and it supports most dbs

@auvipy
Copy link
Collaborator

auvipy commented Sep 13, 2020

contributions wellcome

@lociii
Copy link
Contributor

lociii commented Nov 27, 2020

I'd like to provide a pull request. However there are some design decisions to be clarified in advance.

As django-jsonfield-backport will make the JSONField available in all Django versions supported by this app (>= 2.2) and all major databases, there is no need to make it optional anymore.

  1. Would you agree that we should drop support for all current methods and only support django-jsonfield-backport?
  2. Would you agree that we should drop support for USE_JSONFIELD config and have the field available unconditionally?

@jayvdb
Copy link

jayvdb commented Nov 28, 2020

@lociii , IMO that sounds like a good approach.

@auvipy
Copy link
Collaborator

auvipy commented Nov 28, 2020

I'd like to provide a pull request. However there are some design decisions to be clarified in advance.

As django-jsonfield-backport will make the JSONField available in all Django versions supported by this app (>= 2.2) and all major databases, there is no need to make it optional anymore.

1. Would you agree that we should drop support for all current methods and only support `django-jsonfield-backport`?

2. Would you agree that we should drop support for USE_JSONFIELD config and have the field available unconditionally?

while I support number 1, but the 2nd one needs to go through a deprecation warning to keep BC.

@lociii
Copy link
Contributor

lociii commented Nov 28, 2020

OK, then let's start with replacing the libs.
Getting rid of the option is far more complicated anyway because of the inconsistent state of the migration.

@auvipy
Copy link
Collaborator

auvipy commented Nov 28, 2020

yeah sure, I goose step at a time

@lociii
Copy link
Contributor

lociii commented Jan 14, 2021

Switch from all the different libraries to Django built-in JSONField or the backported package has been merged.

@auvipy auvipy closed this as completed Jan 14, 2021
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

No branches or pull requests

8 participants