Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enforce type geometry properties #94

Merged
merged 6 commits into from
Jan 31, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
python-version: ['3.7', '3.8', '3.9', '3.10', '3.11']
python-version: ['3.8', '3.9', '3.10', '3.11']
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's deprecate python 3.7:

  • because it's old
  • because it doesn't support Literal by default


steps:
- uses: actions/checkout@v3
Expand Down
24 changes: 24 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,30 @@ All notable changes to this project will be documented in this file.
The format is based on [Keep a Changelog](http://keepachangelog.com/)
and this project adheres to [Semantic Versioning](http://semver.org/).

## [0.6.0] - TBD

- Remove python 3.7 support
- Enforce required keys and avoid defaults. This aim to follow the geojson specification to the letter.

```python
# Before
Feature(geometry=Point(coordinates=(0,0)))

# Now
Feature(
type="Feature",
geometry=Point(
type="Point",
coordinates=(0,0)
),
properties=None,
)
```

### Fixed

- Do not validates arbitrary dictionaries. Make `Type` a mandatory key for objects (https://github.com/developmentseed/geojson-pydantic/pull/94)

## [0.5.0] - 2022-12-16

### Added
Expand Down
16 changes: 8 additions & 8 deletions geojson_pydantic/features.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
"""pydantic models for GeoJSON Feature objects."""

from typing import Any, Dict, Generic, Iterator, List, Optional, TypeVar, Union
from typing import Any, Dict, Generic, Iterator, List, Literal, Optional, TypeVar, Union

from pydantic import BaseModel, Field, validator
from pydantic.generics import GenericModel
Expand All @@ -15,9 +15,9 @@
class Feature(GenericModel, Generic[Geom, Props]):
"""Feature Model"""

type: str = Field(default="Feature", const=True)
geometry: Optional[Geom] = None
properties: Optional[Props] = None
type: Literal["Feature"]
geometry: Union[Geom, None] = Field(...)
properties: Union[Props, None] = Field(...)
id: Optional[str] = None
bbox: Optional[BBox] = None

Expand All @@ -31,6 +31,7 @@ def set_geometry(cls, geometry: Any) -> Any:
"""set geometry from geo interface or input"""
if hasattr(geometry, "__geo_interface__"):
return geometry.__geo_interface__

return geometry

@property
Expand All @@ -44,23 +45,22 @@ def __geo_interface__(self) -> Dict[str, Any]:
"geometry": self.geometry.__geo_interface__
if self.geometry is not None
else None,
"properties": self.properties,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

properties should always be set to at least {}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically GeoJSON can have null properties. But geo_interface says

properties (optional) - A mapping of feature properties ...

So, I would say leave this how it was.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the issue then is that you wouldn't be able to do Feature(**Feature(type="Feature", geometry=None, properties=None).__geo_interface__)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: we already allow geometry to be None in the geo_interface while the spec says it should be a mapping 🤷

Copy link
Collaborator

@eseglem eseglem Jan 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is an interesting one. Not being able to put it back into itself is problematic. And None becomes {} in __geo_interface__ that would mean properties would change when fed back in. The least bad thing might be a pre validator that converts None to dict()?

Note: we already allow geometry to be None in the geo_interface while the spec says it should be a mapping 🤷

The current way won't add the geometry key to the dict unless it is truthy. So it would never be None, it just wouldn't exist at all. Edit: Mixed up properties and geometry in my head at some point.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current way won't add the geometry key to the dict unless it is truthy. So it would never be None, it just wouldn't exist at all.

Feature(type="Feature", geometry=None, properties=None).__geo_interface__
>> {'type': 'Feature', 'geometry': None, 'properties': None}

we do add geometry: None in the current implementation

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vincentsarago Sorry, I had started to edit that comment and apparently never saved it. Got lost in the 12 tabs open looking at too many parts at the same time.

Based on a strict reading geometry may be wrong. However jazzband/geojson is just returning self as the __geo_interface__ so geometry could be None there as well.

Which actually seems like a good idea, its supposed to be modeled after geojson, so just return self.dict(). Don't worry about picking fields like it currently does.

}

if self.bbox:
geo["bbox"] = self.bbox

if self.id:
geo["id"] = self.id

if self.properties:
geo["properties"] = self.properties

return geo


class FeatureCollection(GenericModel, Generic[Geom, Props]):
"""FeatureCollection Model"""

type: str = Field(default="FeatureCollection", const=True)
type: Literal["FeatureCollection"]
features: List[Feature[Geom, Props]]
bbox: Optional[BBox] = None

Expand Down
37 changes: 21 additions & 16 deletions geojson_pydantic/geometries.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
"""pydantic models for GeoJSON Geometry objects."""

import abc
from typing import Any, Dict, Iterator, List, Union
from typing import Any, Dict, Iterator, List, Literal, Union

from pydantic import BaseModel, Field, ValidationError, validator
from pydantic import BaseModel, ValidationError, validator
from pydantic.error_wrappers import ErrorWrapper

from geojson_pydantic.types import (
Expand Down Expand Up @@ -56,7 +56,7 @@ def wkt(self) -> str:
class Point(_GeometryBase):
"""Point Model"""

type: str = Field(default="Point", const=True)
type: Literal["Point"]
coordinates: Position

@property
Expand All @@ -71,7 +71,7 @@ def _wkt_inset(self) -> str:
class MultiPoint(_GeometryBase):
"""MultiPoint Model"""

type: str = Field(default="MultiPoint", const=True)
type: Literal["MultiPoint"]
coordinates: MultiPointCoords

@property
Expand All @@ -80,14 +80,14 @@ def _wkt_inset(self) -> str:

@property
def _wkt_coordinates(self) -> str:
points = [Point(coordinates=p) for p in self.coordinates]
points = [Point(type="Point", coordinates=p) for p in self.coordinates]
return ", ".join(point._wkt_coordinates for point in points)


class LineString(_GeometryBase):
"""LineString Model"""

type: str = Field(default="LineString", const=True)
type: Literal["LineString"]
coordinates: LineStringCoords

@property
Expand All @@ -96,14 +96,14 @@ def _wkt_inset(self) -> str:

@property
def _wkt_coordinates(self) -> str:
points = [Point(coordinates=p) for p in self.coordinates]
points = [Point(type="Point", coordinates=p) for p in self.coordinates]
return ", ".join(point._wkt_coordinates for point in points)


class MultiLineString(_GeometryBase):
"""MultiLineString Model"""

type: str = Field(default="MultiLineString", const=True)
type: Literal["MultiLineString"]
coordinates: MultiLineStringCoords

@property
Expand All @@ -112,7 +112,9 @@ def _wkt_inset(self) -> str:

@property
def _wkt_coordinates(self) -> str:
lines = [LineString(coordinates=line) for line in self.coordinates]
lines = [
LineString(type="LineString", coordinates=line) for line in self.coordinates
]
return ",".join(f"({line._wkt_coordinates})" for line in lines)


Expand All @@ -131,7 +133,7 @@ def check_closure(cls, coordinates: List) -> List:
class Polygon(_GeometryBase):
"""Polygon Model"""

type: str = Field(default="Polygon", const=True)
type: Literal["Polygon"]
coordinates: PolygonCoords

@validator("coordinates")
Expand Down Expand Up @@ -161,27 +163,28 @@ def _wkt_inset(self) -> str:
@property
def _wkt_coordinates(self) -> str:
ic = "".join(
f", ({LinearRingGeom(coordinates=interior)._wkt_coordinates})"
f", ({LinearRingGeom(type='LineString', coordinates=interior)._wkt_coordinates})"
for interior in self.interiors
)
return f"({LinearRingGeom(coordinates=self.exterior)._wkt_coordinates}){ic}"
return f"({LinearRingGeom(type='LineString', coordinates=self.exterior)._wkt_coordinates}){ic}"

@classmethod
def from_bounds(
cls, xmin: float, ymin: float, xmax: float, ymax: float
) -> "Polygon":
"""Create a Polygon geometry from a boundingbox."""
return cls(
type="Polygon",
coordinates=[
[(xmin, ymin), (xmax, ymin), (xmax, ymax), (xmin, ymax), (xmin, ymin)]
]
],
)


class MultiPolygon(_GeometryBase):
"""MultiPolygon Model"""

type: str = Field(default="MultiPolygon", const=True)
type: Literal["MultiPolygon"]
coordinates: MultiPolygonCoords

@property
Expand All @@ -190,7 +193,9 @@ def _wkt_inset(self) -> str:

@property
def _wkt_coordinates(self) -> str:
polygons = [Polygon(coordinates=poly) for poly in self.coordinates]
polygons = [
Polygon(type="Polygon", coordinates=poly) for poly in self.coordinates
]
return ",".join(f"({poly._wkt_coordinates})" for poly in polygons)


Expand All @@ -200,7 +205,7 @@ def _wkt_coordinates(self) -> str:
class GeometryCollection(BaseModel):
"""GeometryCollection Model"""

type: str = Field(default="GeometryCollection", const=True)
type: Literal["GeometryCollection"]
geometries: List[Geometry]

def __iter__(self) -> Iterator[Geometry]: # type: ignore [override]
Expand Down
3 changes: 1 addition & 2 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
name = "geojson-pydantic"
description = "Pydantic data models for the GeoJSON spec."
readme = "README.md"
requires-python = ">=3.7"
requires-python = ">=3.8"
license = {file = "LICENSE"}
authors = [
{name = "Drew Bollinger", email = "drew@developmentseed.org"},
Expand All @@ -12,7 +12,6 @@ classifiers = [
"Intended Audience :: Information Technology",
"Intended Audience :: Science/Research",
"License :: OSI Approved :: MIT License",
"Programming Language :: Python :: 3.7",
"Programming Language :: Python :: 3.8",
"Programming Language :: Python :: 3.9",
"Programming Language :: Python :: 3.10",
Expand Down
37 changes: 32 additions & 5 deletions tests/test_features.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,14 +66,18 @@ class GenericProperties(BaseModel):

def test_feature_collection_iteration():
"""test if feature collection is iterable"""
gc = FeatureCollection(features=[test_feature, test_feature])
gc = FeatureCollection(
type="FeatureCollection", features=[test_feature, test_feature]
)
assert hasattr(gc, "__geo_interface__")
iter(gc)


def test_geometry_collection_iteration():
"""test if feature collection is iterable"""
gc = FeatureCollection(features=[test_feature_geometry_collection])
gc = FeatureCollection(
type="FeatureCollection", features=[test_feature_geometry_collection]
)
assert hasattr(gc, "__geo_interface__")
iter(gc)

Expand Down Expand Up @@ -152,7 +156,7 @@ def test_generic_properties_should_raise_for_string():

def test_feature_collection_generic():
fc = FeatureCollection[Polygon, GenericProperties](
features=[test_feature, test_feature]
type="FeatureCollection", features=[test_feature, test_feature]
)
assert len(fc) == 2
assert type(fc[0].properties) == GenericProperties
Expand All @@ -163,7 +167,7 @@ def test_geo_interface_protocol():
class Pointy:
__geo_interface__ = {"type": "Point", "coordinates": (0.0, 0.0)}

feat = Feature(geometry=Pointy())
feat = Feature(type="Feature", geometry=Pointy(), properties={})
assert feat.geometry.dict() == Pointy.__geo_interface__


Expand All @@ -178,7 +182,30 @@ def test_feature_geo_interface_with_null_geometry():


def test_feature_collection_geo_interface_with_null_geometry():
fc = FeatureCollection(features=[test_feature_geom_null, test_feature])
fc = FeatureCollection(
type="FeatureCollection", features=[test_feature_geom_null, test_feature]
)
assert "bbox" not in fc.__geo_interface__
assert "bbox" not in fc.__geo_interface__["features"][0]
assert "bbox" in fc.__geo_interface__["features"][1]


def test_feature_validation():
"""Test default."""
assert Feature(type="Feature", properties=None, geometry=None)

with pytest.raises(ValidationError):
# should be type=Feature
Feature(type="feature", properties=None, geometry=None)

with pytest.raises(ValidationError):
# missing type
Feature(properties=None, geometry=None)

with pytest.raises(ValidationError):
# missing properties
Feature(type="Feature", geometry=None)

with pytest.raises(ValidationError):
# missing geometry
Feature(type="Feature", properties=None)
Loading