Skip to content

feat: add request_distribution_ids to Bench API response #98

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.

Sign up for GitHub

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

Merged
MisterOryon merged 1 commit into from
Jan 8, 2025
Merged

feat: add request_distribution_ids to Bench API response #98

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions app/blueprinter/bench_blueprint.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,8 @@
class BenchBlueprint < Base
# Fields
fields :name, :dimensions, :positions, :greenhouse_id

field :request_distribution_ids do |bench|
bench.request_distributions.pluck(:id)
end
Comment on lines +7 to +9
Copy link
Member

@Tao-Galasse Tao-Galasse Jan 13, 2025

Choose a reason for hiding this comment

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

C'est effectivement une bonne manière d'ajouter les IDs de nos request_distributions sur nos bench :)

En revanche, on crée ainsi une N+1 ; ce qu'il faut comprendre, c'est qu'en renvoyant une liste de benches, on va pour chacun d'entre eux exécuter une requête SQL pour récupérer les IDs des request_distribution de notre bench.

Si on renvoie 10 benches, on va donc exécuter une requête SQL pour récupérer la liste des 10 benches, et pour chacun d'entre eux, faire une requête SQL pour faire un SELECT request_distributions.id FROM ..., donc 11 requêtes en tout (d'où le terme N+1).

Pour éviter ça, dans notre controller, on pourrait déjà charger la liste des request_distributions de tous nos bench, et ainsi ne faire que 2 requêtes en tout, indépendamment de la taille de notre liste de benches :) Pour ça, on peut utiliser le includes par exemple.

En remplaçant donc la ligne benches = policy_scope(@greenhouse.benches) dans notre controller par benches = policy_scope(@greenhouse.benches).includes(:request_distributions), on corrige une N+1 🚀

Oh, et pour finir, petit sucre syntaxique sympa avec rails : à la place du bench.request_distributions.pluck(:id) ici présent, vous pouvez écrire bench.request_distribution_ids 👌

Copy link
Contributor Author

@MisterOryon MisterOryon Jan 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Merci pour ton retour. Effectivement, cela va poser problème lorsqu'il y aura beaucoup de données à gérer.
Je vais suivre tes recommandations et changer cela ce week-end :)

end
3 changes: 3 additions & 0 deletions spec/acceptance/api/v1/benches_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
let!(:bench) { greenhouse.benches.first }
let!(:id) { bench.id }

let!(:request_distribution_ids) { bench.request_distributions.pluck(:id) }

get '/api/v1/greenhouses/:greenhouse_id/benches' do
parameter :'page[number]', "The number of the desired page\n\n" \
"If used, additional information is returned in the response headers:\n" \
Expand Down Expand Up @@ -88,6 +90,7 @@
expect(response['dimensions']).to eq(dimensions)
expect(response['positions']).to eq(positions)
expect(response['greenhouse_id']).to eq(greenhouse_id)
expect(response['request_distribution_ids'].count).to eq(0)
end
end

Expand Down
Loading