🛠️ Zones | Add LAST_ZONE and Remove N+1 for zones queries #11017

thibpoullain · 2024-11-05T14:18:27Z

Bonjour !

J'ai dû résoudre des problématiques liées aux zones sur démarches sociales, et afin de réduire le delta de codebase je me demandais si ce que j'ai fait pouvait vous intéresser.

Nous avons une zone "Autre" que nous souhations mettre à la fin de la liste dans app/views/administrateurs/procedures/zones.html.haml

J'ai donc modifié le model Zones pour définir dans la codebase la zone à mettre à la fin, et j'ai modifié le controlleur pour faire le travail de tri.

J'ai amélioré le test du controlleur pour consolider le tri.

J'en ai profité pour résoudre un soucis de N+1 générées par la méthode self.available_at qui sortait :

ZoneLabel Load (8.4ms)  SELECT "zone_labels".* FROM "zone_labels" WHERE "zone_labels"."zone_id" = $1 AND (designated_on < '2024-11-05 08:56:14.401049') ORDER BY "zone_labels"."designated_on" DESC LIMIT $2  [["zone_id", 1], ["LIMIT", 1]]
  ↳ app/models/zone.rb:22:in `label_at'
  ZoneLabel Load (1.6ms)  SELECT "zone_labels".* FROM "zone_labels" WHERE "zone_labels"."zone_id" = $1 AND (designated_on < '2024-11-05 08:56:14.401049') ORDER BY "zone_labels"."designated_on" DESC LIMIT $2  [["zone_id", 2], ["LIMIT", 1]]
  ↳ app/models/zone.rb:22:in `label_at'
  ZoneLabel Load (1.3ms)  SELECT "zone_labels".* FROM "zone_labels" WHERE "zone_labels"."zone_id" = $1 AND (designated_on < '2024-11-05 08:56:14.401049') ORDER BY "zone_labels"."designated_on" DESC LIMIT $2  [["zone_id", 3], ["LIMIT", 1]]
  ↳ app/models/zone.rb:22:in `label_at'
  ZoneLabel Load (1.6ms)  SELECT "zone_labels".* FROM "zone_labels" WHERE "zone_labels"."zone_id" = $1 AND (designated_on < '2024-11-05 08:56:14.401049') ORDER BY "zone_labels"."designated_on" DESC LIMIT $2  [["zone_id", 4], ["LIMIT", 1]]
  ↳ app/models/zone.rb:22:in `label_at'
  ZoneLabel Load (1.6ms)  SELECT "zone_labels".* FROM "zone_labels" WHERE "zone_labels"."zone_id" = $1 AND (designated_on < '2024-11-05 08:56:14.401049') ORDER BY "zone_labels"."designated_on" DESC LIMIT $2  [["zone_id", 5], ["LIMIT", 1]]
  ↳ app/models/zone.rb:22:in `label_at'
  ZoneLabel Load (1.3ms)  SELECT "zone_labels".* FROM "zone_labels" WHERE "zone_labels"."zone_id" = $1 AND (designated_on < '2024-11-05 08:56:14.401049') ORDER BY "zone_labels"."designated_on" DESC LIMIT $2  [["zone_id", 6], ["LIMIT", 1]]
  ↳ app/models/zone.rb:22:in `label_at'
  ZoneLabel Load (1.0ms)  SELECT "zone_labels".* FROM "zone_labels" WHERE "zone_labels"."zone_id" = $1 AND (designated_on < '2024-11-05 08:56:14.401049') ORDER BY "zone_labels"."designated_on" DESC LIMIT $2  [["zone_id", 7], ["LIMIT", 1]]
  ↳ app/models/zone.rb:22:in `label_at'
  ZoneLabel Load (1.0ms)  SELECT "zone_labels".* FROM "zone_labels" WHERE "zone_labels"."zone_id" = $1 AND (designated_on < '2024-11-05 08:56:14.401049') ORDER BY "zone_labels"."designated_on" DESC LIMIT $2  [["zone_id", 8], ["LIMIT", 1]]
  ↳ app/models/zone.rb:22:in `label_at'
  ZoneLabel Load (1.1ms)  SELECT "zone_labels".* FROM "zone_labels" WHERE "zone_labels"."zone_id" = $1 AND (designated_on < '2024-11-05 08:56:14.401049') ORDER BY "zone_labels"."designated_on" DESC LIMIT $2  [["zone_id", 9], ["LIMIT", 1]]
  ↳ app/models/zone.rb:22:in `label_at'
...

Merci à vous ! N'hésitez pas à me faire un retour ! :)

codecov · 2024-11-05T14:26:15Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.49%. Comparing base (d61078d) to head (3a15a7f).
Report is 103 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #11017   +/-   ##
=======================================
  Coverage   84.48%   84.49%           
=======================================
  Files        1164     1163    -1     
  Lines       25691    25747   +56     
  Branches     4862     4875   +13     
=======================================
+ Hits        21705    21754   +49     
- Misses       3986     3993    +7

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

tchak · 2024-11-05T16:19:38Z

app/models/zone.rb

+    available_zones = zones.filter { |zone| zone_labels[zone.id] != 'Non attribué' }
+
+    available_zones.map do |zone|
+      OpenStruct.new(id: zone.id, label: zone_labels[zone.id])


Suggested change

OpenStruct.new(id: zone.id, label: zone_labels[zone.id])

LabelModel.new(id: zone.id, label: zone_labels[zone.id])

LeSim

voici qq pistes d'amélioration.

Faut que je vois avec tout le monde si l'inscription de la variable autre est possible alors que nous ne l'utilisons pas.

LeSim · 2024-11-05T14:56:18Z

app/models/zone.rb

@@ -5,23 +5,32 @@ class Zone < ApplicationRecord
  has_many :labels, -> { order(designated_on: :desc) }, class_name: 'ZoneLabel', inverse_of: :zone
  has_and_belongs_to_many :procedures, -> { order(published_at: :desc) }, inverse_of: :zone

+  LAST_ZONE = 'Autre'.freeze


Suggested change

LAST_ZONE = 'Autre'.freeze

OTHER_ZONE = 'Autre'

(pas besoin du frozen string avec le magic frozen_string_literal: true en ligne 1)

Oui c'est une partie que je voulais voir avec vous, car effectivement nous avons des zones différentes entre les différents forks, l'intention était de laisser la possibilité de choisir le nom de la zone "Autre" qui se placerait à la fin.

J'ai proposé ça mais s'il y a une idée plus générique qui peut convenir à tous clairement let's go !

LeSim · 2024-11-05T14:57:42Z

app/controllers/administrateurs/procedures_controller.rb

+        .partition { |zone| zone.label == Zone::LAST_ZONE }
+        .then { |last_zone, other_zones| other_zones + last_zone }


Suggested change

.partition { |zone| zone.label == Zone::LAST_ZONE }

.then { |last_zone, other_zones| other_zones + last_zone }

.partition { |zone| zone.label == Zone::OTHER_ZONE }

.then { |other_zone, zones| zones + other_zone }

LeSim · 2024-11-05T20:11:04Z

spec/controllers/administrateurs/procedures_controller_spec.rb

+    it 'assigns @zones with the correct order' do
+      subject
+      assigned_labels = assigns(:zones).map(&:label)
+      expect(assigned_labels).to eq(['Zone 1', 'Zone 2', 'Autre'])


j'ai l'impression que ce test comprends les tests 508 et 514 non ?

LeSim · 2024-11-05T20:44:38Z

app/models/zone.rb

+    zones = Zone.includes(:labels).where.not(id: without_zones).to_a
+
+    zone_labels = zones.each_with_object({}) do |zone, labels|
+      label = zone.labels.filter { |l| l.designated_on < date }.max_by(&:designated_on)
+      labels[zone.id] = label&.name || ''
+    end
+
+    available_zones = zones.filter { |zone| zone_labels[zone.id] != 'Non attribué' }
+
+    available_zones.map do |zone|


Alors, je l'ai pas fait tournée mais voici ma proposition :

d'abords on peut passer label_at(date) en privée.
ensuite, on peut la redéfinir pour qu'elle ne fasse plus d'AR

def label_at(date) labels .sort { _1.designated_on }.reverse .filter { _1.designated_on < date}.first&.name || labels.last&.name end

puis simplifier ce code

Suggested change

zones = Zone.includes(:labels).where.not(id: without_zones).to_a

zone_labels = zones.each_with_object({}) do |zone, labels|

label = zone.labels.filter { |l| l.designated_on < date }.max_by(&:designated_on)

labels[zone.id] = label&.name || ''

end

available_zones = zones.filter { |zone| zone_labels[zone.id] != 'Non attribué' }

available_zones.map do |zone|

Zone.includes(:labels).where.not(id: without_zones)

.filter { _1available_at?(date) }

.map do |zone|

WDYT ?

Bien vu pour éviter les allers retours ! Du coup j'ai regardé plus attentivement avec rack profiler !

Pour éviter les aller retours j'ai trouvé ceci, t'en pense quoi ? On se passe du label_at totalement du coup.

On passe de 58 requetes sql à 6 !

def self.available_at(date, without_zones = []) # Charger toutes les zones qui ne sont pas dans without_zones zones = Zone.where.not(id: without_zones).order(:acronym).to_a # Charger tous les labels pertinents (ceux désignés avant la date donnée) en une seule requête labels_by_zone = ZoneLabel .where('designated_on <= ?', date) .where(zone_id: zones.map(&:id)) .order(designated_on: :desc) .group_by(&:zone_id) # Préparer la liste des zones avec leurs labels zones.map do |zone| label = labels_by_zone[zone.id]&.first&.name next if label.nil? || label == 'Non attribué' # Exclure les zones sans label ou avec "Non attribué" LabelModel.new(id: zone.id, label: label) end.compact end

Ce qui nous permet de ne pas toucher à la méthode "label_at". Car là en l'état c'est une requete sql donc c'est pas mal.

enchainer les manip ça me parait un peu complexe non ?
faire un .sort -> O(n log n)
puis un .reverse -> O(n)
puis un .filter -> O(n)
ça nous fait enchainer du lourd niveau ressource je pense.

Une petite requête SQL avec le where et hop ! Qu'en penses tu ?

LeSim

plusieurs choses:

le code me parait bon, j'approuve
les arguments de perfs m’embête un peu ici : c'est une page relativement peu usité et le nb de query était acceptable. Je pense que ce qui prime ici est la lisibilité du code. Si tu veux du challenge, regarde ici : https://oss.skylight.io/app/applications/zAvWTaqO0mu1/recent/6h/endpoints
des 0(n) pour des centaines d'entrées, c'est non mesurable.

finalement, on a codecov qui rale un peu au niveau de la couverture de test. t'es d'accord avec moi qu'il hallucine ?

LeSim · 2024-11-13T10:36:51Z

spec/views/administrateurs/procedures/zones.html.haml_spec.rb

-  let(:administrateur) { administrateurs(:default_admin) }
-  let(:procedure) { create(:procedure) }
-  let(:populate_zones_task) { Rake::Task['after_party:populate_zones_with_tchap_hs'] }
+require 'rails_helper'


je crois qu'on load systématiquement le rails_helper

Suggested change

require 'rails_helper'

LeSim · 2024-11-13T10:41:20Z

app/models/zone.rb

  def current_label
    labels.where.not(name: 'Non attribué').first.name
  end

  def label_at(date)
-    label = labels.where('designated_on < ?', date)&.first || labels.last
-    label.name
+    labels.where('designated_on < ?', date).first&.name || labels.last&.name
  end

  def available_at?(date)


pourrais tu supprimer cette méthode qui n'est pas utilisé dans un autre commit ?

tchak reviewed Nov 5, 2024

View reviewed changes

thibpoullain force-pushed the zones_improvement branch 2 times, most recently from 77ea7c1 to 19fb7de Compare November 5, 2024 18:38

LeSim reviewed Nov 5, 2024

View reviewed changes

🛠️ Zones | Add LAST_ZONE and Remove N+1 for zones queries

3a15a7f

thibpoullain force-pushed the zones_improvement branch from 19fb7de to 3a15a7f Compare November 6, 2024 10:14

LeSim approved these changes Nov 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🛠️ Zones | Add LAST_ZONE and Remove N+1 for zones queries #11017

🛠️ Zones | Add LAST_ZONE and Remove N+1 for zones queries #11017

thibpoullain commented Nov 5, 2024 •

edited

Loading

codecov bot commented Nov 5, 2024 •

edited

Loading

tchak Nov 5, 2024

LeSim left a comment

LeSim Nov 5, 2024 •

edited

Loading

thibpoullain Nov 6, 2024

LeSim Nov 5, 2024

LeSim Nov 5, 2024

LeSim Nov 5, 2024

thibpoullain Nov 6, 2024

LeSim left a comment

LeSim Nov 13, 2024

LeSim Nov 13, 2024

	OpenStruct.new(id: zone.id, label: zone_labels[zone.id])
	LabelModel.new(id: zone.id, label: zone_labels[zone.id])

		.partition { \|zone\| zone.label == Zone::LAST_ZONE }
		.then { \|last_zone, other_zones\| other_zones + last_zone }

🛠️ Zones | Add LAST_ZONE and Remove N+1 for zones queries #11017

Are you sure you want to change the base?

🛠️ Zones | Add LAST_ZONE and Remove N+1 for zones queries #11017

Conversation

thibpoullain commented Nov 5, 2024 • edited Loading

codecov bot commented Nov 5, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

LeSim left a comment

Choose a reason for hiding this comment

LeSim Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeSim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thibpoullain commented Nov 5, 2024 •

edited

Loading

codecov bot commented Nov 5, 2024 •

edited

Loading

LeSim Nov 5, 2024 •

edited

Loading