feat: implement RequestError
#119
Conversation
| except web.HTTPException: | ||
| raise | ||
| except tuple(exception_map.keys()) as e: | ||
| exception = exception_map[e.__class__] |
There was a problem hiding this comment.
hier besser type(e) benutzen
| exception = exception_map[e.__class__] | ||
| raise exception(reason=e.reason, temporary=e.temporary) from e | ||
| except Exception as e: | ||
| raise ServerError(reason=e.__class__.__name__, temporary=True) from e |
There was a problem hiding this comment.
Bei unbekannten Exceptions sollten wir nach Außen hin keine Details geben, aber es sollte geloggt werden, inkl. Traceback.
| from questionpy_server.worker.runtime.messages import WorkerMemoryLimitExceededError, WorkerUnknownError | ||
|
|
||
| exception_map: dict[type[QPyBaseError], type[QpyWebError]] = { | ||
| InvalidQuestionStateError: InvalidPackageError, |
There was a problem hiding this comment.
Das ist eher ein PackageError.
| exception_map: dict[type[QPyBaseError], type[QpyWebError]] = { | ||
| InvalidQuestionStateError: InvalidPackageError, | ||
| StaticFileSizeMismatchError: InvalidPackageError, | ||
| WorkerNotRunningError: InvalidPackageError, |
There was a problem hiding this comment.
WorkerNotRunningError deutet eher auf einen Programmierfehler hin. Sollte daher in diese map nicht aufgenommen werden.
| except web.HTTPException: | ||
| raise |
There was a problem hiding this comment.
Ich denke es wäre wünschenswert, dass alle generierten HTTP-Fehler einen Body der gleichen Struktur haben. Wenn ich das richtig sehe, werden noch an 4 Stellen direkt HTTPExceptions ohne Body geworfen. Ich würde gerne entweder diese Stellen durch QPyBaseErrors ersetzen, oder ihnen hier einen Body verpassen, wenn sie noch keinen haben.
| class InvalidRequestError(web.HTTPBadRequest, _ExceptionMixin): | ||
| def __init__(self, *, reason: str | None, temporary: bool) -> None: | ||
| super().__init__( | ||
| "Invalid request body was provided", | ||
| RequestError( | ||
| error_code=RequestErrorCode.INVALID_REQUEST, | ||
| reson=reason, | ||
| temporary=temporary, | ||
| ), | ||
| ) |
There was a problem hiding this comment.
-
Die Meinungen gehen hier auseinander, aber in Feat/validation errors questionpy-sdk#126 hat @tumidi zuletzt
422 Unprocessable Entityverwendet, was für einen semantischen Fehler (in Abgrenzung zu einem syntaktischen Fehler wie kaputtes JSON) steht. Ich habe da selbst keine starke Meinung zu, außer, dass wir konsistent sein sollten. -
Ich bin mir generell der Sinnhaftigkeit der
temporary-Flag nicht so ganz sicher, zumindest sollte es aber an dieser Stelle kein Konstruktor-Argument sein. Die meisten HTTP-Status-Codes implizieren, ob der Fehler bei gleichem Request erneut zu erwarten ist. So steht bei 400 z.B.The client SHOULD NOT repeat the request without modifications.
D.h. ein 400er muss eigentlich immer
temporary=Falsesein, daher sollte das auch fest für diese Klasse so sein. Das gilt analog vermutlich für einige andere Fehler auch, aber dieser ist ein gutes Beispiel.
There was a problem hiding this comment.
Ja, da hast du genau genommen recht.
Der Sinn hinter dem Flag und warum ich den gerne in allen Fehlerfällen ausgegeben haben will, ist dass das LMS nur an einer Stelle gucken muss, ob es sich um einen temporären oder permanenten Fehler handelt. Wir sind dann in der Lage, neue Error-Codes zu definieren, die das LMS noch nicht kennt, und trotzdem weiß das LMS, ob es die Anfrage später nochmal stellen sollte.
| class RequestErrorCode(Enum): | ||
| QUEUE_WAITING_TIMEOUT = "QUEUE_WAITING_TIMEOUT" | ||
| WORKER_TIMEOUT = "WORKER_TIMEOUT" | ||
| OUT_OF_MEMORY = "OUT_OF_MEMORY" | ||
| INVALID_PACKAGE = "INVALID_PACKAGE" | ||
| INVALID_REQUEST = "INVALID_REQUEST" | ||
| PACKAGE_ERROR = "PACKAGE_ERROR" | ||
| CALLBACK_API_ERROR = "CALLBACK_API_ERROR" | ||
| SERVER_ERROR = "SERVER_ERROR" |
There was a problem hiding this comment.
- Ich würde mir mehr Trennschärfe wünschen. Zumindest folgende Fälle halte ich für sinnvoll, zu unterscheiden, damit wir passende Fehlermeldungen anzeigen können:
- Paket existiert nicht
- Options invalide
- Question state kaputt
- Attempt state kaputt
Hier ginge auchauto()für weniger duplication.
There was a problem hiding this comment.
-
Wenn wir in allen Fehlern (status code != 2xx) ein gleichartiges JSON-Objekt zurückgeben, was ich begrüße, ist der Fall "Paket existiert nicht" natürlich sinnvoll. Die anderen drei genannten Fälle fallen eigentlich unter den generischen
PACKAGE_ERROR. Wir können hier aber auch differenzieren.InvalidQuestionStateErrorist zumindest schon definiert in common, wird benutzt und kann dann leicht als Fehlerfall kommuniziert werden.
Welchen Fehlerfall meinst du bei "Options invalide"? -
"Using auto with StrEnum results in the lower-cased member name as the value."
ist daher in diesem Fall nicht so gut
Generell würde ichauto()nicht im Zusammenhang mit Schnittstellen benutzen, da sich das Verhalten möglicherweise in neueren Versionen ändern kann.
There was a problem hiding this comment.
Mit "Options invalide" meine ich OptionsFormValidationError, wobei das tbf auch auf #118 warten musste.
Ich denke, "Question state kaputt" und "Attempt state kaputt" wäre insofern hilfreich zu unterscheiden, dass wir dann in der Fehlermeldung erkennbar machen können, dass entweder die ganze Frage oder "nur" der Attempt fehlerhaft ist. Das hilft zwar keiner Studi, aber womöglich schon Lehrenden (und Entwickler:innen eh)
Zu 2.: Pardon, ich war davon ausgegangen, dass auto() den Namen verbatim nimmt
There was a problem hiding this comment.
Bezüglich OptionsFormValidationError kommen wir an einen guten Punkt, den wir aber nicht hier angehen sollten. Ich habe dafür Issue #122 erstellt.
Ich denke, "Question state kaputt" und "Attempt state kaputt" wäre insofern hilfreich zu unterscheiden, dass wir dann in der Fehlermeldung erkennbar machen können, dass entweder die ganze Frage oder "nur" der Attempt fehlerhaft ist. Das hilft zwar keiner Studi, aber womöglich schon Lehrenden (und Entwickler:innen eh)
Dem schließe ich mich an. @janbritz Bitte führe in common noch eine neue Exception InvalidAttemptStateError ein und ergänze zwei neue Error Codes INVALID_QUESTION_STATE und INVALID_ATTEMPT_STATE.
| class PackageError(web.HTTPBadRequest, _ExceptionMixin): | ||
| def __init__(self, *, reason: str | None, temporary: bool) -> None: | ||
| super().__init__( | ||
| "An error occurred within the package", | ||
| RequestError( | ||
| error_code=RequestErrorCode.PACKAGE_ERROR, | ||
| temporary=temporary, | ||
| reason=reason, | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Aus Sicht des Clients ist das vermutlich auch eher ein Internal Server Error
There was a problem hiding this comment.
Stimmt, ebenso WorkerTimeoutError und OutOfMemoryError. Bei InvalidPackageError hinge es davon ab, ob das Paket durch das LMS bereitgestellt wurde oder vom Server angeboten wurde, aber wir sollten das nicht zu akademisch betrachten und auch hier 500 zurückgeben.
Mithilfe einer Middleware werden alle Exceptions zu dem in der QPPE definierten
RequestErrortransformiert.Sollte eine unbekannte Exception geraised werden, wird diese zu einem
ServerErrorumgewandelt.Außerdem wird die veraltete
NotFoundStatus-Komponente aus der QPPE entfernt.PS: da ich Probleme mit
pytesthatte, musste ichpoe upausführen, das Update kann ich aber gerne auch aus dem PR entfernen.