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

Compare objects MRO with encoders at runtime #5718

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

Conversation

ramnes
Copy link

@ramnes ramnes commented Nov 30, 2022

The current implementation doesn't handle subclass instances when fastapi.encoders.ENCODERS_BY_TYPE is modified after fastapi.encoders import.

For example, in this example we could expect the / route to return an ObjectId string. But currently, it returns {} instead:

import bson
import uvicorn
from fastapi import FastAPI
from fastapi.encoders import ENCODERS_BY_TYPE


ENCODERS_BY_TYPE |= {bson.ObjectId: str}

app = FastAPI()


class CustomObjectId(bson.ObjectId):
    pass


@app.get("/")
def get_child():
    return CustomObjectId()


uvicorn.run(app)

This PR resolves this (and simplifies the code, as well).

Continuation of #755 and #756.

cc @RmStorm

@github-actions
Copy link
Contributor

📝 Docs preview for commit 1eecf8d at: https://63879599e9e83c30ae90119c--fastapi.netlify.app

@ramnes
Copy link
Author

ramnes commented Dec 7, 2022

@tiangolo While it wasn't my first purpose, it also seems to make jsonable_encoder ~40% faster on the benchmark you used in #756:

(.venv) $ # without #5718
(.venv) $ pip install -q -I https://github.com/tiangolo/fastapi/archive/83bcdc40d853e50752d4d4a30bbc8b23a8dbc4c1.zip
(.venv) $ hyperfine --warmup=3 'python performance.py'
Benchmark 1: python performance.py
  Time (mean ± σ):     27.843 s ±  0.308 s    [User: 27.651 s, System: 0.162 s]
  Range (min … max):   27.555 s … 28.495 s    10 runs

(.venv) $ # with #5718
(.venv) $ pip install -q -I https://github.com/Refty/fastapi/archive/1eecf8dda26e23c7f3b6dcd30f9a27171f5ae217.zip
(.venv) $ hyperfine --warmup=3 'python performance.py'
Benchmark 1: python performance.py
  Time (mean ± σ):     16.105 s ±  0.247 s    [User: 15.981 s, System: 0.110 s]
  Range (min … max):   15.907 s … 16.663 s    10 runs

If you want to replicate, I just changed the ranges to make it work better with hyperfine:

import time
import uuid
from fastapi.encoders import jsonable_encoder


class SubStr(str):
    pass


class MyUuid:
    def __init__(self, uuid_string: str):
        self.uuid = uuid_string

    def __str__(self):
        return self.uuid

    @property
    def __class__(self):
        return uuid.UUID

    @property
    def __dict__(self):
        """Spoof a missing __dict__ by raising TypeError, this is how
        asyncpg.pgroto.pgproto.UUID behaves"""
        raise TypeError("vars() argument must have __dict__ attribute")


test_input_3 = [MyUuid(str(uuid.uuid4())) for _ in range(1_000)]

for _ in range(10_000):
    jsonable_encoder(test_input_3)

Copy link

@PraveenNanda124 PraveenNanda124 left a comment

Choose a reason for hiding this comment

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

Good work

@github-actions
Copy link
Contributor

github-actions bot commented Jan 7, 2023

📝 Docs preview for commit bdc4ac5 at: https://63b9e627bed8f476843378f2--fastapi.netlify.app

@ramnes
Copy link
Author

ramnes commented Feb 21, 2023

I just updated the first message with a code example to make the problem clearer.

@tiangolo Any hint on the direction that this PR should take?

@ramnes
Copy link
Author

ramnes commented Feb 21, 2023

Copy link

@dkatzan dkatzan left a comment

Choose a reason for hiding this comment

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

LGTM
I was facing a similar issue, and wondered why did fastapi took a different approach on this from pydantic
consider using the snippet u added to the PR as a base for a test for this fix

@ramnes
Copy link
Author

ramnes commented Feb 24, 2023

Thanks for the feedback @dkatzan! Yeah, I can definitely add a test or anything else that would help; I'd prefer a confirmation that it's the correct approach before putting more work into that PR though, especially since @tiangolo wrote most of the code I'm removing. 😅

@ramnes
Copy link
Author

ramnes commented Jul 12, 2023

rebased

@ramnes
Copy link
Author

ramnes commented Sep 26, 2023

rebased

@tiangolo tiangolo added the feature New feature or request label Oct 2, 2023
@ramnes
Copy link
Author

ramnes commented Jan 22, 2024

rebased

The previous implementation doesn't handle subclass instances when
pydantic.json.ENCODERS_BY_TYPE is modified after fastapi.encoders import.

This diff makes it easier for developers to add custom encoders that also work
with subclass instances (and it simplifies the code, as well).
@ramnes
Copy link
Author

ramnes commented Apr 3, 2024

rebased

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

Successfully merging this pull request may close these issues.

None yet

4 participants