`Feature` validates arbitrary dictionaries

[LukeWeidenwalker]

Hi @vincentsarago, I stumbled across some unexpected behaviour in these pydantic models and thought I'd ask for guidance here!

1. Feature seems to validate arbitrary dictionaries:

>>> geojson_pydantic.Feature.parse_obj({"hi this is": "not a feature"})
Feature(type='Feature', geometry=None, properties=None, id=None, bbox=None)

This seems kind of counter-productive, is this intentional? At least judging from the geojson json-schema, id should not be optional!

2. Feature validates the list ["vh", "vv"], but not the list ["not", "feature"]. This is causing problems in my application, because the list of bands ["vv", "vh"] is incorrectly parsed into a GeoJson, even though it's just a list.

>>> geojson_pydantic.Feature.parse_obj(["vh", "vv"])
Feature(type='Feature', geometry=None, properties=None, id=None, bbox=None)

>>> geojson_pydantic.Feature.parse_obj(["not", "feature"])

ValueError Traceback (most recent call last)
File .venv/lib/python3.9/site-packages/pydantic/main.py:522, in pydantic.main.BaseModel.parse_obj()
ValueError: dictionary update sequence element #0 has length 3; 2 is required
The above exception was the direct cause of the following exception:
ValidationError Traceback (most recent call last) Cell In[4], line 1 ----> 1 geojson_pydantic.Feature.parse_obj(["not", "feature"])
File .venv/lib/python3.9/site-packages/pydantic/main.py:525, in pydantic.main.BaseModel.parse_obj()
ValidationError: 1 validation error for Feature root Feature expected dict not list (type=type_error)

This seems super strange too, do you have any ideas on why that specific list would validate and not the other? I have not been able to find another example.

---

I've run all these in a fresh virtual environment (Python 3.9.5) with only geojson_pydantic installed. Versions are the following:

print(geojson_pydantic.__version__)
print(pydantic.__version__)
>>> 0.5.0 
>>> 1.10.2

Thanks!

[vincentsarago]

That is indeed really weird and I don't think is what we expect!

[geospatial-jeff]

Feature seems to validate arbitrary dictionaries:

I believe this is because the pydantic default is to ignore additional fields. Pydantic checks if the hi is this field is on the model, it isn't, so the value is ignored.

Feature validates the list ["vh", "vv"], but not the list ["not", "feature"]. This is causing problems in my application, because the list of bands ["vv", "vh"] is incorrectly parsed into a GeoJson, even though it's just a list.

The BaseModel.parse_obj class method will coerce the object to a dictionary:

>>> dict(["vh", "vv"])
{'v': 'v'}

>>> dict(["not", "feature"])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: dictionary update sequence element #0 has length 3; 2 is required

The first case works for the same reason, the v field is not on the model so pydantic ignores it. The error in the second case is because parse_obj is breaking when coercing the object to a dictionary, it doesn't even hit validation yet because the Feature class is never instantiated (see the parse_obj source code for more details).

You can change the global behavior of pydantic to tell it to forbid extra fields (https://docs.pydantic.dev/usage/model_config/#change-behaviour-globally), but I think it would be a good idea to enforce this in geojson-pydantic. I'm not really sure what to do about the second case.

[vincentsarago]

but I think it would be a good idea to enforce this in geojson-pydantic. I'm not really sure what to do about the second case.

@geospatial-jeff not allowing extra could be a big breaking change and looking at https://datatracker.ietf.org/doc/html/rfc7946 I don't see why extra wouldn't be allowed.

I'm not sure if it's because with the latest changes but, to me, I believe that even if null you would have to set the geometr y in the feature

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

# IMO The following shouldn't work but they do
Feature.parse_obj({"type": "Feature"})
>> Feature(type='Feature', geometry=None, properties=None, id=None, bbox=None)

Feature.parse_obj({})
>> Feature(type='Feature', geometry=None, properties=None, id=None, bbox=None)

ping @eseglem (ref: https://github.com/developmentseed/geojson-pydantic/pull/87/files#r939570703)

[eseglem]

This does feel like it is more one of the flaws of current pydantic than the library itself. Unfortunately pydantic does some weird stuff like this. Pydantic v2 should hopefully fix a lot of it.

Are you able to provide a little more context on how that list ends up getting passed in? What does the data look like where a List or a Feature could be at the same level? Pydantic is order dependent, and will pick the first valid option unless you tell it to check every type first.

from typing import Union, List
from pydantic import BaseModel
from geojson_pydantic import Feature

class Foo(BaseModel):
    bar: Union[List[str], Feature]

Foo.parse_obj({"bar": ["vv", "vh"]})
# Foo(bar=['vv', 'vh'])
Foo.parse_obj({"bar": {}})
# Foo(bar=Feature(type='Feature', geometry=None, properties=None, id=None, bbox=None))

class Foo(BaseModel):
    bar: Union[Feature, List[str]]

Foo.parse_obj({"bar": ["vv", "vh"]})
# Foo(bar=Feature(type='Feature', geometry=None, properties=None, id=None, bbox=None))
Foo.parse_obj({"bar": {}})
# Foo(bar=Feature(type='Feature', geometry=None, properties=None, id=None, bbox=None))

I also believe extra should be allowed, so I don't think that would be the right route.

One thing that could be done in this library is moving those to geometry: Union[Geometry, None] = Field(...) and require the key actually be specified. That would eliminate the ability to do Feature() which seems like it should be allowed. It may be less bad though?

[vincentsarago]

I kinda agree that we should do geometry: Union[Geometry, None] = Field(...)

[geospatial-jeff]

That sounds good to me

[eseglem]

Are there any other fields (across all the models) where it would make sense to do the same? I think there are probably many instances where an input that technically isn't valid GeoJSON would be accepted by `parse_obj`. As it stands you can do `Feature.parse_obj({"geometry": None})` which is less bad, but still technically not valid because the "type" is missing. Would need to switch to something like `type: Literal["Feature"]` with no default to be truly strict about the input parsing. Personally, I like having defaults since I mostly use the library to build and manipulate my own data. I may do `feature = Feature()` and then build on it from there. But I also think the fact that it will accept a list without error is wrong. I probably need to add a test to some of my other code to see if this would cause issues for API input. And I don't think there is a great way to balance the two. The dict coercion happens https://github.com/pydantic/pydantic/blob/1.10.X-fixes/pydantic/main.py#L521 and I think it mostly makes sense from their side. But apparently `dict()` on a list with only two character strings is a very interesting edge case. I am trying to think about if there is some other way to not break there. Obviously could override `parse_obj` but that would probably introduce more issues. Trying to figure out if there is a way to enforce something along the lines of "if (input is truthy) and (no overlap in keys): ValueError" So if you pass in something that is dict like, or coercible to a dict, but none of the keys are right, break. Seems like that should be an option somewhere, but haven't found anything. Would a `strict_parse_obj` function make any sense? Probably not the best idea. Seems a bit rough too. Lots of other discussion here: pydantic/pydantic#1098 but I haven't been able to read all of it yet.

[vincentsarago]

from the spec POV I think we should require type, geometry and properties

A Feature object represents a spatially bounded thing. Every Feature
object is a GeoJSON object no matter where it occurs in a GeoJSON
text.

• A Feature object HAS a "type" member with the value "Feature".
• A Feature object HAS a member with the name "geometry". The value
  of the geometry member SHALL be either a Geometry object as
  defined above or, in the case that the Feature is unlocated, a
  JSON null value.
• 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).
• If a Feature has a commonly used identifier, that identifier
  SHOULD be included as a member of the Feature object with the name
  "id", and the value of this member is either a JSON string or
  number.
• A GeoJSON object MAY have a member named "bbox" to include
  information on the coordinate range for its Geometries, Features, or
  FeatureCollections.

I think it's a difficult subject because users can use geojson-pydantic for 2 purposes:

• Validation
• Construction

For validation I think we should follow the specification to the letter: requiring all 3 type, geometry, properties. This would limit the construction usage but I can't think of a better solution.

Other

On the subject of allowing more attributes here is what the spec says:

• Members not described in this specification ("foreign members") MAY
  be used in a GeoJSON document.

For Geometries

A Geometry object represents points, curves, and surfaces in
coordinate space. Every Geometry object is a GeoJSON object no
matter where it occurs in a GeoJSON text.

• The value of a Geometry object's "type" member MUST be one of the
  seven geometry types (see Section 1.4).
• A GeoJSON Geometry object of any type other than
  "GeometryCollection" has a member with the name "coordinates".
  The value of the "coordinates" member is an array. The structure
  of the elements in this array is determined by the type of
  geometry. GeoJSON processors MAY interpret Geometry objects with
  empty "coordinates" arrays as null objects.

ref: https://datatracker.ietf.org/doc/html/rfc7946

[vincentsarago]

I've started a PR over #94

I understand this would have a lot of implication (requiring Type, Geometry and Properties) but I'm happy to discuss it more 🙏