fix: visualize web with context #326
Conversation
nsenave
added
the
deploy-snapshot
github-actions
bot
removed
the
deploy-snapshot
|
👋 Version 4.9.2-SNAPSHOT deployed on docker hub |
nsenave
added
the
deploy-snapshot
github-actions
bot
removed
the
deploy-snapshot
|
👋 Version 4.9.2-SNAPSHOT.1 deployed on docker hub |
* test: add json questionnaire in resources * fix: add context param in pogues to lunatic endpoint * refactor: eno client and visualize related classes * chore: bump version
| @Service | ||
| @RequiredArgsConstructor | ||
| @Slf4j | ||
| public class EnoHttpClient implements EnoClient { |
| public class EnoHttpClient implements EnoClient { | ||
|
|
||
| @Value("${application.eno.host}") | ||
| String enoHost; |
| @Value("${application.eno.host}") | ||
| String enoHost; | ||
|
|
||
| private final WebClient webClient; |
There was a problem hiding this comment.
UItiliser RestClient avec enoHost en baseUrl. Le même client peut être partagé par toutes les méthodes
Il est peut probable que le client http sous jacent par défaut utilise la compression web : ce sont des données qui peuvent être volumineuses, il faudrait l'activer par défaut. le client Oktttp le fait (cf. https://www.wiremock.io/post/java-http-client-comparison#the-clients) avec du http 2 automatiquement également. En principe Apache http client fait la compression automatiquement aussi mais pas le http2 (moins important à l'intérieur d'un datacenter je pense)
Voir aussi la minification des fichiers envoyés : (enlever espaces, retour à la ligne, indentations ...) : ça fait gagner du poids également.
|
|
||
| /** {@link EnoClient#getParameters()} */ | ||
| @Override | ||
| public void getParameters() { |
There was a problem hiding this comment.
A quoi sert cette méthode qui n'envoie pas d'information à eno et qui n'utilise pas les données récupérées dans eno
There was a problem hiding this comment.
Bonne question, et mauvaise réponse 😈 : c'est uniquement utilisé comme un healthcheck : si la méthode plante pas => le WS Eno répond. J'ai laissé ça comme ça pour l'instant, c'est documenté dans la méthode côté interface cf. le @link (peut-être mettre la doc en question dans l'implem mais bon ça a vocation a être viré)
| */ | ||
| @Override | ||
| @Deprecated(since = "4.9.2") | ||
| public String getDDIToLunaticJSON(String inputAsString, Map<String, Object> params) throws EnoException, PoguesException { |
There was a problem hiding this comment.
Dans l'idéal, il faudrait un objet java Parametres qui possède un attribut mode. Mais on fait avec ce qu'on a ;-)
There was a problem hiding this comment.
C'est ça, j'ai pas essayé de changer la signature de la méthode pour faire plus propre et voir les impacts, mais à vue de nez j'avais pas envie je me suis dit que ça n’emmènerait trop loin.
| @@ -12,9 +12,9 @@ | |||
| import static fr.insee.pogues.utils.IOStreamsUtils.string2BOAS; | |||
|
|
|||
| @Service | |||
| @AllArgsConstructor | |||
| public class PoguesJSONToLunaticJSONImpl implements PoguesJSONToLunaticJSON { | |||
| public class PoguesJSONToLunaticJSONImpl implements PoguesJSONToLunaticJSON { | ||
|
|
||
| @Autowired | ||
| private EnoClient enoClient; |
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertThrows; | ||
|
|
||
| class EnoHttpClientTest { |
There was a problem hiding this comment.
Ajouter un test qui vérifie que la configuration du client http (quel qu'iil soit) est bien faite : avec okhttp mockServer ou wiremock (exemple ici : https://github.com/FBibonne/spring-httpexchange-extension/blob/main/src/test/java/fr/insee/demo/httpexchange/SimpleExampleTest.java#L109)
There was a problem hiding this comment.
Oui bonne idée, j'avoue que j'y ai pensé mais je l'ai pas fait finalement 🙃
|
Merci @FBibonne pour tes retours, j'ai tracé ça dans une issue pour que ça puisse servir à un prochain refacto dans l'appli : |
Summary
Done
Après le merge de la PR de @laurentC35 sur la requête Eno "Pogues -> Lunatic" direct (à la place du pipeline "Pogues -> DDI" puis "DDI -> Lunatic", la méthode qui fait DDI to Lunatic du EnoClient n'est plus appelée et c'est sur cette méthode que tu avais ajouté le query param avec le contexte @RemiVerriez
Corrigé + pas mal de refacto