NO TICKET: eager load refactor #277
Conversation
Because eager loading was defined in the models, it was being applied every time those models were queried, leading to unnecessary data being loaded in many cases. This change moves the eager loading logic to the service layer, applying it only at the endpoints that require it. This optimizes performance by reducing redundant data fetching and ensures that only the necessary related data is loaded when needed.
If a full ThingResponse is used in ContactResponse, it can lead to performance issues due to lazy loading of unneeded fields. To preclude this from happening, a new ThingResponseForContact schema has been created since all that is needed in this context are the id and name of the Things related to a Contact.
moving forward all eager loading will be defined at endpoints, not in the model definitions (unless they are in polymorphic models)
every observation is linked to a parameter, so eager loading of parameters when loading observations will always be needed
jacob-a-brown
requested review from
TylerAdamMartinez,
chasetmartin,
jirhiker,
kbighorse and
ksmuczynski
Codecov Report❌ Patch coverage is
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
schemas/contact.py
Outdated
| class ThingResponseForContact(BaseModel): | ||
| """ | ||
| Response schema for thing details related to a contact. All that is needed | ||
| are the id and name | ||
| """ | ||
|
|
||
| id: int | ||
| name: str | None = None |
There was a problem hiding this comment.
Switching ContactResponse.things to ThingResponseForContact introduced a model that inherits BaseModel without model_config = ConfigDict(from_attributes=True). Contact responses are built directly from SQLAlchemy Contact objects, and the things association proxy yields Thing instances; without from_attributes the nested model cannot read their attributes and will throw validation errors whenever a contact has related things (affecting GET /contact* and sample responses). The previous ThingResponse worked because it inherited BaseResponseModel with from_attributes enabled.
Useful? React with 👍 / 👎.
This task does not have a Jira ticket. @jirhiker and I discussed this refactor to improve API performance Friday, December 5.
Why
This PR addresses the following problem / context:
/geospatialand other routers when it was not neededHow
Implementation summary - the following was changed / added / removed:
selectinload. This gives us more granular control over when eager loading is utilized.thingsin theContactResponseonly need theidandnameof thething. Because theThingResponserequires a number of fields from related tables, and because eager loading is no longer the default as defined in theThingmodel, every item in thethingslist just returns theidandname. If the fullThingResponsewas used we'd run into an N+1 issue due to lazy loading.parameterremains eagerly loaded for eachobservationbecause that is a relationship that will always be loaded, used, and displayed.Notes
Any special considerations, workarounds, or follow-up work to note?
selectsince that's used elsewhere in the repo?