Skip to content

propose .make method to create Features without needing to specify all parameters #174

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kopp
Copy link

@kopp kopp commented Jun 8, 2025

I would like to propos to add a make function to the pydantic models that will take the arguments but with 'sane defaults'.

Using this, some redundant information can be omitted:

feature_now = Feature(type="Feature", geometry=LineString(type="LineString", coordinates=[(0,1),(1,1)]))
feature_proposed = Feature.make(geometry=LineString.make(coordinates=[(0,1),(1,1)]))

If you think this is a fair idea, I can add makes for the other types as well.

@kopp
Copy link
Author

kopp commented Jun 8, 2025

Actually I just wanted to add a default value to the type, i.e.

class Feature(_GeoJsonBase, Generic[Geom, Props]):
    """Feature Model"""

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

but then I realized that this will stop pydantic to raise ValidationErrors when parsing a json document like {"geometry": [[1,0],[1,1]], properties: null} as valid Feature -- which we do not want.

I'm not entirely happy with the make proposal myself -- I was hoping that pydantic supports some way to annotate that

  • use the default value when constructing the BaseModel in python code
  • request a value when validating a dict/json input
    but I did not find that.

@geospatial-jeff
Copy link
Contributor

geospatial-jeff commented Jun 8, 2025

@kopp Are you sure that breaks? It's a bit hard to tell because your example is not repeatable. This is working for me:

from typing import Generic, Literal, Union, Optional

from pydantic import Field, StrictInt, StrictStr

from geojson_pydantic.base import _GeoJsonBase
from geojson_pydantic.features import Props, Geom


class Feature(_GeoJsonBase, Generic[Geom, Props]):
    """Feature Model"""

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


feat = {
    "geometry": {
        "type": "Polygon",
        "coordinates": [
            [[100.0, 0.0], [101.0, 0.0], [101.0, 1.0], [100.0, 1.0], [100.0, 0.0]]
        ],
    },
    "properties": {"prop0": "value0", "prop1": {"this": "that"}},
}

m = Feature.model_validate(feat)
assert m.type == "Feature"

We won't be able to have the type optional while still raising a validation error if it is missing. It's either required, or optionally required. Have to choose one or the other and live with it. My preference is to keep it required, as adding a type key yourself is not that hard to do. And making it optionally required is against the geojson spec (type is required).

Pydantic isn't aware if your data is coming from a JSON document or from constructing the model directly.

@geospatial-jeff
Copy link
Contributor

I'd recommend subclassing if you have a use-case that does require an optional type. But again I'd reckon it's simple enough to just always include the type.

from typing import Literal

from geojson_pydantic.features import Feature


class FeatureWithOptionalType(Feature):
    """Feature Model"""

    type: Literal["Feature"] = "Feature"


feat = {
    "geometry": {
        "type": "Polygon",
        "coordinates": [
            [[100.0, 0.0], [101.0, 0.0], [101.0, 1.0], [100.0, 1.0], [100.0, 0.0]]
        ],
    },
    "properties": {"prop0": "value0", "prop1": {"this": "that"}},
}

m = FeatureWithOptionalType.model_validate(feat)
assert m.type == "Feature"

@kopp
Copy link
Author

kopp commented Jun 8, 2025

Thanks @geospatial-jeff for the reply and sorry for not being precise in my post.

My point was, that your first example should actually fail with a ValidationError, because the type is missing. You do not want to allow a json/dict without type. (At least that is my understanding of the design choices in the library.)

Subclassing (also my first attempt) will also allow that you parse an invalid GeoJSON.

My point is, I actually want to still only allow valid GeoJSON to be read and written, but when creating python objects I do not want to repeat myself (DRY) I saying that a Feature is of type Feature. Using .make in python code is a workaround that satisfied these two requirements.

@kopp
Copy link
Author

kopp commented Jun 8, 2025

You can consider this proposal as compromise for the same issue raised in #171 .

@geospatial-jeff
Copy link
Contributor

I understand your point, and I do wish that pydantic had better behavior here. Personally I prefer the const=True syntax in pydatic 1.X over Literal in pydantic 2.X any day of the week. We'd have no issues implementing this in 1.X 😭

However I'm not sure that Feature.make fixes the issue, because you can still not get a ValidationError if the type is missing, as the make constructor defaults it for you. We are relying on the end user to know that:

  • A use case that requires type validation should NOT call .make.
  • A use case that doesn't require type validation should call .make.

While the GeoJSON spec says that type should always be present. I'll let @vincentsarago make the decision here as he is more involved in the repo and this comes down to his original point in #171 on the importance of validation vs. other features (like DRY):

We understand when building object using the model (e.g Point(coordinates= [0, 0])) it's annoying to have to pass Point(coordinates= [0, 0], type="point") but as mentioned, the validation feature is considered as more important for the library

@kopp
Copy link
Author

kopp commented Jun 9, 2025

Yes, exactly. I would even extend that -- users wanting validation should call a .model_validate method while people wanting to create a type just call .make.

If I'm not mistaken, calling .make will also validate the data type (as it goes through pydantic's constructor) -- you will still end up with a valid object.

Accessing the .type and using .model_dump will hence always generate valid GeoJSON.

@vincentsarago
Copy link
Member

Thanks for jumping into this @geospatial-jeff 🙏

To be honest this feels a bit weird to have to use .make just because user do not want to write type: "Feature" 🤷

if we were to move forward with this, users will come and ask for this to be implemented for feature collections and Geometries... at the end of the day it will add a lot to maintain.

I get the frustration of having to pass type when calling Feature(... or Polygon(... classes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants