Skip to content

INTPYTHON-749: Fix bug in null matching on queryoptimizer #401

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.

Sign up for GitHub

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

Closed
wants to merge 10 commits into from
Closed

INTPYTHON-749: Fix bug in null matching on queryoptimizer #401

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6f2052a
Fix bug in null matching on queryoptimizer
Jibola Sep 11, 2025
139d0b4
include none tests for objects.filter
Jibola Sep 11, 2025
fafe479
migrated tests to lookup_ and added data
Jibola Sep 12, 2025
50177bc
call filter once in each test
Jibola Sep 12, 2025
e8011b9
include missing field specifier
Jibola Sep 12, 2025
0ca2a5e
fix field resolution and value lookup
Jibola Sep 12, 2025
4af823a
refactored the tests to remove generics
Jibola Sep 15, 2025
0161dfb
remove the CONSTANT dictionary
Jibola Sep 15, 2025
0b29263
update tests to support ordering and have a default for self.title
Jibola Sep 15, 2025
7802896
added a string safety-check + allowed unordered lists to be compared
Jibola Sep 15, 2025
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions django_mongodb_backend/query_conversion/expression_converters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,12 @@ def convert(cls, args):
):
field_name = field_expr[1:] # Remove the $ prefix.
if cls.operator == "$eq":
return {field_name: value}
return {field_name: {cls.operator: value}}
query = {field_name: value}
else:
query = {field_name: {cls.operator: value}}
if value is None:
query = {"$and": [{field_name: {"$exists": True}}, query]}
return query
return None


Expand Down Expand Up @@ -102,7 +106,12 @@ def convert(cls, in_args):
if isinstance(values, (list, tuple, set)) and all(
cls.is_simple_value(v) for v in values
):
return {field_name: {"$in": values}}
core_check = {field_name: {"$in": values}}
return (
{"$and": [{field_name: {"$exists": True}}, core_check]}
if None in values
else core_check
)
return None


Expand Down
15 changes: 15 additions & 0 deletions tests/expression_converter_/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
from django.db import models


class NullableJSONModel(models.Model):
value = models.JSONField(blank=True, null=True)

class Meta:
required_db_features = {"supports_json_field"}


class Tag(models.Model):
name = models.CharField(max_length=10)

def __str__(self):
return self.name
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please try to use existing test apps and existing tests. The expression_converter_ app will be removed when query generation is refactored. In the case of JSONField, since Django's tests/model_fields/test_jsonfield.py doesn't seem to catch the issue, we can add a file of the same name to this project's tests/model_fields_.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

My pushback here is that it they're placed here since we're testing a direct consequence of the QueryOptimizer and a bugfix to the QueryOptimizer; the original code has no bug. From my perspective the query refactor is larger lift and it makes more sense there to then fold in models -- especially if this will be temporary.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The tests are organized by behavior, not by what part of the code caused the problem. Presumably we want any future query generation algorithm to generate the same query, or at least a query that behaves the same. From my perspective, there is no advantage to regression tests like this in expression_converter_, as it creates more work down the line to move them elsewhere. Looking at this again, tests/lookup_ might be more appropriate than model_fields_. InConverter is also modified but I don't see a regression test for that.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I'll also include a test for $in as well since it doesn't behave the same way and also move them to lookup_.
34 changes: 34 additions & 0 deletions tests/expression_converter_/test_filter_conversion.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
from django.test import TestCase

from django_mongodb_backend.test import MongoTestCaseMixin

from .models import NullableJSONModel, Tag


class MQLTests(MongoTestCaseMixin, TestCase):
def test_none_filter_nullable_json(self):
with self.assertNumQueries(1) as ctx:
list(NullableJSONModel.objects.filter(value=None))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please also add the test data and assert the query results. It's unclear to me why existing tests don't catch the problem unless, in the case of Tag, this is about polymorphic data where the "name" field doesn't exist in the data.

As for JSONField, perhaps it's separate from this regression (if there is indeed one), but there are limitations about null/nonexistent queries.
Copy link
Contributor Author

@Jibola Jibola Sep 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Sure I can add data.

I'll also say JSONField isn't separate and was the main way I found the issue. (I'll add the details to the PR description), but it's specifically that the un-optimized JSONFIeld call would generate this:

{ $match: { $expr: { $eq: [ '$field', null] } } }

This explicitly checks for the presence of a field + the field being null.

The optimization made it:

{ $match: { field: null } }

Which checks for null OR the field as undefined

So it's the same regression simply because we've broken the underlying expectation same as before.

Honestly, in cases outside of JSONField the data integrity remains the same because we still enforce the $exists: false check redundantly.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the case of Tag, we cant add polymorphic tests yet because testing Polymorphic data won't yield any results since polymorphic data requires querying an embedded field. That triggers a $getField which doesn't get converted in this PR. It gets converted in #392 . So the test will need to wait til that change.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Caveat: To add data that would render some worthwhile test results, required injecting data outside of the ORM -- since the only way to get to the current case (outside of $getField) stems from someone injecting information outside of the bounds of the ORM CRUD operations.
self.assertAggregateQuery(
ctx.captured_queries[0]["sql"],
"queries__nullablejsonmodel",
[{"$match": {"$and": [{"$exists": False}, {"value": None}]}}],
)

def test_none_filter(self):
with self.assertNumQueries(1) as ctx:
list(Tag.objects.filter(name=None))
self.assertAggregateQuery(
ctx.captured_queries[0]["sql"],
"queries__nullablejsonmodel",
[
{
"$match": {
"$or": [
{"$and": [{"name": {"$exists": True}}, {"name": None}]},
{"$expr": {"$eq": [{"$type": "$name"}, "missing"]}},
]
}
}
],
)
36 changes: 29 additions & 7 deletions tests/expression_converter_/test_op_expressions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,12 @@
from django_mongodb_backend.query_conversion.expression_converters import convert_expression


def _wrap_condition_if_null(_type, condition, path):
if _type is None:
return {"$and": [{path: {"$exists": True}}, condition]}
return condition


class ConversionTestCase(SimpleTestCase):
CONVERTIBLE_TYPES = {
"int": 42,
Expand Down Expand Up @@ -53,10 +59,14 @@ def test_no_conversion_dict_value(self):
self.assertNotOptimizable({"$eq": ["$status", {"$gt": 5}]})

def _test_conversion_valid_type(self, _type):
self.assertConversionEqual({"$eq": ["$age", _type]}, {"age": _type})
self.assertConversionEqual(
{"$eq": ["$age", _type]}, _wrap_condition_if_null(_type, {"age": _type}, "age")
)

def _test_conversion_valid_array_type(self, _type):
self.assertConversionEqual({"$eq": ["$age", _type]}, {"age": _type})
self.assertConversionEqual(
{"$eq": ["$age", _type]}, _wrap_condition_if_null(_type, {"age": _type}, "age")
)

def test_conversion_various_types(self):
self._test_conversion_various_types(self._test_conversion_valid_type)
Expand All @@ -78,7 +88,10 @@ def test_no_conversion_dict_value(self):
self.assertNotOptimizable({"$in": ["$status", [{"bad": "val"}]]})

def _test_conversion_valid_type(self, _type):
self.assertConversionEqual({"$in": ["$age", [_type]]}, {"age": {"$in": [_type]}})
self.assertConversionEqual(
{"$in": ["$age", [_type]]},
_wrap_condition_if_null(_type, {"age": {"$in": [_type]}}, "age"),
)

def test_conversion_various_types(self):
for _type, val in self.CONVERTIBLE_TYPES.items():
Expand Down Expand Up @@ -170,7 +183,10 @@ def test_no_conversion_dict_value(self):
self.assertNotOptimizable({"$gt": ["$price", {}]})

def _test_conversion_valid_type(self, _type):
self.assertConversionEqual({"$gt": ["$price", _type]}, {"price": {"$gt": _type}})
self.assertConversionEqual(
{"$gt": ["$price", _type]},
_wrap_condition_if_null(_type, {"price": {"$gt": _type}}, "price"),
)

def test_conversion_various_types(self):
self._test_conversion_various_types(self._test_conversion_valid_type)
Expand All @@ -193,7 +209,7 @@ def test_no_conversion_dict_value(self):
def _test_conversion_valid_type(self, _type):
expr = {"$gte": ["$price", _type]}
expected = {"price": {"$gte": _type}}
self.assertConversionEqual(expr, expected)
self.assertConversionEqual(expr, _wrap_condition_if_null(_type, expected, "price"))

def test_conversion_various_types(self):
self._test_conversion_various_types(self._test_conversion_valid_type)
Expand All @@ -210,7 +226,10 @@ def test_no_conversion_dict_value(self):
self.assertNotOptimizable({"$lt": ["$price", {}]})

def _test_conversion_valid_type(self, _type):
self.assertConversionEqual({"$lt": ["$price", _type]}, {"price": {"$lt": _type}})
self.assertConversionEqual(
{"$lt": ["$price", _type]},
_wrap_condition_if_null(_type, {"price": {"$lt": _type}}, "price"),
)

def test_conversion_various_types(self):
self._test_conversion_various_types(self._test_conversion_valid_type)
Expand All @@ -227,7 +246,10 @@ def test_no_conversion_dict_value(self):
self.assertNotOptimizable({"$lte": ["$price", {}]})

def _test_conversion_valid_type(self, _type):
self.assertConversionEqual({"$lte": ["$price", _type]}, {"price": {"$lte": _type}})
self.assertConversionEqual(
{"$lte": ["$price", _type]},
_wrap_condition_if_null(_type, {"price": {"$lte": _type}}, "price"),
)

def test_conversion_various_types(self):
self._test_conversion_various_types(self._test_conversion_valid_type)
Loading