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

Enhance the performance and avoid the response repeat encoder #217

Closed
wants to merge 1 commit into from
Closed

Enhance the performance and avoid the response repeat encoder #217

wants to merge 1 commit into from

Conversation

bs32g1038
Copy link

@bs32g1038 bs32g1038 commented May 12, 2019

Issue

I think the pydantic can cope with the data json serialization. so we should give the data to pydantic directly and avoid repeatedly use jsonable_encoder. The performance is enhanced obviously, especially in use the response_model

# old code
def serialize_response(*, field: Field = None, response: Response) -> Any:
    encoded = jsonable_encoder(response)
    if field:
        errors = []
        value, errors_ = field.validate(encoded, {}, loc=("response",))
        if isinstance(errors_, ErrorWrapper):
            errors.append(errors_)
        elif isinstance(errors_, list):
            errors.extend(errors_)
        if errors:
            raise ValidationError(errors)
        return jsonable_encoder(value)
    else:
        return encoded
# new code
def serialize_response(*, field: Field = None, response: Response) -> Any:
        if field:
        errors = []

        # when return a pydantic model, if the data not convert to json or dict, fields cannot be ignored
        # compatible
        if isinstance(response, BaseModel):
            response = jsonable_encoder(response)

        value, errors_ = field.validate(response, {}, loc=("response",))
        if isinstance(errors_, ErrorWrapper):
            errors.append(errors_)
        elif isinstance(errors_, list):
            errors.extend(errors_)
        if errors:
            raise ValidationError(errors)
        return jsonable_encoder(value)
    else:
        return jsonable_encoder(response)

performance

when you return directly a dict or json, if the data is bigger, the response time is obviously shortened.

I'm sorry I didn't generate the performance chart

@codecov
Copy link

codecov bot commented May 12, 2019

Codecov Report

Merging #217 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #217   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files         170    170           
  Lines        4122   4121    -1     
=====================================
- Hits         4122   4121    -1
Impacted Files Coverage Δ
fastapi/routing.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f13b53...ee769a1. Read the comment docs.

@codecov
Copy link

codecov bot commented May 12, 2019

Codecov Report

Merging #217 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #217   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files         170    170           
  Lines        4122   4123    +1     
=====================================
+ Hits         4122   4123    +1
Impacted Files Coverage Δ
fastapi/routing.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f13b53...210396d. Read the comment docs.

@tiangolo
Copy link
Owner

Thanks!

The problem with this is that for it to work it would require the path operation to return a dict.

If it returned an ORM model (or similar) it wouldn't work.

But I'm currently refactoring the way that works, to use the Pydantic model's fields to extract the data from whatever object was returned. So, that should help with this. Let's see how it goes.

@tiangolo
Copy link
Owner

I'm currently waiting on pydantic/pydantic#520.

After that, integrating it should help solve this use case.

@tiangolo
Copy link
Owner

tiangolo commented Aug 7, 2019

Thanks for your effort! 🍰

This should be solved in recent versions that use the Pydantic model directly. And the docs suggest using ORM mode when decoding something different than a dict (like a DB model).

I think this should be covered now, so I'll close this, but feel free to add more comments, issues, etc. 😄

@tiangolo tiangolo closed this Aug 7, 2019
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.

None yet

2 participants