CaenCamp / jobs-caen-camp

Gestion d'offres d'emploi pour les CaenCamp
https://www.caen.camp
GNU General Public License v3.0
9 stars 5 forks source link

Refonte de la pagination de l'API #55

Closed alexisjanvier closed 4 years ago

alexisjanvier commented 4 years ago

Une mise à jour récente de React-Admin a fait apparaitre un problème au niveau de l'implémentation actuelle de la pagination des listes de l'API.

Pour rappel, voici comment est déclarée la pagination de la contrat OpenAPI :

// dans jobs-caen-camp/apps/api/openapi/openapi.yaml, ligne 1072
// https://github.com/CaenCamp/jobs-caen-camp/blob/master/apps/api/openapi/openapi.yaml#L1072

Pagination:
    name: pagination
    in: query
    description: "Les paramètres pour la pagination. C'est un tableau stringifié de la forme [perPage, currentPage]"
    required: false
    explode: false
    schema:
    type: array
    maxItems: 2
    minItems: 2
    items:
        type: string

Soit un tableau de type [perPage, currentPage]. Alors tout d'abord, l'utilisation d'un tableau à deux valeurs n'est pas forcement très parlant. On se pose un peu toujours le question de savoir à quoi correspond le premier et le second, ces paramètres n'étant pas nommés. Mais un second problème provient du fait que c'est un tableau qui est déclaré dans OpenAPI, et non un string. Hors, voici ce que faisait jusqu'à présent le dataProvider de react-admin

// https://github.com/CaenCamp/jobs-caen-camp/blob/master/apps/admin/src/jobBoardDataProvider.js#L24
const query = {
    sort: JSON.stringify([field, order]),
    filters: JSON.stringify(params.filter),
    pagination: JSON.stringify([perPage, page])
};

C'est à dire que l'on transforme le tableau en string. C'est d'ailleurs ce qui est implémenter dans tous les tests, par exemple :

// https://github.com/CaenCamp/jobs-caen-camp/blob/master/tests-e2e/api/job-postings.spec.js#L139
await frisby
    .get(
        `http://api:3001/api/job-postings?pagination=${JSON.stringify(
            [2, 1]
        )}`
    )
    ...

Or depuis la mise à jours de react-admin, voici ce que retourne le serveur :

RequestValidationError: Schema validation error (/pagination: minItems should NOT have fewer than 2 items)

Le premier patch envisagé à consister à ne plus stringifier pagination depuis la dataProvider. Cela a effectivement réglé le problème de l'erreur, mais du coup, la pagination ne fonctionnait plus. Ce qui est logique, puisque si l'on regarde ce que fait l'API :

// https://github.com/CaenCamp/jobs-caen-camp/blob/master/apps/api/src/job-posting/router.js#L20
const { jobPostings, contentRange } = await getJobPostingPaginatedList({
    client: ctx.db,
    filters: parseJsonQueryParameter(ctx.query.filters),
    sort: parseJsonQueryParameter(ctx.query.sort),
    pagination: parseJsonQueryParameter(ctx.query.pagination),
});

// https://github.com/CaenCamp/jobs-caen-camp/blob/f474868ba4d81456c7a8a97ce256cd27f3acbc51/apps/api/src/toolbox/sanitizers.js#L96
const parseJsonQueryParameter = parameter => {
    if (parameter === undefined) {
        return false;
    }
    try {
        return JSON.parse(parameter);
    } catch (e) {
        return false;
    }
};

C'est à dire que l'on s'attend au niveau de l'API à avoir une pagination stringifiée, qu'on essaye de la parser, et que l'on renvoi false si on a une erreur. C'est ce qui se passe si l'on ne fait plus pagination: parseJsonQueryParameter(ctx.query.pagination) dans le dataProvider.

Du coup, le seconde solution de résolution de problème a été de modifier le contrat OpenAPI en déclarant une string pour la pagination plutôt qu'un array (comme pour les filters, ce qui est un autre problème, voir l'issue #). Si cela resolvait la problème côté react-admin, cela a provoqué beaucoup d'echec dans les tests déjà implémenté ...

Et c'est là qu'on peut commencer à se dire sérieusement : "cette manière de gérer la pagination est-elle vraiment la bonne ?"

En se renseignant un peu sur les Best Practices en la matière, il semblerait bien que non ^^

Voici donc la nouvelle implémentation proposée pour la pagination :

Utilisation des paramètres de requête page et perPage

Ce qui consiste simplement à renvoyer les paramètres qui étaient jusqu'à présent envoyés dans le tableau pagination en deux paramètre explicitement nommés.

Ce qui transformerait une ancienne requête /api/organizations?pagination=%22%5B10%2C1%5D%22 en api/organizations?page=1&perPage=10

On pourrait discuter de méthode plus efficiente de pagination (Fast pagination on PostgreSQL ou Five ways to paginate in Postgres, from the basic to the exotic), mais pour autant, et vu nos besoins et nos volumes de données les plus optimistes, il semble que page et perPage soit très compréhensibles et ils conviennent parfaitement à l'implémentation de la pagination déjà en place.

Suppression du Content-Range comme support d'information sur la pagination

Jusqu'à présent, nous utilisons le header http Content-Range pour renvoyer les informations concernant la pagination (information sur la page en cours, le nombre de résultats retournés et le nombre total de page). Cela nous permet entre autre de ne pas utiliser un formatage du retour d'API qui inclurait les informations de pagination (problématique de HATEOAS)

// https://github.com/CaenCamp/jobs-caen-camp/blob/f474868ba4d81456c7a8a97ce256cd27f3acbc51/apps/api/src/toolbox/sanitizers.js#L85

/**
 * Transforms the Knex paging object into a string compatible with the "content-Range" header.
 * https://developer.mozilla.org/fr/docs/Web/HTTP/Headers/Content-Range
 *
 * @param {string} objectType - type of object returned in the paginated collection
 * @param {object} pagination - Knex pagination object (https://github.com/felixmosh/knex-paginate#pagination-object)
 * @returns {string} string ready to be set has "Content-Range" http header
 * @example Content-Range: posts 0-24/319
 */
const formatPaginationContentRange = (objectType, pagination) =>
`${objectType.toLowerCase()} ${pagination.from}-${pagination.to}/${
    pagination.total
}`;

Or, si on lit bien la spécification de Content-Range on se rend vite compte que l'utilisation de ce header pour la pagination n'est pas la bonne manière de faire, car ce header a une autre signification.

The Content-Range response HTTP header indicates where in a full body message a partial message belongs.

Ce qui correspond donc à un retour http de type 206 Partial Content.

Ajout d'un header http X-Total-Count

Donc, pour ne pas avoir à reformater le corps de notre réponse pour y inclure la pagination, mais retourner un paramètre indispensable à la pagination, c'est à dire le nombre total de résultats possible, nous allons implémenter une header http spécifique : le X-Total-Count. C'est un header certes spécifique, mais assez largement utilisé.

Ajout d'une balise Link dans le header concernant la pagination

Toujours pour ne pas à avoir à implémenter une structure particulière au body de la réponse de l'API, nous allons rajouter une ou des balises Link dans l'en-tête de la réponse afin de respecter les bonnes pratiques. L'utilisation de cette balise est décrite dans ce post Link header introduced by RFC 5988 et prendrait la forme :

Link: <https://job.caen.camp/api/organization?page=2&perPage=10>; rel="next", <https://job.caen.camp/api/organization?page=6&perPage=10>; rel="last"

On pourra s'inspirer de l'implémentation de Github.

Todo list

Bref, c'est une très bonne occasion de rentrer dans le code du projet !

gaelreyrol commented 4 years ago

Allez ! Je m'y colle !