Enforce type geometry properties #94

vincentsarago · 2023-01-27T14:07:20Z

closes #92

vincentsarago · 2023-01-27T14:07:57Z

.github/workflows/ci.yml

@@ -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']


Let's deprecate python 3.7:

because it's old

because it doesn't support Literal by default

vincentsarago · 2023-01-27T14:08:07Z

geojson_pydantic/features.py

-    properties: Optional[Props] = None
+    type: Literal["Feature"]
+    geometry: Union[Geom, None] = Field(...)
+    properties: Props = Field(...)


TO BE DISCUSSED

A Feature object has a member with the name "properties". The value of the properties member is an object (any JSON object or a JSON null value).

Probably should be: properties: Union[Props, None] = Field(...)

right so we should accept properties=None

vincentsarago · 2023-01-27T14:08:31Z

geojson_pydantic/features.py

@@ -44,23 +46,22 @@ def __geo_interface__(self) -> Dict[str, Any]:
            "geometry": self.geometry.__geo_interface__
            if self.geometry is not None
            else None,
+            "properties": self.properties,


properties should always be set to at least {}

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.

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

🤔 https://github.com/jazzband/geojson/blob/main/geojson/feature.py#L31 always default to {}

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

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.

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

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

geojson_pydantic/features.py

eseglem · 2023-01-27T15:09:08Z

geojson_pydantic/features.py

-    properties: Optional[Props] = None
+    type: Literal["Feature"]
+    geometry: Union[Geom, None] = Field(...)
+    properties: Props = Field(...)


A Feature object has a member with the name "properties". The value of the properties member is an object (any JSON object or a JSON null value).

Probably should be: properties: Union[Props, None] = Field(...)

tests/test_features.py

eseglem · 2023-01-27T15:46:14Z

geojson_pydantic/features.py

@@ -44,23 +46,22 @@ def __geo_interface__(self) -> Dict[str, Any]:
            "geometry": self.geometry.__geo_interface__
            if self.geometry is not None
            else None,
+            "properties": self.properties,


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.

vincentsarago · 2023-01-30T13:46:26Z

@eseglem are we happy with the changes proposed here?

eseglem

I think __geo_interface__ may be worth additional discussion but that is outside the scope of this PR. Looks good to me.

vincentsarago · 2023-01-30T14:06:22Z

@geospatial-jeff do you have opinions? the TLDR of this pr is:

makes type=Feature, geometry and properties mandatory (but they can be None) for Feature
makes type=FeatureCollection and features mandatory for FeatureCollection
makes type mandatory for geometries

Those are quite breaking change but better match the specification. We could argue that type do not have to be passed when constructing a Feature or Geometry but if we think as geojson-pydantic more as the validation library then it makes more sense...

Another change that will be discussed in another Issue is the fact that we don't follow the geo_interface_ specifiation to the letter because we set values to None for geometry and properties. The spec for Feature is kinda not clear about what we should do but there were a lot of discussion about geometries in the shapely repo which I'm going link in the new issue.

geospatial-jeff · 2023-01-31T00:56:01Z

I agree with these changes, it better matches the spec which is what I care about. Keeping in mind that this library was spun out of stac-pydantic which itself didn't attempt to implement the full geojson spec, just enough to make it work for STAC. So I think these changes are good and much needed.

vincentsarago added 2 commits January 27, 2023 15:03

enforce type, geometry and properties

5f98bdb

add more tests

8c52f31

vincentsarago commented Jan 27, 2023

View reviewed changes

vincentsarago mentioned this pull request Jan 27, 2023

Feature validates arbitrary dictionaries #92

Closed

eseglem reviewed Jan 27, 2023

View reviewed changes

geojson_pydantic/features.py Outdated Show resolved Hide resolved

eseglem requested changes Jan 27, 2023

View reviewed changes

vincentsarago added 2 commits January 27, 2023 17:05

revert if test

1ccae1f

allow properties to be None

9c65e5c

eseglem mentioned this pull request Jan 27, 2023

Verbose validation error is difficult to understand #93

Closed

vincentsarago requested review from eseglem and removed request for eseglem January 30, 2023 10:01

vincentsarago marked this pull request as ready for review January 30, 2023 10:02

eseglem approved these changes Jan 30, 2023

View reviewed changes

vincentsarago mentioned this pull request Jan 30, 2023

__geo_interface__ specification and implementation #96

Closed

vincentsarago added 2 commits January 31, 2023 08:41

update changelog

9c801bf

add not about python 3.7 deprecation

cb6fbc4

vincentsarago merged commit d65cdc0 into main Jan 31, 2023

vincentsarago deleted the EnforceTypeGeometryProperties branch January 31, 2023 07:45

vincentsarago mentioned this pull request Apr 10, 2024

Would it be possible to default the type fields? #157

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce type geometry properties #94

Enforce type geometry properties #94

vincentsarago commented Jan 27, 2023

vincentsarago Jan 27, 2023

vincentsarago Jan 27, 2023

eseglem Jan 27, 2023 •

edited

Loading

vincentsarago Jan 27, 2023

vincentsarago Jan 27, 2023

eseglem Jan 27, 2023

vincentsarago Jan 27, 2023

vincentsarago Jan 27, 2023

vincentsarago Jan 27, 2023

eseglem Jan 27, 2023 •

edited

Loading

vincentsarago Jan 30, 2023

eseglem Jan 30, 2023

eseglem Jan 27, 2023 •

edited

Loading

eseglem Jan 27, 2023

vincentsarago commented Jan 30, 2023

eseglem left a comment

vincentsarago commented Jan 30, 2023

geospatial-jeff commented Jan 31, 2023

Enforce type geometry properties #94

Enforce type geometry properties #94

Conversation

vincentsarago commented Jan 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eseglem Jan 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eseglem Jan 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eseglem Jan 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincentsarago commented Jan 30, 2023

eseglem left a comment

Choose a reason for hiding this comment

vincentsarago commented Jan 30, 2023

geospatial-jeff commented Jan 31, 2023

eseglem Jan 27, 2023 •

edited

Loading

eseglem Jan 27, 2023 •

edited

Loading

eseglem Jan 27, 2023 •

edited

Loading