carl: add Carl connector (#86683) #462
Loading…
Reference in New Issue
No description provided.
Delete Branch "wip/86683-carl-connector-implementation"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
f7f7fd631b
toce06277a6f
ce06277a6f
tob1ecafd7ee
b1ecafd7ee
to7050e685b8
7050e685b8
to3d5f69cc25
3d5f69cc25
to074e913c7f
28429a2fe1
to359e9d995c
359e9d995c
to5b366506b6
5b366506b6
toaa2bddd654
aa2bddd654
to5008e456f0
5008e456f0
to4897ccf462
4897ccf462
to716ea12d4b
716ea12d4b
to7428256d53
7428256d53
toeeffd825c9
eeffd825c9
to69d5be1b77
69d5be1b77
to1c1d9bcc0c
WIP: carl: implements Token authentication & sites references (#86683)to WIP: carl: add Carl connector (#86683)1c1d9bcc0c
to5c9ceed71e
5c9ceed71e
to73ae25d2ef
73ae25d2ef
tob5b0295f75
b5b0295f75
toef2f70291d
ef2f70291d
to30f52467cd
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'},
Le singulier de entities n'est pas entity ?
Ben tristement si t_t
C'est corrigé, merci !
@ -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',
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)Fait, merci
@ -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',
J'ai la vague impression qu'un
'type': 'bool'
est supporté iciBien vue ! Merci :)
@ -0,0 +148,4 @@
},
},
)
def entitie(self, request, **kwargs):
À 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
(à vérifier)
J'y ai clairement pensé, mais comme "id" et "type" sont des builtins je suis pas forcément fan comme nom d'argument.
@ -0,0 +184,4 @@
data={'code': 'not-found', 'url': url},
)
if 'relationships' not in data['data']:
La forme sur une ligne me paraît assez claire
relations = data['data'].get('relationships') or {}
Bien vu ! Fait, merci
@ -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
Pas d'espace avant les : et already un seul l :)
Il y en avait partout (des espaces avant les : ...) merci !
@ -0,0 +380,4 @@
return {'data': {'status': _('No token in cache')}}
expiration = tok_infos[1]
ts = datetime.datetime.fromtimestamp(expiration, datetime.timezone.utc)
Souvent on enclôt explicitement ce genre d'expression dans un bool(), je trouve ça plus sympa à lire
ok, fait
@ -0,0 +390,4 @@
@property
def token_authentication(self):
'''Is True when token authentication wanted'''
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)
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 ;)
Juste
can_authenticate
?@ -0,0 +436,4 @@
TODO : handle paginated results
'''
filter_params = None
if entitie_id is not None:
Plutôt que ça, à l'endroit où la méthode est appelée on peut faire
et dans la méthode plus simplement juste raise ValueError('une chaine')
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)
@ -0,0 +509,4 @@
- e_id : Entitie ID
'''
url = self.entitie_url(e_type, e_id)
try:
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
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
@ -0,0 +595,4 @@
Handles common error cases.
Handles token authentication.
Defaults timeout to 2s.
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 :
C'est vrai, fait, merci !
@ -0,0 +607,4 @@
'''
return self._json_request(True, http_method, url, **kwargs)
def _json_request(self, retry_auth, http_method, url, **kwargs):
Directement
getattr(self.requests, http_method)(url, **kwargs)
me paraîtrait bienGitea a perdu la ligne sur laquelle j'avais posé ce commentaire, il s'agit de :
Merci, mais même si cç me semble un détail, mais je préfère avoir le getattr en dehors du try.
@ -0,0 +658,4 @@
return data
else:
err_fmt = _('Got an HTTP error code (%d) from Carl : %s')
Ç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
Gitea a perdu la ligne sur laquelle j'avais posé ce commentaire, il s'agit de :
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() )
@ -0,0 +679,4 @@
src = '(%s)' % src
else:
src = ''
if error['title'] == error['detail']:
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)
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
30f52467cd
to1e3f3e8ed6
1e3f3e8ed6
to668f98aaf0
668f98aaf0
toecf60c555f
ecf60c555f
tob536495ba0
b536495ba0
todbc48cf9ee
dbc48cf9ee
to832f98b6ab
832f98b6ab
tof448276646
f448276646
todacedaf3d3
dacedaf3d3
toc90fb2026d
c90fb2026d
to0ca0555acd
0ca0555acd
toadd1b5388f
add1b5388f
tof02827b78b
f02827b78b
to379a790601
379a790601
to37e8699aa6
37e8699aa6
to2d019663af
2d019663af
toe7d65466f6
e7d65466f6
tod697301c3e
d697301c3e
to2996c4b214
2996c4b214
to7fc29b9b53
7fc29b9b53
tof1bab7535b
f1bab7535b
to7224348379
7224348379
to726c2a34b3
726c2a34b3
toe4b3bab87c
e4b3bab87c
tod79b7698fd
d79b7698fd
tofd805cbca8
fd805cbca8
to519fdffd6c
519fdffd6c
to825610ed06
29ddbc1289
to0cc501cbfe
0cc501cbfe
to6804af40c4
6804af40c4
toc400da3f78
c400da3f78
toce2b5b5fed
ce2b5b5fed
to19be63b4d2
WIP: carl: add Carl connector (#86683)to carl: add Carl connector (#86683)19be63b4d2
to7663074248
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
On est en 2024 ;)
Bien vu ! Merci
@ -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/'),
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/ ».
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 interrogerhttps://carlsource.server.com/gmaoCS02/public/status
.La ce n'est pas en place car la redirection qu'on a nous limite à
/api/
@ -0,0 +129,4 @@
)
carl_username = models.CharField(
max_length=128, verbose_name=_('Carl token authentication username'), blank=False
Plutôt blank=True : on ne sait jamais, permettons un usage sans token.
Oui, d'autant que c'est prévu pour être optionnel dans le reste du code ;) Merci !
@ -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
Même chose, plutôt blank=True.
@ -0,0 +143,4 @@
@endpoint(
name='entity',
pattern=r'^by_id$',
example_pattern='entity/by_id',
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 :
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 :
/entities
tel quel/entity/by_id
en/entity/(?P<carl_type>...)/(?P<carl_id>...)$
/entity/by_values
en/entity/(?P<carl_type>...)$
@ -0,0 +175,4 @@
},
},
'properties': {
'carl_relationships': {
é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.
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 ?
@ -0,0 +183,4 @@
},
)
def entity_by_id(self, request, **kwargs):
missing = [param for param in ('type', 'id') if param not in kwargs]
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:
et notre décorateur @endpoint fera le travail de l'erreur 400, gratuitement :)
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
ettype
.Si ça ne vous fait pas plus peur que ça je passe deux arguments
id
ettype
:)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.@ -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):
De façon plus pythonique, faire juste un "if missing:" pour savoir si une liste n'est pas vide.
ok, merci, fait :)
@ -0,0 +191,4 @@
carl_type = kwargs.pop('type')
carl_id = kwargs.pop('id')
want_relationships = kwargs.pop('relationships', False)
Pour ce genre de cas, ajouter un « relationships=False » dans les arguments de la fonction.
@ -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]
Ici aussi remplacer "if len(rel)>0" par un simple "if rel"
@ -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
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.
Normalement j'ai fais le tour, il n'en reste plus dans
models.py
@ -0,0 +211,4 @@
@endpoint(
name='entity',
pattern=r'^by_values$',
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=...
@ -0,0 +296,4 @@
@endpoint(
name='entity',
pattern='^create$',
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 ».
C'est fait
@ -0,0 +311,4 @@
},
},
'properties': {
'carl_id': {'type': 'string'},
Comme vu plus haut, inutile de rappeler le nom "card", laisser juste id et type.
@ -0,0 +316,4 @@
},
},
)
def create_entity_endpoint(self, request, post_data):
Nommer juste « create_entity », on sait que c'est un endpoint.
Et je préfixe la méthode
create_entity
par un underscore (celle qui n'est pas un endpoint) ?@ -0,0 +436,4 @@
)
missing = [field for field in ('id', 'type', 'attributes') if field not in entity['data']]
if len(missing) > 0:
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.
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 ;)
@ -0,0 +526,4 @@
)
@endpoint(
name='token_status',
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).
Comme indiqué dans la description c'était un endpoint de debug, j'avais complètement oublié de le supprimé, merci ;)
@ -0,0 +588,4 @@
Returns a dict suitable to be returned by an endpoint
'''
related = [] if related is None else related
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).
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).
@ -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
Sauf cas très particuliers, on ne fait jamais de pagination dans les webservices, c'est bien trop complexe à gérer.
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.
@ -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()
]
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 :
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 :)
@ -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)
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.
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 ?
@ -0,0 +722,4 @@
'''
url = self.entity_url(entity['data']['type'])
headers = {'Content-Type': 'application/vnd.api+json'}
data = json.dumps(entity)
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).
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 :)
@ -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
Je ne comprends pas l'utilité de cette fonction, requests fait cet encodage tout seul...?
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
@ -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/')
Rappel : je propose que service_url contienne le /api/, on pourra donc juste faire le urljoin avec 'entities/v1'
@ -0,0 +812,4 @@
break
return url
def filters_from_qsdict(self, qsdict):
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)
7663074248
to83469e6778
83469e6778
toe9ec8ca9ff
e9ec8ca9ff
to2a1c25b696
2a1c25b696
toe45df9968b
e45df9968b
to37d333777a
7ac2dcd353
to1e8339862b
1e8339862b
to5d309f8a00
5d309f8a00
todf0dc82dc0
carl: add Carl connector (#86683)to WIP: carl: add Carl connector (#86683)df0dc82dc0
tof21d0ef303
f21d0ef303
toe03baaa8cf
e03baaa8cf
to60630f146b
60630f146b
to3b534f3fb3
3b534f3fb3
to5bb003c5e8
5bb003c5e8
toab9669f4c7
ab9669f4c7
toa988d2be39
a988d2be39
to04a59bf7e7
04a59bf7e7
tocf970166a0
cf970166a0
to82d3c1adb5
WIP: carl: add Carl connector (#86683)to carl: add Carl connector (#86683)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'],
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.
En effet :)
@ -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'},
Plutôt utiliser err_code='creation-error' que mettre des données dans data.
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'unerr_code
en argument ? Sinon je remplace tout cescode
dans data par deserr_code
en argument.J'ai remplacé par err_code partout.
@ -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'},
Même chose, plutôt err_code
@ -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)
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.
En fait je me dis que tu pourrais envoyer un err_code='creation-error[rollback-on-related]' dans le APIError
ok :)
@ -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')
Idem, poser le mot "rollback" dans le message (là je te laisse trouver comment, l'anglais et moi...)
Ou plus simplement err_code='creation-error[rollback]'
Fait
@ -0,0 +281,4 @@
data=data,
)
missing = [field for field in ('id', 'type', 'attributes') if field not in entity['data']]
Je pensais qu'on était d'accord pour abandonner ce rollback, et considérer juste le renvoie de entity['data']:
Ah ok, c'est pas ce que j'avais compris, mais ça me va !
@ -0,0 +315,4 @@
},
'id': {
'description': _('Only return the entity with corresponding ID'),
},
Il manque l'entrée 'q' dans ces parameters.
@ -0,0 +356,4 @@
)
result = [result]
else:
if q is not None:
"if q:" va suffire, on veut pas faire un LIKE vide.
@ -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
En fait text_attrname is None ça ne peut pas arriver, pour moi cette ligne est inutile.
effectivement !
@ -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]
Je trouve que ça fait un peu mal au yeux. Je préfère ce format de code:
ok !
@ -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
C'est filter_eq dont on parle ici
@ -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()}
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.
Bien vu, merci !
@ -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:
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.
En effet :)
@ -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]
Comme on est en 2024 tu peux écrire
mais même avant := on préférait écrire
(perso toujours pas fan, toujours un gros ralentissement dans la compréhension quand je vois la forme avec := ).
J'avoue que je ne suis pas encore hyper à l'aise non plus avec le
:=
, je vais plutôt répéter lestrip()
@ -0,0 +441,4 @@
if relattr not in relations:
missing.append(relattr)
continue
if missing:
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.
@ -0,0 +449,4 @@
result['related'] = {}
result['related'][relattr] = self.entity_data(rel_data['data'])
if missing:
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à.
Ok, à priori ça me fait un peu peur ce type de comportement, mais je te fais confiance sur les usages :)
@ -0,0 +463,4 @@
)
return result
def entity_data(self, entity):
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 ?
Ok, je l'ai renommé.
Par ce qu'elles sont vraiment nombreuses/massive et inutiles à priori non ?
@ -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),
entitie → entity
Merci !
@ -0,0 +519,4 @@
except APIError as expt:
expts.append(expt)
not_cleaned.append('%s(%s)' % (e_type, e_id))
continue
plutôt utiliser else que de faire un continue
@ -0,0 +552,4 @@
url = urllib.parse.urljoin(url + '/', urllib.parse.quote(str(part)))
else:
break
return url
Y'a plus générique à faire
Voire pire:
Clairement ! Merci.
Personnellement je préfère la version moins générique, mais dis moi si tu penses que c'est mieux.
@ -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)
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
@ -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)
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)
oui
@ -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
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.
ok !
@ -0,0 +707,4 @@
token_data = cache.get(key)
if token_data is not None:
token, expiration = token_data
if expiration <= utcnow_ts():
Ce test n'a pas de sens : si le token est expiré, cache.get ne va pas le renvoyer.
@ -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)
Ok je vois le truc, en fait il faut juste poser
Le système de cache gère l'expiration lui-même, inutile de l'enregistrer dans le cache.
Ok, et comme vu avec Valentin : au pire si le token en cache a expiré on a un mécanisme de retry.
82d3c1adb5
toc0219425e3
c0219425e3
to0c7a9e4c3f
0c7a9e4c3f
tofc05b51186
fc05b51186
to34802bb624
34802bb624
to40419eb8dd
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):
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.
ok ! Fait
@ -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),
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.
Bien vu, merci !
@ -0,0 +695,4 @@
)
err_fmt = _('Got an invalid token when authentication on Carl: %s')
missing = []
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)
ok, fait !
@ -0,0 +13,4 @@
from passerelle.base.models import AccessRight, ApiUser
APIERROR_CLS = 'passerelle.utils.jsonresponse.APIError'
SERVICE_URL = 'http://test.carl.notld/'
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
Ah je ne connaissais pas du tout ces RFC ! Merci :)
40419eb8dd
tof3dcf835d9
f3dcf835d9
to3f947bd953
3f947bd953
tof32d06b474
Yes !