[TECH] Montée de version des dépendances de Pix API #1433

jbuget · 2020-05-17T09:29:37Z

🦄 Problème

Il y a longtemps que nous n'avons pas fait une passe pour monter de version les dépendances du projet Pix API.

Au lancement de Pix, les montées de version des dépendances étaient multi-hebdomadaires.

Il est important de mettre à jour régulièrement les dépendances pour les raisons courantes suivantes :

correction de failles de sécurité ou renforcement de mesures existantes
correction de bugs et amélioration de la stabilité
gain de performances
ajout ou enrichissement de fonctionnalités
amélioration ou standardisation des API, signatures ou mécanismes proposés
modernisation des façons de faire qui permettent de découvrir, apprendre et maîtriser des concepts, techniques ou pratiques au-delà du projet (donc connaissances réutilisables sur d’autres contextes / projets / technos)
amélioration de la navigation et exploitation de la documentation (plutôt qu’essayer des options qui finalement ne correspondent pas à la bonne version)
et de façon générale, sentiment d’hygiène et de confiance en l’équipe qui maintient le projet

Les risques (qui sont le pendant des gains ci-dessus) :

incompatibilité des plugins (notamment entre eux)
bugs ou régression
manque de doc à jour
montée en compétence quand nouveaux concepts (ex : syntaxe AngleBracket)
efforts d’adaptation (c’est pour ça qu’il vaut mieux le faire très souvent)

🤖 Solution

Toutes les dépendances de Pix API ont été montées (patch, minor ou major) sauf celles trop problématiques :

bookshelf :
- passage de v0.15.x à 1.1.x
- la mise en conformité couvre une dizaine de fichiers
- c'est pourquoi une PR dédiée ([TECH] Montée de version de Bookshelf #1434) a été initiée
pg :
- tentative de passage de 7.x à 8.x
- la nouvelle version renforce la sécurité par défaut
- ce qui empêche le déploiement sur Scalingo (du fait de l'auto-génération de certificats SSL par Scalingo)
- on a déjà prisla main sur l'option knex.Client ssl qui est variabilisée via DATABASE_SSL_ENABLED sauf que l'ancienne option était un booléen quand maintenant il faut faut passer un objet JSON
xml-crypto :
- j'étais surpris de voir une version minor bloquée (via ~1.0.x plutôt que ^1.y.z)
- la version minor introduit une problème
- du fait de cette particularité, je n'ai pas cherché plus loin et j'ai laissé la lib en l'état

🌈 Remarques

J'ai monté chaque lib une-à-une, en jouant bien tous les tests d'API à chaque fois, et en relançant l'application from scratch (avec suppression des conteneurs Docker-Compose) pour certaines libs que j'ai estimées plus sensibles :

hapi
knex
redis
redlock
pg (qui fonctionnait bien en local, avant de péter Scalingo)
xlsx : avec test d'export CSV

J'ai vérifié qu'il n'y avait pas plus de messages de warning côté console navigateur pour les apps Ember.

J'ai aussi vérifié la connexion tout ça.

Pour la montée de hapi, j'ai rencontré les points suivants :

le code HTTP par défaut remonté par hapi n'est plus 200 mais 204
par défaut, les requêtes de type form-multipart ne sont plus autorisées

💯 Pour tester

Pas le choix. Il faut faire une repasse globale.

pix-service · 2020-05-17T09:45:04Z

I'm deploying this PR to these urls:

App: https://app-pr1433.review.pix.fr
Orga: https://orga-pr1433.review.pix.fr
Certif: https://certif-pr1433.review.pix.fr
Admin: https://admin-pr1433.review.pix.fr
API: https://api-pr1433.review.pix.fr/api/

Please check it out!

.../integration/application/organization-invitations/organization-invitation-controller_test.js

api/tests/unit/application/session/index_test.js

aceol

OK a part le nom du test. Ca fait beaucoup de MAJ en tout cas!

laura-bergoens

Good to know -> knexjs n'utilise désormais plus les promesses Bluebird 👀

jbuget · 2020-05-19T07:45:57Z

Good to know -> knexjs n'utilise désormais plus les promesses Bluebird 👀

Ca va modifier l'erreur Sentry sur Knex.

jonathanperret · 2020-05-19T16:54:21Z

api/tests/integration/application/authentication/index_test.js

      // when
      const response = await server.inject(options);

      // then
-      expect(response.statusCode).to.equal(200);
+      expect(response.statusCode).to.equal(204);


Mais euh… ? Il y a un corps de réponse à POST /api/token, c'est faux de renvoyer 204 ? 🤯

Bien vu. J'ai un peu corrigé les tests pour qu'ils correspondent mieux à la réalité, mais je pense qu'il faudrait aller beaucoup plus loin , quite à revoir les tests de POST /api/token complètement, les centraliser, uniformiser et surtout renforcer.

Ben déjà les tests qui regarde le statusCode mais pas le contenu qui va avec je les trouve pénibles à comprendre. En l'occurrence il semblerait que le code renvoyait une réponse vide (ce qui déclenchait un 204 dans le nouvel Hapi) mais je ne comprends pas pourquoi 🤯

jonathanperret · 2020-05-19T17:53:14Z

le code HTTP par défaut remonté par hapi n'est plus 200 mais 204

Le lien est cassé. Est-ce que tu fais référence à ce changement : hapijs/hapi#3919 ?

Si oui je m'attendrais à ce que cette ligne, ajoutée pour obtenir le comportement qui est maintenant par défaut, soit enlevée du coup :

pix/api/server.js

Line 22 in df1d922

emptyStatusCode: 204

Le fait que des tests soient cassés par ce changement semble indiquer qu'ils n'utilisent pas la même configuration Hapi que le serveur de production…

api/package.json

This hapi version contains breaking changes, cf. https://hapi.dev/resources/changelog. We are impacted by 2 of them that will be fixed in next commits : - default empty reponse status code is now 204 (instead of 200) - enforce media type checking

… reponse See hapijs/hapi#4017 section "Change default empty status code to 204"

See hapijs/hapi#4017 section "Change route options.payload.multipart to false by default"

Concerned dependencies: - sentry - swagger

…yment

The endpoint `POST /api/token` must not return a 204 HTTP status code. Concerned tests (integ + accept) are weak, messy over-complicated or the three in the same time.

There is no problem with the new knex version, but it is better to upgrade `knex` with `bookshelf` (see PR #1434)

jbuget added 👀 Func Review Needed team-acces team-captains This is your captain speaking team-certif team-evaluation PR relatives à l'expérience d'évaluation team-prescription 👀 Tech Review Needed labels May 17, 2020

jbuget marked this pull request as ready for review May 18, 2020 13:49

VincentHardouin reviewed May 18, 2020

View reviewed changes

.../integration/application/organization-invitations/organization-invitation-controller_test.js Show resolved Hide resolved

jbuget mentioned this pull request May 18, 2020

[TECH] Montée de version de Bookshelf #1434

Merged

aceol reviewed May 18, 2020

View reviewed changes

api/tests/unit/application/session/index_test.js Show resolved Hide resolved

aceol approved these changes May 18, 2020

View reviewed changes

jbuget force-pushed the tech-bump-dependencies branch from 40b71dd to 1d115ff Compare May 18, 2020 21:53

laura-bergoens reviewed May 19, 2020

View reviewed changes

VincentHardouin approved these changes May 19, 2020

View reviewed changes

laura-bergoens force-pushed the tech-bump-dependencies branch from 1d115ff to fa5d660 Compare May 19, 2020 16:45

jonathanperret reviewed May 19, 2020

View reviewed changes

jonathanperret reviewed May 20, 2020

View reviewed changes

api/package.json Outdated Show resolved Hide resolved

jbuget added 9 commits May 22, 2020 10:22

Bump @hapi/hapi from v18.4.1 to v19.1.1

4c57609

This hapi version contains breaking changes, cf. https://hapi.dev/resources/changelog. We are impacted by 2 of them that will be fixed in next commits : - default empty reponse status code is now 204 (instead of 200) - enforce media type checking

Fix @hapi/hapi breaking tests about 204 default status code for empty…

ace6440

… reponse See hapijs/hapi#4017 section "Change default empty status code to 204"

Fix @hapi/hapi breaking tests about form-multipart refused by default

1699a10

See hapijs/hapi#4017 section "Change route options.payload.multipart to false by default"

Bump other @Hapi non-core dependencies

a992bef

Bump other hapi unofficial plugins

39f0e24

Concerned dependencies: - sentry - swagger

Bump airtable from 0.8.0 to 0.8.1

3c91e7d

Bump bcrypt from 3.0.7 to 4.0.1

8911d6d

Bump bluebird from 3.7.1 to 3.7.2

0672508

Bump json2csv from 4.5.4 to 5.0.1

18caeb3

jbuget added 24 commits May 22, 2020 10:22

Bump knex from 0.20.2 to 0.21.1

ac8d76a

Bump moment from 2.24.0 to 2.25.3

136c4d5

Bump moment-timezone from 0.5.27 to 0.5.29

e64ddcc

Bump node-cache from 5.0.2 to 5.1.0

b8bb8c5

Bump papaparse from 5.1.0 to 5.2.0

81f2c99

Bump pg from 7.14.0 to 8.2.1

779b59a

Bump redis from 2.8.0 to 3.0.2

63daab7

Bump redlock from 3.1.2 to 4.1.0

b15bb4f

Bump request from 2.88.0 to 2.88.2

4d33f47

Bump sib-api-v3-sdk from 7.1.4 to 7.2.2

51a6240

Bump xlsx from 0.15.3 to 0.16.1

a40fffa

Bump xmldom from 0.1.27 to 0.3.0

110db8c

Bump xregexp from 4.2.4 to 4.3.0

b422fef

Bump mocha from 6.2.2 to 7.1.2

d28cd60

Bump nock from 11.7.0 to 12.0.3

7049e2b

Bump nodemon from 2.0.1 to 2.0.4

b2b5583

Bump nyc from 14.1.1 to 15.0.1

d46a32f

Bump sinon from 7.5.0 to 9.0.2

4fba88b

Bump iconvlite from 0.5.0 to 0.5.1

b910afe

Bump eslint (and eslint-plugin-mocha) from 6.7.1 to 7.0.0

fe83c06

Downgrade pg from 8.x to 7.x because it seems to break Scalingo deplo…

7801525

…yment

Rename few tests to correspond to new expectations

62e2785

Fix wrong test.

b0172b1

The endpoint `POST /api/token` must not return a 204 HTTP status code. Concerned tests (integ + accept) are weak, messy over-complicated or the three in the same time.

Revert knex from 0.21.1 to 0.20.2

fe0bac3

There is no problem with the new knex version, but it is better to upgrade `knex` with `bookshelf` (see PR #1434)

jbuget force-pushed the tech-bump-dependencies branch from c30e129 to fe0bac3 Compare May 22, 2020 08:23

Bump moment, moment-timezone and hapi-swagger

fdf846f

jbuget merged commit 6d5eafe into dev May 22, 2020

jbuget deleted the tech-bump-dependencies branch May 22, 2020 08:45

jbuget mentioned this pull request May 28, 2020

[TECH] Merge automatique grâce aux Github Actions #1456

Merged

jonathanperret mentioned this pull request Aug 19, 2020

[TECH] Monter les versions des packages de l'API. #1767

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TECH] Montée de version des dépendances de Pix API #1433

[TECH] Montée de version des dépendances de Pix API #1433

jbuget commented May 17, 2020 •

edited

Loading

pix-service commented May 17, 2020

aceol left a comment

laura-bergoens left a comment

jbuget commented May 19, 2020

jonathanperret May 19, 2020

jbuget May 20, 2020

jonathanperret May 20, 2020

jonathanperret commented May 19, 2020

[TECH] Montée de version des dépendances de Pix API #1433

[TECH] Montée de version des dépendances de Pix API #1433

Conversation

jbuget commented May 17, 2020 • edited Loading

🦄 Problème

🤖 Solution

🌈 Remarques

💯 Pour tester

pix-service commented May 17, 2020

aceol left a comment

Choose a reason for hiding this comment

laura-bergoens left a comment

Choose a reason for hiding this comment

jbuget commented May 19, 2020

jonathanperret May 19, 2020

Choose a reason for hiding this comment

jbuget May 20, 2020

Choose a reason for hiding this comment

jonathanperret May 20, 2020

Choose a reason for hiding this comment

jonathanperret commented May 19, 2020

jbuget commented May 17, 2020 •

edited

Loading