Merge pull request #561 from duckontheweb/change/gh-315-required-prop…

…erty-exception Use get_required to raise RequiredPropertyMissing when appropriate
stac-utils · Jul 17, 2021 · ac7b1d6 · ac7b1d6
2 parents 0ba1a1d + c12f907
commit ac7b1d6
Show file tree

Hide file tree

Showing 8 changed files with 43 additions and 69 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -26,7 +26,11 @@
 - Calling `ExtensionManagementMixin.validate_has_extension` with `add_if_missing = True`
   on an ownerless `Asset` will raise a `STACError` ([#554](https://github.com/stac-utils/pystac/pull/554))
 - `PointcloudSchema` -> `Schema`, `PointcloudStatistic` -> `Statistic` for consistency
-  with naming convention in other extensions ([#548](https://github.com/stac-utils/pystac/pull/548))
+  with naming convention in other extensions
+  ([#548](https://github.com/stac-utils/pystac/pull/548))
+- `RequiredPropertyMissing` always raised when trying to get a required property that is
+  `None` (`STACError` or `KeyError` was previously being raised in some cases)
+  ([#561](https://github.com/stac-utils/pystac/pull/561))
 
 ### Fixed
 

diff --git a/pystac/extensions/datacube.py b/pystac/extensions/datacube.py
@@ -12,7 +12,7 @@
     PropertiesExtension,
 )
 from pystac.extensions.hooks import ExtensionHooks
-from pystac.utils import get_required, map_opt
+from pystac.utils import get_required
 
 T = TypeVar("T", pystac.Collection, pystac.Item, pystac.Asset)
 
@@ -63,13 +63,13 @@ def to_dict(self) -> Dict[str, Any]:
 
     @staticmethod
     def from_dict(d: Dict[str, Any]) -> "Dimension":
-        dim_type = d.get(DIM_TYPE_PROP)
-        if dim_type is None:
-            raise pystac.RequiredPropertyMissing("cube_dimension", DIM_TYPE_PROP)
+        dim_type: str = get_required(
+            d.get(DIM_TYPE_PROP), "cube_dimension", DIM_TYPE_PROP
+        )
         if dim_type == "spatial":
-            axis = d.get(DIM_AXIS_PROP)
-            if axis is None:
-                raise pystac.RequiredPropertyMissing("cube_dimension", DIM_AXIS_PROP)
+            axis: str = get_required(
+                d.get(DIM_AXIS_PROP), "cube_dimension", DIM_AXIS_PROP
+            )
             if axis == "z":
                 return VerticalSpatialDimension(d)
             else:
@@ -320,14 +320,10 @@ def apply(self, dimensions: Dict[str, Dimension]) -> None:
 
     @property
     def dimensions(self) -> Dict[str, Dimension]:
-        return get_required(
-            map_opt(
-                lambda d: {k: Dimension.from_dict(v) for k, v in d.items()},
-                self._get_property(DIMENSIONS_PROP, Dict[str, Any]),
-            ),
-            self,
-            DIMENSIONS_PROP,
+        result = get_required(
+            self._get_property(DIMENSIONS_PROP, Dict[str, Any]), self, DIMENSIONS_PROP
         )
+        return {k: Dimension.from_dict(v) for k, v in result.items()}
 
     @dimensions.setter
     def dimensions(self, v: Dict[str, Dimension]) -> None:

diff --git a/pystac/extensions/eo.py b/pystac/extensions/eo.py
@@ -121,7 +121,7 @@ def name(self) -> str:
         Returns:
             str
         """
-        return get_required(self.properties["name"], self, "name")
+        return get_required(self.properties.get("name"), self, "name")
 
     @name.setter
     def name(self, v: str) -> None:

diff --git a/pystac/extensions/file.py b/pystac/extensions/file.py
@@ -69,7 +69,7 @@ def create(cls, values: List[Any], summary: str) -> "MappingObject":
     def values(self) -> List[Any]:
         """Gets or sets the list of value(s) in the file. At least one array element is
         required."""
-        return get_required(self.properties["values"], self, "values")
+        return get_required(self.properties.get("values"), self, "values")
 
     @values.setter
     def values(self, v: List[Any]) -> None:
@@ -78,7 +78,7 @@ def values(self, v: List[Any]) -> None:
     @property
     def summary(self) -> str:
         """Gets or sets the short description of the value(s)."""
-        return get_required(self.properties["summary"], self, "summary")
+        return get_required(self.properties.get("summary"), self, "summary")
 
     @summary.setter
     def summary(self, v: str) -> None:

diff --git a/pystac/extensions/pointcloud.py b/pystac/extensions/pointcloud.py
@@ -88,12 +88,7 @@ def create(cls, name: str, size: int, type: SchemaType) -> "Schema":
     @property
     def size(self) -> int:
         """Gets or sets the size value."""
-        result: Optional[int] = self.properties.get("size")
-        if result is None:
-            raise pystac.STACError(
-                f"Pointcloud schema does not have size property: {self.properties}"
-            )
-        return result
+        return get_required(self.properties.get("size"), self, "size")
 
     @size.setter
     def size(self, v: int) -> None:
@@ -105,12 +100,7 @@ def size(self, v: int) -> None:
     @property
     def name(self) -> str:
         """Gets or sets the name property for this Schema."""
-        result: Optional[str] = self.properties.get("name")
-        if result is None:
-            raise pystac.STACError(
-                f"Pointcloud schema does not have name property: {self.properties}"
-            )
-        return result
+        return get_required(self.properties.get("name"), self, "name")
 
     @name.setter
     def name(self, v: str) -> None:
@@ -128,7 +118,9 @@ def type(self, v: SchemaType) -> None:
 
     def __repr__(self) -> str:
         return "<Schema name={} size={} type={}>".format(
-            self.name, self.size, self.type
+            self.properties.get("name"),
+            self.properties.get("size"),
+            self.properties.get("type"),
         )
 
     def to_dict(self) -> Dict[str, Any]:
@@ -217,12 +209,7 @@ def create(
     @property
     def name(self) -> str:
         """Gets or sets the name property."""
-        result: Optional[str] = self.properties.get("name")
-        if result is None:
-            raise pystac.STACError(
-                f"Pointcloud statistics does not have name property: {self.properties}"
-            )
-        return result
+        return get_required(self.properties.get("name"), self, "name")
 
     @name.setter
     def name(self, v: str) -> None:
@@ -381,10 +368,7 @@ def apply(
     @property
     def count(self) -> int:
         """Gets or sets the number of points in the Item."""
-        result = self._get_property(COUNT_PROP, int)
-        if result is None:
-            raise pystac.RequiredPropertyMissing(self, COUNT_PROP)
-        return result
+        return get_required(self._get_property(COUNT_PROP, int), self, COUNT_PROP)
 
     @count.setter
     def count(self, v: int) -> None:
@@ -393,11 +377,7 @@ def count(self, v: int) -> None:
     @property
     def type(self) -> Union[PhenomenologyType, str]:
         """Gets or sets the phenomenology type for the point cloud."""
-        return get_required(
-            self._get_property(TYPE_PROP, str),
-            self,
-            TYPE_PROP,
-        )
+        return get_required(self._get_property(TYPE_PROP, str), self, TYPE_PROP)
 
     @type.setter
     def type(self, v: Union[PhenomenologyType, str]) -> None:
@@ -406,10 +386,7 @@ def type(self, v: Union[PhenomenologyType, str]) -> None:
     @property
     def encoding(self) -> str:
         """Gets or sets the content encoding or format of the data."""
-        result = self._get_property(ENCODING_PROP, str)
-        if result is None:
-            raise pystac.RequiredPropertyMissing(self, ENCODING_PROP)
-        return result
+        return get_required(self._get_property(ENCODING_PROP, str), self, ENCODING_PROP)
 
     @encoding.setter
     def encoding(self, v: str) -> None:
@@ -420,9 +397,9 @@ def schemas(self) -> List[Schema]:
         """Gets or sets the list of :class:`Schema` instances defining
         dimensions and types for the data.
         """
-        result = self._get_property(SCHEMAS_PROP, List[Dict[str, Any]])
-        if result is None:
-            raise pystac.RequiredPropertyMissing(self, SCHEMAS_PROP)
+        result = get_required(
+            self._get_property(SCHEMAS_PROP, List[Dict[str, Any]]), self, SCHEMAS_PROP
+        )
         return [Schema(s) for s in result]
 
     @schemas.setter

diff --git a/pystac/extensions/raster.py b/pystac/extensions/raster.py
@@ -272,7 +272,7 @@ def count(self) -> int:
         Returns:
             int
         """
-        return get_required(self.properties["count"], self, "count")
+        return get_required(self.properties.get("count"), self, "count")
 
     @count.setter
     def count(self, v: int) -> None:
@@ -285,7 +285,7 @@ def min(self) -> float:
         Returns:
             float
         """
-        return get_required(self.properties["min"], self, "min")
+        return get_required(self.properties.get("min"), self, "min")
 
     @min.setter
     def min(self, v: float) -> None:
@@ -298,7 +298,7 @@ def max(self) -> float:
         Returns:
             float
         """
-        return get_required(self.properties["max"], self, "max")
+        return get_required(self.properties.get("max"), self, "max")
 
     @max.setter
     def max(self, v: float) -> None:
@@ -312,7 +312,7 @@ def buckets(self) -> List[int]:
         Returns:
             List[int]
         """
-        return get_required(self.properties["buckets"], self, "buckets")
+        return get_required(self.properties.get("buckets"), self, "buckets")
 
     @buckets.setter
     def buckets(self, v: List[int]) -> None:

diff --git a/pystac/extensions/sar.py b/pystac/extensions/sar.py
@@ -289,10 +289,9 @@ def looks_equivalent_number(self, v: Optional[float]) -> None:
     @property
     def observation_direction(self) -> Optional[ObservationDirection]:
         """Gets or sets an observation direction for the item."""
-        result = self._get_property(OBSERVATION_DIRECTION_PROP, str)
-        if result is None:
-            return None
-        return ObservationDirection(result)
+        return map_opt(
+            ObservationDirection, self._get_property(OBSERVATION_DIRECTION_PROP, str)
+        )
 
     @observation_direction.setter
     def observation_direction(self, v: Optional[ObservationDirection]) -> None:

diff --git a/tests/extensions/test_pointcloud.py b/tests/extensions/test_pointcloud.py
@@ -6,7 +6,7 @@
 
 import pystac
 from pystac.asset import Asset
-from pystac.errors import ExtensionTypeError, STACError
+from pystac.errors import ExtensionTypeError, STACError, RequiredPropertyMissing
 from pystac.extensions.pointcloud import (
     AssetPointcloudExtension,
     PhenomenologyType,
@@ -193,12 +193,10 @@ def test_pointcloud_schema(self) -> None:
             schema.size = 0.5  # type: ignore
 
         empty_schema = Schema({})
-        with self.assertRaises(STACError):
-            empty_schema.size
-        with self.assertRaises(STACError):
-            empty_schema.name
-        with self.assertRaises(STACError):
-            empty_schema.type
+        for required_prop in {"size", "name", "type"}:
+            with self.subTest(attr=required_prop):
+                with self.assertRaises(RequiredPropertyMissing):
+                    getattr(empty_schema, required_prop)
 
     def test_pointcloud_statistics(self) -> None:
         props: Dict[str, Any] = {
@@ -251,7 +249,7 @@ def test_pointcloud_statistics(self) -> None:
         self.assertNotIn("variance", stat.properties)
 
         empty_stat = Statistic({})
-        with self.assertRaises(STACError):
+        with self.assertRaises(RequiredPropertyMissing):
             empty_stat.name
 
     def test_statistics_accessor_when_no_stats(self) -> None: