GALAglobal / translationapi

The API is for anyone who wants to adopt best practices for a translation services API to interact with counterparts directly from your application or content management system. The API helps ensure interoperability for the most common tasks.
https://translate.taus.net/translate/taus-translation-api
MIT License
12 stars 3 forks source link

Links attribute of translation request has incorrect/missing HTTP verbs #25

Open achimr opened 9 years ago

achimr commented 9 years ago

Steps to reproduce:

  1. Launch server with "node tausapiserver.js"
  2. Post new translation request: { "translationRequest": { "id": "2b575fdc-f6af-4b9e-850d-9dc0884c6595", "sourceLanguage": "de-DE", "targetLanguage": "en-US", "source": "Hallo Welt", "professional": true, "mt": false, "creationDatetime": "2014-05-20T19:20+01:00", "updateCounter": 0, "status": "initial" } } Result: Successful creation of request: { "id": "2b575fdc-f6af-4b9e-850d-9dc0884c6595", "sourceLanguage": "de-DE", "targetLanguage": "en-US", "source": "Hallo Welt", "mt": false, "professional": true, "status": "initial", "creationDatetime": "2014-10-14T14:51:06.670Z", "updateCounter": 0, "links": [ { "rel": "translation", "href": "http://localhost:3412/v2.0/translation/2b575fdc-f6af-4b9e-850d-9dc0884c6595", "type": "application/json", "title": "Newly created translation request 2b575fdc-f6af-4b9e-850d-9dc0884c6595 + created on Tue Oct 14 2014 10:51:06 GMT-0400 (Eastern Daylight Time)", "verb": "GET" }, { "rel": "translation.cancel", "href": "http://localhost:3412/v2.0/translation/cancel/2b575fdc-f6af-4b9e-850d-9dc0884c6595", "title": "Cancel translation request 2b575fdc-f6af-4b9e-850d-9dc0884c6595", "type": "application/json", "verb": "PATCH" }, { "rel": "translation.reject", "href": "http://localhost:3412/v2.0/translation/reject/2b575fdc-f6af-4b9e-850d-9dc0884c6595", "title": "Reject translation request 2b575fdc-f6af-4b9e-850d-9dc0884c6595", "type": "application/json", "verb": "PATCH" }, { "rel": "translation.confirm", "href": "http://localhost:3412/v2.0/translation/confirm/2b575fdc-f6af-4b9e-850d-9dc0884c6595", "title": "Confirm translation request 2b575fdc-f6af-4b9e-850d-9dc0884c6595", "type": "application/json", "verb": "PATCH" }, { "rel": "translation.accept", "href": "http://localhost:3412/v2.0/translation/accept/2b575fdc-f6af-4b9e-850d-9dc0884c6595", "title": "Accept translation request 2b575fdc-f6af-4b9e-850d-9dc0884c6595", "type": "application/json", "verb": "PATCH" }, { "rel": "translation.patch", "href": "http://localhost:3412/v2.0/translation/2b575fdc-f6af-4b9e-850d-9dc0884c6595", "title": "Patch translation request 2b575fdc-f6af-4b9e-850d-9dc0884c6595", "type": "application/json", "verb": "PATCH" } ] }

Issues:

heartsomeXPhantom commented 9 years ago

The link model is a very basic one. The allowed operations are up to the specific operations. Thus the curretn version just adds a some, not all possible lin options. HTTP verb PATCH - PUT should be used due to the HTTP spec only for replacing the full request, not parts of the request

DELETE, PUT, STATUS added

achimr commented 9 years ago

there is now a link returned that seems to be wrong: rel: "translation.reject" href: "http://localhost:3412/v2.0/translation/status/2b575fdc-f6af-4b9e-850d-9dc0884c6595" title: "Reject translation request 2b575fdc-f6af-4b9e-850d-9dc0884c6595" type: "application/json" verb: "PATCH"

heartsomeXPhantom commented 9 years ago

corrected

achimr commented 9 years ago

The returned link for "translate.status" has "PATCH" as an HTTP verb. Expected: "GET" for verb

heartsomeXPhantom commented 9 years ago

PATCH because you can change the translation status; But i have add a get methods for status too.

achimr commented 9 years ago

Currently accept/reject/confirm are documented in the spec with the HTTP verb PUT, not PATCH. There is no PATCH action documented for status.

heartsomeXPhantom commented 9 years ago

I think we should allow a PATCH for each method; like PATCH: http://localhost:3412/v2.0/translation/confirm etc. and the body just contains true/false (as plain text) in this case. In this case we would not need a JSON body. Would make updates easier, also for the source and target...

achimr commented 9 years ago

I agree. I assume the identifier for the different values (methods) would be http://localhost:3412/v2.0/translation/{guid}/{value name} for example http://localhost:3412/v2.0/translation/2b575fdc-f6af-4b9e-850d-9dc0884c6595/professional It would be up to the server to verify if the requested change is allowed (based on process constraints - e.g. setting back the status to "initial" should maybe not be allowed; or authorization)

The question is then why we still have the accept/confirm/reject/confirm methods - the same could be achieved with setting the status value. For convenience?

heartsomeXPhantom commented 9 years ago

As far as I understand REST and PATCH

http://localhost:3412/v2.0/translation/professional/2b575fdc-f6af-4b9e-850d-9dc0884c6595/ and body content: true

Similar to get and PUT.

This brings me to the other question/problem: As rest is quite "vague", it is not really clear if PUT is - in the strong meaning - allowed for a single attribute. As far as I saw PUT should always contain the whole request and replaces the request.

There is also an interesting aspect for POST and PUT.

PUT should always replace an object if it exists (idempotent) while POST always creates a new object. So what shall we do if the client provides two identical objects with POST? That’s was the reason why I said no GUID with POST, let do it the server.

Maybe we should replace the PUT operation on the attributes with PATCH, seems better.

achimr commented 9 years ago

Why the ID in the last part of the URL? Isn't professional a sub-attribute of the translation request?

Googling "rest put vs patch" it seems that using PATCH is very contentious and somewhat underdefined. For example: http://restful-api-design.readthedocs.org/en/latest/methods.html http://restcookbook.com/HTTP%20Methods/patch/ http://williamdurand.fr/2014/02/14/please-do-not-patch-like-an-idiot/ (offensive title)

It seems that when using JSON in PATCH one needs to use a special format to replace a resource: https://tools.ietf.org/html/draft-pbryan-json-patch-04#section-4.3

PUT seems much more straightforward and if we allow PUT on every sub-attribute we don't need to worry about having to replace the entire request (translation, comment, score) for large requests. This was the reason why we introduced PATCH in the first place.

The resource http://localhost:3412/v2.0/translation/2b575fdc-f6af-4b9e-850d-9dc0884c6595/professional is the value of professional the originally POSTed translation request and can be entirely, idempotently replaced with PUT. It is its own resource.

Regarding the POSTing of duplicate GUIDs per server we can define this as an error in the spec. GUIDs are by definition supposed to be "globally unique".