Fix delta PUT message format

[rob42]

The jsonschema for a PUT message uses multiple put objects in an array, the example in docs doesnt. This fixes docs to properly reflect schema

[tkurki]

Oh, we should have used the mdpInsert mechanism for all the samples in the documentation so that the sample is coming from a snippet that is validated as part of the test suite: c375cd7e4c50200a1380eb688c6409bec2e05d1a.

So the proper fix would be to change this (and all other recently added snippets in docs) to use mdpInsert. Care to look into that @rob42 ?

[tkurki]

Oh this goes deeper, I had totally forgotten about PUTs in the delta schema:

• no correlationId in delta schema
• with essentially multiple PUTs per one message one correlationId does not work: how do you say that one of the PUTs failed and another succeeded?

I would be willing to change this by going with the way the docs are now, one PUT per message, and add correlationId to delta schema. PUTs were never mentioned in the docs, just somewhat hidden in the schema previously.

Ping @sbender9

[timmathews]

I think one PUT per request may be the easiest solution. If we allow an array of PUTs, how would you handle the case where permissions allow one of the PUTs, but don’t allow another?

We can always expand to allow an array of PUTs later on.

[rob42]

The current method is published as v1 so changing it is breaks the spec. Adding correlation key is easy and good.

Multiple paths only occur in signalk messages from non http transports. Http PUT only allows one value, so its always an array of one.

The permissions problem is easily solved. The paths get filtered by the role, the offending paths are deleted. Same goes for updates and any other message. You cant change or see what you have no permission for. This is why permissions per key are necessary BTW. Only real question is do you return 202 or a fail code when you have a partial success?

@tkurki whats mdpInsert?

[sbender9]

Puts should not be an array, I’m sorry that I missed that there was something there before. I think the existing document should override that.

Changing this would only break the only existing implementation of PUT and multiple existing clients.

The schema needs to be changed, not this document.

[tkurki]

See the commit I linked above for mdpInsert.

[rob42]

@sbender9 Since when do we make arbitrary decisions to change the spec against other views without resolution first? Are we going to go forwards with each person applying their view without explanation and closing issues? Have I missed something and now you have complete control of signalk?

[rob42]

BTW the java servers have always supported PUT messages, way before it was added to the node server. The existing apps that use PUT do it via http PUT (I think), which is a single value anyway. This is about the signalk message.

[sbender9]

Really sorry, I did not mean to close this. Must have clicked the close and comment button by mistake.

[rob42]

@sbender9 ok, sorry for going overboard, There were several previous statements against my PR, then I read yours as a final statement and decision on the matter (especially as you also closed the PR).

Back to the PR issue. My change only affects the non-http PUTs, eg via MQTT etc. It wraps them in an array, which is consistent with the spec and with the other delta format messages.

See https://stackoverflow.com/questions/8472935/http-status-code-for-a-partial-successful-request.

Answer 2 suggests 207 Multi-status. This works for the signalk message eg MQTT, but not at the http layer. The http server needs to handle that and translate to 200, 202, or something

[tkurki]

The way I see this:

The specification document and the delta schema are now inconsistent: specification examples show a single value, delta schema an array.

The PUT mechanism document and the request-response mechanism are inconsistent: put.md talks about correlationId, request_response.md about requestId. CorrelationId should be fixed to requestId.

The original delta.json schema is very underspecified: it does not specify how the result of the put operation is communicated and there is no way to correlate incoming responses to sent requests.

Sending multiple put requests in one message with only one requestId is not in line with the request-response mechanism we specified. Trying to keep things KISS it seems to me that the simplest way is to allow only one put per message. 207 Multi status is essentially inventing your own protocol (I assume you did not mean using WebDAV xml responses). I realise that with put over ws we are creating our own protocol, but we have tried to keep parity with http where it makes sense. Here I think simplicity overweighs flexibility: if you need to send several puts send several messages with individual requestIds, get replies to them individually.

As for delta json schema file: in 1.0.0 through 1.1.1 there was no mention of message based put/list/get functionality in the documentation, only in the schema files. I admit I have a personal bias, but I feel that being as underspecified and underdocumented as they are and not available in Node server I would like to remove them and replace with what we have in the documentation: single put messages, in line with request response. I can not remember anybody asking about get/put/list over ws, which shows something about their usage so far.

If somebody wants to document list and get so that they are in line with the request-response paradigm they can be readded.

At the same time I recognise the need to go beyond http and embrace messaging. I think that as we move beyond a single vessel asynchronous messaging patterns will become more relevant. I know that I've been sometimes pretty adamant about "http should be enough", but...things change.

So I propose:

1. change correlationId to requestId in put.md
2. change delta.json schema to be in line with the documentation about put
3. remove list and get requests from delta.json

[fabdrol]

I've been reading through this thread.. there seems to be a lot of emotion going on. Maybe better to contact each other directly next time, so we keep the (software) issue threads dispassionate and more formal?

Regarding the issue at hand: I think Teppo's comments make a lot of sense. Having said that, I'd like to know Rob's view on how the current spec as it stand (in particular the request/response stuff) would work in Artemis' MQTT implementation - if at all. I still stand by our original goal of being transport-agnostic - which means that we shouldn't bias too much towards HTTP/ws, but also not towards MQTT or something else. But, that should be a discussion on Slack - not on this issue.

[rob42]

The artemis server and the java server before it both use the PUT and GET messages. They support LIST but its not used (it was added after a request from some-one to be able to list the available keys).

Both servers ONLY process signalk messages. The REST APIs and web components are just a front-end to convert REST calls to signalk messages and back (eg a gateway). This ensures all messages pass through the same processing and security, and makes it simple to add new transports.

I'm happy to more fully spec GET and LIST. coorelationId should become requestId and be consistent with other uses.

GET,LIST and PUT all use arrays (as does UPDATES). Since UPDATES is fire-and-forget semantics we can tolerate partial failures, where PUT cannot. GET also does not really make sense with multiple paths, but modifying these will break the published spec.

The solution is to leave the arrays in PUT, GET, LIST for now but make them max 1 element. That solves all the issues I think?

[fabdrol]

The idea behind the arrays is to be able to send >1 update/change in a single request?

[rob42]

yes, it was based on the same format as updates, for consistency, and to allow code reuse.

[rob42]

If we make more than one array element return HTTP 400 Bad Request we dont break the spec, and fulfill the other needs.

[fabdrol]

I'm sensitive to the don't break the spec argument - we should not do breaking spec changes (will erode trust by organisations) until 2.0. That said, however, does anyone beyond Artemis, Java and Node server use this feature right now? And I guess Wilhelm SK uses the PUT feature without arrays right now (@sbender9)? If only Java, Artemis, Node servers and Wilhelm SK use this feature, I would say that the impact of a breaking change to the spec is minimal (and as the current spec is "broken" on this anyway, it needs changing regardless).

Moreover, I feel that the arguments for or against based on a current implementation are null and void. @rob42 I understand that it sucks to have to change something that already works (and same for @sbender9 and @tkurki) - especially if it is a technically elegant solution. However, the future of the specification and Signal K should come first, even if that means more work for implementors or technically less impressive servers

Therefore, could you guys list the pros and cons of array vs. no array in abstract terms, i.e. regardless of implementation?

Edit: In my original version, I wrote: "don't bread the spec". And despite that I'm all for breading the spec, I feel that don't break the spec captures my thoughts better.

[rob42]

PUT Array vs PUT object:

• PUT Array
  • Pros:
    • Efficient for multiple keys
    • Consistent with UPDATES
    • Spec compliant
  • Cons:
    • Request/response semantics for partial failures are undefined.
    • Breaks current non-spec PUT message implementations (not http REST)
• PUT Object:
  • Pros:
    • Request/Response semantics work.
  • Cons:
    • Breaks/changes spec
    • Breaks current spec compliant implementations

The actual changes to code implementations are trivial, either add or remove the array object and processing loop for PUT messages. As Ive pointed out above, this only affects the signalk PUT message, not the REST PUTs via HTTP, so existing clients are only affected if they do PUT over ws.

Basically we can solve the problem one of two ways: 1) Add response code for partial failure - which may be prove useful elsewhere 2) Break the spec and remove the array.

[fabdrol]

@rob42 thanks, clear story. I need to read up on the request/response semantics problem to formulate an intelligent response myself.

Generally speaking I'm against adding large amounts of error keys, as it just adds more failure cases to handle. An alternative option may be to consider the array (i.e. list of mutations) as a single transaction: they either all succeed (200) or all fail (400 or 500). In case of failure, the first reason is given (similar to database transactions, JS Promises, etc). If multiple items fail, the 2nd to Nth failure reason are opaque to the client, but that's okay - it may just lead to multiple failed requests until the client gets it right (in case of 400 of course). In case of a 500 the client will probably not do anything, as it's a server error.

@tkurki/@sbender?

[rob42]

Yes, 200 = all succeed, 400 = at least one failed. Remember this is a signalk message, not a REST reply. The message can have more information than the REST reply, the web server just strips it back on sending an http reply.

From https://github.com/SignalK/specification/blob/master/gitbook-docs/request_response.md "The response object may contain other response data depending on the specific request being made. For example, a response to an authentication request could contain a login object."

It would make sense to have an errors object. In the case of PUT it would hold detailed codes per PUT.

[tkurki]

I don’t think adding complex error handling is a real option. If you need to make several requests send several messages. This choice would be much complexity for very little gain.

To me the only options are

• making put a single value
• constraining to array of one

An array of exactly one value is so unintuitive that my choice is the single value.

[tkurki]

Well, the point about breaking backwards compatibility seems moot, as the current delta schema has not allowed anything without an updates element since 31dc9706, so a single put like we are talking about here has never been valid without at least an empty updates property.

The delta schema is clearly buggy / inconsistent, because we have never really exercised anything but the updates mechanism in it. Going forward I think it would make sense to split the current delta.json to separate schemas for

• delta proper (updates etc)
• subscribe/unsubscribe
• request/response with put (add others here when documented properly)

and create a superschema for "stream message" or something (notice I am going a level more abstract here from a WebSocket message).

[tkurki]

...and we have subscribe/unsubscibe in both delta.json and under messages. I wonder why?

[fabdrol]

The delta schema is clearly buggy / inconsistent, because we have never really exercised anything but the updates mechanism in it. Going forward I think it would make sense to split the current delta.json to separate schemas

@tkurki good points, but not the issue right here. Let's get on with that after we close this one.

I feel we're deadlocked on this issue; to me both arguments have up and downsides. For message-based transports the overhead of sending multiple messages is not that big anyway. Since @tkurki seems to feel quite strongly about the issue, let's go with a single update per PUT and close this issue (@rob42 you said it wasn't too big a change, right?)

[fabdrol]

@tkurki @rob42 @sbender9 please respond, otherwise I'll close the PR due to inability to agree/lack of consensus.

[fencer]

If I can put in my 2c: I would go for the Array route to keep it in line with the spec, if it was not for that reason I would go with the Object option.

[fabdrol]

@fencer thanks for your input - could you expand on the 2nd bit though? Forgetting about that it is in the spec, why is the object option a better choice in your opinion?

[tkurki]

We need to fix the spec anyway and break backwards compatibility as it does not allow message like in this PR.

[rob42]

See https://signalk.org/specification/1.3.0/doc/versioning.html

Do we move to version 2.0 of the spec, or do we break the spec and the versioning and just sweep it all under the carpet? Is that the norm for future issues?

To me it seems that since we have arrays in the current spec we cannot remove them without moving to v2 etc. The argument that they are "not used" is only valid for the node server, the java server has always used them, thats why I added them to the spec. Sure I can change it but thats the effect breaking the spec has on implementors.

The correct solution is to flag the array as deprecated, and recommend only one item be sent so that the request/reply semantics make sense. The request/reply should be 200 (success), 400 (at least one fail).

We can add an additional GET/PUT/LIST format that uses an object instead of an array.

Additionally we can state that the v2 spec will only have object, and the array support will be dropped.

[tkurki]

Deprecate and add object put could work. Some problems remain:

• no requestId, so no way to correlate responses unless they are always in order and not too asynchronous
• current schema does not allow an array put without an updates property.

But I guess we can deprecate the old put array method and just add the new one beside it.

[rob42]

Adding the requestId should not be a problem, as we have only just added the request/response semantics, hence its in the same spec version increment.

Would making the updates property optional break the schema? - possibly, but all implementations seem to handle that at present? Again we can deprecate that too and recommend lenient parsing for v1.

BTW my implementation will process any combination of GET/UPDATES/SUBSCRIBE/UNSUBSCRIBE/PUT/LIST, eg you can have several in one message. Probably need to spec if its oneOf, or anyOf i the spec.

[fabdrol]

Okay, sounds like we're slowly getting somewhere. @rob42 can you amend the original PUT to reflect the proposed solution of deprecating the array, adding requestId etc?

[rob42]

ready to merge?

[fabdrol]

Looks good to me, I missed the commits @rob42 - sorry about that. I'd say go, @tkurki any final thoughts?

[tkurki]

A couple of points:

• I think it would make sense to have the current way of doing things first in the document
• I think it would be wise to have a deprecation warning before and after an example, so that the deprecation note does not go unnoticed
• use mdpInsert: the point there is that we do not want to have separate test samples and documentation samples - they drift over time and we end up with inconsistencies

If these are ok I can do them as well.

[fabdrol]

Looking good to me, and has been open for quite some time. @rob42 okay if I merge, or do you plan to add more commits?