Review the Web API draft

[krvoigt]

OCR-D/spec#173

[joschrew]

I talked to Triet and I think we came to the conclusion that I should try a review. I don't know how a review should look like, so here is everything i wrote down which possibly could be improved. Maybe it would be better if i change the document (openapi.yml) and you could make a diff? Maybe this review is not about small corrections but about discussing how I would structure the API? Maybe the review has to put into the pull-request? I took my notes in German.
I think it don't makes sense to translate everything to English, please tell me if necessary.

• Rechtschreibung:
  • iff - in der Beschreibung
  • gettProcessor in /processor/{executeable}:, sollte wohl getProcessor heißen
• Ungenutzte tags: acl, training
• Die Version ist 1.0.0, aber es gab seit 2018 schon eine andere Version (Datei ocrd_api_swagger), die hatte Version 1.1.0.
• bei servers steht https://example.org/ocrd/v1. Hier könnte man ggf. noch eine Beschreibung einfügen wie "hier könnte deine URL stehen" oder so. Oder ggf. schreiben dass die Base-URL des Servers mit ocrd/v1 enden sollte. Ggf. Variablen nutzen?
• braucht man Authentification (z.B. wegen DoS oder so)? Wäre das Teil der OpenAPI?
• für einige oder alle in paths könnte man eine description einfügen. Wäre imo sinnvoll
• warum post bei '/processor/{executable}/{job-id}/log', sieht für mich wie eine fehlerhafte Kopie von dem get vorher aus. Wenn ich mich da irre würde hier auch die schon angesprochene description helfen. Verstehe den sinn des ganzen post nicht.
• get: '/workflow/{workflow-id}': response description möglicherweise falsch, es gibt einen Workflow zurück und keinen Processor-Job
• post: '/workflow/{workflow-id}': Vermutlich soll damit ein Workflow-Job gestartet werden. Braucht man da keinen Input? Hier fände ich auch die Beschreibung hilfreich, da der Unterschied zwischen diesem post und dem put vorher nicht sofort zu verstehen ist für mich
• get: '/workflow/{workflow-id}/{job-id}': response Description sollte ggf. "Return WorkflowJob" sein, entsprechend den anderen Beschreibungen
• post '/workspace': summary sollte "create a new workspace" und nicht "replace" sein. Wenn diese Anmerkung von mit richtig ist, dann müsste auch der wert von description entsprechend geändert werden.
• put '/workspace': Hier ist es vermutlich so gedacht, dass man ein vorhandenes Zip an die API schickt und dann das Zip auf dem Server geändert wird. Die Id holt sich dann das Programm selber aus dem zip. Ich finde das ja nicht RESTfull und würde deswegen das put an /workspace/{workspace-id} erwarten, also das die Id als parameter mit übergeben wird
• get `'/workspace/{workspace-id}': Hier bekommt man vermutlich eine Url mit, wo man sich den Workspace als Zip laden kann. Wenn das so ist, sollte in die Beschreibung dass man ein Zip bekommt. Ansonsten, wenn man kein Zip bekommt dann fehlt die Funktionalität von dem Endpunkt irgendwie ein Zip holen zu können.
• components.schema.LogEntry: vorher gab es ein Loglevel-Enum, das hätte ich behalten, bzw. es wie bei JobState gemacht mit der Regex
• Die API-Validierung (https://editor.swagger.io/) ergab bei mir 2 Warnungen, nämlich in components.schema sind Log und ProcessorCall unbenutzt

[kba]

I talked to Triet and I think we came to the conclusion that I should try a review. I don't know how a review should look like, so here is everything i wrote down which possibly could be improved. Maybe it would be better if i change the document (openapi.yml) and you could make a diff? Maybe this review is not about small corrections but about discussing how I would structure the API? Maybe the review has to put into the pull-request? I took my notes in German. I think it don't makes sense to translate everything to English, please tell me if necessary.

• Rechtschreibung:

  • iff - in der Beschreibung

iff means if and only if

• gettProcessor in /processor/{executeable}:, sollte wohl getProcessor heißen

fixed fb1b2d8

• Ungenutzte tags: acl, training

Ja, weil ich mir da nicht sicher bin, was Sinn macht, das ist abhängig von der jeweiligen Infrastruktur (acl) bzw. dem scope der Web API - ob da Training mit drin sein sollte.

• Die Version ist 1.0.0, aber es gab seit 2018 schon eine andere Version (Datei ocrd_api_swagger), die hatte Version 1.1.0.

The old ocrd_api_swagger is long obsolete, the version number can be ignored for now. I tend to start versioning projects with 0.0.1 where anything can change at any time and then 1.0.0 once it's published and we must give some guarantees on backwards compatibility.

• bei servers steht https://example.org/ocrd/v1. Hier könnte man ggf. noch eine Beschreibung einfügen wie "hier könnte deine URL stehen" oder so. Oder ggf. schreiben dass die Base-URL des Servers mit ocrd/v1 enden sollte. Ggf. Variablen nutzen?

Yes, we can extend the servers info with description and variables

• braucht man Authentification (z.B. wegen DoS oder so)? Wäre das Teil der OpenAPI?

That's a good question and also the reason why there's an acl tag with no endpoints. We need to talk with the implementers about this.

• für einige oder alle in paths könnte man eine description einfügen. Wäre imo sinnvoll
• warum post bei '/processor/{executable}/{job-id}/log', sieht für mich wie eine fehlerhafte Kopie von dem get vorher aus. Wenn ich mich da irre würde hier auch die schon angesprochene description helfen. Verstehe den sinn des ganzen post nicht.

POST /processor/{executable}/{job-id}/log is for creating new log entries.

GET /processor/{executable}/{job-id}/log returns the full log.

• get: '/workflow/{workflow-id}': response description möglicherweise falsch, es gibt einen Workflow zurück und keinen Processor-Job

This is supposed to return a workflow, i.e. the abstract orchestration of processors. POSTing a concrete mapping for this workflow will start/return a WorkflowJob.

• post: '/workflow/{workflow-id}': Vermutlich soll damit ein Workflow-Job gestartet werden. Braucht man da keinen Input? Hier fände ich auch die Beschreibung hilfreich, da der Unterschied zwischen diesem post und dem put vorher nicht sofort zu verstehen ist für mich

PUT /workflow/{workflow-id} is for replacing an existing, earlier POSTed workflow

It does require input of course, but since we don't know the format of workflows, it's hard to say much about the format of the mappings. But likely a JSON object mapping workflow parameters to concrete values.

• get: '/workflow/{workflow-id}/{job-id}': response Description sollte ggf. "Return WorkflowJob" sein, entsprechend den anderen Beschreibungen

fixed ed8be88

• post '/workspace': summary sollte "create a new workspace" und nicht "replace" sein. Wenn diese Anmerkung von mit richtig ist, dann müsste auch der wert von description entsprechend geändert werden.

fixed 9fb6eb3

• put '/workspace': Hier ist es vermutlich so gedacht, dass man ein vorhandenes Zip an die API schickt und dann das Zip auf dem Server geändert wird. Die Id holt sich dann das Programm selber aus dem zip. Ich finde das ja nicht RESTfull und würde deswegen das put an /workspace/{workspace-id} erwarten, also das die Id als parameter mit übergeben wird

Typo, this should have been part of `workspace/{workspace-id} as you suggested. 2442139

• get `'/workspace/{workspace-id}': Hier bekommt man vermutlich eine Url mit, wo man sich den Workspace als Zip laden kann. Wenn das so ist, sollte in die Beschreibung dass man ein Zip bekommt. Ansonsten, wenn man kein Zip bekommt dann fehlt die Funktionalität von dem Endpunkt irgendwie ein Zip holen zu können.

I think this should be handled with content-negotiation. I have so far only defined the JSON response, but if you do curl -H Accept: application/zip you'd get the OCRD-ZIP. Not sure how this is modelled in OpenAPI, will find out.

• components.schema.LogEntry: vorher gab es ein Loglevel-Enum, das hätte ich behalten, bzw. es wie bei JobState gemacht mit der Regex

added an enum a924db4

[mweidling]

See OCR-D/spec#173 where I put my thoughts in a review.

[mweidling]

Since I've finished my review I'll remove myself from the list of assignees. @kba @krvoigt feel free to re-add me if there is something else I could/should do.