-
Notifications
You must be signed in to change notification settings - Fork 30
INTPYTHON-827 Fix Model.save() updates and QuerySet.update() for $-prefixed strings #445
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -211,9 +211,9 @@ def when(self, compiler, connection): | |
|
|
||
| def value(self, compiler, connection, as_expr=False): # noqa: ARG001 | ||
| value = self.value | ||
| if isinstance(value, (list, int)) and as_expr: | ||
| # Wrap lists & numbers in $literal to prevent ambiguity when Value | ||
| # appears in $project. | ||
| if isinstance(value, (list, int, str, dict, tuple)) and as_expr: | ||
| # Wrap lists, numbers, strings, dict and tuple in $literal to avoid ambiguity when Value | ||
| # is used in queries' aggregate or update_many's $set. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Out of curiosity, you replaced $project with aggregate. Is there another place in aggregate besides $project where this could be an issue?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, in place like $group or $match (for having or filter). Do we need a test for that? |
||
| return {"$literal": value} | ||
| if isinstance(value, Decimal): | ||
| return Decimal128(value) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| from django.db import models | ||
|
|
||
|
|
||
| class Author(models.Model): | ||
| name = models.CharField(max_length=10) | ||
|
|
||
| def __str__(self): | ||
| return self.name | ||
|
|
||
|
|
||
| class Book(models.Model): | ||
| name = models.CharField(max_length=10) | ||
| data = models.JSONField(null=True) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| from operator import attrgetter | ||
|
|
||
| from django.db.models import Value | ||
| from django.test import TestCase | ||
|
|
||
| from django_mongodb_backend.test import MongoTestCaseMixin | ||
|
|
||
| from .models import Author, Book | ||
|
|
||
|
|
||
| class SaveDollarPrefixTests(MongoTestCaseMixin, TestCase): | ||
| def test_insert_dollar_prefix(self): | ||
| """$-prefixed values are correctly saved on insert.""" | ||
| obj = Author.objects.create(name="$foobar") | ||
| obj.refresh_from_db() | ||
| self.assertEqual(obj.name, "$foobar") | ||
|
|
||
|
|
||
| class UpdateDollarPrefixTests(MongoTestCaseMixin, TestCase): | ||
| def test_update_dollar_prefix(self): | ||
| """$-prefixed values are correctly saved on update.""" | ||
| obj = Author.objects.create(name="foobar") | ||
| obj.name = "$updated" | ||
| with self.assertNumQueries(1) as ctx: | ||
| obj.save() | ||
| obj.refresh_from_db() | ||
| self.assertEqual(obj.name, "$updated") | ||
| self.assertUpdateQuery( | ||
| ctx.captured_queries[0]["sql"], | ||
| "basic__author", | ||
| {"_id": obj.id}, | ||
| [{"$set": {"name": {"$literal": "$updated"}}}], | ||
| ) | ||
|
|
||
| def test_update_dollar_prefix_in_value_expression(self): | ||
| """$-prefixed Value() expressions are correctly handled on update.""" | ||
| obj = Author.objects.create(name="foobar") | ||
| obj.name = Value("$updated") | ||
| with self.assertNumQueries(1) as ctx: | ||
| obj.save() | ||
| obj.refresh_from_db() | ||
| self.assertEqual(obj.name, "$updated") | ||
| self.assertUpdateQuery( | ||
| ctx.captured_queries[0]["sql"], | ||
| "basic__author", | ||
| {"_id": obj.id}, | ||
| [{"$set": {"name": {"$literal": "$updated"}}}], | ||
| ) | ||
|
|
||
| def test_update_dict_value(self): | ||
| """MQL-like dict Value() expressions are correctly handled on update.""" | ||
| obj = Book.objects.create(name="foobar", data={}) | ||
| obj.data = Value({"$concat": ["$name", "-", "$name"]}) | ||
| obj.save() | ||
| obj.refresh_from_db() | ||
| self.assertEqual(obj.data, {"$concat": ["$name", "-", "$name"]}) | ||
|
|
||
| def test_update_dict(self): | ||
| """MQL-like dict updates are correctly handled on update.""" | ||
| obj = Book.objects.create(name="foobar") | ||
| obj.data = {"$concat": ["$name", "-", "$name"]} | ||
| obj.save() | ||
| obj.refresh_from_db() | ||
| self.assertEqual(obj.data, {"$concat": ["$name", "-", "$name"]}) | ||
|
|
||
| def test_update_tuple(self): | ||
| """MQL-like tuple updates are correctly handled on update.""" | ||
| obj = Book.objects.create(name="foobar") | ||
| obj.data = ("$name", "-", "$name") | ||
| obj.save() | ||
| obj.refresh_from_db() | ||
| self.assertEqual(obj.data, ["$name", "-", "$name"]) | ||
|
|
||
| def test_update_tuple_value(self): | ||
| """MQL-like tuple Value() expressions are correctly handled on update.""" | ||
| obj = Book.objects.create(name="foobar") | ||
| obj.data = Value(("$name", "-", "$name")) | ||
| obj.save() | ||
| obj.refresh_from_db() | ||
| self.assertEqual(obj.data, ["$name", "-", "$name"]) | ||
|
|
||
|
|
||
| class QueryDollarPrefixTests(MongoTestCaseMixin, TestCase): | ||
| def test_query_injection(self): | ||
| """$-prefixed Value() expressions are correctly handled on query.""" | ||
| Author.objects.create(name="Gustavo") | ||
| Author.objects.create(name="Walter") | ||
| with self.assertNumQueries(1) as ctx: | ||
| qs = list(Author.objects.annotate(a_value=Value("$name"))) | ||
| self.assertQuerySetEqual(qs, ["$name"] * 2, attrgetter("a_value")) | ||
| self.assertAggregateQuery( | ||
| ctx.captured_queries[0]["sql"], | ||
| "basic__author", | ||
| [{"$project": {"a_value": {"$literal": "$name"}, "_id": 1, "name": 1}}], | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still question whether the $literal escaping is overly broad. Better safe than sorry, sure, but it decreases readability a bit and makes queries larger. But I'm not sure how to compare the tradeoffs between some more complicated logic (
isinstance()CPU time) and leaving it as is. It seems if we did wrapping of only the types thatValue()wraps, it should be safe, unless the logic in Value() is deficient. And are there any string values besides those that start with $ that could be problematic? Perhaps to be discussed in chat tomorrow.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 I agree. Seeing $literal everywhere is annoying, but Value's resolver is probably covering the most common types used in queries. On the other hand, implementing the same escaping logic that Value uses is not a big deal.