fix handling of required keys in configuration (#927)

* fix handling of required keys in configuration

If a key wasn't provided, we didn't check if it was required or not.
This led to a problem that if no `service_name` was configured,
we'd send invalid data to APM Server.

fixes #921

* when copying configs, discard errors of the fresh copy

* raise exception explicitly in test

we seem to have relied on buggy behaviour without noticing here before

* raise a proper exception in the test instead of relying on buggy behavior

* Add changelog

Co-authored-by: Colton Myers <[email protected]>

Author: beniwohli

CHANGELOG.asciidoc

@@ -43,6 +43,8 @@ endif::[]
 [float]
 ===== Bug fixes
 
+* Fix validation of config to properly require `required` config items. {pull}927[#927]
+
 
 [[release-notes-5.x]]
 === Python Agent version 5.x

elasticapm/conf/__init__.py

@@ -69,9 +69,9 @@ def __get__(self, instance, owner):
         else:
             return self.default
 
-    def __set__(self, instance, value):
-        value = self._validate(instance, value)
-        instance._values[self.dict_key] = value
+    def __set__(self, config_instance, value):
+        value = self._validate(config_instance, value)
+        config_instance._values[self.dict_key] = value
 
     def _validate(self, instance, value):
         if value is None and self.required:
@@ -237,6 +237,11 @@ def update(self, config_dict=None, env_dict=None, inline_dict=None):
                     setattr(self, field, new_value)
                 except ConfigurationError as e:
                     self._errors[e.field_name] = str(e)
+            # if a field has not been provided by any config source, we have to check separately if it is required
+            if config_value.required and getattr(self, field) is None:
+                self._errors[config_value.dict_key] = "Configuration error: value for {} is required.".format(
+                    config_value.dict_key
+                )
 
     @property
     def values(self):
@@ -250,6 +255,12 @@ def values(self, values):
     def errors(self):
         return self._errors
 
+    def copy(self):
+        c = self.__class__()
+        c._errors = {}
+        c.values = self.values.copy()
+        return c
+
 
 class Config(_ConfigBase):
     service_name = _ConfigValue("SERVICE_NAME", validators=[RegexValidator("^[a-zA-Z0-9 _-]+$")], required=True)
@@ -393,8 +404,7 @@ def update(self, version, **config):
         :param config: a key/value map of new configuration
         :return: configuration errors, if any
         """
-        new_config = Config()
-        new_config.values = self._config.values.copy()
+        new_config = self._config.copy()
 
         # pass an empty env dict to ensure the environment doesn't get precedence
         new_config.update(inline_dict=config, env_dict={})

tests/config/tests.py

@@ -301,3 +301,17 @@ def test_capture_body_mapping(val, expected):
 def test_is_recording(enabled, recording, is_recording):
     c = Config(inline_dict={"enabled": enabled, "recording": recording, "service_name": "foo"})
     assert c.is_recording is is_recording
+
+
+def test_required_is_checked_if_field_not_provided():
+    class MyConfig(_ConfigBase):
+        this_one_is_required = _ConfigValue("this_one_is_required", type=int, required=True)
+        this_one_isnt = _ConfigValue("this_one_isnt", type=int, required=False)
+
+    assert MyConfig({"this_one_is_required": None}).errors
+    assert MyConfig({}).errors
+    assert MyConfig({"this_one_isnt": 1}).errors
+
+    c = MyConfig({"this_one_is_required": 1})
+    c.update({"this_one_isnt": 0})
+    assert not c.errors

tests/contrib/django/django_tests.py

@@ -1313,18 +1313,18 @@ def test_test_exception(urlopen_mock):
     assert "Success! We tracked the error successfully!" in output
 
 
-@pytest.mark.parametrize(
-    "django_elasticapm_client", [{"transport_class": "elasticapm.transport.http.Transport"}], indirect=True
-)
-def test_test_exception_fails(django_elasticapm_client):
+@mock.patch("elasticapm.transport.http.Transport.send")
+def test_test_exception_fails(mock_send):
     stdout = compat.StringIO()
+    mock_send.side_effect = Exception("boom")
     with override_settings(
-        ELASTIC_APM={"TRANSPORT_CLASS": "elasticapm.transport.http.Transport"},
+        ELASTIC_APM={"TRANSPORT_CLASS": "elasticapm.transport.http.Transport", "SERVICE_NAME": "testapp"},
         **middleware_setting(django.VERSION, ["foo", "elasticapm.contrib.django.middleware.TracingMiddleware"])
     ):
         call_command("elasticapm", "test", stdout=stdout, stderr=stdout)
     output = stdout.getvalue()
     assert "Oops" in output
+    assert "boom" in output
 
 
 def test_tracing_middleware_uses_test_client(client, django_elasticapm_client):