Réparation /dashboard cassé - SendConsolidateBNLCView : pas d'atoms pour la session

[AntoineAugusti]

Corrige un bug introduit dans #3797, les clés de session dans TransportWeb.Live.SendConsolidateBNLCView ne sont pas des atoms existants.

Je passe le tout en string pour éviter le problème.

Je suis étonné que le problème ne remonte pas dans les tests par contre.

La LiveView est crée dans le controller via un live_render et on a plusieurs tests qui font une requête sur cette page 🤔. Une idée @thbar ?

mix test apps/transport/test/transport_web/controllers/backoffice/page_controller_test.exs passe en local avec l'ancien code, sans exception d'atom qui n'existe pas.

Voir exception Sentry

[thbar]

Titre modifié pour des questions de clarté (je n'étais pas sûr de l'impact concret pour nous-même).

Je vais commenter avec ce qui se passe et comment éviter de le reproduire.

[thbar]

Je note mon analyse de la situation et ma conclusion.

L'erreur levée dans /dashboard est 1st argument: not an already existing atom.

Ces erreurs sont générées par String.to_existing_atom (voir https://hexdocs.pm/elixir/1.16.2/String.html#to_existing_atom/1) avec la note suivante:

Since Elixir is a compiled language, the atoms defined in a module will only exist after said module is loaded, which typically happens whenever a function in the module is executed. Therefore, it is generally recommended to call String.to_existing_atom/1 only to convert atoms defined within the module making the function call to to_existing_atom/1.

Le comportement est différent entre la production/staging (mode :prod) et le mode :test qui est plus flexible au niveau du chargement des modules et symboles en général.

Le call à cette méthode un peu délicate a été introduit ici https://github.com/etalab/transport-site/pull/3797/files#diff-171440ea74fce9f9019831795919a905cbe3c738f3d52f8cd11ed717fbed6a44R15.

Je n'ai pas de solution simple et sans trop de complexité pour détecter actuellement (mais je vais chercher) ce genre de choses durant les tests automatisés / unitaires.

De façon générale c'est une méthode à utiliser avec prudence notamment quand on a ce qui peut s'apparenter à de l'user input. Voir https://paraxial.io/blog/atom-dos sur pourquoi ça reste une méthode utile !

Ces erreurs sont difficiles à reproduire en test automatisé actuellement (mais je verrai si je trouve une astuce), il existe deux façons de détecter ça, qui sont manuelles:

• tester sur prochainement (et globalement je gagnerais à être plus "assertif" quand je pense que c'est nécessaire, là c'était le cas) car le code est compilé en mode :prod là bas
• passer build_embedded à true dans le Mixfile (ça reproduit bien le souci avec mix phx.server après, mais ça ne fait pas échouer mix test)

J'ai pensé au début à un changement potentiel dans LiveView lui-même (mis à jour dans #3797) que j'aurais pu rater, mais en seconde lecture détaillée je n'ai rien vu sur https://diff.hex.pm/diff/phoenix_live_view/0.18.18..0.19.5 qui expliquait cela.

C'est comme ça que je suis orienté vers notre propre code, où je n'ai pas repéré cet appel comme étant problématique !

À l'avenir je pense que le mieux reste de prendre le temps de faire un test sur prochainement quand on a un doute (cf #3797 (review)), surtout si ça impacte des fonctionnalités davantage "publiques", et en attendant mieux avec d'éventuelles idées pour "attraper ça" pendant la suite de tests.

Cela étant dit (pfiou c'était long), le fix fait le taff (j'ai testé avec build_embedded: false à la main en local et mix phx.server), on peut déployer et tester !