Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
achimr opened this issue Oct 14, 2014 · 10 comments
Open

Comments

@achimr
Copy link
Contributor

achimr commented Oct 14, 2014

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:

  • GET link for status/{id} missing
  • HTTP verb PATCH instead of PUT for translation.confirm|accept|cancel|reject (should be PUT per specification)
  • PUT link to replace translation request translation/{id} is missing
  • DELETE link to delete translation request is missing
@heartsomeXPhantom
Copy link
Collaborator

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
Copy link
Contributor Author

achimr commented Oct 15, 2014

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"

  • there is already a link labeled "translate.reject"
  • the status request should have the HTTP verb "get"
  • title attribute mentions "Reject", not "Status"

@heartsomeXPhantom
Copy link
Collaborator

corrected

@achimr
Copy link
Contributor Author

achimr commented Nov 10, 2014

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

@heartsomeXPhantom
Copy link
Collaborator

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

@achimr
Copy link
Contributor Author

achimr commented Nov 12, 2014

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

@achimr achimr reopened this Nov 12, 2014
@achimr achimr assigned achimr and unassigned heartsomeXPhantom Nov 12, 2014
@heartsomeXPhantom
Copy link
Collaborator

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
Copy link
Contributor Author

achimr commented Nov 13, 2014

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
Copy link
Collaborator

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
Copy link
Contributor Author

achimr commented Nov 14, 2014

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".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants