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

Support returning lists of unserialized models #335

Conversation

jean-edouard-boulanger
Copy link
Contributor

@jean-edouard-boulanger jean-edouard-boulanger commented Aug 4, 2023

Description

Currently, spectree only partially supports validation of list responses. For example:

class User(BaseModel):
    user_id: str

@app.route()
@api.validate(
    resp=Response(HTTP_200=List[User])
)
def get_users():
    return [
        User(user_id="user1").dict(),  # pre-serialization of individual entries is required, or spectree crashes
        User(user_id="user2").dict()
    ]

You'll notice in particular that individual entries must currently be serialized before returning or spectree crashes. This is inconsistent with the rest of the interface, because it's perfectly okay to return models (without serializing them) - so why not list of models?

The change basically updates individual plugins to serialize individual list entries (when needed) before the final response is created. The change is conservative and the above logic is only applied when all of the below applies:

  • Routes / responses that expect a list result.
  • Result is indeed a list.
  • Individual list entry is a BaseModel subclass.

I have not changed any of the existing validation logic in this case: we still rely on the generated root model for validation.

Change summary:

  • Update the validation logic to support returning lists of models from endpoints (for all plugins). I have made sure that returning lists of serialized models remains supported.
  • Small typing update so this Response(HTTP_200=List[User]) no longer triggers IDEs / type validation frameworks.
  • Added tests for this use case (for all plugins).

@jean-edouard-boulanger jean-edouard-boulanger force-pushed the properly-support-pydantic-model-list-return branch 2 times, most recently from f7bd362 to 98995e6 Compare August 4, 2023 08:54
@jean-edouard-boulanger jean-edouard-boulanger changed the title Draft: Fully support validation of list responses Draft: Support returning lists of unserialized models Aug 4, 2023
@jean-edouard-boulanger jean-edouard-boulanger force-pushed the properly-support-pydantic-model-list-return branch from 98995e6 to 3708dd3 Compare August 4, 2023 08:57
@jean-edouard-boulanger jean-edouard-boulanger changed the title Draft: Support returning lists of unserialized models Support returning lists of unserialized models Aug 4, 2023
spectree/plugins/falcon_plugin.py Outdated Show resolved Hide resolved
spectree/plugins/falcon_plugin.py Outdated Show resolved Hide resolved
Copy link
Member

@kemingy kemingy left a comment

Choose a reason for hiding this comment

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

Hi @jean-edouard-boulanger, if I understand it correctly, your expectation is to return a list like [User(user_id="1"), User(user_id="2")] directly. I think it's possible but will need to use some tricks.

SpecTree has a construct function called gen_list_model. I guess you already notice that during this PR. A possible way is:

UserList = gen_list_model(User)
return UserList(__root__=[User(user_id="1"), User(user_id="2")])

spectree/plugins/falcon_plugin.py Outdated Show resolved Hide resolved
spectree/plugins/falcon_plugin.py Outdated Show resolved Hide resolved
@jean-edouard-boulanger
Copy link
Contributor Author

Hi @jean-edouard-boulanger, if I understand it correctly, your expectation is to return a list like [User(user_id="1"), User(user_id="2")] directly. I think it's possible but will need to use some tricks.

Yes this is absolutely correct! I don't particularly like the alternative (using gen_list_model in the view) and I think it'd be much nicer if spectree allowed returning a list of models directly.

@jean-edouard-boulanger jean-edouard-boulanger changed the title Support returning lists of unserialized models Draft: Support returning lists of unserialized models Aug 7, 2023
@kemingy
Copy link
Member

kemingy commented Aug 8, 2023

Yes this is absolutely correct! I don't particularly like the alternative (using gen_list_model in the view) and I think it'd be much nicer if spectree allowed returning a list of models directly.

Yes, returning a list directly is more convenient.

As for the skip_validation part, if it can return both BaseModel instances and dictionaries, it might be hard to decide whether to validate it or not.

@jean-edouard-boulanger jean-edouard-boulanger force-pushed the properly-support-pydantic-model-list-return branch from e5dd80b to 2e42762 Compare August 9, 2023 06:37
@jean-edouard-boulanger jean-edouard-boulanger force-pushed the properly-support-pydantic-model-list-return branch from 2e42762 to bae8ff9 Compare August 9, 2023 06:40
@jean-edouard-boulanger
Copy link
Contributor Author

jean-edouard-boulanger commented Aug 9, 2023

With the commit I have just pushed, we're getting much closer to your suggestion:

  • BaseModel list items are automatically serialized (.dict())
  • We skip validation when all list items have the desired/specified type (except with the starlette plugin where this is not possible)
  • For all other cases, validation is delegated to Pydantic using the generated model (this is consistent with the existing logic, where Pydantic is always used to validate list results).

Couple of examples:

Example 1: all item types have the expected/specified type (User), validation is skipped

@app.route("/api/example1", methods=["GET"])
@api.validate(resp=Response(HTTP_200=List[User]))
def return_list():
    return [User(id=1), User(id=2)]

Example 2: some items are serialized, some items are not serialized, validation is delegated to Pydantic

@app.route("/api/example2", methods=["GET"])
@api.validate(resp=Response(HTTP_200=List[User]))
def return_list():
    return [User(id=1), {"id": 2}]

Example 3: all items are serialized, validation is delegated to Pydantic (existing use case)

@app.route("/api/example3", methods=["GET"])
@api.validate(resp=Response(HTTP_200=List[User]))
def return_list():
    return [{"id": 1}, {"id": 2}]

Example 4: none of the items have the expected type, validation is delegated to Pydantic (same as example 3)

@app.route("/api/example4", methods=["GET"])
@api.validate(resp=Response(HTTP_200=List[User]))
def return_list():
    return [OtherUserModel(id=1), OtherUserModel(id=2)]

@jean-edouard-boulanger jean-edouard-boulanger changed the title Draft: Support returning lists of unserialized models Support returning lists of unserialized models Aug 9, 2023
spectree/plugins/falcon_plugin.py Outdated Show resolved Hide resolved
spectree/plugins/quart_plugin.py Outdated Show resolved Hide resolved
spectree/plugins/starlette_plugin.py Show resolved Hide resolved
@jean-edouard-boulanger
Copy link
Contributor Author

@kemingy Common validation logic now implemented for both Falcon sync/async implementations (see FalconPlugin.response_validation). Tests were updated accordingly.

Copy link
Member

@kemingy kemingy left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! 🎉

@kemingy kemingy added this pull request to the merge queue Aug 16, 2023
Merged via the queue into 0b01001001:master with commit d7e19da Aug 16, 2023
8 checks passed
@jean-edouard-boulanger
Copy link
Contributor Author

Thanks for the help merging this @kemingy 🙌

Btw, I have noticed that returning Pydantic root models from views is also not supported. Would you be interested in a fix for this too?

@kemingy
Copy link
Member

kemingy commented Aug 17, 2023

Btw, I have noticed that returning Pydantic root models from views is also not supported. Would you be interested in a fix for this too?

I'll take #329. You can fix the views if you're interested. Thanks.

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.

2 participants