Repository: googleapis/proto-plus-python

Issue:
struct.Value fields raise an AttributeError when empty
Is this the expected behavior? I found it slightly confusing as I was playing with the AIPlatform library https://github.com/googleapis/python-aiplatform/pull/15#pullrequestreview-500502756

```py
import proto
from google.protobuf import struct_pb2 as struct  # type: ignore

class Slice(proto.Message):
    metrics = proto.Field(proto.MESSAGE, number=4, message=struct.Value,)
```

```py
>>> Slice.metrics
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: type object 'Slice' has no attribute 'metrics'
>>> Slice.metrics = {"x", 1.3}
>>> Slice.metrics
{'x': 1.3}
```

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/googleapis/proto-plus-python/pull/140?src=pr&el=h1) Report
> Merging [#140](https://codecov.io/gh/googleapis/proto-plus-python/pull/140?src=pr&el=desc) into [master](https://codecov.io/gh/googleapis/proto-plus-python/commit/6888d71d680a6adf9fb38e4fa0f78fa217ea952d?el=desc) will **not change** coverage.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/googleapis/proto-plus-python/pull/140/graphs/tree.svg?width=650&height=150&src=pr&token=pGYJa23k39)](https://codecov.io/gh/googleapis/proto-plus-python/pull/140?src=pr&el=tree)

```diff
@@            Coverage Diff            @@
##            master      #140   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           20        20           
  Lines          857       856    -1     
  Branches       149       148    -1     
=========================================
- Hits           857       856    -1     
```


| [Impacted Files](https://codecov.io/gh/googleapis/proto-plus-python/pull/140?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [proto/marshal/rules/struct.py](https://codecov.io/gh/googleapis/proto-plus-python/pull/140/diff?src=pr&el=tree#diff-cHJvdG8vbWFyc2hhbC9ydWxlcy9zdHJ1Y3QucHk=) | `100.00% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/googleapis/proto-plus-python/pull/140?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/googleapis/proto-plus-python/pull/140?src=pr&el=footer). Last update [6888d71...dfe264d](https://codecov.io/gh/googleapis/proto-plus-python/pull/140?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/proto/marshal/rules/struct.py b/proto/marshal/rules/struct.py
index 53ef34f..27dfaf5 100644
--- a/proto/marshal/rules/struct.py
+++ b/proto/marshal/rules/struct.py
@@ -29,11 +29,15 @@ class ValueRule:
     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 @@ class ValueRule:
             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."""


Test patch:
diff --git a/tests/conftest.py b/tests/conftest.py
index 7e7265f..b60b91d 100644
--- 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
index 82a40c2..914730c 100644
--- a/tests/test_marshal_types_struct.py
+++ b/tests/test_marshal_types_struct.py
@@ -30,6 +30,13 @@ def test_value_primitives_read():
     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 @@ def test_value_unset():
         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():