diff --git a/AUTHORS b/AUTHORS index 17fae84ea..a5d228c39 100644 --- a/AUTHORS +++ b/AUTHORS @@ -266,3 +266,4 @@ that much better: * Terence Honles (https://github.com/terencehonles) * Sean Bermejo (https://github.com/seanbermejo) * Juan Gutierrez (https://github.com/juannyg) + * DSeaStar (https://github.com/DSeaStar) diff --git a/docs/changelog.rst b/docs/changelog.rst index 66336591f..960a0ff2c 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -9,7 +9,7 @@ Development - (Fill this out as you fix issues and develop your features). Changes in 1.0.0 -=========== +================ - Add support for transaction through run_in_transaction (kudos to juannyG for this) #2569 Some considerations: @@ -33,6 +33,7 @@ Changes in 1.0.0 - BREAKING CHANGE: The obsolete ``slaves`` and ``is_slave`` connection options were silently ignored since 2014 and will now raise ``ConnectionFailure`` if provided #2920. - BugFix - Calling .clear on a ListField wasn't being marked as changed (and flushed to db upon .save()) #2858 - Improve error message in case a document assigned to a ReferenceField wasn't saved yet #1955 +- Fix inc/dec atomic updates rejecting deltas outside a field's min_value/max_value #2339 - BugFix - Take `where()` into account when using `.modify()`, as in MyDocument.objects().where("this[field] >= this[otherfield]").modify(field='new') #2044 Changes in 0.29.3 diff --git a/docs/guide/querying.rst b/docs/guide/querying.rst index b9eb6c293..1d1f8c53e 100644 --- a/docs/guide/querying.rst +++ b/docs/guide/querying.rst @@ -596,6 +596,7 @@ There are several different "modifiers" that you may use with these methods: * ``min`` -- update only if value is smaller * ``inc`` -- increment a value by a given amount * ``dec`` -- decrement a value by a given amount +* ``mul`` -- multiply a value by a given amount * ``push`` -- append a value to a list * ``push_all`` -- append several values to a list * ``pop`` -- remove the first or last element of a list `depending on the value`_ @@ -604,6 +605,13 @@ There are several different "modifiers" that you may use with these methods: * ``add_to_set`` -- add value to a list only if its not in the list already * ``rename`` -- rename the key name +.. note:: + + The operands passed to ``inc``, ``dec``, and ``mul`` are deltas or + multipliers, not replacement field values. MongoEngine therefore does not + validate them against the field's ``min_value`` or ``max_value`` as this would require a lookup. These + atomic updates can leave the stored value outside those bounds. + .. _need to add upsert=True: http://docs.mongodb.org/manual/reference/operator/update/setOnInsert .. _depending on the value: http://docs.mongodb.org/manual/reference/operator/update/pop/ diff --git a/mongoengine/base/fields.py b/mongoengine/base/fields.py index 07962ecde..308530046 100644 --- a/mongoengine/base/fields.py +++ b/mongoengine/base/fields.py @@ -236,7 +236,9 @@ def _to_mongo_safe_call(self, value, use_db_field=True, fields=None): def prepare_query_value(self, op, value): """Prepare a value that is being used in a query for PyMongo.""" - if op in UPDATE_OPERATORS: + # Do not validate $inc/$mul operands against stored-value min/max bounds. + # dec is normalized to inc with a negative value before this point. + if op in UPDATE_OPERATORS and op not in ("inc", "mul"): self.validate(value) return value diff --git a/mongoengine/queryset/transform.py b/mongoengine/queryset/transform.py index 701ca649b..c0e58eb45 100644 --- a/mongoengine/queryset/transform.py +++ b/mongoengine/queryset/transform.py @@ -365,7 +365,8 @@ def update(_doc_cls=None, **update): value = field.prepare_query_value(op, value) elif op == "unset": value = 1 - elif op == "inc": + # dec is normalized to inc above. + elif op in ("inc", "mul"): value = field.prepare_query_value(op, value) if match: diff --git a/tests/queryset/test_queryset.py b/tests/queryset/test_queryset.py index d64384670..fae2c0ff8 100644 --- a/tests/queryset/test_queryset.py +++ b/tests/queryset/test_queryset.py @@ -2148,6 +2148,55 @@ class BlogPost(Document): post.reload() assert post.hits == 11 + def test_update_number_operators__operand_outside_bounds__bypasses_validation(self): + """inc, dec, and mul operands are not stored values and bypass bounds.""" + + class Counter(Document): + number = IntField(min_value=1, max_value=5) + + Counter.drop_collection() + counter = Counter(number=1).save() + + Counter.objects.update(inc__number=5) + counter.reload() + assert counter.number == 6 + + Counter.objects.update(dec__number=6) + counter.reload() + assert counter.number == 0 + + counter.number = 2 + counter.save() + + Counter.objects.update(mul__number=10) + counter.reload() + assert counter.number == 20 + + def test_update_inc__numeric_string__converts_to_number(self): + """inc converts a numeric string through IntField before updating.""" + + class Counter(Document): + number = IntField() + + Counter.drop_collection() + counter = Counter(number=1).save() + + Counter.objects.update(inc__number="5") + counter.reload() + assert counter.number == 6 + + def test_update_inc__string_field__relies_on_server_validation(self): + """MongoDB rejects inc after MongoEngine skips client-side validation.""" + + class Person(Document): + name = StringField() + + Person.drop_collection() + Person(name="Alice").save() + + with pytest.raises(OperationError): + Person.objects.update(inc__name=1) + def test_update_decimalfield_operator(self): class BlogPost(Document): review = DecimalField() diff --git a/tests/queryset/test_transform.py b/tests/queryset/test_transform.py index 8cb8ad426..db4ad8fc2 100644 --- a/tests/queryset/test_transform.py +++ b/tests/queryset/test_transform.py @@ -1,6 +1,7 @@ import unittest import pytest +from bson.decimal128 import Decimal128 from bson.son import SON from mongoengine import * @@ -83,6 +84,48 @@ class BlogPost(Document): update = transform.update(BlogPost, push_all__tags=["mongo", "db"]) assert update == {"$push": {"tags": {"$each": ["mongo", "db"]}}} + def test_transform_update_inc_dec_ignores_min_max(self): + """inc/dec pass a delta; min_value/max_value apply to stored values (#2339).""" + + class Account(Document): + amount = FloatField(min_value=0, required=True) + count = IntField(min_value=0, max_value=100) + money = DecimalField(min_value=0) + money128 = Decimal128Field(min_value=0) + + update = transform.update(Account, dec__amount=10) + assert update == {"$inc": {"amount": -10.0}} + + update = transform.update(Account, inc__count=200) + assert update == {"$inc": {"count": 200}} + + update = transform.update(Account, mul__count="200") + assert update == {"$mul": {"count": 200}} + + update = transform.update(Account, dec__count=5) + assert update == {"$inc": {"count": -5}} + + update = transform.update(Account, dec__money=3) + assert update == {"$inc": {"money": -3.0}} + + update = transform.update(Account, dec__money128=3) + assert update == {"$inc": {"money128": Decimal128("-3")}} + + with pytest.raises(ValidationError): + transform.update(Account, set__amount=-1) + + with pytest.raises(ValidationError): + transform.update(Account, set__count=101) + + def test_transform_update__inc_on_string_field__skips_validation(self): + """inc leaves nonnumeric field validation to MongoDB.""" + + class Person(Document): + name = StringField() + + update = transform.update(Person, inc__name=1) + assert update == {"$inc": {"name": 1}} + def test_transform_update_no_operator_default_to_set(self): """Ensure the differences in behvaior between 'push' and 'push_all'"""