👌[#586] Process PR review

src/openforms/prefill/contrib/suwinet/plugin.py

@@ -3,8 +3,6 @@
 from django.urls import reverse
 from django.utils.translation import gettext_lazy as _
 
-from glom import glom
-
 from openforms.authentication.constants import AuthAttribute
 from openforms.plugins.exceptions import InvalidPluginConfiguration
 from openforms.submissions.models import Submission
@@ -58,33 +56,24 @@ def get_prefill_values(
             return {}
 
         def get_value(attr) -> dict | None:
+            service_name, operation = attr.split(".")
+            service = getattr(client, service_name)
+            perform_soap_call = getattr(service, operation)
             try:
-                return glom(client, attr)(bsn)
+                return perform_soap_call(bsn)
             except Exception:
-                logger.exception("Suwinet raised exception")
+                logger.exception(
+                    "Suwinet operation '%s' on service '%s' failed.",
+                    operation,
+                    service_name,
+                    extra={"service": service_name, "operation": operation},
+                )
                 return None
 
         with client:
+            # these are independent requests and should be performed async
             return {attr: value for attr in attributes if (value := get_value(attr))}
 
-    def get_co_sign_values(
-        self, identifier: str, submission: Submission | None = None
-    ) -> tuple[dict[str, dict], str]:
-        """
-        Given an identifier, fetch the co-sign specific values.
-
-        The return value is a dict keyed by field name as specified in
-        ``self.co_sign_fields``.
-
-        :param identifier: the unique co-signer identifier used to look up the details
-          in the pre-fill backend.
-        :return: a key-value dictionary, where the key is the requested attribute and
-          the value is the prefill value to use for that attribute.
-        """
-        raise NotImplementedError(
-            "You must implement the 'get_co_sign_values' method."
-        )  # pragma: nocover
-
     def check_config(self):
         try:
             client = get_client()
@@ -95,7 +84,7 @@ def check_config(self):
             )
         if not len(client):
             raise InvalidPluginConfiguration(
-                _("No services found. Check the binding addresses or provide a wsdl.")
+                _("No services found. Check the binding addresses.")
             )
 
     def get_config_actions(self):

src/openforms/prefill/contrib/suwinet/tests/test_plugin.py

@@ -20,11 +20,18 @@ class SuwinetPrefillTests(SuwinetTestCase):
     @classmethod
     def setUpTestData(cls):
         super().setUpTestData()
-        cls.config = SuwinetConfigFactory(all_endpoints=True)
+        cls.config = SuwinetConfigFactory.create(all_endpoints=True)
+
+    def setUp(self):
+        super().setUp()
+        solo_patcher = patch(
+            "suwinet.client.SuwinetConfig.get_solo", return_value=self.config
+        )
+        solo_patcher.start()
+        self.addCleanup(solo_patcher.stop)
 
     def test_unconfigured_service_gets_no_attributes(self):
         self.config.service = None
-        self.config.save()
 
         plugin = SuwinetPrefill(identifier="suwinet")
 
@@ -124,7 +131,7 @@ def test_get_attributes(self):
 
     def test_get_identifier_value(self):
         plugin = SuwinetPrefill(identifier="suwinet")
-        submission = SubmissionFactory(
+        submission = SubmissionFactory.create(
             auth_info__value="444444440",
         )
         bsn = plugin.get_identifier_value(submission, IdentifierRoles.main)
@@ -133,7 +140,7 @@ def test_get_identifier_value(self):
 
     def test_get_kadaster_values(self):
         plugin = SuwinetPrefill(identifier="suwinet")
-        submission = SubmissionFactory(auth_info__value="444444440")
+        submission = SubmissionFactory.create(auth_info__value="444444440")
 
         values = plugin.get_prefill_values(
             submission, ["KadasterDossierGSD.PersoonsInfo"]
@@ -145,26 +152,39 @@ def test_get_kadaster_values(self):
     def test_binding_to_variable(self):
         register = Registry()
         register("suwinet")(SuwinetPrefill)
-        form_var = FormVariableFactory(
+        form_var = FormVariableFactory.create(
             form__generate_minimal_setup=True,
             data_type=FormVariableDataTypes.object,
             prefill_plugin="suwinet",
             prefill_attribute="KadasterDossierGSD.PersoonsInfo",
         )
-        submission = SubmissionFactory(
+        submission = SubmissionFactory.create(
             form=form_var.form,
             auth_info__value="444444440",
         )
-        with patch("openforms.prefill.parallel") as mock:
-            # running in a thread deadlocks
-            mock.return_value.__enter__.return_value.map = lambda f, args: [f(*args)]
-            prefill_variables(submission, register)
+
+        prefill_variables(submission, register)
+        var = submission.submissionvaluevariable_set.get()
+
+        self.assertTrue(
+            var.value
+        )  # contents is already asserted in the Suwinet client tests
 
     @expectedFailure
     def test_collect_gateway_mock_failures(self):
         # As we speak we only know how to get mock data from the KadasterDossierGSD
-        # this test collects a VHS cassette with GW failures and empty endpoint responses
-        # the bsns are bsns, that have been reported to return mock data.
+        # this test collects a VHS cassette with (Den Haag) Layer 7 Gateway failures
+        # and empty endpoint responses. These bsns are bsns that have been reported
+        # to return mock data.
+        #
+        # So the cassette does not represent "correct" responses, but just gives
+        # insight into what operations we haven't seen working, either because
+        # we don't get through the gateway, or we don't know a bsn for the operation
+        # that returns mock data.
+        #
+        # this "try what could work and see what sticks" test should be replaced with
+        # a mapping like {"Service.Operation": "bsn"}, and just try one, known good mock
+        # operation and check it returns a dict.
 
         plugin = SuwinetPrefill(identifier="suwinet")
         # What can I get for 5 dollars?
@@ -176,7 +196,9 @@ def test_collect_gateway_mock_failures(self):
             "112233454",
             "999996769",
         ]
-        for submission in (SubmissionFactory(auth_info__value=bsn) for bsn in bsns):
+        for submission in (
+            SubmissionFactory.create(auth_info__value=bsn) for bsn in bsns
+        ):
             with self.subTest(f"Get ALL the things for {submission.auth_info}"):
                 values = plugin.get_prefill_values(submission, everything)
                 self.assertTrue(values)

src/openforms/submissions/models/submission_value_variable.py

@@ -25,9 +25,8 @@
 
 class ValueEncoder(DjangoJSONEncoder):
     def default(self, obj: JSONEncodable | JSONSerializable) -> JSONEncodable:
-        if hasattr(obj, "__json__"):
-            return obj.__json__()
-        return super().default(obj)
+        to_json = getattr(obj, "__json__", None)
+        return to_json() if callable(to_json) else super().default(obj)
 
 
 @dataclass

src/openforms/typing.py

@@ -1,17 +1,14 @@
-from typing import TYPE_CHECKING, Any, Dict, List, NewType, Protocol, Union
+import datetime
+import decimal
+import uuid
+from typing import Any, Dict, List, NewType, Protocol, Union
 
 from django.http import HttpRequest
 from django.http.response import HttpResponseBase
+from django.utils.functional import Promise
 
 from rest_framework.request import Request
 
-if TYPE_CHECKING:
-    import datetime
-    import decimal
-    import uuid
-
-    from django.utils.functional import Promise
-
 JSONPrimitive = Union[str, int, None, float, bool]
 
 JSONValue = Union[JSONPrimitive, "JSONObject", List["JSONValue"]]
@@ -33,18 +30,18 @@ def __call__(self, request: HttpRequest) -> HttpResponseBase:  # pragma: no cove
 # Types that `django.core.serializers.json.DjangoJSONEncoder` can handle
 DjangoJSONEncodable = Union[
     JSONValue,
-    "datetime.datetime",
-    "datetime.date",
-    "datetime.time",
-    "datetime.timedelta",
-    "decimal.Decimal",
-    "uuid.UUID",
-    "Promise",
+    datetime.datetime,
+    datetime.date,
+    datetime.time,
+    datetime.timedelta,
+    decimal.Decimal,
+    uuid.UUID,
+    Promise,
 ]
 
 
 class JSONSerializable(Protocol):
-    def __json__(self) -> DjangoJSONEncodable:
+    def __json__(self) -> DjangoJSONEncodable:  # pragma: no cover
         ...
 
 

src/soap/client.py

@@ -27,6 +27,9 @@ def build_client(
     transport = transport_factory(
         session=session,
         timeout=settings.DEFAULT_TIMEOUT_REQUESTS,
+        # operation_timeout gets passed as a parameter on all requests, overriding any
+        # monkeypatched requests.Session defaults
+        operation_timeout=settings.DEFAULT_TIMEOUT_REQUESTS,
     )
     kwargs.setdefault("wsdl", service.url)
     client = client_factory(

src/soap/models.py

@@ -8,24 +8,6 @@
 from .constants import EndpointSecurity, SOAPVersion
 
 
-class UnknownChoiceError(ValueError):
-    def __init__(self, instance: models.Model, field_name: str):
-        self.model = type(instance)
-        self.field: models.Field = getattr(self.model, field_name)
-        self.value = getattr(instance, field_name)
-        # This won't contain the
-        valid_values = self.field.choices
-        super().__init__(
-            _(
-                "Unexpected value %(value) for %(field). Expected one from %(valid_values)r"
-            ).format(
-                value=self.value,
-                field=self.field.verbose_name,
-                valid_values=valid_values,
-            )
-        )
-
-
 class _Signature(Signature):
     def verify(self, envelope):
         return envelope
@@ -147,4 +129,6 @@ def get_wsse(
             case "":
                 return None
 
-        raise UnknownChoiceError(instance=self, field_name="endpoint_security")
+        raise ValueError(
+            f"invalid SoapService.endpoint_security: {self.endpoint_security}"
+        )

src/soap/tests/test_client.py

@@ -1,14 +1,14 @@
 """
 Test the client factory from SOAPService configuration.
 """
-import unittest
 from pathlib import Path
 
 from django.test import TestCase, override_settings
 
 import requests_mock
 from ape_pie import InvalidURLError
 from requests.exceptions import RequestException
+from zeep.exceptions import XMLSyntaxError
 from zeep.wsse import Signature, UsernameToken
 
 from openforms.utils.tests.vcr import OFVCRMixin
@@ -71,9 +71,6 @@ def test_server_cert_specified(self):
             client.transport.session.verify, self.server_cert.public_certificate.path
         )
 
-    def _get_vcr_kwargs(self):
-        return {**super()._get_vcr_kwargs(), "record_mode": "none"}
-
     def test_server_cert_pair(self):
         # Should just use the certificate (chain) of the pair
         service = SoapServiceFactory.build(
@@ -121,7 +118,7 @@ def test_client_cert_public_cert_and_privkey_specified(self):
     def test_incomplete_client_cert_configured(self):
         service = SoapServiceFactory.build(
             url=WSDL_URI,
-            client_certificate=CertificateFactory.build(
+            client_certificate=CertificateFactory.create(
                 public_certificate=None,
                 private_key=None,
             ),
@@ -206,6 +203,12 @@ def test_wsse_basicauth(self):
         self.assertEqual(usertoken.username, "admin")
         self.assertEqual(usertoken.password, "supersecret")
 
+    def test_wrong_wsse(self):
+        service = SoapServiceFactory.create(endpoint_security="my blue eyes")
+
+        with self.assertRaises(ValueError):
+            service.get_wsse()
+
     @requests_mock.Mocker()
     def test_no_absolute_url_sanitization(self, m):
         m.post("https://other-base.example.com")
@@ -246,36 +249,21 @@ def test_it_can_build_a_functional_client(self):
             "Your input parameters are under the normal run and things",
         )
 
-    @unittest.skip("Hangs GH CI")
+
+class ClientTransportTimeoutTests(TestCase):
     @override_settings(DEFAULT_TIMEOUT_REQUESTS=1)
     def test_the_client_obeys_timeout_requests(self):
         "We don't want an unresponsive service DoS us."
-        # Rig the cassette for failure
-        # import signal
-        # from time import sleep
-        # self.cassette.play_response = lambda request: sleep(2)
-        # self.cassette.can_play_response_for = lambda request: True
-        self.cassette.record_mode = "all"
-
         service = SoapServiceFactory.build(
-            # this service acts like some slow lorris on https
-            url="https://www.soapclient.com/xml/soapresponder.wsdl"
+            # this service acts like some slow lorris, will eventually
+            # respond with something that's not a wsdl
+            url="https://httpstat.us/200?sleep=3000"
         )
 
-        # signals aren't thread save
-        org_handler = signal.getsignal(signal.SIGALRM)
-        self.addCleanup(lambda: signal.signal(signal.SIGALRM, org_handler))
-        signal.signal(
-            signal.SIGALRM,
-            lambda _sig, _frm: self.fail("Client seems to hang")
-            # but there is a chance be that the service started responding, but we couldn't
-            # process the wsdl in time
-        )
         with self.assertRaises(RequestException):
-            signal.alarm(5)
-            # zeep will try to read the wsdl
-            build_client(service)
-
-            # Passed this point, the test has broken, find or create another test service
-            # that opens the socket, but doesn't respond.
-            self.fail("The service unexpectedly responded!")
+            try:
+                # zeep will try to read the "wsdl"
+                build_client(service)
+            except XMLSyntaxError:
+                # DEFAULT_TIMOUT_REQUESTS time has passed and we're trying
+                self.fail("DEFAULT_TIMEOUT_REQUESTS not honoured by SOAP client")