fix: accessing an unset struct_pb2.Value field does not raise

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."""

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):

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():