FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

🐛 Fix issue with double model parsing by uriyyo · Pull Request #4089 · fastapi/fastapi · GitHub

/ fastapi Public

🐛 Fix issue with double model parsing - #4089

Closed
uriyyo wants to merge 3 commits into
fastapi:masterfrom
uriyyo:bugfix/deps-caching
ConversationCommits (3)ChecksFiles changed
Closed

🐛 Fix issue with double model parsing#4089
uriyyo wants to merge 3 commits into
fastapi:masterfrom
uriyyo:bugfix/deps-caching

Conversation

uriyyo commented Oct 23, 2021

Copy link
Copy Markdown
Contributor

Fix caching issue when instead of cache lookup dependency solving was performed all the time:

Code example:

from asyncio import run

from httpx import AsyncClient
from pydantic import BaseModel, root_validator

from fastapi import FastAPI, Depends

app = FastAPI()


class Model(BaseModel):
    @root_validator
    def __root_validator__(cls, values):
        print("parsing")
        return values


def dependency(request: Model):
    return request


@app.post("/")
def foo(
        a: Model = Depends(dependency),
        b: Model = Depends(dependency),
):
    return {}


async def main():
    async with AsyncClient(app=app, base_url="http://testserver.com/") as client:
        await client.post("/", json={})


if __name__ == '__main__':
    run(main())

Expected behaviour - parsing printed only one time
Actual behaviour - parsing printed two times

codecov Bot commented Oct 23, 2021 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (cf73051) 100.00% compared to head (1c84c7b) 100.00%.
Report is 1270 commits behind head on master.

❗ Current head 1c84c7b differs from pull request most recent head 866f6f0. Consider uploading reports for the commit 866f6f0 to get more accurate results

Additional details and impacted files 
@@             Coverage Diff             @@
##            master     #4089     +/-   ##
===========================================
  Coverage   100.00%   100.00%             
===========================================
  Files          540       409    -131     
  Lines        13969     10284   -3685     
===========================================
- Hits         13969     10284   -3685

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions Bot commented Oct 23, 2021

Copy link
Copy Markdown
Contributor

📝 Docs preview for commit 1c84c7b at: https://617463f634b44503a0606ef3--fastapi.netlify.app

yezz123 approved these changes Nov 7, 2021
View reviewed changes

yezz123 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM+1 🚀, Waiting for this in the next version this one gonna help me fix a lot of issues related to my package 👌🏻
Thanks @uriyyo ✨

uriyyo commented Apr 3, 2022

Copy link
Copy Markdown
Contributor Author

@tiangolo Could you please review this PR? It would my nice to fix this issue

tiangolo added the bug Something isn't working label Jun 28, 2023
uriyyo added 2 commits February 7, 2024 11:51
Merge remote-tracking branch 'upstream/master' into bugfix/deps-caching
946f8bd 
# Conflicts:
#	fastapi/dependencies/utils.py
#	tests/test_dependency_cache.py
merge with upstream/master
866f6f0

uriyyo commented Feb 7, 2024

Copy link
Copy Markdown
Contributor Author

@tiangolo @Kludex Any chance to take a look at this PR?

YuriiMotov approved these changes Mar 4, 2024
View reviewed changes

YuriiMotov left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can confirm that:

  • the issue is still relevant to FastAPI 0.110.0
  • new added test fails before changes are made and passes after
  • solution looks nice and reasonable

Comment thread fastapi/dependencies/utils.py
Comment on lines +571 to +574
solved = dependency_cache[sub_dependant.cache_key]

if sub_dependant.name is not None:
values[sub_dependant.name] = solved

YuriiMotov Mar 4, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
solved = dependency_cache[sub_dependant.cache_key]
if sub_dependant.name is not None:
values[sub_dependant.name] = solved
if sub_dependant.name is not None:
values[sub_dependant.name] = dependency_cache[sub_dependant.cache_key]

Comment thread fastapi/dependencies/utils.py
Comment on lines +545 to +546
if dependency_cache is None:
dependency_cache = {}

YuriiMotov Mar 4, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Since we now use the same dependency_cache dict for all recursive calls, we can remove dependency_cache.update(sub_dependency_cache) line below, and get rid of sub_dependency_cache variable to make code cleaner:

(
    sub_values,
    sub_errors,
    background_tasks,
    _,  # the subdependency returns the same response we have
    sub_dependency_cache,    # <==  Change this line to "_,"
) = solved_result
dependency_cache.update(sub_dependency_cache)   # <== Remove this line

uriyyo commented Mar 4, 2024

Copy link
Copy Markdown
Contributor Author

@Kludex Any chase you can take a look?)

YuriiMotov mentioned this pull request Mar 20, 2024
Open
Jaza reviewed May 1, 2024
View reviewed changes
Comment thread tests/test_dependency_cache.py
app = FastAPI()

counter_holder = {"counter": 0}
counter_holder = {"counter": 0, "parsing_counter": 0}

Jaza May 1, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

IMHO this should be set in pytest session-level context or similar, it shouldn't just be a global variable, I feel that there's too high a risk that the test could end up running with the counter initialised to a non-zero value (e.g. if for some reason the same test runs twice in one pytest invocation, if we end up adding another test in the future that depends on the same counter, etc).

Jaza commented May 1, 2024

Copy link
Copy Markdown

Mentioning this perf improvement somewhere in the docs might also not be a bad idea?

svlandeg changed the title Fix issue with double model parsing 🐛 Fix issue with double model parsing Feb 18, 2025
svlandeg reviewed Mar 28, 2025
View reviewed changes

svlandeg left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Apologies for the late follow-up on this one.

PR #11323 has a very similar fix, and has been updated to include the unit test from this PR, and is in sync with current master. As such, I'm going to close this PR so we can hopefully wrap this up relatively quickly in #11323.

Thanks to everyone who contributed and helped review this one! If you have time, please consider having a quick look at #11323 as well. 🙏

svlandeg closed this Mar 28, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

svlandeg svlandeg left review comments
YuriiMotov YuriiMotov approved these changes
+3 more reviewers
Jaza Jaza left review comments
BilalAlpaslan BilalAlpaslan approved these changes
yezz123 yezz123 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug Something isn't working hacktoberfest-accepted

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL