Repository: pydicom/pynetdicom

Issue:
QR Move SCP associates with Store SCP even when no suboperations are required
**Expected behavior**
When no suboperations are to be performed the SCP shouldn't associate with the Storage SCP

**Steps To Reproduce**
Yield 0 subops in `EVT_C_MOVE` handler

Hmm, but should it return `0xA701` if the destination is unknown or `0x0000`?

Discussion:

Patch:
diff --git a/docs/changelog/v1.5.0.rst b/docs/changelog/v1.5.0.rst
index 07e944b4a..6d9e1f47c 100644
--- a/docs/changelog/v1.5.0.rst
+++ b/docs/changelog/v1.5.0.rst
@@ -16,14 +16,17 @@ Fixes
 * Changed ``ImplantationPlanSRDocumentStorage`` to
   :attr:`~pynetdicom.sop_class.ImplantationPlanSRStorage` to match the name
   from Part 6 of the DICOM Standard rather than Part 4.
+* Stopped the QR Move SCP from associating with the move destination when no
+  suboperations were required (:issue:`436`)
+
 
 Enhancements
 ............
 
 * Added configuration option :attr:`_config.USE_SHORT_DIMSE_AET
   <pynetdicom._config.USE_SHORT_DIMSE_AET>` so elements with a VR of **AE** in
-  DIMSE messages are only padded to the shortest even length rather than to
-  the maximum 16 characters. (:issue:`403`)
+  DIMSE messages are no longer padded to the maximum 16 characters.
+  (:issue:`403`)
 * When acting as the association Acceptor check that the user hasn't sent
   an A-ABORT or A-ASSOCIATE-RJ before attempting association negotiation
   (:issue:`408`)
@@ -39,8 +42,8 @@ Enhancements
   contains all of the Storage Service Class' SOP Class UIDs rather than just
   the first 128 as in
   :attr:`~pynetdicom.presentation.StoragePresentationContexts`.
-* Overhaul of ``echoscu.py``, ``storescu.py`` and added support for
-  ``findscu.py``, ``getscu.py`` and ``movescu.py`` apps.
+* Overhaul of ``echoscu.py``, ``storescu.py`` and added non-toy implementations
+  for ``findscu.py``, ``getscu.py`` and ``movescu.py`` apps.
 
 Changes
 .......
@@ -60,3 +63,9 @@ Changes
   <pynetdicom.acse.ACSE.is_aborted>`
 * Support for Python 2 is deprecated and will be removed starting in version
   2.0.
+
+
+Known Issues
+............
+
+* TLS v1.3 (with OpenSSL 1.1.1 or higher) is not currently supported.
diff --git a/pynetdicom/service_class.py b/pynetdicom/service_class.py
index 22566f4b7..b52ff2985 100644
--- a/pynetdicom/service_class.py
+++ b/pynetdicom/service_class.py
@@ -2000,6 +2000,15 @@ class QueryRetrieveServiceClass(ServiceClass):
             self.dimse.send_msg(rsp, context.context_id)
             return
 
+        if no_suboperations < 1:
+            rsp.Status = 0x0000
+            rsp.NumberOfRemainingSuboperations = 0
+            rsp.NumberOfFailedSuboperations = 0
+            rsp.NumberOfWarningSuboperations = 0
+            rsp.NumberOfCompletedSuboperations = 0
+            self.dimse.send_msg(rsp, context.context_id)
+            return
+
         # Request new association with Move Destination
         try:
             # Unknown Move Destination
@@ -2011,9 +2020,9 @@ class QueryRetrieveServiceClass(ServiceClass):
                 self.dimse.send_msg(rsp, context.context_id)
                 return
 
-            store_assoc = self.ae.associate(destination[0],
-                                            destination[1],
-                                            ae_title=req.MoveDestination)
+            store_assoc = self.ae.associate(
+                destination[0], destination[1], ae_title=req.MoveDestination
+            )
         except Exception as exc:
             LOGGER.error(
                 "The handler bound to 'evt.EVT_C_MOVE' yielded an invalid "
diff --git a/pynetdicom/utils.py b/pynetdicom/utils.py
index 89a240d63..a86087ccb 100644
--- a/pynetdicom/utils.py
+++ b/pynetdicom/utils.py
@@ -87,7 +87,7 @@ def validate_ae_title(ae_title, use_short=False):
 
     If the supplied `ae_title` is less than 16 characters once non-significant
     spaces have been removed, the spare trailing characters will be set to
-    space (``0x20``).
+    space (``0x20``) provided `use_short` is ``False``.
 
     .. versionchanged:: 1.1
 
@@ -103,8 +103,8 @@ def validate_ae_title(ae_title, use_short=False):
         The AE title to check.
     use_short : bool, optional
         If ``False`` (default) then pad AE titles with trailing spaces up to
-        the maximum allowable length (16 bytes), otherwise only pad odd length
-        AE titles with a single trailing space to make it even length.
+        the maximum allowable length (16 bytes), otherwise no padding will
+        be added.
 
     Returns
     -------
@@ -146,9 +146,6 @@ def validate_ae_title(ae_title, use_short=False):
     if not use_short:
         # Pad out to 16 characters using spaces
         ae_title = ae_title.ljust(16)
-    elif len(ae_title) % 2:
-        # Pad to even length
-        ae_title += ' '
 
     # Unicode category: 'Cc' is control characters
     invalid = [


Test patch:
diff --git a/pynetdicom/apps/tests/test_movescu.py b/pynetdicom/apps/tests/test_movescu.py
index 798702d94..31c61817a 100644
--- a/pynetdicom/apps/tests/test_movescu.py
+++ b/pynetdicom/apps/tests/test_movescu.py
@@ -299,9 +299,6 @@ class TestMoveSCU(object):
     def test_flag_aet(self):
         """Test --calling-aet flag."""
         events = []
-        def handle_store(event):
-            return 0x0000
-
         def handle_move(event):
             events.append(event)
             yield 'localhost', 11113
@@ -320,16 +317,11 @@ class TestMoveSCU(object):
         ae.supported_contexts = QueryRetrievePresentationContexts
         scp = ae.start_server(('', 11112), block=False, evt_handlers=handlers)
         ae.supported_contexts = StoragePresentationContexts
-        store_scp = ae.start_server(
-            ('', 11113), block=False,
-            evt_handlers=[(evt.EVT_C_STORE, handle_store)]
-        )
 
         p = start_movescu(['-aet', 'MYSCU', '-k', 'PatientName='])
         p.wait()
         assert p.returncode == 0
 
-        store_scp.shutdown()
         scp.shutdown()
 
         assert events[0].event == evt.EVT_C_MOVE
@@ -339,10 +331,6 @@ class TestMoveSCU(object):
     def test_flag_aec(self):
         """Test --called-aet flag."""
         events = []
-
-        def handle_store(event):
-            return 0x0000
-
         def handle_move(event):
             events.append(event)
             yield 'localhost', 11113
@@ -360,17 +348,11 @@ class TestMoveSCU(object):
         ae.requested_contexts = StoragePresentationContexts
         ae.supported_contexts = QueryRetrievePresentationContexts
         scp = ae.start_server(('', 11112), block=False, evt_handlers=handlers)
-        ae.supported_contexts = StoragePresentationContexts
-        store_scp = ae.start_server(
-            ('', 11113), block=False,
-            evt_handlers=[(evt.EVT_C_STORE, handle_store)]
-        )
 
         p = start_movescu(['-aec', 'YOURSCP', '-k', 'PatientName='])
         p.wait()
         assert p.returncode == 0
 
-        store_scp.shutdown()
         scp.shutdown()
 
         assert events[0].event == evt.EVT_C_MOVE
@@ -380,10 +362,6 @@ class TestMoveSCU(object):
     def test_flag_aem(self):
         """Test --called-aem flag."""
         events = []
-
-        def handle_store(event):
-            return 0x0000
-
         def handle_move(event):
             events.append(event)
             yield 'localhost', 11113
@@ -402,16 +380,11 @@ class TestMoveSCU(object):
         ae.supported_contexts = QueryRetrievePresentationContexts
         scp = ae.start_server(('', 11112), block=False, evt_handlers=handlers)
         ae.supported_contexts = StoragePresentationContexts
-        store_scp = ae.start_server(
-            ('', 11113), block=False,
-            evt_handlers=[(evt.EVT_C_STORE, handle_store)]
-        )
 
         p = start_movescu(['-aem', 'SOMESCP', '-k', 'PatientName='])
         p.wait()
         assert p.returncode == 0
 
-        store_scp.shutdown()
         scp.shutdown()
 
         assert events[0].event == evt.EVT_C_MOVE
@@ -420,9 +393,6 @@ class TestMoveSCU(object):
     def test_flag_ta(self, capfd):
         """Test --acse-timeout flag."""
         events = []
-        def handle_store(event):
-            return 0x0000
-
         def handle_requested(event):
             events.append(event)
             time.sleep(0.1)
@@ -449,18 +419,12 @@ class TestMoveSCU(object):
         ae.requested_contexts = StoragePresentationContexts
         ae.supported_contexts = QueryRetrievePresentationContexts
         scp = ae.start_server(('', 11112), block=False, evt_handlers=handlers)
-        ae.supported_contexts = StoragePresentationContexts
-        store_scp = ae.start_server(
-            ('', 11113), block=False,
-            evt_handlers=[(evt.EVT_C_STORE, handle_store)]
-        )
 
         p = start_movescu(['-ta', '0.05', '-d', '-k', 'PatientName='])
         p.wait()
         assert p.returncode == 1
 
         time.sleep(0.1)
-        store_scp.shutdown()
         scp.shutdown()
 
         out, err = capfd.readouterr()
@@ -471,9 +435,6 @@ class TestMoveSCU(object):
     def test_flag_td(self, capfd):
         """Test --dimse-timeout flag."""
         events = []
-        def handle_store(event):
-            return 0x0000
-
         def handle_move(event):
             events.append(event)
             time.sleep(0.1)
@@ -496,18 +457,12 @@ class TestMoveSCU(object):
         ae.requested_contexts = StoragePresentationContexts
         ae.supported_contexts = QueryRetrievePresentationContexts
         scp = ae.start_server(('', 11112), block=False, evt_handlers=handlers)
-        ae.supported_contexts = StoragePresentationContexts
-        store_scp = ae.start_server(
-            ('', 11113), block=False,
-            evt_handlers=[(evt.EVT_C_STORE, handle_store)]
-        )
 
         p = start_movescu(['-td', '0.05', '-d', '-k', 'PatientName='])
         p.wait()
         assert p.returncode == 0
 
         time.sleep(0.1)
-        store_scp.shutdown()
         scp.shutdown()
 
         out, err = capfd.readouterr()
@@ -523,9 +478,6 @@ class TestMoveSCU(object):
     def test_flag_max_pdu(self):
         """Test --max-pdu flag."""
         events = []
-        def handle_store(event):
-            return 0x0000
-
         def handle_move(event):
             events.append(event)
             yield 'localhost', 11113
@@ -547,17 +499,11 @@ class TestMoveSCU(object):
         ae.requested_contexts = StoragePresentationContexts
         ae.supported_contexts = QueryRetrievePresentationContexts
         scp = ae.start_server(('', 11112), block=False, evt_handlers=handlers)
-        ae.supported_contexts = StoragePresentationContexts
-        store_scp = ae.start_server(
-            ('', 11113), block=False,
-            evt_handlers=[(evt.EVT_C_STORE, handle_store)]
-        )
 
         p = start_movescu(['--max-pdu', '123456', '-k', 'PatientName='])
         p.wait()
         assert p.returncode == 0
 
-        store_scp.shutdown()
         scp.shutdown()
 
         assert events[0].event == evt.EVT_C_MOVE
@@ -568,9 +514,6 @@ class TestMoveSCU(object):
     def test_flag_patient(self):
         """Test the -P flag."""
         events = []
-        def handle_store(event):
-            return 0x0000
-
         def handle_move(event):
             events.append(event)
             yield 'localhost', 11113
@@ -588,17 +531,11 @@ class TestMoveSCU(object):
         ae.requested_contexts = StoragePresentationContexts
         ae.supported_contexts = QueryRetrievePresentationContexts
         scp = ae.start_server(('', 11112), block=False, evt_handlers=handlers)
-        ae.supported_contexts = StoragePresentationContexts
-        store_scp = ae.start_server(
-            ('', 11113), block=False,
-            evt_handlers=[(evt.EVT_C_STORE, handle_store)]
-        )
 
         p = start_movescu(['-P', '-k', 'PatientName='])
         p.wait()
         assert p.returncode == 0
 
-        store_scp.shutdown()
         scp.shutdown()
 
         assert events[0].event == evt.EVT_C_MOVE
@@ -610,9 +547,6 @@ class TestMoveSCU(object):
     def test_flag_study(self):
         """Test the -S flag."""
         events = []
-        def handle_store(event):
-            return 0x0000
-
         def handle_move(event):
             events.append(event)
             yield 'localhost', 11113
@@ -630,17 +564,11 @@ class TestMoveSCU(object):
         ae.requested_contexts = StoragePresentationContexts
         ae.supported_contexts = QueryRetrievePresentationContexts
         scp = ae.start_server(('', 11112), block=False, evt_handlers=handlers)
-        ae.supported_contexts = StoragePresentationContexts
-        store_scp = ae.start_server(
-            ('', 11113), block=False,
-            evt_handlers=[(evt.EVT_C_STORE, handle_store)]
-        )
 
         p = start_movescu(['-S', '-k', 'PatientName='])
         p.wait()
         assert p.returncode == 0
 
-        store_scp.shutdown()
         scp.shutdown()
 
         assert events[0].event == evt.EVT_C_MOVE
@@ -650,9 +578,6 @@ class TestMoveSCU(object):
     def test_flag_patient_study(self):
         """Test the -O flag."""
         events = []
-        def handle_store(event):
-            return 0x0000
-
         def handle_move(event):
             events.append(event)
             yield 'localhost', 11113
@@ -670,17 +595,11 @@ class TestMoveSCU(object):
         ae.requested_contexts = StoragePresentationContexts
         ae.supported_contexts = QueryRetrievePresentationContexts
         scp = ae.start_server(('', 11112), block=False, evt_handlers=handlers)
-        ae.supported_contexts = StoragePresentationContexts
-        store_scp = ae.start_server(
-            ('', 11113), block=False,
-            evt_handlers=[(evt.EVT_C_STORE, handle_store)]
-        )
 
         p = start_movescu(['-O', '-k', 'PatientName='])
         p.wait()
         assert p.returncode == 0
 
-        store_scp.shutdown()
         scp.shutdown()
 
         assert events[0].event == evt.EVT_C_MOVE
diff --git a/pynetdicom/tests/test_assoc.py b/pynetdicom/tests/test_assoc.py
index 370a8ce56..352d00b76 100644
--- a/pynetdicom/tests/test_assoc.py
+++ b/pynetdicom/tests/test_assoc.py
@@ -3058,10 +3058,11 @@ class TestAssociationSendCMove(object):
 
     def test_move_destination_no_assoc(self):
         """Test move destination failed to assoc"""
+        # Move SCP
         def handle_move(event):
             yield 'localhost', 11113
-            yield 0
-            yield 0x0000, None
+            yield 2
+            yield 0xFF00, self.good
 
         self.ae = ae = AE()
         ae.acse_timeout = 5
@@ -3088,8 +3089,8 @@ class TestAssociationSendCMove(object):
         """Test unknown move destination"""
         def handle_move(event):
             yield None, None
-            yield 0
-            yield 0x0000, None
+            yield 1
+            yield 0xFF00, self.good
 
         self.ae = ae = AE()
         ae.acse_timeout = 5
diff --git a/pynetdicom/tests/test_dimse_c.py b/pynetdicom/tests/test_dimse_c.py
index d3adeef3c..1c8152edb 100644
--- a/pynetdicom/tests/test_dimse_c.py
+++ b/pynetdicom/tests/test_dimse_c.py
@@ -340,11 +340,11 @@ class TestPrimitive_C_STORE(object):
 
         primitive.MoveOriginatorApplicationEntityTitle = b'A'
         aet = primitive.MoveOriginatorApplicationEntityTitle
-        assert b'A ' == aet
+        assert b'A' == aet
 
         primitive.MoveOriginatorApplicationEntityTitle = b'ABCDEFGHIJKLMNO'
         aet = primitive.MoveOriginatorApplicationEntityTitle
-        assert b'ABCDEFGHIJKLMNO ' == aet
+        assert b'ABCDEFGHIJKLMNO' == aet
 
         primitive.MoveOriginatorApplicationEntityTitle = b'ABCDEFGHIJKLMNOP'
         aet = primitive.MoveOriginatorApplicationEntityTitle
@@ -1100,10 +1100,10 @@ class TestPrimitive_C_MOVE(object):
 
         primitive.MoveDestination = b'A'
         aet = primitive.MoveDestination
-        assert b'A ' == primitive.MoveDestination
+        assert b'A' == primitive.MoveDestination
 
         primitive.MoveDestination = b'ABCDEFGHIJKLMNO'
-        assert b'ABCDEFGHIJKLMNO ' == primitive.MoveDestination
+        assert b'ABCDEFGHIJKLMNO' == primitive.MoveDestination
 
         primitive.MoveDestination = b'ABCDEFGHIJKLMNOP'
         assert b'ABCDEFGHIJKLMNOP' == primitive.MoveDestination
diff --git a/pynetdicom/tests/test_service_qr.py b/pynetdicom/tests/test_service_qr.py
index 67da4e8c6..2d2a616e9 100644
--- a/pynetdicom/tests/test_service_qr.py
+++ b/pynetdicom/tests/test_service_qr.py
@@ -4263,7 +4263,7 @@ class TestQRMoveServiceClass(object):
         scp.shutdown()
 
     def test_move_success(self):
-        """Test when handler returns failure status"""
+        """Test when handler returns success status"""
         def handle(event):
             yield self.destination
             yield 1
@@ -4285,7 +4285,10 @@ class TestQRMoveServiceClass(object):
         ae.dimse_timeout = 5
         assoc = ae.associate('localhost', 11112)
         assert assoc.is_established
-        result = assoc.send_c_move(self.query, b'TESTMOVE', PatientRootQueryRetrieveInformationModelMove)
+        result = assoc.send_c_move(
+            self.query, b'TESTMOVE',
+            PatientRootQueryRetrieveInformationModelMove
+        )
         status, identifier = next(result)
         assert status.Status == 0xFF00
         assert identifier is None
@@ -5153,6 +5156,58 @@ class TestQRMoveServiceClass(object):
         assert assoc.is_aborted
         scp.shutdown()
 
+    def test_no_subops_no_association(self):
+        """Test that the Move SCP doesn't try and associate if not needed."""
+        events = []
+        def handle(event):
+            yield ('localhost', 11113)
+            yield 0
+            yield 0x0000, None
+
+        def handle_store(event):
+            return 0x0000
+
+        def handle_assoc(event):
+            events.append(event)
+
+        handlers = [
+            (evt.EVT_REQUESTED, handle_assoc),
+            (evt.EVT_C_STORE, handle_store),
+        ]
+
+        self.ae = ae = AE()
+        ae.add_supported_context(PatientRootQueryRetrieveInformationModelMove)
+        ae.add_supported_context(CTImageStorage)
+        store_scp = ae.start_server(
+            ('', 11113), block=False, evt_handlers=handlers
+        )
+        ae.add_requested_context(PatientRootQueryRetrieveInformationModelMove)
+        ae.add_requested_context(CTImageStorage)
+        scp = ae.start_server(
+            ('', 11112), block=False, evt_handlers=[(evt.EVT_C_MOVE, handle)]
+        )
+
+        ae.acse_timeout = 5
+        ae.dimse_timeout = 5
+        assoc = ae.associate('localhost', 11112)
+        assert assoc.is_established
+        result = assoc.send_c_move(
+            self.query, b'TESTMOVE',
+            PatientRootQueryRetrieveInformationModelMove
+        )
+        status, identifier = next(result)
+        assert status.Status == 0x0000
+        assert identifier is None
+        assert status.NumberOfFailedSuboperations == 0
+        assert status.NumberOfWarningSuboperations == 0
+        assert status.NumberOfCompletedSuboperations == 0
+        pytest.raises(StopIteration, next, result)
+
+        assoc.release()
+        scp.shutdown()
+
+        assert len(events) == 0
+
 
 class TestQRCompositeInstanceWithoutBulk(object):
     """Tests for QR + Composite Instance Without Bulk Data"""
diff --git a/pynetdicom/tests/test_transport.py b/pynetdicom/tests/test_transport.py
index 7e55562be..e2f0cae3c 100644
--- a/pynetdicom/tests/test_transport.py
+++ b/pynetdicom/tests/test_transport.py
@@ -284,7 +284,7 @@ class TestTLS(object):
         assert len(server.active_associations) == 0
 
     def test_tls_yes_server_not_client(self, server_context):
-        """Test wrapping the requestor socket with TLS (and server)."""
+        """Test wrapping the acceptor socket with TLS (and not client)."""
         self.ae = ae = AE()
         ae.add_supported_context('1.2.840.10008.1.1')
         server = ae.start_server(
@@ -302,7 +302,7 @@ class TestTLS(object):
         assert len(server.active_associations) == 0
 
     def test_tls_yes_server_yes_client(self, server_context, client_context):
-        """Test associating with no TLS on either end."""
+        """Test associating with TLS on both ends."""
         self.ae = ae = AE()
         ae.acse_timeout = 5
         ae.dimse_timeout = 5