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

Don't include farm.json data in /farms endpoint #90

Closed
mstenta opened this issue May 22, 2020 · 1 comment
Closed

Don't include farm.json data in /farms endpoint #90

mstenta opened this issue May 22, 2020 · 1 comment
Labels
bug Something isn't working

Comments

@mstenta
Copy link
Member

mstenta commented May 22, 2020

Currently the /farms endpoint includes the farm.json data alongside every farm. This makes the responses very large, the more farms there are (eg: > 2.5 mb for > 100 farms).

Discussed with @paul121 and decided the easiest thing to do is add an include_info query parameter that must be set in order to include the farm.json data. Exclude it by default.

@mstenta mstenta added the bug Something isn't working label May 22, 2020
@paul121
Copy link
Member

paul121 commented Jun 3, 2020

@mstenta and I chatted more about this in chat. We decided that an include_info query param isn't the best API design choice. Instead, we're only including the farm.info property on requests to /farms/{farm_id}.

This means that there is no way to get the farm.info property for ALL farms in a single request. Until there is a use case for this, we're removing this functionality from the API.

For now, it was super easy to trim farm.info into a separate FastAPI response model (Pydantic schema) that is only used on the /farms/{farm_id} endpoint. I also added deferred loading to the info column on the SQLAlchemy model so that the info property is not always selected from the DB.

@mstenta originally we discussed moving farm.info to a separate DB table. I started down this path... and then realized it would be rather complicated because we're using a JSONB column. This is because the new table would still need a top-level info column, as opposed to many columns for each set of data that returned from farm.json. This means the resulting structure returned via API would be farm.info.info.{farm.json keyword} - perhaps in the future once we have a need to better-structure the farm.info properties we can migrate away from a JSONB column, and have a true farm_info table.

For reference, I did put together a data migration for this (tested once and it worked): https://gist.github.com/paul121/3f36f834725649f9abb7791933ce8d8f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

No branches or pull requests

2 participants