add support for JSONField #47

WaVEV · 2024-06-09T22:34:03Z

fixes #8

timgraham

Some initial comments.

django_mongodb/features.py

django_mongodb/fields.py

django_mongodb/query_utils.py

django_mongodb/operations.py

django_mongodb/expressions.py

django_mongodb/fields.py

django_mongodb/features.py

django_mongodb/compiler.py

timgraham

This isn't an exhaustive review but here are a few things to work on while I continue reviewing.

django_mongodb/features.py

timgraham · 2024-06-20T15:03:55Z

django_mongodb/features.py

@@ -202,6 +204,9 @@ class DatabaseFeatures(BaseDatabaseFeatures):
            "annotations.tests.NonAggregateAnnotationTestCase.test_order_by_annotation",
            # annotate().filter().count() gives incorrect results.
            "db_functions.datetime.test_extract_trunc.DateFunctionTests.test_extract_year_exact_lookup",
+            "model_fields.test_jsonfield.TestQuerying.test_nested_key_transform_on_subquery",
+            "model_fields.test_jsonfield.TestQuerying.test_ordering_grouping_by_count",


This test is "FieldDoesNotExist with ordering."

What I meant is that there is already a section (a few lines above) with this name.

django_mongodb/features.py

timgraham · 2024-06-20T15:09:08Z

django_mongodb/features.py

@@ -279,6 +284,10 @@ class DatabaseFeatures(BaseDatabaseFeatures):
            "update.tests.SimpleTest.test_empty_update_with_inheritance",
            "update.tests.SimpleTest.test_foreign_key_update_with_id",
            "update.tests.SimpleTest.test_nonempty_update_with_inheritance",
+            "model_fields.test_jsonfield.TestQuerying.test_join_key_transform_annotation_expression",
+            "model_fields.test_jsonfield.TestQuerying.test_order_grouping_custom_decoder",


I think these last three don't actually do joins but the logic in SQLCompiler._get_ordering() is naive to the fact that __ is a key transform. You can defer this to a separate commit or PR, but I would put these skips in a separate section that describes the issue.

Mmh you are right, so for this one I think you should create a new PR. The if that check joins is wonky (only checks for __) I think it should be another short task. I will move those test to expected failures.

django_mongodb/fields/json_field.py

django_mongodb/lookups.py

django_mongodb/functions.py

django_mongodb/features.py

timgraham · 2024-06-24T15:07:06Z

django_mongodb/features.py

@@ -8,8 +8,9 @@ class DatabaseFeatures(BaseDatabaseFeatures):
    supports_date_lookup_using_string = False
    supports_foreign_keys = False
    supports_ignore_conflicts = False
-    # Not implemented: https://github.com/mongodb-labs/django-mongodb/issues/8
-    supports_json_field = False
+    supports_json_field = True


You can remove this line since it's what it's the superclass.

timgraham · 2024-06-24T17:14:18Z

django_mongodb/features.py

@@ -202,6 +204,9 @@ class DatabaseFeatures(BaseDatabaseFeatures):
            "annotations.tests.NonAggregateAnnotationTestCase.test_order_by_annotation",
            # annotate().filter().count() gives incorrect results.
            "db_functions.datetime.test_extract_trunc.DateFunctionTests.test_extract_year_exact_lookup",
+            "model_fields.test_jsonfield.TestQuerying.test_nested_key_transform_on_subquery",
+            "model_fields.test_jsonfield.TestQuerying.test_ordering_grouping_by_count",


What I meant is that there is already a section (a few lines above) with this name.

timgraham · 2024-06-24T17:43:34Z

django_mongodb/query_utils.py

@@ -48,4 +48,6 @@ def process_rhs(node, compiler, connection):
 def regex_match(field, value, regex, *re_args, **re_kwargs):
    regex = re.compile(regex % re.escape(value), *re_args, **re_kwargs)
    options = "i" if regex.flags & re.I else ""
-    return {"$regexMatch": {"input": field, "regex": regex.pattern, "options": options}}
+    return {
+        "$regexMatch": {"input": {"$toString": field}, "regex": regex.pattern, "options": options}


Does this change end up solving anything for JSONField? I reverted and model_fields.test_jsonfield still passed. Maybe there's another affected test. Anyway, I'd make it a separate PR since it fixes other non-JSONField tests.

No, it doesn't. It solve only when a json field is a string, number, or null. But It does not pass any new test.

timgraham · 2024-06-26T15:42:06Z

This is looking pretty good to me. I added a couple of commits.

Two remaining concerns:

Would you like to add some comments to the logic inside json.py's functions. I didn't dive into it to try to understand them, but they look pretty gnarly. Even if you as the author understand it now, I wonder if it's going to be easy to understand if you return it later.
I'm not sure what "MongoDB's null behavior is different from SQL's." means. Only some of the tests there seem to query for None. If this is an issue you don't think we'll be able to solve later, it might be time to document it in some more detail in the README.

WaVEV · 2024-06-26T16:43:36Z

This is looking pretty good to me. I added a couple of commits.

Two remaining concerns:

Would you like to add some comments to the logic inside json.py's functions. I didn't dive into it to try to understand them, but they look pretty gnarly. Even if you as the author understand it now, I wonder if it's going to be easy to understand if you return it later.

Yes, I can, I will add some comments there. There are two method that I patched because I wasn't able to find a way to handle them in process_rhs or process_lhs.

I'm not sure what "MongoDB's null behavior is different from SQL's." means. Only some of the tests there seem to query for None. If this is an issue you don't think we'll be able to solve later, it might be time to document it in some more detail in the README.

The behavior of NULL in SQL is unique. For instance, NULL is NULL gives True, NULL = NULL is NULL, NULL <> NULL is NULL, and so forth. When a JSONField is indexed with a non-existent field, it returns NULL. This allows queries like:

condition = Q(value__foo="bax")

To return less values than expected when using (I expect everything with: B or ~B, that is the question)

NullableJSONModel.objects.filter(condition | ~condition)

In SQL, this query could return unexpected results because NULL can make a predicate and its negation both true in some cases.

Handling this behavior in MongoDB requires extra logic. In MongoDB, NULL sometimes behaves like False and other times like True, requiring careful handling to allow this kind of inconsistencies in query results.

timgraham · 2024-06-27T15:19:31Z

django_mongodb/fields/json.py

+    for path in paths:
+        keys.append(_has_key_predicate(path, lhs))
+    if self.mongo_operator is None:
+        assert len(keys) == 1


Was this mainly for debugging? Can we remove it now? We shouldn't rely on assert for anything critical since asserts are removed when running with python -O. If so, I'll make the update along with my next batch of language edits.

Yes, will remove. It was for debugging. Shall we also remove this assert: https://github.com/mongodb-labs/django-mongodb/blob/main/django_mongodb/base.py#L121?

I made a brief attempt to refactor the connection code to conform to Django's normal API, but it wasn't successful. I may return to it later. For now I wouldn't bother with a commit just to remove that line.

timgraham · 2024-06-27T23:03:57Z

Thanks for the documentation. I found some of it a little redundant, so I made it more concise, hopefully without losing any important info. I'll check this once more tomorrow, but I think we're close to a merge. 👍

Jibola · 2024-06-28T13:12:07Z

django_mongodb/fields/json.py

+    result = {
+        "$and": [
+            # The path must exist (i.e. not be "missing").
+            {"$ne": [{"$type": path}, "missing"]},


In what case would the path return missing? Wouldn't it always return null?

If the argument is a field that is missing in the input document, $type returns the string "missing".

ShaneHarvey · 2024-06-28T17:45:26Z

django_mongodb/operations.py

@@ -49,6 +51,12 @@ def adapt_decimalfield_value(self, value, max_digits=None, decimal_places=None):
            return None
        return Decimal128(value)

+    def adapt_json_value(self, value, encoder):


When is this method called and what's the purpose? I ask because json.loads+json.dumps is expensive and ends up with the same value. Also it won't work with pymongo's extended JSON types like ObjectId, Code, Binary, etc...

Good point. We can skip it unless the user has specified a custom encoder.

It's called before saving a value to the database. Normally it's just json.dumps() on databases that don't have a json data type.

Co-authored-by: Tim Graham <[email protected]>

WaVEV marked this pull request as draft June 9, 2024 22:34

WaVEV force-pushed the supports-json-field branch from a661dac to f0f46f4 Compare June 9, 2024 22:46

timgraham changed the title ~~Supports json field~~ add support for JSONField Jun 10, 2024

timgraham reviewed Jun 11, 2024

View reviewed changes

WaVEV force-pushed the supports-json-field branch 2 times, most recently from 9f29d68 to e8d257a Compare June 15, 2024 19:12

WaVEV force-pushed the supports-json-field branch from 617dc03 to 13f58ab Compare June 19, 2024 14:36

WaVEV marked this pull request as ready for review June 19, 2024 14:42

timgraham mentioned this pull request Jun 20, 2024

add support for ExpressionWrapper #55

Merged

timgraham reviewed Jun 20, 2024

View reviewed changes

WaVEV force-pushed the supports-json-field branch from 64e25e3 to 1395430 Compare June 23, 2024 19:53

timgraham mentioned this pull request Jun 24, 2024

add support for Transforms in QuerySet.values()/values_list() #59

Merged

WaVEV requested a review from timgraham June 24, 2024 17:06

timgraham reviewed Jun 24, 2024

View reviewed changes

WaVEV force-pushed the supports-json-field branch 2 times, most recently from 30a3188 to c475a78 Compare June 25, 2024 18:30

timgraham reviewed Jun 27, 2024

View reviewed changes

timgraham force-pushed the supports-json-field branch from 7aa1ec0 to e509877 Compare June 28, 2024 12:53

timgraham approved these changes Jun 28, 2024

View reviewed changes

Jibola approved these changes Jun 28, 2024

View reviewed changes

mongodb deleted a comment from Jibola Jun 28, 2024

ShaneHarvey reviewed Jun 28, 2024

View reviewed changes

ShaneHarvey approved these changes Jun 29, 2024

View reviewed changes

add support for JSONField

9019a33

Co-authored-by: Tim Graham <[email protected]>

timgraham force-pushed the supports-json-field branch from 226e9f2 to 9019a33 Compare July 1, 2024 13:48

timgraham merged commit 9019a33 into mongodb:main Jul 1, 2024
3 checks passed

WaVEV deleted the supports-json-field branch August 26, 2024 15:44

add support for JSONField #47

add support for JSONField #47

Uh oh!

Conversation

WaVEV commented Jun 9, 2024 • edited by timgraham Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timgraham left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

timgraham left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timgraham commented Jun 26, 2024

Uh oh!

WaVEV commented Jun 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timgraham commented Jun 27, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timgraham Jun 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

WaVEV commented Jun 9, 2024 •

edited by timgraham

Loading

timgraham Jun 28, 2024 •

edited

Loading