Projet

Général

Profil

Bug #13184

Ne pas émettre de 403 sur une requête à /api/user/forms avec un NameID inconnu

Ajouté par Benjamin Dauvergne il y a plus de 7 ans. Mis à jour il y a plus de 5 ans.

Statut:
Fermé
Priorité:
Normal
Assigné à:
Version cible:
-
Début:
14 septembre 2016
Echéance:
% réalisé:

0%

Temps estimé:
Patch proposed:
Oui
Planning:

Description

Vu à Montpellier en arrivant sur combo après la création d'un nouvel utilisateur, on a:

failed to get data from any /api/user/forms (set([403]))

Le endpoint devrait simplement renvoyer une liste vide dans ce cas.


Fichiers

0001-make-api-user-form-returns-on-unknown-NameID-13184.patch (1,07 ko) 0001-make-api-user-form-returns-on-unknown-NameID-13184.patch Benjamin Dauvergne, 14 septembre 2016 16:21
0001-add-HttpRequest.get_version-to-help-in-API-versionni.patch (868 octets) 0001-add-HttpRequest.get_version-to-help-in-API-versionni.patch Benjamin Dauvergne, 29 septembre 2016 13:47
0002-add-new-version-to-api-user-form-API-fixes-13184.patch (5,18 ko) 0002-add-new-version-to-api-user-form-API-fixes-13184.patch Benjamin Dauvergne, 29 septembre 2016 13:54
0001-add-new-version-to-api-user-form-API-fixes-13184.patch (9,28 ko) 0001-add-new-version-to-api-user-form-API-fixes-13184.patch Benjamin Dauvergne, 30 septembre 2016 09:18
0001-change-schema-of-api-formdefs-api-user-forms-and-api.patch (20 ko) 0001-change-schema-of-api-formdefs-api-user-forms-and-api.patch Benjamin Dauvergne, 09 octobre 2016 18:54
0001-change-schema-of-api-formdefs-api-user-forms-and-api.patch (20,2 ko) 0001-change-schema-of-api-formdefs-api-user-forms-and-api.patch Benjamin Dauvergne, 09 octobre 2016 18:55
0001-api-change-schema-of-formdefs-user-forms-and-user-dr.patch (21,2 ko) 0001-api-change-schema-of-formdefs-user-forms-and-user-dr.patch Benjamin Dauvergne, 28 mars 2017 15:32
0001-api-change-schema-of-formdefs-user-forms-and-user-dr.patch (21,3 ko) 0001-api-change-schema-of-formdefs-user-forms-and-user-dr.patch Thomas Noël, 04 novembre 2017 03:36
0001-api-change-schema-of-formdefs-user-forms-and-user-dr.patch (24,9 ko) 0001-api-change-schema-of-formdefs-user-forms-and-user-dr.patch Thomas Noël, 16 janvier 2018 11:38
0001-api-change-schema-of-formdefs-user-forms-and-user-dr.patch (25,2 ko) 0001-api-change-schema-of-formdefs-user-forms-and-user-dr.patch Thomas Noël, 16 janvier 2018 12:07
0001-api-change-schema-of-formdefs-user-forms-and-user-dr.patch (25,6 ko) 0001-api-change-schema-of-formdefs-user-forms-and-user-dr.patch Frédéric Péters, 16 janvier 2018 12:21

Demandes liées

Lié à Combo - Development #13521: wcs : gérer la transition des API passant d'un retour de type liste à un retour de type {"data": []}Fermé09 octobre 2016

Actions
Lié à Publik - Bug #18748: trace d'erreurs 403 quand l'utilisateur vient d'être crée, le temps du provisionningRejeté17 septembre 2017

Actions
Lié à Publik - Development #19853: Provisionning "full"Fermé02 novembre 201715 décembre 2017

Actions
Lié à OLAP / Business Intelligence pour Publik - Development #20383: adapter wcs_api pour le changement de format retour de /api/formdefs/Fermé01 décembre 2017

Actions

Révisions associées

Révision 5cc38478 (diff)
Ajouté par Benjamin Dauvergne il y a environ 6 ans

api: change schema of formdefs, user/forms and user/drafts APIs (#13184)

All responses now have the form {"err": x, "data": y}.

Historique

#2

Mis à jour par Frédéric Péters il y a plus de 7 ans

Je ne suis pas sûr de vouloir ça1; mais si on fait ça il faut au moins que les autres endpoints sous /api/user/ se comportent de la même manière. (et des tests sur ces situations)

[1] (d'abord parce que ça vient comme réponse à une lenteur du provisioning)

#3

Mis à jour par Benjamin Dauvergne il y a plus de 7 ans

Je peux aussi renvoyer un 404 pour pouvoir différencier les situations (il faut qu'on continue de considérer les 403, i.e. erreurs de signature comme des erreurs) mais il me faudra ensuite modifier le code des cellules. Dis mois ton choix entre 404 ou renvoyer une information vide et je ferai la modification aux autres endpoints.

#4

Mis à jour par Frédéric Péters il y a plus de 7 ans

Absolument pas enthousiaste sur la 404, mais ce que je proposerais c'est retourner {"err": 1, "data": []}, et modifier le endpoint pour également retourner cette structure quand tout va bien; et là il est peut-être nécessaire d'avoir une url d'endpoint différente pour ne pas casser la compatibilité (je ne sais pas trop les appels qui peuvent être faits à Vincennes), puis modifier combo en conséquence.

#5

Mis à jour par Benjamin Dauvergne il y a plus de 7 ans

Nouvelle proposition, j'ai ajouté une méthode à HttpRequest pour aider dans le versionning des APIs, il suffit d'ajouter &api_version=x pour obtenir une version particulière, par défaut get_request().get_api_version() renvoie 0.

Ici l'api /user/forms/ continue à fonctionner normalement, mais si api_version > 0 alors on retourne le nouveau format.

#6

Mis à jour par Benjamin Dauvergne il y a plus de 7 ans

Avec mise à jour de la doc.

Peut-être au passage devrais-je traiter les autres endpoints autour de l'utilisateur (/api/user et /api/user/drafts).

#7

Mis à jour par Benjamin Dauvergne il y a plus de 7 ans

  • Assigné à mis à Benjamin Dauvergne
#8

Mis à jour par Frédéric Péters il y a plus de 7 ans

J'ai regardé ce qui était utilisé comme API à Vincennes et user/forms n'en fait pas partie; c'est donc uniquement utilisé par Combo. Ma préférence du coup c'est de simplement modifier l'API dans w.c.s. après avoir modifié combo pour gérer correctement la présente et la nouvelle API (#13521).

Très basiquement, la partie "retours" de ce patch deviendrait :

--- a/wcs/api.py
+++ b/wcs/api.py
@@ -461,7 +461,7 @@ class ApiUserDirectory(Directory):
                 d.update(form.get_json_export_dict(include_files=False))
             drafts.append(d)

-        return json.dumps(drafts,
+        return json.dumps({'err': 0, 'data': drafts},
                 cls=misc.JSONEncoder,
                 encoding=get_publisher().site_charset)

@@ -499,7 +499,7 @@ class ApiUserDirectory(Directory):
                 d.update(form.get_json_export_dict(include_files=False))
             forms.append(d)

-        return json.dumps(forms,
+        return json.dumps({'err': 0, 'data': forms},
                 cls=misc.JSONEncoder,
                 encoding=get_publisher().site_charset)

#9

Mis à jour par Frédéric Péters il y a plus de 7 ans

Dans la partie "ne pas retourner de liste mais un dictionnaire avec une clé data contenant la liste", il y a aussi le retour du _q_index de ApiFormdefsDirectory.

#10

Mis à jour par Benjamin Dauvergne il y a plus de 7 ans

J'ai supprimé tout ce qui concerne api_version, j'ai voulu passé aussi sur /api/user puis je me suis dit que ça pouvait attendre.

#12

Mis à jour par Frédéric Péters il y a environ 7 ans

  • Lié à Development #13521: wcs : gérer la transition des API passant d'un retour de type liste à un retour de type {"data": []} ajouté
#13

Mis à jour par Frédéric Péters il y a environ 7 ans

À rebaser, + préfixer le message de commit par "api: ".

Dans la documentation il y a aussi un exemple de retour de /api/user/drafts, à actualiser également.

#14

Mis à jour par Benjamin Dauvergne il y a environ 7 ans

Rebasé, préfixe "api: " ajouté au message de commit et exemple pour /api/user/drafts modifié. (j'ai aussi modifié l'adresse qui était /myspace/drafts dans l'exemple, par /api/user/drafts.

#15

Mis à jour par Thomas Noël il y a plus de 6 ans

  • Lié à Bug #18748: trace d'erreurs 403 quand l'utilisateur vient d'être crée, le temps du provisionning ajouté
#16

Mis à jour par Thomas Noël il y a plus de 6 ans

Rebasé, parce que je ne vois pas d'autre moyen d'éviter les alertes "403" de combo sur un usager tout neuf.

#17

Mis à jour par Benjamin Dauvergne il y a plus de 6 ans

Et qui c'est qui va acker si tu modifies mon patch ? :) C'est malin faut qu'on trouve une troisième personne maintenant.

#18

Mis à jour par Thomas Noël il y a plus de 6 ans

:-D

On peut considérer qu'il est acké, allez... mais on va de toute façon attendre la MEP de jeudi 9 avant de pusher, donc plutôt attendre mardi 14 (et d'ici là, si un liégeois de passage...).

#19

Mis à jour par Frédéric Péters il y a plus de 6 ans

J'ai regardé ce qui était utilisé comme API à Vincennes et user/forms n'en fait pas partie; c'est donc uniquement utilisé par Combo. Ma préférence du coup c'est de simplement modifier l'API dans w.c.s. après avoir modifié combo pour gérer correctement la présente et la nouvelle API (#13521).

Je me trouvais donc à faire ce travail mais c'était "il y a environ un an". Avant de reprendre ici, il me semble nécessaire de refaire un tour exhaustif de nos installations.

#20

Mis à jour par Frédéric Péters il y a plus de 6 ans

#23

Mis à jour par Benjamin Dauvergne il y a plus de 6 ans

  • Lié à Development #20383: adapter wcs_api pour le changement de format retour de /api/formdefs/ ajouté
#25

Mis à jour par Thomas Noël il y a plus de 6 ans

Avec le patch proposé par Benjamin, on ne change pas tout :

Les listes qui passent en {"err": 0, "data": ...} :
  • /api/formdefs/ et /api/category/<slug>/formdefs
  • /api/user/forms
  • /api/user/drafts/
Mais reste en donnée "liste" directe :
  • /api/forms/<formdef>/list

Donc je proposer de migrer ça aussi, afin que ceux qui jouent avec les API n'aient pas à deviner où ça a changé : quand c'était une liste, c'est maintenant une err/data. Ça simplifiera le discours à tenir pour informer du changement.

(avis bienvenus)

#26

Mis à jour par Frédéric Péters il y a plus de 6 ans

(avis bienvenus)

Déjà arriver à faire évoluer Vincennes pour gérer les deux; et puis pour /api/forms/<formdef>/list il y aura aussi Liège qui utilise cette API.

#27

Mis à jour par Benjamin Dauvergne il y a plus de 6 ans

Le versionning c'était pas si mal finalement :)

#30

Mis à jour par Frédéric Péters il y a environ 6 ans

Les tests font disparaitre un test sur form_receipt_datetime :

-    assert len(resp.json) == 1
-    assert resp.json[0]['form_name'] == 'test'
-    assert resp.json[0]['form_slug'] == 'test'
-    assert resp.json[0]['form_status'] == 'New'
-    assert datetime.datetime.strptime(resp.json[0]['form_receipt_datetime'],
-                                      '%Y-%m-%dT%H:%M:%S')
-    assert resp.json[0]['keywords'] == ['hello', 'world']
+    assert resp.json['err'] == 0
+    assert len(resp.json['data']) == 1
+    assert resp.json['data'][0]['form_name'] == 'test'
+    assert resp.json['data'][0]['form_slug'] == 'test'
+    assert resp.json['data'][0]['form_status'] == 'New'
+    assert resp.json['data'][0]['keywords'] == ['hello', 'world']

Je l'ai remis.

Il y a aussi quelques appels qui se font avec ?&params=... dans l'URL, je les ai doublé en ?params=... pour garder la forme qui sera communément utilisée par les appelants.

#31

Mis à jour par Thomas Noël il y a environ 6 ans

  • Statut changé de En cours à Résolu (à déployer)

Parfait, merci ; j'ai poussé ainsi.

commit 5cc3847843e9106cc343a6ed1b2ee98e26b9fe12
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Sat Nov 4 03:01:13 2017 +0100

    api: change schema of formdefs, user/forms and user/drafts APIs (#13184)

    All responses now have the form {"err": x, "data": y}.

#32

Mis à jour par Frédéric Péters il y a plus de 5 ans

  • Statut changé de Résolu (à déployer) à Solution déployée

Formats disponibles : Atom PDF