carl: add Carl connector (#86683) #462

Merged
yweber merged 1 commits from wip/86683-carl-connector-implementation into main 2024-03-07 16:56:40 +01:00
Owner
No description provided.
yweber added 1 commit 2024-02-07 17:32:26 +01:00
gitea/passerelle/pipeline/head There was a failure building this commit Details
f7f7fd631b
carl: implements Token authentication & sites references (#86683)
yweber force-pushed wip/86683-carl-connector-implementation from f7f7fd631b to ce06277a6f 2024-02-08 08:57:57 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from ce06277a6f to b1ecafd7ee 2024-02-08 09:12:55 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from b1ecafd7ee to 7050e685b8 2024-02-08 10:52:55 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from 7050e685b8 to 3d5f69cc25 2024-02-08 11:59:19 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from 3d5f69cc25 to 074e913c7f 2024-02-08 12:03:15 +01:00 Compare
yweber added 1 commit 2024-02-08 15:33:26 +01:00
gitea/passerelle/pipeline/head There was a failure building this commit Details
28429a2fe1
carl: add entitie getter endpoint (#86683)
yweber force-pushed wip/86683-carl-connector-implementation from 28429a2fe1 to 359e9d995c 2024-02-08 15:41:14 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from 359e9d995c to 5b366506b6 2024-02-08 15:42:54 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from 5b366506b6 to aa2bddd654 2024-02-08 16:18:15 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from aa2bddd654 to 5008e456f0 2024-02-08 16:56:03 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from 5008e456f0 to 4897ccf462 2024-02-08 18:09:29 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from 4897ccf462 to 716ea12d4b 2024-02-08 18:18:39 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from 716ea12d4b to 7428256d53 2024-02-08 18:21:12 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from 7428256d53 to eeffd825c9 2024-02-08 18:33:13 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from eeffd825c9 to 69d5be1b77 2024-02-08 19:09:00 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from 69d5be1b77 to 1c1d9bcc0c 2024-02-08 19:12:47 +01:00 Compare
yweber changed title from WIP: carl: implements Token authentication & sites references (#86683) to WIP: carl: add Carl connector (#86683) 2024-02-12 08:46:16 +01:00
yweber force-pushed wip/86683-carl-connector-implementation from 1c1d9bcc0c to 5c9ceed71e 2024-02-12 12:17:48 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from 5c9ceed71e to 73ae25d2ef 2024-02-12 14:48:20 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from 73ae25d2ef to b5b0295f75 2024-02-12 15:03:34 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from b5b0295f75 to ef2f70291d 2024-02-12 15:11:35 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from ef2f70291d to 30f52467cd 2024-02-12 15:27:07 +01:00 Compare
vdeniaud requested changes 2024-02-12 15:40:42 +01:00
vdeniaud left a comment
Owner

Lecture assez rapide et partielle, avec des remarques purement de forme, dans l'idée que Thomas en remettra une couche et davantage sur le fond

Disclaimer : je ne sais pas ce que c'est Carl :)

Aussi, ça serait bien que le coverage soit tout vert, et ça permettra de trouver des endroits devenus inutiles au fur et à mesure de l'écriture du code

Lecture assez rapide et partielle, avec des remarques purement de forme, dans l'idée que Thomas en remettra une couche et davantage sur le fond Disclaimer : je ne sais pas ce que c'est Carl :) Aussi, ça serait bien que le coverage soit tout vert, et ça permettra de trouver des endroits devenus inutiles au fur et à mesure de l'écriture du code
@ -0,0 +132,4 @@
),
methods=['get'],
parameters={
'type': {'description': _('Carl entitie type name'), 'example_value': 'wo'},
Owner

Le singulier de entities n'est pas entity ?

Le singulier de entities n'est pas entity ?
Author
Owner

Ben tristement si t_t
C'est corrigé, merci !

Ben tristement si t_t C'est corrigé, merci !
yweber marked this conversation as resolved
@ -0,0 +138,4 @@
'description': _(
'Comma separated list of fieldname pointing on foreign entitie we want data to be merged in the reply'
),
'example_value': 'site, NMaddress',
Owner

Tant mieux si les espaces sont supportés mais il vaudrait mieux que l'exemple n'en contienne pas site,NMaddress (c'est ce qui est le plus simple à produire côté wcs et amènera le moins le questions)

Tant mieux si les espaces sont supportés mais il vaudrait mieux que l'exemple n'en contienne pas `site,NMaddress` (c'est ce qui est le plus simple à produire côté wcs et amènera le moins le questions)
Author
Owner

Fait, merci

Fait, merci
yweber marked this conversation as resolved
@ -0,0 +144,4 @@
'description': _(
'When 1 given, add a carl_relationships field in reply containing a list of fields referencing another entitie'
),
'example_value': '1',
Owner

J'ai la vague impression qu'un 'type': 'bool' est supporté ici

J'ai la vague impression qu'un `'type': 'bool'` est supporté ici
Author
Owner

Bien vue ! Merci :)

Bien vue ! Merci :)
yweber marked this conversation as resolved
@ -0,0 +148,4 @@
},
},
)
def entitie(self, request, **kwargs):
Owner

À mon avis on peut être explicites et écrire les paramètres attendus à la place de **kwargs.

Le bonus c'est peut-être qu'on peut s'éviter de vérifier que les paramètres id et type sont là (la ligne d'en dessous) en écrivant

def entities(self, request, id, type, related=None, relationships=None):

(à vérifier)

À mon avis on peut être explicites et écrire les paramètres attendus à la place de `**kwargs`. Le bonus c'est peut-être qu'on peut s'éviter de vérifier que les paramètres id et type sont là (la ligne d'en dessous) en écrivant ``` def entities(self, request, id, type, related=None, relationships=None): ``` (à vérifier)
Author
Owner

J'y ai clairement pensé, mais comme "id" et "type" sont des builtins je suis pas forcément fan comme nom d'argument.

J'y ai clairement pensé, mais comme "id" et "type" sont des builtins je suis pas forcément fan comme nom d'argument.
yweber marked this conversation as resolved
@ -0,0 +184,4 @@
data={'code': 'not-found', 'url': url},
)
if 'relationships' not in data['data']:
Owner

La forme sur une ligne me paraît assez claire relations = data['data'].get('relationships') or {}

La forme sur une ligne me paraît assez claire `relations = data['data'].get('relationships') or {}`
Author
Owner

Bien vu ! Fait, merci

Bien vu ! Fait, merci
yweber marked this conversation as resolved
@ -0,0 +281,4 @@
errors[fieldname] = _('Field %s allready in linked fields : cannot create')
continue
if len(errors):
continue # do not attempt to create anything : allready failed
Owner

Pas d'espace avant les : et already un seul l :)

Pas d'espace avant les : et already un seul l :)
Author
Owner

Il y en avait partout (des espaces avant les : ...) merci !

Il y en avait partout (des espaces avant les : ...) merci !
yweber marked this conversation as resolved
@ -0,0 +380,4 @@
return {'data': {'status': _('No token in cache')}}
expiration = tok_infos[1]
ts = datetime.datetime.fromtimestamp(expiration, datetime.timezone.utc)
Owner

Souvent on enclôt explicitement ce genre d'expression dans un bool(), je trouve ça plus sympa à lire

Souvent on enclôt explicitement ce genre d'expression dans un bool(), je trouve ça plus sympa à lire
Author
Owner

ok, fait

ok, fait
yweber marked this conversation as resolved
@ -0,0 +390,4 @@
@property
def token_authentication(self):
'''Is True when token authentication wanted'''
Owner

return bool(self.carl_username and self.carl_password) suffirait je pense

(et petite préférence pour que la méthode s'appelle token_authentication_available)

`return bool(self.carl_username and self.carl_password)` suffirait je pense (et petite préférence pour que la méthode s'appelle token_authentication_available)
Author
Owner

Merci pour le bool(...), c'est changé.

Pour le nom de l'attribut, je comprend l'idée, mais _available ne me convient qu'a moitié ( _chosen, _needed, _activated ?) : dans le doute une propriété booléenne 'token_authentication' me semblait raisonnable ;)

Merci pour le bool(...), c'est changé. Pour le nom de l'attribut, je comprend l'idée, mais _available ne me convient qu'a moitié ( _chosen, _needed, _activated ?) : dans le doute une propriété booléenne 'token_authentication' me semblait raisonnable ;)
Owner

Juste can_authenticate ?

Juste `can_authenticate` ?
yweber marked this conversation as resolved
@ -0,0 +436,4 @@
TODO : handle paginated results
'''
filter_params = None
if entitie_id is not None:
Owner

Plutôt que ça, à l'endroit où la méthode est appelée on peut faire

try:
    self.list_entities(...)
except ValueError as e:
    raise APIError(_('Wrong parameters when calling list_entities() : %s') % str(e), http_status=500, data=...)

et dans la méthode plus simplement juste raise ValueError('une chaine')

Plutôt que ça, à l'endroit où la méthode est appelée on peut faire ``` try: self.list_entities(...) except ValueError as e: raise APIError(_('Wrong parameters when calling list_entities() : %s') % str(e), http_status=500, data=...) ``` et dans la méthode plus simplement juste raise ValueError('une chaine')
Author
Owner

Oui, je vois l'idée, mais je trouve ça arrangeant de pouvoir déclarer les endpoints de référentiel en une seule ligne (et sans se préoccuper de savoir ce qui est raise)

Oui, je vois l'idée, mais je trouve ça arrangeant de pouvoir déclarer les endpoints de référentiel en une seule ligne (et sans se préoccuper de savoir ce qui est raise)
yweber marked this conversation as resolved
@ -0,0 +509,4 @@
- e_id : Entitie ID
'''
url = self.entitie_url(e_type, e_id)
try:
Owner

M'est avis qu'il faut éviter ces « au cas où », ça laisse la porte ouverte au scénario où la suppression a bien lieu mais l'appel retourne une erreur, pas ouf (mais ça correspond peut-être à quelque chose que tu as observé ?)

Par contre actuellement le code passe si la réponse n'est pas 200, pour que ça ne soit pas le cas il faut ajouter un response.raise_for_status() dans le try/except

M'est avis qu'il faut éviter ces « au cas où », ça laisse la porte ouverte au scénario où la suppression a bien lieu mais l'appel retourne une erreur, pas ouf (mais ça correspond peut-être à quelque chose que tu as observé ?) Par contre actuellement le code passe si la réponse n'est pas 200, pour que ça ne soit pas le cas il faut ajouter un response.raise_for_status() dans le try/except
Author
Owner

Oui, j'ai enlevé le test sur le retour vide (non c'était pas quelque chose que j'avais observé ;) C'était un mauvais "au cas ou").

Et j'ai géré le cas status != 2XX avec un if not response.ok

Oui, j'ai enlevé le test sur le retour vide (non c'était pas quelque chose que j'avais observé ;) C'était un mauvais "au cas ou"). Et j'ai géré le cas status != 2XX avec un `if not response.ok`
yweber marked this conversation as resolved
@ -0,0 +595,4 @@
Handles common error cases.
Handles token authentication.
Defaults timeout to 2s.
Owner

Le paramètre http_method est contrôlé par le code, il n'y a pas besoin de prévoir d'autres valeurs potentielles (d'ailleurs cette ligne n'est pas couverte)

Gitea a perdu la ligne sur laquelle j'avais posé ce commentaire, il s'agit de :

if http_method not in ('get', 'post', 'put', 'patch', 'delete'):
Le paramètre http_method est contrôlé par le code, il n'y a pas besoin de prévoir d'autres valeurs potentielles (d'ailleurs cette ligne n'est pas couverte) Gitea a perdu la ligne sur laquelle j'avais posé ce commentaire, il s'agit de : ``` if http_method not in ('get', 'post', 'put', 'patch', 'delete'): ```
Author
Owner

C'est vrai, fait, merci !

C'est vrai, fait, merci !
yweber marked this conversation as resolved
@ -0,0 +607,4 @@
'''
return self._json_request(True, http_method, url, **kwargs)
def _json_request(self, retry_auth, http_method, url, **kwargs):
Owner

Directement getattr(self.requests, http_method)(url, **kwargs) me paraîtrait bien

Gitea a perdu la ligne sur laquelle j'avais posé ce commentaire, il s'agit de :

            response = method_fun(url, **kwargs)
Directement `getattr(self.requests, http_method)(url, **kwargs)` me paraîtrait bien Gitea a perdu la ligne sur laquelle j'avais posé ce commentaire, il s'agit de : ``` response = method_fun(url, **kwargs) ```
Author
Owner

Merci, mais même si cç me semble un détail, mais je préfère avoir le getattr en dehors du try.

Merci, mais même si cç me semble un détail, mais je préfère avoir le getattr en dehors du try.
yweber marked this conversation as resolved
@ -0,0 +658,4 @@
return data
else:
err_fmt = _('Got an HTTP error code (%d) from Carl : %s')
Owner

Ça semble beaucoup de travail de parser tout ça, peut-être pourrait-on être flemmards et juste renvoyer ce err_data['errors'] dans le dico data de APIError

 data={
                    'code': err_code,
                    'http-method': http_method,
                    'url': url,
                    'status-code': response.status_code,
                    'carl_errors': err_data['errors'],
                },

Gitea a perdu la ligne sur laquelle j'avais posé ce commentaire, il s'agit de :

                       for error in err_data['errors']:
Ça semble beaucoup de travail de parser tout ça, peut-être pourrait-on être flemmards et juste renvoyer ce err_data['errors'] dans le dico data de APIError ``` data={ 'code': err_code, 'http-method': http_method, 'url': url, 'status-code': response.status_code, 'carl_errors': err_data['errors'], }, ``` Gitea a perdu la ligne sur laquelle j'avais posé ce commentaire, il s'agit de : ``` for error in err_data['errors']: ```
Author
Owner

Oui c'est beaucoup de travail ;) Mais plutôt que d'être flemmard j'ai déplacé ça dans une fonction (me permettant d'avoir un parse propre des erreurs pour le delete_entity() )

Oui c'est beaucoup de travail ;) Mais plutôt que d'être flemmard j'ai déplacé ça dans une fonction (me permettant d'avoir un parse propre des erreurs pour le delete_entity() )
yweber marked this conversation as resolved
@ -0,0 +679,4 @@
src = '(%s)' % src
else:
src = ''
if error['title'] == error['detail']:
Owner

Le bloc entre try est except me paraît trop gros, il faudrait essayer de restreindre aux lignes qui peuvent vraiment lever des exceptions (json.JSONDecodeError me semble concerner juste une ligne)

Le bloc entre try est except me paraît trop gros, il faudrait essayer de restreindre aux lignes qui peuvent vraiment lever des exceptions (json.JSONDecodeError me semble concerner juste une ligne)
Author
Owner

Gitea a perdu la référence dans le code d'origine, on parle du try de la fonction carlerror_to_apierror()

Oui, ce bloc try est vraiment gros, c'est un genre de catch-all qui me permet de raise une exception générique si le parse de l'erreur plante (si c'est pas du json ou pas formaté comme attendu).
Mais je crois que je trouve ça mieux qu'ajouter un cran d'indentation avec un

try:
    ...
except ...:
    previous_error = True

if not previous_error:
    try:
        ...
    except ...:
_Gitea a perdu la référence dans le code d'origine, on parle du try de la fonction carlerror_to_apierror()_ Oui, ce bloc try est vraiment gros, c'est un genre de catch-all qui me permet de raise une exception générique si le parse de l'erreur plante (si c'est pas du json ou pas formaté comme attendu). Mais je crois que je trouve ça mieux qu'ajouter un cran d'indentation avec un ``` try: ... except ...: previous_error = True if not previous_error: try: ... except ...: ```
yweber marked this conversation as resolved
yweber force-pushed wip/86683-carl-connector-implementation from 30f52467cd to 1e3f3e8ed6 2024-02-12 16:19:28 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from 1e3f3e8ed6 to 668f98aaf0 2024-02-12 16:38:46 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from 668f98aaf0 to ecf60c555f 2024-02-12 16:56:54 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from ecf60c555f to b536495ba0 2024-02-12 16:59:28 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from b536495ba0 to dbc48cf9ee 2024-02-12 17:04:28 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from dbc48cf9ee to 832f98b6ab 2024-02-12 17:07:27 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from 832f98b6ab to f448276646 2024-02-12 17:09:12 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from f448276646 to dacedaf3d3 2024-02-12 17:13:41 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from dacedaf3d3 to c90fb2026d 2024-02-12 17:30:33 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from c90fb2026d to 0ca0555acd 2024-02-12 18:36:07 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from 0ca0555acd to add1b5388f 2024-02-13 11:07:46 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from add1b5388f to f02827b78b 2024-02-13 11:13:28 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from f02827b78b to 379a790601 2024-02-13 11:15:11 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from 379a790601 to 37e8699aa6 2024-02-13 12:08:23 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from 37e8699aa6 to 2d019663af 2024-02-13 12:20:15 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from 2d019663af to e7d65466f6 2024-02-13 15:16:15 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from e7d65466f6 to d697301c3e 2024-02-13 15:18:50 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from d697301c3e to 2996c4b214 2024-02-13 15:30:13 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from 2996c4b214 to 7fc29b9b53 2024-02-13 15:45:36 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from 7fc29b9b53 to f1bab7535b 2024-02-13 15:55:30 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from f1bab7535b to 7224348379 2024-02-13 16:08:53 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from 7224348379 to 726c2a34b3 2024-02-13 17:27:33 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from 726c2a34b3 to e4b3bab87c 2024-02-13 17:41:31 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from e4b3bab87c to d79b7698fd 2024-02-13 17:48:06 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from d79b7698fd to fd805cbca8 2024-02-14 09:50:43 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from fd805cbca8 to 519fdffd6c 2024-02-14 10:14:56 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from 519fdffd6c to 825610ed06 2024-02-14 10:19:10 +01:00 Compare
yweber added 1 commit 2024-02-14 14:45:49 +01:00
yweber added 1 commit 2024-02-14 15:37:11 +01:00
gitea/passerelle/pipeline/head This commit looks good Details
f9c1bd3ba6
carl: add json_schema_response for endpoints (#86683)
yweber added 1 commit 2024-02-14 16:03:58 +01:00
gitea/passerelle/pipeline/head This commit looks good Details
29ddbc1289
carl: add a generic external reference endpoint (#86683)
yweber force-pushed wip/86683-carl-connector-implementation from 29ddbc1289 to 0cc501cbfe 2024-02-14 16:05:15 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from 0cc501cbfe to 6804af40c4 2024-02-14 16:06:35 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from 6804af40c4 to c400da3f78 2024-02-14 16:14:15 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from c400da3f78 to ce2b5b5fed 2024-02-14 16:21:48 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from ce2b5b5fed to 19be63b4d2 2024-02-14 16:28:23 +01:00 Compare
yweber changed title from WIP: carl: add Carl connector (#86683) to carl: add Carl connector (#86683) 2024-02-20 14:20:13 +01:00
yweber force-pushed wip/86683-carl-connector-implementation from 19be63b4d2 to 7663074248 2024-02-22 11:23:29 +01:00 Compare
tnoel requested changes 2024-02-26 16:25:00 +01:00
Dismissed
tnoel left a comment
Owner

J'arrête ici ma relecture, j'ai déjà posé beaucoup trop de commentaires de pinailleur ! Le code est bon mais je pense que tu vas un peu trop loin parfois :) Mon idée est ici de simplifier la partie entities pour n'avoir qu'un seul endpoint de remontée (entities/type/ avec ?id ou ?q). Et aussi, plus généralement, d'ignorer silencieusement quand il y a des paramètres reçus en trop.

J'arrête ici ma relecture, j'ai déjà posé beaucoup trop de commentaires de pinailleur ! Le code est bon mais je pense que tu vas un peu trop loin parfois :) Mon idée est ici de simplifier la partie entities pour n'avoir qu'un seul endpoint de remontée (entities/type/ avec ?id ou ?q). Et aussi, plus généralement, d'ignorer silencieusement quand il y a des paramètres reçus en trop.
@ -0,0 +1,1086 @@
# passerelle - uniform access to multiple data sources and services
# Copyright (C) 2019 Entr'ouvert
Owner

On est en 2024 ;)

On est en 2024 ;)
Author
Owner

Bien vu ! Merci

Bien vu ! Merci
yweber marked this conversation as resolved
@ -0,0 +125,4 @@
service_url = models.URLField(
blank=False,
verbose_name=_('Service URL'),
help_text=_('Base webservice URL (such as https://example.tld/foo/bar/'),
Owner

Comme a priori on va toujours taper dans https://.../api/ je propose que service_url attende ce format « https://site-carl/api/ ».

Poser donc https://site-carl/api/ comme exemple dans le help_text, et modifier son usage dans le code ensuite en considérant que le /api/ est déjà là.

Ça aura l'avantage de clarifier ce qui est attendu ici, à savoir « une URL qui se termine en /api/ ».

Comme a priori on va toujours taper dans https://.../api/ je propose que service_url attende ce format « https://site-carl/api/ ». Poser donc https://site-carl/api/ comme exemple dans le help_text, et modifier son usage dans le code ensuite en considérant que le /api/ est déjà là. Ça aura l'avantage de clarifier ce qui est attendu ici, à savoir « une URL qui se termine en /api/ ».
Author
Owner

Sur une instance "normale" de Carl il y a un cas ou on ne taperais pas sur un truc en /api/ : pour récupérer le status d'une instance il faut interroger https://carlsource.server.com/gmaoCS02/public/status.

La ce n'est pas en place car la redirection qu'on a nous limite à /api/

Sur une instance "normale" de Carl il y a un cas ou on ne taperais pas sur un truc en `/api/` : pour récupérer le status d'une instance il faut interroger `https://carlsource.server.com/gmaoCS02/public/status`. La ce n'est pas en place car la redirection qu'on a nous limite à `/api/`
yweber marked this conversation as resolved
@ -0,0 +129,4 @@
)
carl_username = models.CharField(
max_length=128, verbose_name=_('Carl token authentication username'), blank=False
Owner

Plutôt blank=True : on ne sait jamais, permettons un usage sans token.

Plutôt blank=True : on ne sait jamais, permettons un usage sans token.
Author
Owner

Oui, d'autant que c'est prévu pour être optionnel dans le reste du code ;) Merci !

Oui, d'autant que c'est prévu pour être optionnel dans le reste du code ;) Merci !
yweber marked this conversation as resolved
@ -0,0 +132,4 @@
max_length=128, verbose_name=_('Carl token authentication username'), blank=False
)
carl_password = models.CharField(
max_length=128, verbose_name=_('Carl token authentication password'), blank=False
Owner

Même chose, plutôt blank=True.

Même chose, plutôt blank=True.
yweber marked this conversation as resolved
@ -0,0 +143,4 @@
@endpoint(
name='entity',
pattern=r'^by_id$',
example_pattern='entity/by_id',
Owner

Je n'ai pas compris l'usage de pattern ici... ça va donner une URL bizarre /entity/by_id?id=xxx alors qu'on préférerait avoir juste /entity?id=xxx

Mais en fait, plus globalement j'ai l'impression que les deux endpoints entity/by_id et entity/by_value ne sont que deux cas particulier du endpoin entities, qui sera largement suffisant pour nos besoins. On aurait :

  • entities/<?P(type)>/?id=xxx pour remonter une liste contenant le (seul) élément avec l'identifiant "xxx"
  • entities/<?P(type)>/?q=yyy pour remonter une liste de tous les élements contenant la valeur "yyy"
Je n'ai pas compris l'usage de pattern ici... ça va donner une URL bizarre /entity/by_id?id=xxx alors qu'on préférerait avoir juste /entity?id=xxx Mais en fait, plus globalement j'ai l'impression que les deux endpoints entity/by_id et entity/by_value ne sont que deux cas particulier du endpoin entities, qui sera largement suffisant pour nos besoins. On aurait : - entities/<?P(type)>/?id=xxx pour remonter une liste contenant le (seul) élément avec l'identifiant "xxx" - entities/<?P(type)>/?q=yyy pour remonter une liste de tous les élements contenant la valeur "yyy"
Author
Owner

Oui l'utilisation des patterns était la pour ajouter de la profondeur à /entity pour éviter des /entity_by_id et /entity_by_values.
J'avais imaginé que, côté webservice, c'était pas super pratique de passer des arguments à deux endroit différents (l'url et la QS).

Néanmoins il ne me semble pas que entity/* soit un cas particulier de '/entities` :

  • /entities est la pour récupérer une liste d'identifiants d'entités
  • /entity/* sont la pour récupérer les propriétés d'une entité (et éventuellement des entités liés)

Ce que j'ai fais pour le moment :

  • laisser /entities tel quel
  • redéfinir /entity/by_id en /entity/(?P<carl_type>...)/(?P<carl_id>...)$
  • redéfinir /entity/by_values en /entity/(?P<carl_type>...)$
Oui l'utilisation des patterns était la pour ajouter de la profondeur à `/entity` pour éviter des `/entity_by_id` et `/entity_by_values`. J'avais imaginé que, côté webservice, c'était pas super pratique de passer des arguments à deux endroit différents (l'url et la QS). Néanmoins il ne me semble pas que `entity/*` soit un cas particulier de '/entities` : - `/entities` est la pour récupérer une liste d'identifiants d'entités - `/entity/*` sont la pour récupérer les propriétés d'une entité (et éventuellement des entités liés) Ce que j'ai fais pour le moment : - laisser `/entities` tel quel - redéfinir `/entity/by_id` en `/entity/(?P<carl_type>...)/(?P<carl_id>...)$` - redéfinir `/entity/by_values` en `/entity/(?P<carl_type>...)$`
yweber marked this conversation as resolved
@ -0,0 +175,4 @@
},
},
'properties': {
'carl_relationships': {
Owner

évitons de renvoyer des clés avec des "_" dedans, qui sont un caractère un peu spécial dans Publik en général (sorte de séparateur de "namespace"). Ici on peut juste renvoyer « relationships » car on sait très bien qu'on parle à Carl. Même chose pour carl_id ou carl_type plus bas.

évitons de renvoyer des clés avec des "_" dedans, qui sont un caractère un peu spécial dans Publik en général (sorte de séparateur de "namespace"). Ici on peut juste renvoyer « relationships » car on sait très bien qu'on parle à Carl. Même chose pour carl_id ou carl_type plus bas.
Author
Owner

Je ne suis pas certain du code cité (gitea a perdu la ref je crois), mais il me semble que c'est dans le schéma des données renvoyés à Publik.

Ici l'idée était justement un peu d'avoir un autre "namespace" dans un dict à plat : une manière peu couteuse d'éviter des collisions avec des attributs 'type', 'id' ou 'relationships' d'une future entité. (le carl_ signifie plus que c'est une info de Carl et pas une propriété de l'entité)

Il y a une manière plus élégante d'éviter ces collisions ? Renvoyer un dictionnaire qui n'est pas à plat ?

Je ne suis pas certain du code cité (gitea a perdu la ref je crois), mais il me semble que c'est dans le schéma des données renvoyés à Publik. Ici l'idée était justement un peu d'avoir un autre "namespace" dans un dict à plat : une manière peu couteuse d'éviter des collisions avec des attributs 'type', 'id' ou 'relationships' d'une future entité. (le `carl_` signifie plus que c'est une info de Carl et pas une propriété de l'entité) Il y a une manière plus élégante d'éviter ces collisions ? Renvoyer un dictionnaire qui n'est pas à plat ?
yweber marked this conversation as resolved
@ -0,0 +183,4 @@
},
)
def entity_by_id(self, request, **kwargs):
missing = [param for param in ('type', 'id') if param not in kwargs]
Owner

si "type" et "id" sont obligatoires dans l'appel, il suffit de les ajouter explicitement dans les arguments de la fonction, ici ça serait genre:

def entity_by_id(self, request, type, id, **kwargs):

et notre décorateur @endpoint fera le travail de l'erreur 400, gratuitement :)

si "type" et "id" sont obligatoires dans l'appel, il suffit de les ajouter explicitement dans les arguments de la fonction, ici ça serait genre: def entity_by_id(self, request, type, id, **kwargs): et notre décorateur @endpoint fera le travail de l'erreur 400, gratuitement :)
Author
Owner

Valentin m'avait déjà proposé cette modification, mais personnellement je suis pas super à l'aise avec le fait de surcharger les deux builtins id et type.

Si ça ne vous fait pas plus peur que ça je passe deux arguments id et type :)

Valentin m'avait déjà proposé cette modification, mais personnellement je suis pas super à l'aise avec le fait de surcharger les deux builtins `id` et `type`. Si ça ne vous fait pas plus peur que ça je passe deux arguments `id` et `type` :)
Author
Owner

En utilisant les URL pour passer ces deux paramètres on peut les préfixer avec carl_ et les passer explicitement en arguments a la fonction sans remplacer de builtins.

En utilisant les URL pour passer ces deux paramètres on peut les préfixer avec `carl_` et les passer explicitement en arguments a la fonction sans remplacer de builtins.
yweber marked this conversation as resolved
@ -0,0 +184,4 @@
)
def entity_by_id(self, request, **kwargs):
missing = [param for param in ('type', 'id') if param not in kwargs]
if len(missing):
Owner

De façon plus pythonique, faire juste un "if missing:" pour savoir si une liste n'est pas vide.

De façon plus pythonique, faire juste un "if missing:" pour savoir si une liste n'est pas vide.
Author
Owner

ok, merci, fait :)

ok, merci, fait :)
yweber marked this conversation as resolved
@ -0,0 +191,4 @@
carl_type = kwargs.pop('type')
carl_id = kwargs.pop('id')
want_relationships = kwargs.pop('relationships', False)
Owner

Pour ce genre de cas, ajouter un « relationships=False » dans les arguments de la fonction.

Pour ce genre de cas, ajouter un « relationships=False » dans les arguments de la fonction.
yweber marked this conversation as resolved
@ -0,0 +193,4 @@
carl_id = kwargs.pop('id')
want_relationships = kwargs.pop('relationships', False)
related = kwargs.pop('related', '')
related = [rel for rel in (sp.strip() for sp in related.split(',')) if len(rel) > 0]
Owner

Ici aussi remplacer "if len(rel)>0" par un simple "if rel"

Ici aussi remplacer "if len(rel)>0" par un simple "if rel"
yweber marked this conversation as resolved
@ -0,0 +195,4 @@
related = kwargs.pop('related', '')
related = [rel for rel in (sp.strip() for sp in related.split(',')) if len(rel) > 0]
if len(kwargs): # unexpected extra parameters
Owner

Toujours l'usage inutile du len(). Par ailleurs, en général on laisse filer silencieusement les paramètres en trop... du moins je trouve que ça fait du code en moins à gérer... et ça permet un peu plus de souplesse lors des appels.

Toujours l'usage inutile du len(). Par ailleurs, en général on laisse filer silencieusement les paramètres en trop... du moins je trouve que ça fait du code en moins à gérer... et ça permet un peu plus de souplesse lors des appels.
Author
Owner

Normalement j'ai fais le tour, il n'en reste plus dans models.py

Normalement j'ai fais le tour, il n'en reste plus dans `models.py`
yweber marked this conversation as resolved
@ -0,0 +211,4 @@
@endpoint(
name='entity',
pattern=r'^by_values$',
Owner

Comme vu pour le endpoint précédent, à mon avis celui-ci est inutile, du moins il pourrait avantageusement être pris en charge par /entities/<?P(type)>/?q=...

Comme vu pour le endpoint précédent, à mon avis celui-ci est inutile, du moins il pourrait avantageusement être pris en charge par /entities/<?P(type)>/?q=...
yweber marked this conversation as resolved
@ -0,0 +296,4 @@
@endpoint(
name='entity',
pattern='^create$',
Owner

Si comme je le propose ci-dessus on évite les endpoints entity/by_id ou entity/by_value au profit d'un endpoint global « entities », alors ce endpoint de création pourra juste être renommé « entity ».

Si comme je le propose ci-dessus on évite les endpoints entity/by_id ou entity/by_value au profit d'un endpoint global « entities », alors ce endpoint de création pourra juste être renommé « entity ».
Author
Owner

C'est fait

C'est fait
yweber marked this conversation as resolved
@ -0,0 +311,4 @@
},
},
'properties': {
'carl_id': {'type': 'string'},
Owner

Comme vu plus haut, inutile de rappeler le nom "card", laisser juste id et type.

Comme vu plus haut, inutile de rappeler le nom "card", laisser juste id et type.
yweber marked this conversation as resolved
@ -0,0 +316,4 @@
},
},
)
def create_entity_endpoint(self, request, post_data):
Owner

Nommer juste « create_entity », on sait que c'est un endpoint.

Nommer juste « create_entity », on sait que c'est un endpoint.
Author
Owner

Et je préfixe la méthode create_entity par un underscore (celle qui n'est pas un endpoint) ?

Et je préfixe la méthode `create_entity` par un underscore (celle qui n'est pas un endpoint) ?
yweber marked this conversation as resolved
@ -0,0 +436,4 @@
)
missing = [field for field in ('id', 'type', 'attributes') if field not in entity['data']]
if len(missing) > 0:
Owner

Si je comprends bien, tu imagines un cas ici où l'API de Carl déclare avoir créé l'objet (nous a retourné un 200 ou 201) mais où elle n'a pas renvoyé ce qu'il faut ... et dans ce cas, tu cherches à détruire l'objet créé ?

Si c'est bien ça, je propose plutôt de ne rien faire, ça me semble juste trop touchy : si leur API est boguée à ce point alors on ne va pas tenter de contourner.

Bref, on fait l'appel à self.create_entity et si ça s'est bien passé, on renvoie la réponse, hop.

Si je comprends bien, tu imagines un cas ici où l'API de Carl déclare avoir créé l'objet (nous a retourné un 200 ou 201) mais où elle n'a pas renvoyé ce qu'il faut ... et dans ce cas, tu cherches à détruire l'objet créé ? Si c'est bien ça, je propose plutôt de ne rien faire, ça me semble juste trop touchy : si leur API est boguée à ce point alors on ne va pas tenter de contourner. Bref, on fait l'appel à self.create_entity et si ça s'est bien passé, on renvoie la réponse, hop.
Author
Owner

Le truc, c'est qu'au lieu de créé l'entité cible (wo) puis les entités liés (NMaddr*) puis les lier, je fais l'inverse : je créé les entitiés liés et au moment de créer l'entité cible j'indique les ID des entités liés (au même titre que le site).

Donc ici l'idée n'est pas de supprimer l'objet créé (ou pas, on sait pas, le JSON renvoyé est pourri), mais de supprimer les entités précédemment créés, destinées à être liés (en gros je clean les NMaddr* créé pour rien).

Mais je ne m'attend pas à ce que ces précisions change grand chose à ta remarque ;)

Le truc, c'est qu'au lieu de créé l'entité cible (wo) puis les entités liés (NMaddr*) puis les lier, je fais l'inverse : je créé les entitiés liés et au moment de créer l'entité cible j'indique les ID des entités liés (au même titre que le site). Donc ici l'idée n'est pas de supprimer l'objet créé (ou pas, on sait pas, le JSON renvoyé est pourri), mais de supprimer les entités précédemment créés, destinées à être liés (en gros je clean les NMaddr* créé pour rien). Mais je ne m'attend pas à ce que ces précisions change grand chose à ta remarque ;)
yweber marked this conversation as resolved
@ -0,0 +526,4 @@
)
@endpoint(
name='token_status',
Owner

On préfère éviter le caractère « _ » dans les endpoints, appelons-le token-status. Ceci dit je doute de l'intérêt de ce endpoint, je le supprimerais pour éviter tout mauvais usage (nos clients sont inventifs).

On préfère éviter le caractère « _ » dans les endpoints, appelons-le token-status. Ceci dit je doute de l'intérêt de ce endpoint, je le supprimerais pour éviter tout mauvais usage (nos clients sont inventifs).
Author
Owner

Comme indiqué dans la description c'était un endpoint de debug, j'avais complètement oublié de le supprimé, merci ;)

Comme indiqué dans la description c'était un endpoint de debug, j'avais complètement oublié de le supprimé, merci ;)
yweber marked this conversation as resolved
@ -0,0 +588,4 @@
Returns a dict suitable to be returned by an endpoint
'''
related = [] if related is None else related
Owner

Plutôt que cette ligne, indiquer « related=[] » dans les arguments de la fonction. Ça aura de plus l'avantage de montrer le type attendu (une liste).

Plutôt que cette ligne, indiquer « related=[] » dans les arguments de la fonction. Ça aura de plus l'avantage de montrer le type attendu (une liste).
Author
Owner

Il me semble que c'est une mauvaise pratique de mettre une valeur non immuable en valeur par défaut d'un argument.

Mais je peux passer le tuple vide en valeur par défaut (comme ici, au final, on ne vient pas modifier related).

Il me semble que c'est une mauvaise pratique de mettre une valeur non immuable en valeur par défaut d'un argument. Mais je peux passer le tuple vide en valeur par défaut (comme ici, au final, on ne vient pas modifier related).
yweber marked this conversation as resolved
@ -0,0 +654,4 @@
Returns: an external reference in Publik format (
https://doc-publik.entrouvert.com/dev/wcs/api-webservices/acces-a-des-referentiels-externe/ )
TODO: handle paginated results
Owner

Sauf cas très particuliers, on ne fait jamais de pagination dans les webservices, c'est bien trop complexe à gérer.

Sauf cas très particuliers, on ne fait jamais de pagination dans les webservices, c'est bien trop complexe à gérer.
Author
Owner

EDIT: je viens de voir le commentaire plus bas : OK, si trop d'entitié on raise.

C'est à dire on ne fait jamais de pagination ? On ne propose pas de réponse paginées ? Ou on ne gère pas les services qui nous répondent en paginant ?

Ici le problème c'est que Carl peut nous répondre en plusieurs "pages", quand c'est le cas on a un référentiel partiel (sans erreur etc.) d'ou le TODO.

EDIT: je viens de voir le commentaire plus bas : OK, si trop d'entitié on raise. C'est à dire on ne fait jamais de pagination ? On ne propose pas de réponse paginées ? Ou on ne gère pas les services qui nous répondent en paginant ? Ici le problème c'est que Carl peut nous répondre en plusieurs "pages", quand c'est le cas on a un référentiel partiel (sans erreur etc.) d'ou le TODO.
yweber marked this conversation as resolved
@ -0,0 +695,4 @@
{'text': ent2text(ent), 'id': ent2id(ent), 'carl_id': ent['id']}
for ent in data['data']
if len(query) == 0 or query in ent2text(ent).lower()
]
Owner

C'est un peu dommage de lever une exception et tout arrêt si un seul élément est mal formaté. Je proposerais de plutôt organiser l'affaire avec un for et des continue quand ça va. Quelque chose du genre :

result = []
for entity in data['data']:
    if not ent_id := end['attributes'].get(id_attrname) if id_attrname else ent.get('id'):
        continue
    if not end_text := ent['attributes'].get(text_attrname):
        continue
    if query and query not in end_text.lower():
         continue
    ent.update({'id': ent_id, 'text': ent_text, 'orig_id': ent['id']})
    result.add(ent)
C'est un peu dommage de lever une exception et tout arrêt si un seul élément est mal formaté. Je proposerais de plutôt organiser l'affaire avec un for et des continue quand ça va. Quelque chose du genre : ``` result = [] for entity in data['data']: if not ent_id := end['attributes'].get(id_attrname) if id_attrname else ent.get('id'): continue if not end_text := ent['attributes'].get(text_attrname): continue if query and query not in end_text.lower(): continue ent.update({'id': ent_id, 'text': ent_text, 'orig_id': ent['id']}) result.add(ent) ```
Author
Owner

C'est vrai, c'est dommage mais c'est plus safe : j'imaginais qu'il vallait mieux comme on est dans le endpoint qui fournit des référentiels externes.

Si la version plus permissive que tu proposes te conviens mieux j'implémente ça :)

C'est vrai, c'est dommage mais c'est plus safe : j'imaginais qu'il vallait mieux comme on est dans le endpoint qui fournit des référentiels externes. Si la version plus permissive que tu proposes te conviens mieux j'implémente ça :)
yweber marked this conversation as resolved
@ -0,0 +708,4 @@
Returns: a dict with 'data' being a list of Carl entities
NOTE : TODO handle paginated results (if too many entities returned)
Owner

Tu peux supprimer le todo, s'il y a trop d'entités c'est que plouf ça marche pas. Gérer de la pagination est hors de portée.

Tu peux supprimer le todo, s'il y a trop d'entités c'est que plouf ça marche pas. Gérer de la pagination est hors de portée.
Author
Owner

Je te propose de supprimer le TODO en implémentant une détection du "trop d'entités" pour raise une APIError. Qu'en penses tu ?

Je te propose de supprimer le TODO en implémentant une détection du "trop d'entités" pour raise une APIError. Qu'en penses tu ?
yweber marked this conversation as resolved
@ -0,0 +722,4 @@
'''
url = self.entity_url(entity['data']['type'])
headers = {'Content-Type': 'application/vnd.api+json'}
data = json.dumps(entity)
Owner

Ce json.dumps n'est pas utile, on est sur un request qui peut manger du json directement avec l'argument json=entity (à la place de data=data). Ou bien ça n'est pas compatible avec leur étrange content-type ? (dans ce cas, il ajouter un petit commentaire qui explique cela).

Ce json.dumps n'est pas utile, on est sur un request qui peut manger du json directement avec l'argument json=entity (à la place de data=data). Ou bien ça n'est pas compatible avec leur étrange content-type ? (dans ce cas, il ajouter un petit commentaire qui explique cela).
Author
Owner

Il m'avait semblé que l'argument json forçait le content-type, mais visiblement ce n'est pas le cas...

J'ai ajouté des tests sur le content-type et j'ai enlevé le json.dumps :)

Il m'avait semblé que l'argument json forçait le content-type, mais visiblement ce n'est pas le cas... J'ai ajouté des tests sur le content-type et j'ai enlevé le json.dumps :)
yweber marked this conversation as resolved
@ -0,0 +787,4 @@
def filters_to_params(filters):
'''Return a dict suitable for requests params argument given filters
Arguments:
- filters: A dict with fieldname as key and wanted value as value
Owner

Je ne comprends pas l'utilité de cette fonction, requests fait cet encodage tout seul...?

Je ne comprends pas l'utilité de cette fonction, requests fait cet encodage tout seul...?
Author
Owner

Oui navré, le commentaire n'était pas clair (et il y avait effectivement un double encodage...)

Cette fonction sert à construire les paramètres de filtres carl {'filter[field1]': 'val1', ...} à partir de {'field1': 'val1', ...}

edit : c'est fix, le commentaire est plus clair et il n'y a plus l'encodage

Oui navré, le commentaire n'était pas clair (et il y avait effectivement un double encodage...) Cette fonction sert à construire les paramètres de filtres carl `{'filter[field1]': 'val1', ...}` à partir de `{'field1': 'val1', ...}` edit : c'est fix, le commentaire est plus clair et il n'y a plus l'encodage
yweber marked this conversation as resolved
@ -0,0 +798,4 @@
'''Forge an entity URL of the form
SERVICE_URL/api/entities/v1/[entity_type[/entity_id[/related_fieldname]]]
'''
url = urllib.parse.urljoin(self.service_url, 'api/entities/v1/')
Owner

Rappel : je propose que service_url contienne le /api/, on pourra donc juste faire le urljoin avec 'entities/v1'

Rappel : je propose que service_url contienne le /api/, on pourra donc juste faire le urljoin avec 'entities/v1'
yweber marked this conversation as resolved
@ -0,0 +812,4 @@
break
return url
def filters_from_qsdict(self, qsdict):
Owner

A voir si, avec un seul endpoint "entities" on aura encore besoin de cette fonction. Et ici comme ailleurs je pense qu'il faut silencieusement ignorer les possibles « filter_=... » (faire un continue et par une APIError)

A voir si, avec un seul endpoint "entities" on aura encore besoin de cette fonction. Et ici comme ailleurs je pense qu'il faut silencieusement ignorer les possibles « filter_=... » (faire un continue et par une APIError)
yweber marked this conversation as resolved
yweber force-pushed wip/86683-carl-connector-implementation from 7663074248 to 83469e6778 2024-02-26 16:42:01 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from 83469e6778 to e9ec8ca9ff 2024-02-26 17:26:43 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from e9ec8ca9ff to 2a1c25b696 2024-02-26 17:45:24 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from 2a1c25b696 to e45df9968b 2024-02-26 18:04:38 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from e45df9968b to 37d333777a 2024-02-26 18:26:45 +01:00 Compare
yweber added 1 commit 2024-02-27 09:53:31 +01:00
gitea/passerelle/pipeline/head This commit looks good Details
7ac2dcd353
carl: change endpoint url patterns
yweber force-pushed wip/86683-carl-connector-implementation from 7ac2dcd353 to 1e8339862b 2024-02-27 09:58:28 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from 1e8339862b to 5d309f8a00 2024-02-27 10:06:57 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from 5d309f8a00 to df0dc82dc0 2024-02-27 13:33:13 +01:00 Compare
yweber changed title from carl: add Carl connector (#86683) to WIP: carl: add Carl connector (#86683) 2024-02-27 16:36:01 +01:00
yweber force-pushed wip/86683-carl-connector-implementation from df0dc82dc0 to f21d0ef303 2024-02-27 16:50:23 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from f21d0ef303 to e03baaa8cf 2024-02-27 17:06:54 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from e03baaa8cf to 60630f146b 2024-02-27 17:09:15 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from 60630f146b to 3b534f3fb3 2024-02-27 17:19:35 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from 3b534f3fb3 to 5bb003c5e8 2024-02-27 17:27:37 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from 5bb003c5e8 to ab9669f4c7 2024-02-27 17:31:05 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from ab9669f4c7 to a988d2be39 2024-02-28 10:19:58 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from a988d2be39 to 04a59bf7e7 2024-02-28 10:21:52 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from 04a59bf7e7 to cf970166a0 2024-02-28 10:24:41 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from cf970166a0 to 82d3c1adb5 2024-02-28 10:30:23 +01:00 Compare
yweber requested review from tnoel 2024-02-28 10:32:42 +01:00
yweber changed title from WIP: carl: add Carl connector (#86683) to carl: add Carl connector (#86683) 2024-02-28 10:33:58 +01:00
tnoel requested changes 2024-03-01 18:16:35 +01:00
Dismissed
tnoel left a comment
Owner

J'ai relu, encore plein de petites remarques pénibles, désolé ! Je n'ai pas encore relu les tests cependant, je te laisse déjà lire ce nouveau round.

J'ai relu, encore plein de petites remarques pénibles, désolé ! Je n'ai pas encore relu les tests cependant, je te laisse déjà lire ce nouveau round.
@ -0,0 +142,4 @@
@endpoint(
name='entity',
methods=['post'],
Owner

Et nouvelle idée, pour suivre le schéma du webservice entities, on pourrait ici avoir un

pattern=r'^(?P<carl_type>[^/]+)$',

et donc ne pas avoir besoin du type dans le post_data.

Et nouvelle idée, pour suivre le schéma du webservice entities, on pourrait ici avoir un ` pattern=r'^(?P<carl_type>[^/]+)$',` et donc ne pas avoir besoin du type dans le post_data.
Author
Owner

En effet :)

En effet :)
yweber marked this conversation as resolved
@ -0,0 +180,4 @@
_('Error with specified linked fields: %s')
% (', '.join(['%s (%s)' % elt for elt in errors.items()])),
http_status=400,
data={'code': 'creation-error'},
Owner

Plutôt utiliser err_code='creation-error' que mettre des données dans data.

Plutôt utiliser err_code='creation-error' que mettre des données dans data.
Author
Owner

Ah zut, dans les exemples que j'avais vu c'est comme ça que c'était fait.

Il y a une raison pour passer un code dans data plutôt qu'un err_code en argument ? Sinon je remplace tout ces code dans data par des err_code en argument.

Ah zut, dans les exemples que j'avais vu c'est comme ça que c'était fait. Il y a une raison pour passer un `code` dans data plutôt qu'un `err_code ` en argument ? Sinon je remplace tout ces `code` dans data par des `err_code` en argument.
Author
Owner

J'ai remplacé par err_code partout.

J'ai remplacé par err_code partout.
yweber marked this conversation as resolved
@ -0,0 +217,4 @@
raise APIError(
_('Error looking for linked fields: %s')
% (', '.join(['%r (%s)' % elt for elt in errors.items()])),
data={'code': 'creation-error'},
Owner

Même chose, plutôt err_code

Même chose, plutôt err_code
yweber marked this conversation as resolved
@ -0,0 +255,4 @@
'errors': '. '.join([_('Error with field %s (%s)') % e for e in errors.items()]),
}
if cleanup_err is not None:
err_msg += '. %s' % str(cleanup_err)
Owner

Je poserais ici quelque chose du genre « err_msg += '. On rollback: %s' % str(cleanup_err) histoire que lors de ce genre de panne le mot "rollback" soit visible au support technique.

Je poserais ici quelque chose du genre « err_msg += '. On rollback: %s' % str(cleanup_err) histoire que lors de ce genre de panne le mot "rollback" soit visible au support technique.
Owner

En fait je me dis que tu pourrais envoyer un err_code='creation-error[rollback-on-related]' dans le APIError

En fait je me dis que tu pourrais envoyer un err_code='creation-error[rollback-on-related]' dans le APIError
Author
Owner

ok :)

ok :)
yweber marked this conversation as resolved
@ -0,0 +275,4 @@
data = expt.data
data.update(cleanup_err.data)
raise APIError(
_('Error creating entity: %s. Then error during related entities cleanup: %s')
Owner

Idem, poser le mot "rollback" dans le message (là je te laisse trouver comment, l'anglais et moi...)

Idem, poser le mot "rollback" dans le message (là je te laisse trouver comment, l'anglais et moi...)
Owner

Ou plus simplement err_code='creation-error[rollback]'

Ou plus simplement err_code='creation-error[rollback]'
Author
Owner

Fait

Fait
yweber marked this conversation as resolved
@ -0,0 +281,4 @@
data=data,
)
missing = [field for field in ('id', 'type', 'attributes') if field not in entity['data']]
Owner

Je pensais qu'on était d'accord pour abandonner ce rollback, et considérer juste le renvoie de entity['data']:

        return {'data': entity['data']}
Je pensais qu'on était d'accord pour abandonner ce rollback, et considérer juste le renvoie de entity['data']: ``` return {'data': entity['data']} ```
Author
Owner

Ah ok, c'est pas ce que j'avais compris, mais ça me va !

Ah ok, c'est pas ce que j'avais compris, mais ça me va !
yweber marked this conversation as resolved
@ -0,0 +315,4 @@
},
'id': {
'description': _('Only return the entity with corresponding ID'),
},
Owner

Il manque l'entrée 'q' dans ces parameters.

Il manque l'entrée 'q' dans ces parameters.
yweber marked this conversation as resolved
@ -0,0 +356,4 @@
)
result = [result]
else:
if q is not None:
Owner

"if q:" va suffire, on veut pas faire un LIKE vide.

"if q:" va suffire, on veut pas faire un LIKE vide.
yweber marked this conversation as resolved
@ -0,0 +367,4 @@
entities = self.filter_entities(carl_type, filters, filters_like)
result = [self.entity_data(ent) for ent in entities['data']]
text_attrname = 'description' if text_attrname is None else text_attrname
Owner

En fait text_attrname is None ça ne peut pas arriver, pour moi cette ligne est inutile.

En fait text_attrname is None ça ne peut pas arriver, pour moi cette ligne est inutile.
Author
Owner

effectivement !

effectivement !
yweber marked this conversation as resolved
@ -0,0 +368,4 @@
result = [self.entity_data(ent) for ent in entities['data']]
text_attrname = 'description' if text_attrname is None else text_attrname
result = [{**ent, 'text': ent['attributes'].get(text_attrname)} for ent in result]
Owner

Je trouve que ça fait un peu mal au yeux. Je préfère ce format de code:

for r in result:
    r['text'] = r.get('attributes', {}).get(text_attrname)
Je trouve que ça fait un peu mal au yeux. Je préfère ce format de code: ``` for r in result: r['text'] = r.get('attributes', {}).get(text_attrname) ```
Author
Owner

ok !

ok !
yweber marked this conversation as resolved
@ -0,0 +399,4 @@
Arguments:
- entity_type: The name of the entity type
- filters: A dict with fieldname as key and wanted value as value. Values
Owner

C'est filter_eq dont on parle ici

C'est filter_eq dont on parle ici
yweber marked this conversation as resolved
@ -0,0 +410,4 @@
will not be complete (only first "page" returned)
'''
url = self.entity_url(entity_type)
params = {'filter[%s]' % fname: fval for fname, fval in filters_eq.items()}
Owner

J'ajouterais un "if fname" à la fin pour ne pas se faire emmeler les pinceaux par un filter_=toto erroné envoyé en paramètre de la requête.

params = {'filter[%s]' % fname: fval for fname, fval in filters_eq.items() if fname}
J'ajouterais un "if fname" à la fin pour ne pas se faire emmeler les pinceaux par un filter_=toto erroné envoyé en paramètre de la requête. ``` params = {'filter[%s]' % fname: fval for fname, fval in filters_eq.items() if fname} ```
Author
Owner

Bien vu, merci !

Bien vu, merci !
yweber marked this conversation as resolved
@ -0,0 +411,4 @@
'''
url = self.entity_url(entity_type)
params = {'filter[%s]' % fname: fval for fname, fval in filters_eq.items()}
if filters_like is not None:
Owner

Tu peux laisser un simple "if filters_like:" ici, None est un cas particulier mais on veut aussi ne rien faire si le dico est vide.

Tu peux laisser un simple "if filters_like:" ici, None est un cas particulier mais on veut aussi ne rien faire si le dico est vide.
Author
Owner

En effet :)

En effet :)
yweber marked this conversation as resolved
@ -0,0 +423,4 @@
- related: str ',' separated list of related entities we want data to be fetched
- relationships: bool If True add a list of related entities field names
'''
related = [rel for rel in (sp.strip() for sp in related.split(',')) if rel]
Owner

Comme on est en 2024 tu peux écrire

[rel for r in related.split(',') if (rel := r.strip())]

mais même avant := on préférait écrire

[r.strip() for r in related.split(',') if r.strip()]
Comme on est en 2024 tu peux écrire ``` [rel for r in related.split(',') if (rel := r.strip())] ``` mais même avant := on préférait écrire ``` [r.strip() for r in related.split(',') if r.strip()] ```
Owner

Comme on est en 2024 tu peux écrire

(perso toujours pas fan, toujours un gros ralentissement dans la compréhension quand je vois la forme avec := ).

> Comme on est en 2024 tu peux écrire (perso toujours pas fan, toujours un gros ralentissement dans la compréhension quand je vois la forme avec := ).
Author
Owner

J'avoue que je ne suis pas encore hyper à l'aise non plus avec le :=, je vais plutôt répéter le strip()

J'avoue que je ne suis pas encore hyper à l'aise non plus avec le `:=`, je vais plutôt répéter le `strip()`
yweber marked this conversation as resolved
@ -0,0 +441,4 @@
if relattr not in relations:
missing.append(relattr)
continue
if missing:
Owner

Mmmh ce "if missing:" va faire que le for va complétement arrêter son parcours dès qu'une relation va être absente.

Mais d'une façon générale je pense que ce missing, on s'en fiche un peu. Il faut juste ne pas aller les chercher, bien sûr, mais ne pas planter avec une APIError. On renvoie ce qu'on a, avec les relations qu'on a trouvé ; celles qu'on n'a pas, tant pis.

Mmmh ce "if missing:" va faire que le for va complétement arrêter son parcours dès qu'une relation va être absente. Mais d'une façon générale je pense que ce missing, on s'en fiche un peu. Il faut juste ne pas aller les chercher, bien sûr, mais ne pas planter avec une APIError. On renvoie ce qu'on a, avec les relations qu'on a trouvé ; celles qu'on n'a pas, tant pis.
yweber marked this conversation as resolved
@ -0,0 +449,4 @@
result['related'] = {}
result['related'][relattr] = self.entity_data(rel_data['data'])
if missing:
Owner

Vu ci-dessus : pour moi c'est dommage de crasher complétement une APIError. C'est pas grave si on ne trouve pas la relation, on renvoie l'objet sans, et voilà.

Vu ci-dessus : pour moi c'est dommage de crasher complétement une APIError. C'est pas grave si on ne trouve pas la relation, on renvoie l'objet sans, et voilà.
Author
Owner

Ok, à priori ça me fait un peu peur ce type de comportement, mais je te fais confiance sur les usages :)

Ok, à priori ça me fait un peu peur ce type de comportement, mais je te fais confiance sur les usages :)
yweber marked this conversation as resolved
@ -0,0 +463,4 @@
)
return result
def entity_data(self, entity):
Owner

J'avais raté cette fonction, mal nommée, il faudrait plus l'appeler « entity_clean » ou « entity_cleanup ». Mais pour le coup, je me pose vraiment la question de son utilité. Pourquoi effacer des informations ?

J'avais raté cette fonction, mal nommée, il faudrait plus l'appeler « entity_clean » ou « entity_cleanup ». Mais pour le coup, je me pose vraiment la question de son utilité. Pourquoi effacer des informations ?
Author
Owner

Ok, je l'ai renommé.

Pourquoi effacer des informations ?

Par ce qu'elles sont vraiment nombreuses/massive et inutiles à priori non ?

Ok, je l'ai renommé. > Pourquoi effacer des informations ? Par ce qu'elles sont vraiment nombreuses/massive et inutiles à priori non ?
yweber marked this conversation as resolved
@ -0,0 +501,4 @@
)
if not response.ok:
raise self.carlerror_to_apierror(
_('HTTP error (%%(status)d) during entitie (%s/%s) deletion: %%(detail)s') % (e_type, e_id),
Owner

entitie → entity

entitie → entity
Author
Owner

Merci !

Merci !
yweber marked this conversation as resolved
@ -0,0 +519,4 @@
except APIError as expt:
expts.append(expt)
not_cleaned.append('%s(%s)' % (e_type, e_id))
continue
Owner

plutôt utiliser else que de faire un continue

try:
     self.delete_entity(e_type, e_id)
except APIError as expt:
    expts.append(expt)
    not_cleaned.append('%s(%s)' % (e_type, e_id))
else:
    cleaned.append('%s(%s)' % (e_type, e_id))
plutôt utiliser else que de faire un continue ``` try: self.delete_entity(e_type, e_id) except APIError as expt: expts.append(expt) not_cleaned.append('%s(%s)' % (e_type, e_id)) else: cleaned.append('%s(%s)' % (e_type, e_id)) ```
yweber marked this conversation as resolved
@ -0,0 +552,4 @@
url = urllib.parse.urljoin(url + '/', urllib.parse.quote(str(part)))
else:
break
return url
Owner

Y'a plus générique à faire

url = urllib.parse.urljoin(self.service_url, 'api/entities/v1')
for part in (entity_type, entity_id, related_fieldname):
    if part is None:
        break
    url = urllib.parse.urljoin(url + '/', urllib.parse.quote(str(part)))
return url
Y'a plus générique à faire ``` url = urllib.parse.urljoin(self.service_url, 'api/entities/v1') for part in (entity_type, entity_id, related_fieldname): if part is None: break url = urllib.parse.urljoin(url + '/', urllib.parse.quote(str(part))) return url ```
Owner

Voire pire:

def entity_url(self, *parts):
    url = urllib.parse.urljoin(self.service_url, 'api/entities/v1')
    for part in parts:
        url = urllib.parse.urljoin(url + '/', urllib.parse.quote(str(part)))
return url
Voire pire: ``` def entity_url(self, *parts): url = urllib.parse.urljoin(self.service_url, 'api/entities/v1') for part in parts: url = urllib.parse.urljoin(url + '/', urllib.parse.quote(str(part))) return url ```
Author
Owner

Clairement ! Merci.

Personnellement je préfère la version moins générique, mais dis moi si tu penses que c'est mieux.

Clairement ! Merci. Personnellement je préfère la version moins générique, mais dis moi si tu penses que c'est mieux.
yweber marked this conversation as resolved
@ -0,0 +627,4 @@
Returns: a dict (with 'data' key present) representing decoded JSON
response from Carl
'''
return self._json_request(True, http_method, url, **kwargs)
Owner

Ce "True" dans l'appel n'est pas compréhensible, mais voir plus bas où je dis qu'il faut juste un retry_auth=True dans la définition de _json_request

Ce "True" dans l'appel n'est pas compréhensible, mais voir plus bas où je dis qu'il faut juste un retry_auth=True dans la définition de _json_request
yweber marked this conversation as resolved
@ -0,0 +676,4 @@
if response.status_code in (401, 403) and retry_auth and self.token_authentication:
# handling early/unexpected token expiration
self.invalidate_token()
self._json_request(False, http_method, url, **kwargs)
Owner

Pour qu'on comprenne mieux l'appel, il faudrait qu'on voit "retry_auth=False" au lieu de juste "False" (et donc poser retry_auth=True en dernier arguement de la fonction)

Pour qu'on comprenne mieux l'appel, il faudrait qu'on voit "retry_auth=False" au lieu de juste "False" (et donc poser retry_auth=True en dernier arguement de la fonction)
Author
Owner

oui

oui
yweber marked this conversation as resolved
@ -0,0 +688,4 @@
'''When needed modify given headers by reference adding authentication token
Arguments:
- headers A dict with HTTP headers or None
Returns: The modificated dict or a new instance with authentication token
Owner

Une fonction qui fait ça, je pense que c'est un nid à bogue. Il est préférable de faire l'un ou l'autre, renvoyer un nouveau dictionnaire ou modifier celui reçu. Dans le cas présent, je pense qu'on devrait juste avoir un auth_headers(self) qui renvoie le header d'authentification ; à l'appelant de l'insérer dans ses headers existants s'il en a.

Une fonction qui fait ça, je pense que c'est un nid à bogue. Il est préférable de faire l'un ou l'autre, renvoyer un nouveau dictionnaire ou modifier celui reçu. Dans le cas présent, je pense qu'on devrait juste avoir un auth_headers(self) qui renvoie le header d'authentification ; à l'appelant de l'insérer dans ses headers existants s'il en a.
Author
Owner

ok !

ok !
yweber marked this conversation as resolved
@ -0,0 +707,4 @@
token_data = cache.get(key)
if token_data is not None:
token, expiration = token_data
if expiration <= utcnow_ts():
Owner

Ce test n'a pas de sens : si le token est expiré, cache.get ne va pas le renvoyer.

Ce test n'a pas de sens : si le token est expiré, cache.get ne va pas le renvoyer.
yweber marked this conversation as resolved
@ -0,0 +712,4 @@
if token_data is None:
token, expires_in = self.fetch_token()
expiration = utcnow_ts() + expires_in
cache.set(key, (token, expiration), expires_in)
Owner

Ok je vois le truc, en fait il faut juste poser

cache.set(key, token expires_in)

Le système de cache gère l'expiration lui-même, inutile de l'enregistrer dans le cache.

Ok je vois le truc, en fait il faut juste poser ``` cache.set(key, token expires_in) ``` Le système de cache gère l'expiration lui-même, inutile de l'enregistrer dans le cache.
Author
Owner

Ok, et comme vu avec Valentin : au pire si le token en cache a expiré on a un mécanisme de retry.

Ok, et comme vu avec Valentin : au pire si le token en cache a expiré on a un mécanisme de retry.
yweber marked this conversation as resolved
yweber force-pushed wip/86683-carl-connector-implementation from 82d3c1adb5 to c0219425e3 2024-03-04 14:36:48 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from c0219425e3 to 0c7a9e4c3f 2024-03-04 14:47:07 +01:00 Compare
yweber requested review from tnoel 2024-03-04 14:50:27 +01:00
yweber force-pushed wip/86683-carl-connector-implementation from 0c7a9e4c3f to fc05b51186 2024-03-04 15:37:38 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from fc05b51186 to 34802bb624 2024-03-04 15:39:35 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from 34802bb624 to 40419eb8dd 2024-03-04 15:41:18 +01:00 Compare
tnoel requested changes 2024-03-05 13:57:27 +01:00
Dismissed
tnoel left a comment
Owner

Je pense que ce sont mes derniers commentaires ! Cette fois j'ai lu les tests et je n'ai rien à y dire, ils sont clairement complets !

Je pense que ce sont mes derniers commentaires ! Cette fois j'ai lu les tests et je n'ai rien à y dire, ils sont clairement complets !
@ -0,0 +648,4 @@
'''Invalidate token in cache'''
cache.delete(self.token_cache_key)
def fetch_token(self):
Owner

Globalement dans cette méthode tu renvoies des APIError avec err_code='authentication-error', ça serait mieux de préciser que c'est au moment du token, genre avec err_code='authentication-error[token]'. Ca nous aidera sans doute lors du support.

Globalement dans cette méthode tu renvoies des APIError avec err_code='authentication-error', ça serait mieux de préciser que c'est au moment du token, genre avec err_code='authentication-error[token]'. Ca nous aidera sans doute lors du support.
Author
Owner

ok ! Fait

ok ! Fait
yweber marked this conversation as resolved
@ -0,0 +662,4 @@
response = self.requests.post(url, data=data, timeout=2)
except requests.RequestException as expt:
raise APIError(
_('Unable to send authentication request to Carl: %s') % exception_to_text(expt),
Owner

Poser un message qui ne dise pas qu'on a un problème pour envoyer la requête, mais plutôt qu'elle échoue. On ne sait pas la nature de la panne, genre on a peut-être bien réussi à envoyer la requête et c'est le serveur qui a timeouté.

Donc plutôt juste «  Failed to get token: %s »

L'idée est qu'en cas de panne, un utilisateur du connecteur ne nous dise pas "passerelle n'arrive pas à envoyer la demande de token" en se basant sur le message d'erreur, alors que le diagnostic sera peut-être tout autre.

Poser un message qui ne dise pas qu'on a un problème pour envoyer la requête, mais plutôt qu'elle échoue. On ne sait pas la nature de la panne, genre on a peut-être bien réussi à envoyer la requête et c'est le serveur qui a timeouté. Donc plutôt juste «  Failed to get token: %s » L'idée est qu'en cas de panne, un utilisateur du connecteur ne nous dise pas "passerelle n'arrive pas à envoyer la demande de token" en se basant sur le message d'erreur, alors que le diagnostic sera peut-être tout autre.
Author
Owner

Bien vu, merci !

Bien vu, merci !
yweber marked this conversation as resolved
@ -0,0 +695,4 @@
)
err_fmt = _('Got an invalid token when authentication on Carl: %s')
missing = []
Owner

Tu te compliques un peu la vie ici. Inutile de générer une liste "missing", tu peux juste faire un APIError dans le "for key". (on se fiche un peu de savoir si l'un ou l'autre ou les deux manquent)

Tu te compliques un peu la vie ici. Inutile de générer une liste "missing", tu peux juste faire un APIError dans le "for key". (on se fiche un peu de savoir si l'un ou l'autre ou les deux manquent)
Author
Owner

ok, fait !

ok, fait !
yweber marked this conversation as resolved
@ -0,0 +13,4 @@
from passerelle.base.models import AccessRight, ApiUser
APIERROR_CLS = 'passerelle.utils.jsonresponse.APIError'
SERVICE_URL = 'http://test.carl.notld/'
Owner

Pour les URL de test, il faut utiliser les domaines de test de INIA, tels que example.net et example.org (cf https://www.iana.org/help/example-domains). Ici tu peux mettre carl.example.org par exemple

Pour les URL de test, il faut utiliser les domaines de test de INIA, tels que example.net et example.org (cf https://www.iana.org/help/example-domains). Ici tu peux mettre carl.example.org par exemple
Author
Owner

Ah je ne connaissais pas du tout ces RFC ! Merci :)

Ah je ne connaissais pas du tout ces RFC ! Merci :)
yweber marked this conversation as resolved
yweber force-pushed wip/86683-carl-connector-implementation from 40419eb8dd to f3dcf835d9 2024-03-05 14:23:36 +01:00 Compare
yweber force-pushed wip/86683-carl-connector-implementation from f3dcf835d9 to 3f947bd953 2024-03-05 14:26:57 +01:00 Compare
yweber requested review from tnoel 2024-03-05 14:27:59 +01:00
yweber force-pushed wip/86683-carl-connector-implementation from 3f947bd953 to f32d06b474 2024-03-05 17:27:54 +01:00 Compare
tnoel approved these changes 2024-03-07 16:44:52 +01:00
tnoel left a comment
Owner

Yes !

Yes !
yweber merged commit f32d06b474 into main 2024-03-07 16:56:40 +01:00
yweber deleted branch wip/86683-carl-connector-implementation 2024-03-07 16:56:40 +01:00
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No Assignees
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: entrouvert/passerelle#462
No description provided.