SignalK / specification

Signal K is a JSON-based format for storing and sharing marine data from different sources (e.g. nmea 0183, 2000, seatalk, etc)
Other
91 stars 68 forks source link

feature: define request reponse semantics #508

Closed sbender9 closed 5 years ago

rob42 commented 5 years ago

"The response when a server successfully processes a login request synchronously" The response should actually be a login object (which is undefined yet.)

If we use this as a generic async message correlation mechanism, then the delta format is better described as the envelope . The header can have any of context, token, requestId, status, result, and one payload object (PUT, LIST, GET, ACCESS, LOGIN, and others later).

A request will be

{
  "context": "vessels.self",
  "token": "gdggdggdggdggdgdgdggdggd",
  "requestId": "1234567890",
  "put": {
    "path": "steering.autopilot.target.headingTrue",
    "source": "actisense.204",
    "value": 1.52
  }
}

a reply will be

{
  "context": "vessels.self",
  "token": "gdggdggdggdggdgdgdggdggd",
  "requestId": "1234567890",
  "state": "COMPLETED",
  "result": "SUCCESS"
}

To query a PENDING request we would use

{
  "context": "vessels.self",
  "token": "gdggdggdggdggdgdgdggdggd",
  "requestId": "1234567890",
  "query": true
}

The same query in http could be made via a well-know api call, /signalk/v1/query/request/{requestId}

rob42 commented 5 years ago

While its a small point we might be better to simply return an HTTP code in state. Its easier/faster to check a numeric value, and saves having to define mappings for other http codes that may prove useful.

Since the use of http codes are well understood its also one less thing to learn for newbies

sbender9 commented 5 years ago

I didn’t the request query would be needed. Thinking the server would just send out updates when the state changes.

rob42 commented 5 years ago

But how long do you wait on an update to confirm your request was accepted/processed? The query allows you to specifically see the current state of the request.

Also we have had people request a mechanism to request an action, then recieve a confirmation it had happened. This fulfils that need too.

sbender9 commented 5 years ago

I don’t see what that buys you, if you have not received a response from the server then a query is not going to give you anything new. Same as if you don’t get an update on wind speed, querying for it won’t help you.

rob42 commented 5 years ago

What about the case where a request goes PENDING, then FAIL. How will you learn about that if no update ever comes out because nothing changed?

sbender9 commented 5 years ago

An response would go out when it fails. And state would change to COMPLETED

sbender9 commented 5 years ago

Let me add a WS example to the document, should make it more clear...

tkurki commented 5 years ago

How does a client know which paths it should use a put message for? Like if I want to create a virtual switch panel web app how do I retrieve the list of switches and their potential states, eg. the values that I can put?

rob42 commented 5 years ago

An response would go out when it fails. And state would change to COMPLETED

The response can be sent over ws, but for REST the client connection is gone so the REST client checks the action url. query is the ws equivalent. Its useful for ws when you want to know current state, eg you may have missed the response message, or it seems to be taking too long.

rob42 commented 5 years ago

How does a client know which paths it should use a put message for? Like if I want to create a virtual switch panel web app how do I retrieve the list of switches and their potential states, eg. the values that I can put?

I think PUT should be valid for any path, just as UPDATES are. The difference is PUT is a one-off change to a path, with success/pending/fail semantics (like TCP - a reliable transaction). An UPDATE is a notification that a value has changed, (like UDP - fire and forget).

tkurki commented 5 years ago

Are you guys absolutely certain that we do need the message-based version of request-response at this stage? I honestly think that we should stick to http for this. If the connection is ws then the client could as well use http for the requests. Why would a client use request-response over WebSocket as opposed to http? Ws implementation makes server implementation needlessly complex for very little added benefit. What is the case that is considerably better implemented with request-response over ws, if the other option is http?

As for the we need to eventually support MQTT/STOMP/plain tcp argument I don't buy it. I don't think considering a connection oriented protocol like WebSocket will provide solutions that will work over other paradigms. Like how exactly will a MQTT client get the response back from the server so that the response reaches only that particular client, not all connected clients? I know request response can be done over MQTT, but it involves different things than the protocol that works over ws.

As for in band authentication: why not use the authentication on the protocol level? MQTT, STOMP and CoAP all include authentication mechanisms.

I don't think it makes sense to consider plain tcp for anything more complex. What would be the situation where tcp was the preferred method, over http+ws or CoAP or MQTT? There are multiple different protocols built over tcp that provide the qualities that we need. Why would you want to do the same things, probably less than perfectly and in a way that is specific to Signal K?

I think it would be interesting to map SK things properly to MQTT and CoAP and figure out where each protocol excels in the SK problem space. But I think we should do that by starting with the protocol's mechanisms, not from the abstract.

Sorry if this comes off as a rant, I just want to seriously consider this from different angles, including "do we really need this".

rob42 commented 5 years ago

Its a valid question. Yes we do need it. It fulfils two use cases in particular: 1) tcp clients without an http stack. Also applies to bluetooth, LO-RA, serial or one-wire connections and whatever comes next year. MQTT and COAP request/response mechanisms are aimed at the current message only, not the signalk 'conversation' problem. eg like in http if you want to check on an async app you need to send another query message.

2) We get regular issues about being able to perform an action and get confirmation the action occurred or not. This solves that.

Assuming that all clients that will want action/confirmation semantics will always have an http(s) stack is short-sighted. What about safety issues like MOB, or auto SOS button, where you want more than a fire and forget update.

Its not a complex or burdensome change. Some additional optional message headers, and an action object definition.

sbender9 commented 5 years ago

@rob42 I've made some changes to incorporate your suggestions. Please review.

sbender9 commented 5 years ago

How does a client know which paths it should use a put message for? Like if I want to create a virtual switch panel web app how do I retrieve the list of switches and their potential states, eg. the values that I can put?

@tkurki I don'y think we should cover that here. We can get into that in the docs on the specific requests, like the 'PUT' document.

rob42 commented 5 years ago

line 26: "result wil one of the following and standard HTTP result codes are used." The result will be any standard HTTP code including the folowing:

Line 37: Needs more detail. "The response object may contain other objects depending on the specific request being made. For example, a response to auth request could contain an auth object."

{
  "context": "vessels.self",
  "requestId": "123345-23232-232323",
  "state": "COMPLETED",
  "result": 200,
  "auth": {
       "token": "....." 
   }
}

Sorry the above is not a great example but I cant find the login/logout issue,

Line 71: "The state of a request can also be by sending the following:" The state of a request can also be found by sending the following:

sbender9 commented 5 years ago

@rob42 I've updated the doc as you requested above.

I've also added a correlationId. This is added because the server cannot guarantee the uniqueness of client generated id's, but still needs to be able to uniquely identify requests.

The correlationId is only used by the client for matching up asynchronous messages.

rob42 commented 5 years ago

If we define requestId as a real uuid rather than a random string then collisions should never happen and we can avoid also having correlationId?

sbender9 commented 5 years ago

I just worry about bad acting clients. There's no way to guarantee that it's unique unless we are generating it and the client has to know the id before it sends the message.

This is quite normal in the world of async messaging.

rob42 commented 5 years ago

I dont mind having both. But it also actually highlights a different issue.

If two devices use the same bad behaviour, or a client sends the same message twice, how should the server respond?

Should we perform the action twice, ignore the second, or do we respond with error invalid requestId.

If we specify that requestId is a uuid and must be unique for its lifecycle, we can test for uniqueness (within currently running requestIds) then we should not have this problem

rob42 commented 5 years ago

Also in the REST interface how should a query on a completed id behave? eg one completed 48 hrs ago? Do we need to keep endless history...

sbender9 commented 5 years ago

Yes, I was thinking requestId is a uuid, I'll add that to the document.

sbender9 commented 5 years ago

Also in the REST interface how should a query a completed id behave?

I just updated it to reflect changes in the put document, but it's the last example in the document.

sbender9 commented 5 years ago

Yes, I was thinking requestId is a uuid, I'll add that to the document.

Actually, this is a server implementation detail, I don't see any reason to put this in the spec.

sbender9 commented 5 years ago

Also in the REST interface how should a query on a completed id behave? eg one completed 48 hrs ago? Do we need to keep endless history...

I think we can leave this up to the server. Some servers might want to persist this info for ever in a db. Good info for auditing, etc. Other's might choose a reasonable timeout to prune the info.

sbender9 commented 5 years ago

@rob42 after some discussions with @tkurki, I've been convinced to remove correlationId. I've gone back to requestId and specified that it must be a valid v4 uuid.

rob42 commented 5 years ago

Im happy with that, possibly a note to say a server may refuse a request if the requestId is not unique...maybe that inherent in the reply options anyway.

tkurki commented 5 years ago

Why is context here - is it part of the request response protocol? If so what problem does it solve and what values could it have?

I think context is just a part of the request payload, for example a put request, and not request response semantics.

tkurki commented 5 years ago

I would structure the examples so that the requestId is always the first property and separate the payload with a newline.

tkurki commented 5 years ago

I mean separate the payload props and the req-resp properties with a newline.

tkurki commented 5 years ago

Non-http transports is not really accurate: CoAP has reqresponse, without further refinement this does not work over MQTT or STOMP and we still have not specified JSON over tcp details (delimiter). I would rephrase this to WebSockets and similar transports.

tkurki commented 5 years ago

Add 401 to results? For the case where a client is not authenticated on a server where read only ws is allowed without authentication.

  • 401 - missing or bad authentication
fabdrol commented 5 years ago

I put some comments in the code, but in general:

sbender9 commented 5 years ago

I put some comments in the code, but in general:

  • Generally this looks good to me, although I would like to suggest some small convention-changes (see my code comments)
  • The envelope for the request and response messages is slightly different across situations. It makes sense to me to standardise this to an envelope with these keys: requestId, context, state, code (result), message (optional), payload (optional). Any request/response-specific data is then put in the payload.

I think this issue about the payload is the only problem remaining here.

@rob42 @fabdrol I don't care, either way is fine with me. Can we come to an agreement?

rob42 commented 5 years ago

I prefer not to use payload to hold the response. The actions (put,authenticate, etc) are already unique, I think payload will become the holder for arbitrary data (eg images) that we want to transfer - I had a use case for that recently. But different issue to this.

sbender9 commented 5 years ago

Unless there are any objections, I think this is ready to go