Skip to content

Conversation

markus-work
Copy link
Contributor

What I am changing

How I did it

  • Changing the typehinting according to workaround mention here to get the model to show up in fast api swagger docs.

How you can test it

Related Issues

@vincentsarago vincentsarago requested a review from eseglem January 8, 2024 09:04

@model_serializer(when_used="always", mode="wrap")
def clean_model(self, serializer: Any, info: SerializationInfo) -> Dict[str, Any]:
def clean_model(self, serializer: Any, info: SerializationInfo) -> Any:
Copy link
Member

Choose a reason for hiding this comment

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

@markus-work what happens if we use Dict or Dict[Any, Any] instead of Any

TBH, I'm not quite sure of all the implication of this change (cc @eseglem)

I would also add a comment in the code directly about this change/issue

@eseglem
Copy link
Collaborator

eseglem commented Jan 8, 2024

I have been digging through this for a bit to get to the core of it, and it has something to do with how pydantic schema generation interacts with the @model_serializer. Essentially, FastAPI will call .model_json_schema(mode="serialization") and then pydantic will use the return type of the serializer function.

Unfortunately, it does not look like -> Any fixes the issue. At least not in my testing. Adjusting return type only works either having no return type at all, or pydantic_core.PydanticUndefined. Neither of which are ideal.

I don't particularly like it, but the cleanest solution appears to be:

    def clean_model(self, serializer: Any, info: SerializationInfo):  # type: ignore [no-untyped-def]

Simple test, which could be parametrized for all the types:

from geojson_pydantic import Point

def test_schema():
    assert Point.model_json_schema(mode="validation") == Point.model_json_schema(
        mode="serialization"
    )

I tried to work a solution with __get_pydantic_core_schema__ and it worked in a simple test, but ended up with a RecursionError when trying to implement it _GeoJsonBase. And I think it may be related to generics. Might be fixable in there somewhere, but not going to be pretty. I may try and put something together as an issue on Pydantic and see what they say, as well.

@markus-work
Copy link
Contributor Author

You're right, Any does not work. Should I change it to # type: ignore [no-untyped-def] for now?

Copy link
Collaborator

@eseglem eseglem left a comment

Choose a reason for hiding this comment

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

Seems like the best solution for the time being. Think its worth adding any tests @vincentsarago ?

@markus-work
Copy link
Contributor Author

I added the test you suggested. I added the type hinting back locally to see if the test failed, and it did. So the test is valid for this problem.

@markus-work
Copy link
Contributor Author

Should I update the changelog and run bump2version as well?

@vincentsarago
Copy link
Member

@markus-work I'll take care of the changelog 🙏

thank for the all the good work

@vincentsarago vincentsarago merged commit c70b5b1 into developmentseed:main Jan 16, 2024
@Kusmeroglu
Copy link

Just popping in here to say thank you! I just ran into this problem and ya'll just fixed it! <3

@eseglem
Copy link
Collaborator

eseglem commented Feb 14, 2024

Thanks for chiming in @Kusmeroglu. It reminded me I still wanted to write up an issue on pydantic: pydantic/pydantic#8791

If anyone wants to thumbs up it, or chime in, could help with getting a nicer (less hacky) solution.

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.

4 participants