Merge pull request #120 from ambitioninc/develop

3.1.0

.github/workflows/tests.yml

@@ -18,17 +18,15 @@ jobs:
         # AttributeError: module 'collections' has no attribute 'Callable'
         # https://github.com/nose-devs/nose/issues/1099
         django:
-        - 'Django~=2.2.0'
-        - 'Django~=3.0.0'
-        - 'Django~=3.1.0'
         - 'Django~=3.2.0'
         - 'Django~=4.0.0'
         - 'Django~=4.1.0'
+        - 'Django~=4.2.0'
         experimental: [false]
-        include:
-        - python: '3.9'
-          django: 'https://github.com/django/django/archive/refs/heads/main.zip#egg=Django'
-          experimental: true
+        # include:
+        # - python: '3.9'
+        #   django: 'https://github.com/django/django/archive/refs/heads/main.zip#egg=Django'
+        #   experimental: true
           # NOTE this job will appear to pass even when it fails because of
           # `continue-on-error: true`. Github Actions apparently does not
           # have this feature, similar to Travis' allow-failure, yet.
@@ -38,6 +36,8 @@ jobs:
           django: 'Django~=4.0.0'
         - python: '3.7'
           django: 'Django~=4.1.0'
+        - python: '3.7'
+          django: 'Django~=4.2.0'
     services:
       postgres:
         image: postgres:latest
@@ -53,8 +53,8 @@ jobs:
           --health-timeout 5s
           --health-retries 5
     steps:
-    - uses: actions/checkout@v2
-    - uses: actions/setup-python@v2
+    - uses: actions/checkout@v3
+    - uses: actions/setup-python@v3
       with:
         python-version: ${{ matrix.python }}
     - name: Setup
@@ -69,7 +69,7 @@ jobs:
       env:
         DB_SETTINGS: >-
           {
-            "ENGINE":"django.db.backends.postgresql_psycopg2",
+            "ENGINE":"django.db.backends.postgresql",
             "NAME":"querybuilder",
             "USER":"postgres",
             "PASSWORD":"postgres",
@@ -78,7 +78,7 @@ jobs:
           }
         DB_SETTINGS2: >-
           {
-            "ENGINE":"django.db.backends.postgresql_psycopg2",
+            "ENGINE":"django.db.backends.postgresql",
             "NAME":"querybuilder2",
             "USER":"postgres",
             "PASSWORD":"postgres",

.gitignore

@@ -38,3 +38,4 @@ docs/_build/
 test_ambition_dev
 
 .tox/
+tmp/

README.rst

@@ -34,9 +34,8 @@ where django querysets simplify the sql generation process for simple queries.
 
 Requirements
 ------------
-* Python 2.7
-* Python 3.3, 3.4
-* Django 1.7+
+* Python 3.7 - 3.9
+* Django 2.2 - 4.1
 * Postgres 9.3+
 
 Installation

docs/release_notes.rst

@@ -1,9 +1,23 @@
 Release Notes
 =============
 
+v3.1.0
+------
+* django 4.2 support
+
+v3.0.4
+------
+* Adjusted querybuilder select functionality to process json values as needed in the result set
+  rather than tweak the deep cursor settings. This was observed to interfere with complex query chains.
+
+v3.0.3
+------
+* Addressed bug in `json_cursor` if Django cursor has extra wrappers
+
 v3.0.2
 ------
-* Add `json_cursor` to handle django no longer automatically parsing json fields
+* Add `json_cursor` context to handle Django3.1.1+ no longer automatically parsing json fields
+* Adjusted query functionality also to handle jsonb columns correctly
 
 v3.0.1
 ------

manage.py

@@ -1,6 +1,13 @@
 #!/usr/bin/env python
 import sys
 
+# Show warnings about django deprecations - uncomment for version upgrade testing
+import warnings
+from django.utils.deprecation import RemovedInNextVersionWarning
+warnings.filterwarnings('always', category=DeprecationWarning)
+warnings.filterwarnings('always', category=PendingDeprecationWarning)
+warnings.filterwarnings('always', category=RemovedInNextVersionWarning)
+
 from settings import configure_settings
 
 

querybuilder/__init__.py

@@ -1,4 +1,2 @@
 # flake8: noqa
 from .version import __version__
-
-default_app_config = 'querybuilder.apps.QueryBuilderConfig'

querybuilder/cursor.py

@@ -1,6 +1,60 @@
 import contextlib
 import json
-import psycopg2
+from psycopg2.extras import register_default_jsonb
+from psycopg2._json import JSONB_OID
+
+
+def jsonify_cursor(django_cursor, enabled=True):
+    """
+    Adjust an already existing cursor to ensure it will return structured types (list or dict)
+    from jsonb columns instead of strings. Django 3.1.1+ returns strings for raw queries.
+    https://code.djangoproject.com/ticket/31956
+    https://code.djangoproject.com/ticket/31973
+    https://www.psycopg.org/docs/extras.html#psycopg2.extras.register_default_jsonb
+    """
+
+    # The thing that is returned by connection.cursor() is (normally) a Django object
+    # of type CursorWrapper that itself has the "real" cursor as a property called cursor.
+    # However, it could be a CursorDebugWrapper instead, or it could be an outer wrapper
+    # wrapping one of those. For example django-debug-toolbar wraps CursorDebugWrapper in
+    # a NormalCursorWrapper. The django-db-readonly package wraps the Django CursorWrapper
+    # in a ReadOnlyCursorWrapper. I'm not sure if they ever nest multiple levels. I tried
+    # looping with `while isinstance(inner_cursor, CursorWrapper)`, but it seems that the
+    # outer wrapper is not necessarily a subclass of the Django wrapper. My next best option
+    # is to make the assumption that we need to get to the last property called `cursor`,
+    # basically assuming that any wrapper is going to have a property called `cursor`
+    # that is the real cursor or the next-level wrapper.
+    # Another option might be to check the class of inner_cursor to see if it is the real
+    # database cursor. That would require importing more django libraries, and probably
+    # having to handle some changes in those libraries over different versions.
+
+    # This register_default_jsonb functionality in psycopg2 does not itself have a "deregister"
+    # capability. So to deregister, we pass in a different value for the loads method; in this
+    # case just the str() built-in, which just returns the value passed in. Note that passing
+    # None for loads does NOT do a deregister; it uses the default value, which as it turns out
+    # is json.loads anyway!
+    loads_func = json.loads if enabled else str
+
+    # We expect that there is always at least one wrapper, but we might as well handle
+    # the possibility that we get passed the inner cursor.
+    inner_cursor = django_cursor
+
+    while hasattr(inner_cursor, 'cursor'):
+        inner_cursor = inner_cursor.cursor
+
+    # Hopefully we have the right thing now, but try/catch so we can get a little better info
+    # if it is not. Another option might be an isinstance, or another function that tests the cursor?
+    try:
+        register_default_jsonb(conn_or_curs=inner_cursor, loads=loads_func)
+    except TypeError as e:
+        raise Exception(f'jsonify_cursor: conn_or_curs was actually a {type(inner_cursor)}: {e}')
+
+
+def dejsonify_cursor(django_cursor):
+    """
+    Re-adjust a cursor that was "jsonified" so it no longer performs the json.loads().
+    """
+    jsonify_cursor(django_cursor, enabled=False)
 
 
 @contextlib.contextmanager
@@ -12,5 +66,62 @@ def json_cursor(django_database_connection):
     https://www.psycopg.org/docs/extras.html#psycopg2.extras.register_default_jsonb
     """
     with django_database_connection.cursor() as cursor:
-        psycopg2.extras.register_default_jsonb(conn_or_curs=cursor.cursor, loads=json.loads)
+        jsonify_cursor(cursor)
         yield cursor
+        # This should really not be necessary, because the cursor context manager will
+        # be closing the cursor on __exit__ anyway. But just in case.
+        dejsonify_cursor(cursor)
+
+
+def json_fetch_all_as_dict(cursor):
+    """
+    Iterates over a result set and converts each row to a dictionary.
+    The cursor passed in is assumed to have just executed a raw Postgresql query.
+    If the cursor's columns include any with the jsonb type, the process includes
+    examining every value from those columns. If the value is a string, a json.loads()
+    is attempted on the value, because in Django 3.1.1 and later, this is not
+    handled automatically for raw sql as it was before. There is no compatibility
+    issue running with older Django versions because if the value is not a string,
+    (e.g. it has already been converted to a list or dict), the loads() is skipped.
+    Note that JSON decoding errors are ignored (and the original result value is provided)
+    because it is possible that the query involved an actual json query, say on a single
+    string property of the underlying column data. In that case, the column type is
+    still jsonb, but the result value is a string as it should be. This ignoring of
+    errors is the same logic used in json handling in Django's from_db_value() method.
+
+    :return: A list of dictionaries where each row is a dictionary
+    :rtype: list of dict
+    """
+
+    colnames = [col.name for col in cursor.description]
+    coltypes = [col.type_code for col in cursor.description]
+    # Identify any jsonb columns in the query, by column index
+    jsonbcols = [i for i, x in enumerate(coltypes) if x == JSONB_OID]
+
+    # Optimize with a simple comprehension if we know there are no jsonb columns to handle.
+    if not jsonbcols:
+        return [
+            dict(zip(colnames, row))
+            for row in cursor.fetchall()
+        ]
+
+    # If there are jsonb columns, intercept the result rows and run a json.loads() on any jsonb
+    # columns that are presenting as strings.
+    # In Django 3.1.0 they would already be a json type (e.g. dict or list) but in Django 3.1.1 it changes
+    # and raw sql queries return strings for jsonb columns.
+    # https://docs.djangoproject.com/en/4.0/releases/3.1.1/
+    results = []
+
+    for row in cursor.fetchall():
+        rowvals = list(row)
+        for colindex in jsonbcols:
+            if type(rowvals[colindex]) is str:  # need to check type to avoid attempting to jsonify a None
+                try:
+                    rowvals[colindex] = json.loads(rowvals[colindex])
+                # It is possible that we are selecting a sub-value from the json in the column. I.e.
+                # we got here because it IS a jsonb column, but what we selected is not json and will
+                # fail to parse. In that case, we already have the value we want in place.
+                except json.JSONDecodeError:
+                    pass
+        results.append(dict(zip(colnames, rowvals)))
+    return results

querybuilder/query.py

@@ -9,10 +9,11 @@
 get_model = apps.get_model
 import six
 
-
 from querybuilder.fields import FieldFactory, CountField, MaxField, MinField, SumField, AvgField
 from querybuilder.helpers import set_value_for_keypath, copy_instance
 from querybuilder.tables import TableFactory, ModelTable, QueryTable
+from querybuilder.cursor import json_fetch_all_as_dict
+
 
 SERIAL_DTYPES = ['serial', 'bigserial']
 
@@ -640,7 +641,11 @@ def get_cursor(self):
         :rtype: :class:`CursorDebugWrapper <django:django.db.backends.util.CursorDebugWrapper>`
         :returns: A database cursor
         """
-        return self.connection.cursor()
+        cursor = self.connection.cursor()
+        # Do not set up the cursor in psycopg2 to run json.loads on jsonb columns here. Do it
+        # right before we run a select, and then set it back after that.
+        # jsonify_cursor(cursor)
+        return cursor
 
     def from_table(self, table=None, fields='*', schema=None, **kwargs):
         """
@@ -1936,11 +1941,7 @@ def _fetch_all_as_dict(self, cursor):
         :return: A list of dictionaries where each row is a dictionary
         :rtype: list of dict
         """
-        desc = cursor.description
-        return [
-            dict(zip([col[0] for col in desc], row))
-            for row in cursor.fetchall()
-        ]
+        return json_fetch_all_as_dict(cursor)
 
 
 class QueryWindow(Query):

querybuilder/tables.py

@@ -1,7 +1,6 @@
-import abc
+from abc import ABCMeta
 
 from django.db.models.base import ModelBase
-from six import string_types, with_metaclass
 
 import querybuilder
 from querybuilder.fields import FieldFactory
@@ -31,7 +30,7 @@ def __new__(cls, table, *args, **kwargs):
             kwargs.update(alias=list(table.keys())[0])
             table = list(table.values())[0]
             table_type = type(table)
-        if isinstance(table, string_types):
+        if isinstance(table, str):
             return SimpleTable(table, **kwargs)
         elif issubclass(table_type, ModelBase):
             return ModelTable(table, **kwargs)
@@ -44,7 +43,7 @@ def __new__(cls, table, *args, **kwargs):
         return None
 
 
-class Table(with_metaclass(abc.ABCMeta, object)):
+class Table(object, metaclass=ABCMeta):
     """
     Abstract table class that all table types extend.
 
@@ -259,7 +258,7 @@ def add_fields(self, fields):
             or ``Field`` instance
         :type fields: str or tuple or list of str or list of Field or :class:`Field <querybuilder.fields.Field>`
         """
-        if isinstance(fields, string_types):
+        if isinstance(fields, str):
             fields = [fields]
         elif type(fields) is tuple:
             fields = list(fields)

querybuilder/tests/__init__.py

@@ -1 +0,0 @@
-default_app_config = 'querybuilder.tests.apps.QuerybuilderTestConfig'

querybuilder/tests/json_tests.py

@@ -1,5 +1,4 @@
 import unittest
-from django import VERSION
 from django.test.utils import override_settings
 from querybuilder.fields import JsonField
 from querybuilder.query import Query, JsonQueryset
@@ -47,11 +46,7 @@ def test_one(self):
             )
         )
 
-        # Django 3.1 changes the raw queryset behavior so querybuilder isn't going to change that behavior
-        if self.is_31_or_above():
-            self.assertEqual(query.select(), [{'my_two_alias': '"two"'}])
-        else:
-            self.assertEqual(query.select(), [{'my_two_alias': 'two'}])
+        self.assertEqual(query.select(), [{'my_two_alias': 'two'}])
 
         query = Query().from_table(MetricRecord, fields=[one_field]).where(**{
             one_field.get_where_key(): '1'
@@ -64,11 +59,7 @@ def test_one(self):
             )
         )
 
-        # Django 3.1 changes the raw queryset behavior so querybuilder isn't going to change that behavior
-        if self.is_31_or_above():
-            self.assertEqual(query.select(), [{'my_one_alias': '1'}])
-        else:
-            self.assertEqual(query.select(), [{'my_one_alias': 1}])
+        self.assertEqual(query.select(), [{'my_one_alias': 1}])
 
         query = Query().from_table(MetricRecord, fields=[one_field]).where(**{
             one_field.get_where_key(): '2'
@@ -82,12 +73,14 @@ def test_one(self):
         )
         self.assertEqual(query.select(), [])
 
-    def is_31_or_above(self):
-        if VERSION[0] == 3 and VERSION[1] >= 1:
-            return True
-        elif VERSION[0] > 3:
-            return True
-        return False
+    # Currently unused (but maybe again sometime) function to check django version for test results
+    # from django import VERSION
+    # def is_31_or_above(self):
+    #     if VERSION[0] == 3 and VERSION[1] >= 1:
+    #         return True
+    #     elif VERSION[0] > 3:
+    #         return True
+    #     return False
 
 
 @override_settings(DEBUG=True)