Bug 27716 - UMC Modul-Entwickler sollten shortcuts an die Hand bekommen
UMC Modul-Entwickler sollten shortcuts an die Hand bekommen
Status: CLOSED FIXED
Product: UCS
Classification: Unclassified
Component: UMC (Generic)
UCS 3.0
Other Linux
: P3 enhancement (vote)
: UCS 3.1
Assigned To: Dirk Wiesenthal
Florian Best
: interim-2
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-21 17:40 CEST by Dirk Wiesenthal
Modified: 2012-12-12 21:09 CET (History)
3 users (show)

See Also:
What kind of report is it?: ---
What type of bug is this?: ---
Who will be affected by this bug?: ---
How will those affected feel about the bug?: ---
User Pain:
Enterprise Customer affected?:
School Customer affected?:
ISV affected?:
Waiting Support:
Flags outvoted (downgraded) after PO Review:
Ticket number:
Bug group (optional): Usability
Max CVSS v3 score:


Attachments
univention-system-setup: Verschlankung der einfachen Funktionen (4.45 KB, text/plain)
2012-06-21 17:40 CEST, Dirk Wiesenthal
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Wiesenthal univentionstaff 2012-06-21 17:40:49 CEST
Created attachment 4464 [details]
univention-system-setup: Verschlankung der einfachen Funktionen

UMCP ist zwar sehr mächtig, aber Entwickler, die auf der Schnittstelle aufbauen
wollen, müssen sich ganz schön in die Dokumentation vertiefen.

Sicherlich gibt es spätestens mit unseren Packages genügend Beispielcode, bei
dem man sich bedienen kann. Dennoch sind gerade die einfachen Dinge, die man
immer wieder machen möchte, unnötig kompliziert.

Den Server und das zugrunde liegende Protokoll sollte man allein schon wegen
seiner Mächtigkeit beibehalten, aber wir Entwickler brauchen eine einfachere
API.

Ich hänge mal ein Beispiel, wie ich mir als Entwickler eine Funktion auf
Python-Modul-Ebene wünschen würde, an.

Vielleicht kann man den Weg noch weiter gehen.
Comment 1 Alexander Kläser univentionstaff 2012-06-21 18:50:06 CEST
Cool :) . Ggf. könnte man das auch so erweitern, dass man den Parametern Typen wie str, list, dict zuweisen kann, damit auch direkt die Typen überprüft werden, das braucht auch immer extra Code und würde die Methoden vereinfachen.
Comment 2 Andreas Büsching univentionstaff 2012-06-22 08:52:10 CEST
Finde ich auch super! Der Decorator könnte wunderbar in umc/modules/__init__.py mit übernommen werden

Momentan ist geplant in Python Notifier 0.10 noch eine Vereinfachung für die Verwendung von Threads zu integrieren mit der dann auch threaded functions "einfach" sind.
Comment 3 Florian Best univentionstaff 2012-07-24 21:44:07 CEST
(In reply to comment #2)
> Finde ich auch super! Der Decorator könnte wunderbar in umc/modules/__init__.py
> mit übernommen werden
Der patch bedarf meiner Meinung nach noch einige kleinere Anpassungen:
Soweit ich das sehe kann momentan keine message, success, status in der methode selbst gesetzt werden. (manuell kann finished aufgerufen werden, der nächste response würde verworfen werden).
Außerdem geht der patch davon aus, dass request.options ein dict ist. Es werden aber oft listen verwendet, die dann dicts enthalten.

(In reply to comment #1)
> Cool :) . Ggf. könnte man das auch so erweitern, dass man den Parametern Typen
> wie str, list, dict zuweisen kann, damit auch direkt die Typen überprüft
> werden, das braucht auch immer extra Code und würde die Methoden vereinfachen.
Dafür sollte man klären, ob das komplex in den syntax XML dateien gemacht werden kann. Dann braucht der python code gar nicht mehr überprüfen, ob illegale Eingaben ankommen.

Was noch eingebaut werden könnte wäre logging.
Normalerweise werden request und response geloggt, sensitive daten müssen rausgeschnitten werden (eventuell mehrere decoratoren erstellen?).

Aus dem "raise e" würde ich ein "raise" machen, damit die Position/Traceback sich nicht ändert.
Comment 4 Dirk Wiesenthal univentionstaff 2012-07-30 09:00:12 CEST
(In reply to comment #3)
> (In reply to comment #2)
> > Finde ich auch super! Der Decorator könnte wunderbar in umc/modules/__init__.py
> > mit übernommen werden
> Der patch bedarf meiner Meinung nach noch einige kleinere Anpassungen:
> Soweit ich das sehe kann momentan keine message, success, status in der methode
> selbst gesetzt werden. (manuell kann finished aufgerufen werden, der nächste
> response würde verworfen werden).
> Außerdem geht der patch davon aus, dass request.options ein dict ist. Es werden
> aber oft listen verwendet, die dann dicts enthalten.
Ja, aber das ist ja ein @simple_response. Das muss ja nicht der einzige Decorator bleiben (@list_response oder so...). Wenn man volle Kontrolle braucht, dann sollte man ihn gar nicht verwenden, um das self.finished selbst anzustoßen. Das ist nur für die einfachsten Methoden gedacht, die im Grunde auch nur zwei Ausgänge kennen (alles in Ordnung und irgendwas ist schief gelaufen).

> 
> (In reply to comment #1)
> > Cool :) . Ggf. könnte man das auch so erweitern, dass man den Parametern Typen
> > wie str, list, dict zuweisen kann, damit auch direkt die Typen überprüft
> > werden, das braucht auch immer extra Code und würde die Methoden vereinfachen.
> Dafür sollte man klären, ob das komplex in den syntax XML dateien gemacht
> werden kann. Dann braucht der python code gar nicht mehr überprüfen, ob
> illegale Eingaben ankommen.
Syntax-Überprüfungen hatte ich mal zu "Forschungszwecken" drin, aber da der SyntaxManager momentan nicht aktiviert ist, habe ich ihn rausgelassen. Ich weise aber auch hier darauf hin, dass es sich bei @simple_response um einen Vorschlag für eine (von vielen) Hilfsfunktionen handelt, die Entwicklern das Leben leichter machen sollen. Man könnte die Originalfunktion ja sowohl mit @simple_response als auch mit @syntax erweitern.
Ich hatte vor ein paar Tagen ein Pad dazu aufgemacht.

> 
> Was noch eingebaut werden könnte wäre logging.
> Normalerweise werden request und response geloggt, sensitive daten müssen
> rausgeschnitten werden (eventuell mehrere decoratoren erstellen?).
Gute Idee. Auch die Lösung gefällt mir.

> 
> Aus dem "raise e" würde ich ein "raise" machen, damit die Position/Traceback
> sich nicht ändert.
Das sollte in der Tat gemacht werden
Comment 5 Dirk Wiesenthal univentionstaff 2012-08-01 13:40:34 CEST
Erster Schritt: simple_response ähnlich wie im patch, nur sehr viel weniger error-handling. Das sollte so weit möglich in UMC direkt gelöst werden.

  univention-management-console (5.0.6-1)
Comment 6 Dirk Wiesenthal univentionstaff 2012-08-01 13:52:55 CEST
1. Nutznießer/Versuchskaninchen:

 univention-management-console-module-services (2.0.0-1)

services/query
Comment 7 Dirk Wiesenthal univentionstaff 2012-08-07 10:34:43 CEST
simple_response kann jetzt with_flavor aufgerufen werden.
  univention-management-console (5.0.8-1)

Erprobt wird das in udm/containers
  univention-management-console-module-udm (2.0.3-1)
Comment 8 Florian Best univentionstaff 2012-08-07 11:28:48 CEST
Es wurden 2 weitere decorator hinzugefügt:
check_request_options → überprüft den typ von request.options (deprecated wenn das endlich mal über syntax-definitionen funktionieren würde)
log_request_options → loggt die request options und löscht sensitive daten (z.b. Passwörter) rekursiv.

Sinnvoll wäre jetzt noch ein decorator, der den response loggen kann.
Comment 9 Dirk Wiesenthal univentionstaff 2012-08-08 20:08:53 CEST
In
  univention-management-console (5.0.10-1)

gibt es jetzt eine Möglichkeit auf valide Argumente zu prüfen. Siehe dazu auch Bug 27720.

@sanitizer(variable=IntegerSanitizer())
def myfunc(self, request):
  ...

Es gibt auch eine ganz neue Datei und damit eine Basisklasse, die man nutzen kann um eigene Sanitizer zu schreiben.
Comment 10 Dirk Wiesenthal univentionstaff 2012-08-10 11:57:36 CEST
In
  univention-management-console (5.0.11-1)

ist ein log-decorator hinzugekommen (nutzbar zusammen mit simple_response oder multi_reponse)
Comment 11 Dirk Wiesenthal univentionstaff 2012-09-25 10:05:49 CEST
"Fertig" in
  univention-management-console 5.0.29-1.679.201209250955

Es ist ein erster Schwall an Decorators implementiert worden, die auch schon genutzt werden (z.B. in packages, services). Besonders wichtig: @simple_response

QA: Dokumentation prüfen und nach diesen Gesichtspunkten testen. Was nicht dokumentiert ist, wird auch nicht offiziell unterstützt. Ich habe z.B. für den Umgang mit dem ModuleStore oder der LDAP-Connection ein paar Hacks eingebaut, die aber obsolet werden, sollte man das einmal grundlegend angehen und die dann auch wieder rausfliegen würden.
Comment 12 Florian Best univentionstaff 2012-10-19 11:26:36 CEST
Ich habe eine Sache auszusetzen:

Der multi_response ist echt nicht schön:
===================================================================
     @multi_response
     def my_multi_func(self, iterator, variable1, variable2=''):
       # here, variable1 and variable2 are yet to be initialised
       # i.e. variable1 and variable2 will be None!
       do_some_initial_stuff()
       try:
         for variable1, variable2 in iterator:
           # now they are set
           yield '%s_%s' % (self._saved_dict[variable1], variable2)
       except KeyError:
         raise UMC_CommandError('Something went wrong')
       else:
         # only when everything went right...
         do_some_cleanup_stuff()
===================================================================
Das macht nichts einfacher sondern eher komplizierter und abhängiger.

Jemand, der cleanup und initial braucht benutzt halt keine decorators!

Einfacher:
===================================================================
@multi_response
def my_multi_func(self, value1, value2):
     yield '%s_%s' % (self._saved_dict[variable1], variable2)
===================================================================

Diese verkomplizierung existiert, weil der objectStore im frondend so hässliche requests sendet.
Ich würde vorschlagen, den aktuell existierenden multi_response in _object_store umzubennen,der dann an diese requests angepasst ist, und einen multi_response hinzuzufügen, der simpel zu benutzen ist ist.
Comment 13 Florian Best univentionstaff 2012-10-19 11:57:57 CEST
(In reply to comment #12)
Hmm, das wird jetzt so gelassen → OK
Comment 14 Dirk Wiesenthal univentionstaff 2012-10-19 12:11:04 CEST
(In reply to comment #12)
> Ich habe eine Sache auszusetzen:
> 
> Der multi_response ist echt nicht schön:
> ===================================================================
>      @multi_response
>      def my_multi_func(self, iterator, variable1, variable2=''):
>        # here, variable1 and variable2 are yet to be initialised
>        # i.e. variable1 and variable2 will be None!
>        do_some_initial_stuff()
>        try:
>          for variable1, variable2 in iterator:
>            # now they are set
>            yield '%s_%s' % (self._saved_dict[variable1], variable2)
>        except KeyError:
>          raise UMC_CommandError('Something went wrong')
>        else:
>          # only when everything went right...
>          do_some_cleanup_stuff()
> ===================================================================
> Das macht nichts einfacher sondern eher komplizierter und abhängiger.
> 
> Jemand, der cleanup und initial braucht benutzt halt keine decorators!
> 
> Einfacher:
> ===================================================================
> @multi_response
> def my_multi_func(self, value1, value2):
>      yield '%s_%s' % (self._saved_dict[variable1], variable2)
> ===================================================================
> 
> Diese verkomplizierung existiert, weil der objectStore im frondend so hässliche
> requests sendet.
> Ich würde vorschlagen, den aktuell existierenden multi_response in
> _object_store umzubennen,der dann an diese requests angepasst ist, und einen
> multi_response hinzuzufügen, der simpel zu benutzen ist ist.

Ich verstehe und auch ich bin nicht überglücklich damit, aber so schlimm ist es auch nicht. Die Verkomplzierung besteht aus einer Variable und einer Zeile:
===================================================================
@multi_response
def my_multi_func(self, iterator, value1, value2):
  for value1, value2 in iterator:
    yield '%s_%s' % (self._saved_dict[variable1], variable2)
===================================================================
Das ist alles. Was bekomme ich dafür?

 * Die Möglichkeit einer setup-Funktion (super-einfach: Nur vor die for-Schleife),
 * eine cleanup-Funktion (super-einfach: Nur hinter die for-Schleife),
 * Informationen über den Fortschritt (indem ich z.B. i+=1 in die Schleife baue),
 * und - imho ganz wichtig - die Möglichkeit, bei einem Fehler ganz gezielt mich so zu verhalten, wie ich möchte (z.B. try-except einmal um die for-Schleife und dann alle bereits gemachten Änderungen wieder rückgängig machen, oder try-except einmal innerhalb der for-Schleife um in diesem Fall eine Fehlernachricht zu "yielden").

Ich denke das ist die Sache wert. Ich würde vorschlagen, wir beobachten im Verlaufe der UMC-Weiterentwicklung, wie oft wir multi_response brauchen und was uns fehlt bzw. was uns nervt und wenn es häufig genug auftritt, dann machen wir uns die Mühe und schreiben den Decorator neu oder um.

Übrigens ist das nicht nur der Module-Store. Ich benutze die multi_response schon (deshalb habe ich sie ja vor ein paar Wochen umgeschrieben, weil sie mir zu unflexibel war) und da brauche ich im "setup" ucr.load() und im "cleanup" ein apt-get update. Das kann ich nicht einfach bei jedem Durchlauf machen.

Alternativ könnte man zig Hook-Funktionen definieren und dann sowas machen:

my_multi_func = multi_response(_setup, _real_function, _exception_handler_local, _exception_handler_global, _cleanup)

Und vielleicht bräuchte man noch mehr. So richtig einfach ist das auch nicht. Und welche Variablen bekommt eigentlich jede Funktion? Wie kann es die Variablen ggf. verändern und dann so an _real_function wieder zurückgeben?

Ich setze es wieder auf RESOLVED. Zumal sich bis interim-2 nichts mehr ändern kann.
Comment 15 Florian Best univentionstaff 2012-10-19 12:53:55 CEST
log
→ OK
simple_response
→ OK
multi_response
→ OK
file_upload
→ OK
sanitizer
→ wurden in Bug #27720 getestet

Changelog
→OK
Comment 16 Stefan Gohmann univentionstaff 2012-12-12 21:09:14 CET
UCS 3.1-0 has been released: 
 http://forum.univention.de/viewtopic.php?f=54&t=2125

If this error occurs again, please use "Clone This Bug".