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

[TECH] Améliorer le parsing et la validation des query parameters côté API. #9416

Merged
merged 19 commits into from
Jul 8, 2024

Conversation

HEYGUL
Copy link
Contributor

@HEYGUL HEYGUL commented Jul 2, 2024

🦄 Problème

Hapi permet de valider les query parameters via des schémas Joi.
Par défaut, les query parameters sont parsés en chaine de caractère.
Il est donc nécessaire de connaitre la façon dont sont encodés ceux-ci pour définir leur validation.
Par exemple, un tableau array inclus dans objet object est encodé : object[array][].

Il faut également parser les query parameters pour retrouver un objet utilisable par le restee du code.
Actuellement, chaque contrôleur doit le faire, en utilisant query-parameters-utils qui se base sur des RegExp pour parser.

🤖 Proposition

Tirer profit de server.options.query.parser de Hapi.
Cela permet de s'affranchir du fichier query-parameters-utils et utiliser la dépendance qs pour parser les query parameters.

Cette responsabilité est alors déplacée des contrôleurs et confiée directement au serveur Hapi.
Dans la définition des routes, on peut alors utiliser un schéma Joi plus juste pour définir les query parameters.
Par exemple, pour la pagination, page[size] = Joi.... devient page: { size: Joi... }

🌈 Remarques

Il y a énormément de fichiers modifiés dans cette PR.
Comme le parsing de query parameter est une option du serveur, il n'est pas possible de le configurer différemment entre 2 routes.
Il a donc fallu passer sur l'ensemble de la base de code pour modifier les définitions de query parameters et ajuster le code du contrôleur et les tests.

Ce travail a mis également en lumière que certaines routes utilisent des query parameters mais ne les spécifient pas dans leur définition.

💯 Pour tester

S'assurer que la CI est verte.
S'assurer que les différents query parameters sont correctement définis et couverts par des tests automatisés.
Faire une passe sur les différents applications pour vérifier que les filtres et la pagination sont fonctionnels.

@HEYGUL HEYGUL requested review from a team as code owners July 2, 2024 05:34
@HEYGUL HEYGUL self-assigned this Jul 2, 2024
@pix-bot-github
Copy link

Une fois les applications déployées, elles seront accessibles via les liens suivants :

Les variables d'environnement seront accessibles via les liens suivants :

@HEYGUL HEYGUL force-pushed the tech-improve-query-parsing branch from 2906be5 to 1ff1e26 Compare July 2, 2024 05:35
Copy link
Contributor

@xav-car xav-car left a comment

Choose a reason for hiding this comment

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

Tech ok

Copy link
Contributor

@aurelie-crouillebois aurelie-crouillebois left a comment

Choose a reason for hiding this comment

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

Juste une ligne à supprimer. Sinon, c'est cool !

@HEYGUL HEYGUL force-pushed the tech-improve-query-parsing branch 4 times, most recently from 59c1e14 to 4750308 Compare July 3, 2024 19:21
@@ -55,7 +54,7 @@ const findPaginatedFilteredCertificationCenters = async function (request) {
const findPaginatedSessionSummaries = async function (request) {
const certificationCenterId = request.params.id;
const userId = request.auth.credentials.userId;
const options = extractParameters(request.query);
const options = request.query;
Copy link
Member

Choose a reason for hiding this comment

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

extractParameters retiré ici mais pas de validation Joi sur la route

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cross-team Toutes les équipes de dev Func Review OK PO validated functionally the PR 🚀 Ready to Merge Tech Review OK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants