RFC - Réflexions sur la validation

[Riron]

Avec cette PR, je voudrais ouvrir le débat sur la maniètre dont est faite la validation (et tout ce qui va autour) dans notre code.

Le code associé n'est pas à review, mais plutot à but illustratif. Il représente l'exploration que j'ai faite sur le BSDA en m'essayant à un début de refactor du BSDA.

L'idée ici est de réfléchir à une meilleure manière de valider nos données, et de s'assurer de manière générale qu'on a des données typées et correctement formattées dans l'ensemble de notre application. Ce n'est donc pas uniquement la validation et les schémas Yup

Voici ce que j'identifie comme pain points dans notre fonctionnement actuel (il n'y a pas d'ordre particulier, les numéros sont là pour les référencer plus tard) :

1. dans les resolvers on travaille parfois avec des input types, parfois avec des types "flatten" adaptés à la db. On devrait avoir un type unique dans le back. Celui de la db à priori, et ne jamais manipuler dans notre code métier les types graphql
2. la validation des données se fait un peu dans Yup, un peu dans les resolvers directement, un peu dans des fichiers common. Idem pour les valeurs par défaut, chaque resolver les déclare au besoin et ce n'est pas associé au schéma. On devrait être capable de tout centraliser
3. si on veut changer des données à la volée sur un input, on doit wrapper les inputs dans des appels comme ici pour les sirets. La "validation" au sens large pourrait tout à fait prendre en compte ces manipulations et réunir validation et autofill
4. les fichiers de validation Yup sont de plus en plus difficiles à maintenir, avec des .when(x.when(x.when())) imbriqués. C'est souvent difficile de comprendre vraiment quand un champ est required ou pas et quelles règles s'appliquent
5. les règles d'édition et de validation recoupent la même information (si emitter est required à la signature EMISSION, c'est qu'en fait il sera vérouillé à cette même signature EMISSION) mais se trouvent dans deux endroits différents. Et qui n'ont aucun lien entre eux

En voyez vous d'autres ? Etes-vous d'accord avec ces points ?

Il me semble que globalement chaque resolver devrait commencer par quelque chose comme ça:

const unsafeObj = flatten(input);
const obj = await schema.parseAsync(unsafeObj);

On commence toujours par appliquer flatten() sur l'input au lieu de parfois le faire tardivement. On pourrait même imaginer que ca soit fait dans une étape de preprocess, genre un middleware chargé de flatten automatiquement les inputs. C'est sans douteoverkill, mais c'est l'idée: essayons de toujours travailler avec un seul et même type en back (c'est le point 1 plus haut).

Ensuite, on ne fait plus de la validation mais du parsing. C'est le "parse don't validate" dont on entend parler de temps en temps et qui me semble important. C'est à dire que notre moulinette de validation doit avoir la capacité de modifier nos données d'entrées (result=parse(input) vs validate(input)). Concrètement, je pense qu'on devrait mettre beaucoup plus de choses dans cette moulinette pour y centraliser validation + remplissage auto + valeurs par défauts. Même pourquoi pas adaptation de l'objet vers un objet compatible Prisma.

Ex:

// J'utilise zod (https://github.com/colinhacks/zod) dans les exemples car c'est avec cette lib que j'ai expérimenté
const schema = z.schema({
  // les valeurs hors input devraient probablement apparaitre dans ce schéma
  id: z.string().default(() => getReadableId(ReadableIdPrefix.BSDA)), // on gère directement toutes les valeurs par défaut de ce genre d'objet au lieu de mettre ces lignes dans le resolver lors de la création
  isDeleted: z.boolean().default(false),
  status: z.nativeEnum(BsdaStatus).default(BsdaStatus.INITIAL),
  ...

  // valeurs dans l'input
  emitterIsPrivateIndividual: z.boolean().default(false), // Si aucune valeur n'est passée, ayons un `false` en DB plutôt qu'un `null` qui n'a pas vraiment de sens
  grouping: z.array(z.string()).nullish().transform(val => val?.length ? { connect: bsda.grouping.map(id => ({ id })) } : undefined) // On peut même faire le mapping vers le type Prisma directement. Ou alors on considère que ce n'est pas du métier et ça doit reste en dehors du schéma, dans le resolver. A réfléchir. Mais c'est possible
  ...
}).transform(val => {
    // Et on peut précompléter directement dans ce schéma tout ce qui est champ calculé
    val.intermediariesOrgIds = val.intermediaries
      ?.flatMap(intermediary => [intermediary.siret, intermediary.vatNumber])
      .filter(Boolean);

    return val;
  });

Ce schéma peut ainsi centraliser toutes les règles métiers associées au type, donner de meilleurs défauts (eg false vs null), et aller aussi loin que l'on veut dans le fait de sortir un type "Prisma compatible". Et à noter ce type de sortie est strictement typé, en prenant en compte les transformation appliquées. Ca permet je crois de résoudre les points 2 & 3.

Pour le point 4, je pense qu'il faudrait bannir l'utilisation de .when() qui est problématique en terme de lisibilité du schéma. Zod n'a volontairement pas ajouté cette méthode. De même il n'y a pas de contexte de validation directement intégré à Zod. A nous de construire le schéma avec notre contexte. Ces contraintes me semblent saines et peuvent à mon sens permettre un code plus clair et lisible.
C'est avec ces contraintes et en écrivant le schéma différemment qu'on rejoint le point 5: la colocation des règles de validation et des règles de vérouillage de signature.

Pour faire ça, on peut commencer par déclarer sur le schéma tous les fields qui peuvent être optionnels, en optionnel. Plus de requiredIf, .when(x then nullable()). Tout est nullable s'il faut pouvoir être dans un cas nullable (nullish probablement, c'est à dire soit null soit undefined).

Ensuite, sur les r_gles d42dition on peut passer de ça:

const editionRules= {
  type: "EMISSION",
  emitterIsPrivateIndividual: "EMISSION",
  emitterCompanyName: "EMISSION",
  ...
}

A ça:

const editionRules= {
  type: {sealedBy: "EMISSION", isRequired: true }, // Je suis vérouillé par signature EMISSION, à partir de cette signature je suis obligatoire
  emitterCompanySiret: { // Vérouillé par EMISSION. **Doit avoir une valeur** si ce n'est pas un particulier, et ne **doit pas avoir de valeur** si s'en est un. 
    sealedBy: "EMISSION",
    isRequired: bsda => !bsda.emitterIsPrivateIndividual,
    isForbidden: isNotRequired
  },
destinationOperationDate: { // Parfois le when n'est pas pour dire si c'est obligatoire ou non mais pour comparer avec d'autres valeurs. On remplace par une étape de `refine`, qui peut soit être ici soit directement dans le fichier schéma.
    sealedBy: "OPERATION",
    isRequired: isNotRefused,
    superRefine: (val, ctx) => {
      if (
        val.destinationReceptionDate &&
        val.destinationOperationDate &&
        val.destinationOperationDate <= val.destinationReceptionDate
      ) {
        ctx.addIssue({
          code: z.ZodIssueCode.custom,
          message: `La date d'opération doit être postérieure à la date de réception`
        });
      }
    }
  }
  ...
}

L'intérêt que je vois à ça c'est la colocation de données liées, et une maitenabilité il me semble bien meilleure. En un coup d'oeil, je sais quand mon champ est required, par quoi il est vérouillé, quand il doit être vide, et même ses relations métiers avec les autres champs.

Ces règles sont automatiquement appliquées au schéma global avec une implémentation comme ce qui a été fait dans le fichier validate.ts. L'idée est qu'on prend un schéma, des règles et un contexte de validation, on mélange tout ça et on obtient un parsing + validation adapté à notre situation.

Voilà pour l'état de ma réflexion. Je serais intéressé par vous outputs. L'idée n'est pas de se lancer dans un tel refactor tant qu'on ne s'est pas mis d'accord sur 1 l'intérêt de ce changement, et 2 la manière de le mener.

A vos commentaires / avis / jet de tomates 🍅 !

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
7 Code Smells

No Coverage information
0.0% Duplication

[GaelFerrand]

J'aime beaucoup les idées lancées ici! Je trouve les suggestions plus lisibles. Sur le sujet de la lisibilité, je dirais même que plus largement des morceaux de code (pas liés à la validation) sont pénibles à relire, surtout pour un nouveau, comme certains endroits qui utilisent à l'excès reduce, par exemple.

Autre idée qui me vient à chaud: serait-il possible, et souhataible de faire des validateurs communs back/front? Si par exemple on met une limite de 40T pour les quantités de déchets, c'est un peu dommage de devoir le faire 2 fois (et ça augmente les risques d'erreur). Le fait d'avoir Yup des 2 bords aurait pu permettre ça, mais ça n'a pas été fait je crois. Peut-être pour une bonne raison?

[Riron]

Autre idée qui me vient à chaud: serait-il possible, et souhataible de faire des validateurs communs back/front? Si par exemple on met une limite de 40T pour les quantités de déchets, c'est un peu dommage de devoir le faire 2 fois (et ça augmente les risques d'erreur). Le fait d'avoir Yup des 2 bords aurait pu permettre ça, mais ça n'a pas été fait je crois. Peut-être pour une bonne raison?

Ca serait bien ! Mais en fait ce n'est pas si évident.

• la grosse diff déjà c'est qu'on a un modèle nested en front, et à plat en back
• et il est possible que certains checks soient souhaitables en back mais pas en front. Et du coup si 100% n'est pas partagé ça complique encore un peu

Mais si on pouvait l'avoir ça serait top. Si on avait une solution qui prenne en compte toutes les contraintes back et nous donne ça en bonus ca serait génial. A garder en tête lors de nos discussions donc.

[elishowk]

Merci !

L'intérêt que je vois à ça c'est la colocation de données liées, et une maintenabilité il me semble bien meilleure. En un coup d’œil, je sais quand mon champ est required, par quoi il est verrouillé, quand il doit être vide, et même ses relations métiers avec les autres champs.

Sur ce principe, je suis 100% pour.
À voir comment on va réussir à regrouper en pratique les validations conditionnelles complexes qu'on a souvent codé dans la logique métier de resolvers compliqués.

Si ça peut aider, j'aime bien l'approche par classes d'input de Nest https://docs.nestjs.com/techniques/validation#auto-validation