fix: Do not clear environment in subprocess integration (getsentry#418)

untitaker · web-flow · commit 84845acbad76 · 2019-07-09T20:39:01.000+02:00
* fix: Do not clear environment in subprocess integration Fix getsentry#417 * fix: Fix test under Python 2 * fix: Do not modify parent process env * fix: Only copy os.environ when necessary
diff --git a/sentry_sdk/integrations/stdlib.py b/sentry_sdk/integrations/stdlib.py
@@ -111,17 +111,28 @@ def getresponse(self, *args, **kwargs):
     HTTPConnection.getresponse = getresponse
 
 
-def _get_argument(args, kwargs, name, position, setdefault=None):
+def _init_argument(args, kwargs, name, position, setdefault_callback=None):
+    """
+    given (*args, **kwargs) of a function call, retrieve (and optionally set a
+    default for) an argument by either name or position.
+
+    This is useful for wrapping functions with complex type signatures and
+    extracting a few arguments without needing to redefine that function's
+    entire type signature.
+    """
+
     if name in kwargs:
         rv = kwargs[name]
-        if rv is None and setdefault is not None:
-            rv = kwargs[name] = setdefault
+        if rv is None and setdefault_callback is not None:
+            rv = kwargs[name] = setdefault_callback()
     elif position < len(args):
         rv = args[position]
-        if rv is None and setdefault is not None:
-            rv = args[position] = setdefault
+        if rv is None and setdefault_callback is not None:
+            rv = args[position] = setdefault_callback()
     else:
-        rv = kwargs[name] = setdefault
+        rv = setdefault_callback and setdefault_callback()
+        if rv is not None:
+            kwargs[name] = rv
 
     return rv
 
@@ -136,11 +147,14 @@ def sentry_patched_popen_init(self, *a, **kw):
 
         # do not setdefault! args is required by Popen, doing setdefault would
         # make invalid calls valid
-        args = _get_argument(a, kw, "args", 0) or []
-        cwd = _get_argument(a, kw, "cwd", 10)
+        args = _init_argument(a, kw, "args", 0) or []
+        cwd = _init_argument(a, kw, "cwd", 10)
+
+        env = None
 
         for k, v in hub.iter_trace_propagation_headers():
-            env = _get_argument(a, kw, "env", 11, {})
+            if env is None:
+                env = _init_argument(a, kw, "env", 11, lambda: dict(os.environ))
             env["SUBPROCESS_" + k.upper().replace("-", "_")] = v
 
         with hub.span(op="subprocess", description=" ".join(map(str, args))) as span:
diff --git a/tests/integrations/stdlib/test_subprocess.py b/tests/integrations/stdlib/test_subprocess.py
@@ -1,25 +1,37 @@
+import os
 import subprocess
 import sys
 
+import pytest
+
 from sentry_sdk import Hub, capture_message
+from sentry_sdk._compat import PY2
 from sentry_sdk.integrations.stdlib import StdlibIntegration
 
 
-def test_subprocess_basic(sentry_init, capture_events):
+def test_subprocess_basic(sentry_init, capture_events, monkeypatch):
+    monkeypatch.setenv("FOO", "bar")
+
+    old_environ = dict(os.environ)
+
     sentry_init(integrations=[StdlibIntegration()], traces_sample_rate=1.0)
 
     with Hub.current.span(transaction="foo", op="foo") as span:
         output = subprocess.check_output(
             [
                 sys.executable,
                 "-c",
+                "import os; "
                 "import sentry_sdk; "
                 "from sentry_sdk.integrations.stdlib import get_subprocess_traceparent_headers; "
                 "sentry_sdk.init(); "
+                "assert os.environ['FOO'] == 'bar'; "
                 "print(dict(get_subprocess_traceparent_headers()))",
             ]
         )
 
+    assert os.environ == old_environ
+
     assert span.trace_id in str(output)
 
     events = capture_events()
@@ -35,3 +47,15 @@ def test_subprocess_basic(sentry_init, capture_events):
         "timestamp": crumb["timestamp"],
         "type": "subprocess",
     }
+
+
+def test_subprocess_invalid_args(sentry_init):
+    sentry_init(integrations=[StdlibIntegration()])
+
+    with pytest.raises(TypeError) as excinfo:
+        subprocess.Popen()
+
+    if PY2:
+        assert "__init__() takes at least 2 arguments (1 given)" in str(excinfo.value)
+    else:
+        assert "missing 1 required positional argument: 'args" in str(excinfo.value)
