before-interop / anomalieAdresse

Ce protocole permet le traitement d'une demande de création ou de modification d'adresses immeuble dans les IPE
https://before-interop.github.io/anomalieAdresse/
1 stars 6 forks source link

Mettre des opérations avec un path définissant l'objet manipulé #71

Closed ericjacq92 closed 5 months ago

ericjacq92 commented 7 months ago

Postée par Bytel. Cette issue est remonté par nos équipe technique en charge d'implémenter le swagger.

Les URI des API ne respectent pas les bonnes pratiques, on devrait avoir une URI qui porte le nom de l'objet qu'il manipule, exemple ci-dessous : https://github.com/before-interop/anomalieAdresse/blob/a3aeb16d8e3260c983cb1f579f7b5367453e6802/openapi.yaml#L353-L355

Dans l'exemple ci-dessous on souhaite avoir quelque chose comme :

paths:
  /anomalieadresses:
    get:

ou

paths:
  /troubleTickets:
    get:

Ce principe devrait être mis en place pour toutes les opérations en path /:

ggrebert commented 7 months ago

Les APIs sont déjà préfixées par anomalie-adresse

https://github.com/before-interop/anomalieAdresse/blob/main/openapi.yaml#L325

ericjacq92 commented 7 months ago

c'est l'url server, ca ne porte pas la notion d'objet manipulé dans la définition des opérations d'API, la bonne pratique est de les définir dans le path.

ggrebert commented 7 months ago

pas forcément:

Les endpoints annexes sont ensuite structurée par une hiérarchie héritée de la base (qui en l'occurrence pour AnomalieAdresse est TroubleTicket). L'API principale (root) est donc /.

ggrebert commented 7 months ago

De plus, basePath fixe la version du protocole.

Si on regarde un appel GET sur le protocole sur / le résultat est:

Exemples:

vmeunier commented 7 months ago

Le problème c'est que c'est la théorie, en pratique aucun générateur de code ne se sert de la notion de serveur, et en l'occurrence une telle façon de procéder indique que le swagger n'est pas autoporteur, il impose que le "serveur" sur lequel on déploie l'API "root" "/" soit paramétrée pour avoir une basePath contenant anomalie-adresse. Je ne vois pas pourquoi le loadbalancer devrait gérer ça, et pas le backend sous-jacent.

ggrebert commented 7 months ago

@vmeunier Pourrais tu m'envoyer le nom du générateur que tu utilises

vmeunier commented 7 months ago

@ggrebert https://github.com/OpenAPITools/openapi-generator ou swagger codegen, ou le générateur derrière editor.swagger.io

ggrebert commented 7 months ago

Exemple de génération de code JAVA Microprofile avec openapi-generator:

  1. Générer le code:

    docker run --rm -v "${PWD}:/local" openapitools/openapi-generator-cli generate \
    -i https://raw.githubusercontent.com/openapitools/openapi-generator/master/modules/openapi-generator/src/test/resources/3_0/petstore.yaml \
    -g java \
    -o /local/out \
    --skip-validate-spec \
    --library microprofile
    1. Appeler le Rest Client avec customisation du basePath:
    var client = RestClientBuilder.newBuilder()
                              .baseUri(oiServer + "/anomalie-adresse")
                              .build(PetApi.class);

Documentation swagger sur l'utilisation de server: https://swagger.io/docs/specification/api-host-and-base-path/

vmeunier commented 7 months ago

Oui donc tu admets qu'il faut coder à la main ce fameux "/anomalie-adresse" c'est justement ce qu'on voudrait éviter.

De plus, les URI en REST sont censées être parlantes "/" c'est parlant pour personne, ça ne porte aucune entité ni de domaine.

L'URL du serveur dans la doc que tu montres ne contient aucune notion métier ni d'URI propre aux API exposées en dessous.

ggrebert commented 7 months ago

C'était un exemple basique. En général, on utilise un framework.

Exemple pour quarkus:

quarkus.rest-client."package.name.generated".url=https://server.name/base-path

Cet exemple peut aussi être dynamique avec des variables d'environnements:

quarkus.rest-client."package.name.generated".url=${SERVER_URL}/base-path

Si on continue sur l'exemple de Quarkus, l'extension de générateur de code est ici: https://docs.quarkiverse.io/quarkus-openapi-generator/dev/client.html Mais ce principe fonctionne aussi pour d'autres frameworks et langages

vmeunier commented 7 months ago

C'est étonnant de voir que quarkus génère la configuration associée, ça c'est quand même pas banale. Donc ça fonctionne parce qu'il fait ça, et là tu te places qu'en mode client. En serveur il fait quoi ? Parce qu'il faut une configuration similaire.

ggrebert commented 7 months ago

Le règle est simple: chaque opérateur peut utiliser le basePath qu'il veut.

Certains utiliseront des subPath (comme vous semblait vouloir faire et Orange aussi), d'autres utiliseront sûrement des sous-domaines DNS.

Quoi qu'il en soit, après ce prefix, la règle est la même pour tous.

ggrebert commented 7 months ago

L'OC n'a qu'un seul endpoint à créer. Mais en tant que serveur la configuration est aussi possible. Mais en général un serveur dédie la tâche à un proxy genre Kong, ApiGee ou Traefik

ggrebert commented 7 months ago

Si tu le veux, je peux organiser un point Teams pour vous accompagner à régler certains points technique comme celui-ci ou #72

vmeunier commented 7 months ago

Mais justement, mon propos c'est de dire que les opérations qu'on doit exposer ici ne portent pas le périmètre métier associé. J'ai bien compris le résultat final attendu, mais ce qui ne va pas c'est que ça serait beaucoup plus clair si la notion de /anomalie-adresse était au niveau de chaque opération et le "/" en GET deviendrait "/anomalie-adresse" en GET. De suite on comprend mieux cette URI avec la description "Get All Trouble tickets" (et encore cette description ne semble pas avoir un lien avec "anomalie adresse" mais je ne connais pas le métier donc je me trompe sans doute.)

En fait le résultat demandé est ISO à celui proposé actuellement, sauf que les URI ont plus de sens dès la lecture et techniquement y a rien de spécial à faire pour qu'un serveur quelconque en langage que tu veux saches exposer dès le localhost la bonne URI attendue. Pour aller dans ce sens, si j'avais un autre swagger à intégrer de ce genre dans mon code, je ne pourrais pas le faire sans tricher et ajouter de la conf spécifique manuelle pour exposer deux "/" distincts, et ça me parait pas être normal.

Je rediscute en interne pour organiser un point, mais pas de soucis pour échanger

ggrebert commented 7 months ago

Si ça peut aider, voici le même principe pour le protocole Interop Malfacon: https://before-interop.github.io/malfacon/

vmeunier commented 7 months ago

Et bien je n'aime pas non plus le fichier malfacon. Il a exactement le même souci que anomalieAdresse.

ggrebert commented 7 months ago

Honnêtement, si BT souhaite préfixer de manière protocolaire les endpoints ça ne me dérange vraiment pas. Par contre, il faudra juste l'accord de tous les opérateurs sur ce préfix (car certains séparent les domaines métier par sous-domaines et pas par subpath). De plus, je t'invite à proposer aussi la modification dans malfacon. Je rappelle aussi que chaque proposition est la bienvenue et je t'encourage donc à préparer un Pull Request à présenter à la prochaine réunion Interop.

ericjacq92 commented 7 months ago

Est ce qu'on peut organiser un atelier de travail Teams comme proposé pour échanger sur nos problématiques et difficultés techniques ? Cela se prêtera mieux au contexte.

ggrebert commented 7 months ago

Avec plaisir. Est ce que tu souhaites que je demande à Corinne pour inviter tous les opérateurs ou souhaites tu qu'on le fasse juste entre nous ?

ericjacq92 commented 7 months ago

Je propose un point bilatéral dans un premier temps pour un premier niveau d'analyse, et on remontera en Interop ce qui débouche sur un impact global ou nécessite un questionnement global.