fix: accessing an unset struct_pb2.Value field does not raise (#140)

software-dov · web-flow · commit d045cbf058cb · 2020-10-01T14:39:29.000-07:00
class Foo(proto.Message):
    value = proto.Field(struct_pb2.Value, number=1)

f = Foo()
assert f.value is None          # This should not raise an exception.
assert "value" not in f         # The attribute has _not_ been set.
f.value = None
assert f.value is None
assert "value" in f             # The attribute _has_ been set.

None of the above should raise an exception.
diff --git a/proto/marshal/rules/struct.py b/proto/marshal/rules/struct.py
@@ -29,11 +29,15 @@ def __init__(self, *, marshal):
     def to_python(self, value, *, absent: bool = None):
         """Coerce the given value to the appropriate Python type.
 
-        Note that setting ``null_value`` is distinct from not setting
-        a value, and the absent value will raise an exception.
+        Note that both NullValue and absent fields return None.
+        In order to disambiguate between these two options,
+        use containment check,
+        E.g.
+        "value" in foo
+        which is True for NullValue and False for an absent value.
         """
         kind = value.WhichOneof("kind")
-        if kind == "null_value":
+        if kind == "null_value" or absent:
             return None
         if kind == "bool_value":
             return bool(value.bool_value)
@@ -49,7 +53,9 @@ def to_python(self, value, *, absent: bool = None):
             return self._marshal.to_python(
                 struct_pb2.ListValue, value.list_value, absent=False,
             )
-        raise AttributeError
+        # If more variants are ever added, we want to fail loudly
+        # instead of tacitly returning None.
+        raise ValueError("Unexpected kind: %s" % kind)  # pragma: NO COVER
 
     def to_proto(self, value) -> struct_pb2.Value:
         """Return a protobuf Value object representing this value."""
diff --git a/tests/conftest.py b/tests/conftest.py
@@ -12,7 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-import imp
+import importlib
 from unittest import mock
 
 from google.protobuf import descriptor_pool
@@ -76,11 +76,11 @@ def pytest_runtest_setup(item):
     # If the marshal had previously registered the old message classes,
     # then reload the appropriate modules so the marshal is using the new ones.
     if "wrappers_pb2" in reloaded:
-        imp.reload(rules.wrappers)
+        importlib.reload(rules.wrappers)
     if "struct_pb2" in reloaded:
-        imp.reload(rules.struct)
+        importlib.reload(rules.struct)
     if reloaded.intersection({"timestamp_pb2", "duration_pb2"}):
-        imp.reload(rules.dates)
+        importlib.reload(rules.dates)
 
 
 def pytest_runtest_teardown(item):
diff --git a/tests/test_marshal_types_struct.py b/tests/test_marshal_types_struct.py
@@ -30,6 +30,13 @@ class Foo(proto.Message):
     assert Foo(value=True).value is True
 
 
+def test_value_absent():
+    class Foo(proto.Message):
+        value = proto.Field(struct_pb2.Value, number=1)
+
+    assert Foo().value is None
+
+
 def test_value_primitives_rmw():
     class Foo(proto.Message):
         value = proto.Field(struct_pb2.Value, number=1)
@@ -158,7 +165,7 @@ class Foo(proto.Message):
         value = proto.Field(struct_pb2.Value, number=1)
 
     foo = Foo()
-    assert not hasattr(foo, "value")
+    assert "value" not in foo
 
 
 def test_list_value_read():
