PROTON-2883/PROTON-2884: Modified dispositions should default to failed = true

Author: astitcher

cpp/src/delivery.cpp

@@ -49,6 +49,6 @@ delivery::~delivery() = default;
 void delivery::accept() { settle_delivery(pn_object(), ACCEPTED); }
 void delivery::reject() { settle_delivery(pn_object(), REJECTED); }
 void delivery::release() { settle_delivery(pn_object(), RELEASED); }
-void delivery::modify() { settle_delivery(pn_object(), MODIFIED); }
+void delivery::modify() { pn_disposition_set_failed(pn_delivery_local(pn_object()), true); settle_delivery(pn_object(), MODIFIED); }
 
 }

python/proton/_delivery.py

@@ -425,7 +425,7 @@ def apply_to(self, local_disposition: LocalDisposition):
 
 class ModifiedDisposition(LocalDisposition):
 
-    def __init__(self, failed: bool = None, undeliverable: bool = None,
+    def __init__(self, failed: bool = True, undeliverable: bool = None,
                  annotations: Optional[dict['symbol', 'PythonAMQPData']] = None):
         self._failed = failed
         self._undeliverable = undeliverable

python/proton/_handlers.py

@@ -24,7 +24,7 @@
 import weakref
 
 from ._condition import Condition
-from ._delivery import Delivery
+from ._delivery import Delivery, ModifiedDisposition
 from ._endpoints import Endpoint
 from ._events import Event, _dispatch
 from ._exceptions import ProtonException
@@ -187,7 +187,7 @@ def reject(self, delivery: Delivery) -> None:
         """
         self.settle(delivery, Delivery.REJECTED)
 
-    def release(self, delivery: Delivery, delivered: bool = True) -> None:
+    def release(self, delivery: Delivery, delivered: bool = True, failed: bool = True, undeliverable: bool = False) -> None:
         """
         Releases a received message, making it available at the source
         for any (other) interested receiver. The ``delivered``
@@ -206,7 +206,8 @@ def release(self, delivery: Delivery, delivered: bool = True) -> None:
             delivery flag :const:`proton.Delivery.RELEASED`
         """
         if delivered:
-            self.settle(delivery, Delivery.MODIFIED)
+            delivery.local = ModifiedDisposition(failed=failed, undeliverable=undeliverable)
+            self.settle(delivery)
         else:
             self.settle(delivery, Delivery.RELEASED)
 

python/tests/proton_tests/engine.py

@@ -2380,6 +2380,25 @@ def check(self, dlv: Delivery):
         assert dlv.remote.annotations == self._annotations, (dlv.remote.annotations, self._annotations)
 
 
+class NewDefaultModifiedTester(DispositionTester):
+    def __init__(self):
+        self._failed = True
+        self._undeliverable = False
+        self._annotations = None
+        super().__init__(Disposition.MODIFIED)
+
+    def apply(self, dlv: Delivery):
+        dlv.local = ModifiedDisposition()
+        dlv.update()
+
+    def check(self, dlv: Delivery):
+        assert dlv.remote_state == self._type
+        assert dlv.remote.type == self._type
+        assert dlv.remote.failed == self._failed, (dlv.remote.failed, self._failed)
+        assert dlv.remote.undeliverable == self._undeliverable, (dlv.remote.undeliverable, self._undeliverable)
+        assert dlv.remote.annotations == self._annotations, (dlv.remote.annotations, self._annotations)
+
+
 class CustomTester(DispositionTester):
     def __init__(self, type, data):
         self._data = data
@@ -2505,6 +2524,9 @@ def testNewModified(self):
         self._testDisposition(NewModifiedTester(failed=True, undeliverable=True,
                                                 annotations={"key": "value"}))
 
+    def testNewDefaultModified(self):
+        self._testDisposition(NewDefaultModifiedTester())
+
     def testTransactional(self):
         self._testDisposition(TransactionalTester(id=b'1324xxx', outcome_type=Disposition.ACCEPTED))