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

Fix/all recipes performance #2062

Merged

Conversation

fleshgolem
Copy link
Contributor

What type of PR is this?

  • bug

What this PR does / why we need it:

Calling /api/recipes with loadFood: false (or any other endpoint that lists recipes) would lead to an interesting case, where all the recipe ingredients are not eager loaded any more but pydantic would still try to fill the recipe_ingredient field of the response model in orm mode, i.e. sqlalchemy will generate a new lazy-loading ingredient query for every single recipe. I assume this issue gets even worse, if you use ingredient foods/units, but I have not tested this any further.
I created a new response model that explicitly has the ingredients and removed them from the normal recipe summary, so only /api/recipes with loadFood enabled should ever try to load them now and all other endpoints that list recipes should behave the correct way.

Additionally - as discussed on discord - I also moved all validation from RecipeSummary to Recipe, since they are never needed for output validation. They are still needed on Recipe, otherwise all the migration tests will fail (and I assume the migrations wont work anymore)

This hopefully should improve performance for loading recipe pages considerably, especially for slower devices that really seem to have problems here

Which issue(s) this PR fixes:

No explicit issue exists AFAIK, but it has been raised on discord a few times

Testing

Ran all automated tests and also tried listing everything on the locally running frontend

Release Notes

* Improved recipe listing performance

@hay-kot
Copy link
Collaborator

hay-kot commented Jan 29, 2023

So good. Thank you! 🚀

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