Define how to respond with an error #56
Conversation
|
This depends on #50 so we can't land it before that, but since the error parts weren't defined I made a quick PR. |
jgraham
force-pushed
the
error_response
branch
from
40e0efd
to
7552be5
Compare
foolip
changed the title
jgraham
force-pushed
the
error_response
branch
from
7552be5
to
15cd3d6
Compare
jgraham
force-pushed
the
error_response
branch
from
15cd3d6
to
f4d456e
Compare
index.bs
Outdated
|
|
||
| 1. Issue: Form a valid JSON |errorObject| given |code|. | ||
| 1. If |command id| is not a number, let |command id| be null. |
There was a problem hiding this comment.
Our handling of invalid id seems a bit broken/incomplete still. If the number wasn't in the CDDL uint range, should we send back that number, or null? I think null would make sense, but I don't think spec makes it so.
This algorithm ("respond with an error") is called by "handle an incoming message", but that just parses JSON and doesn't try to interpret the id member. Then we have "Match data against the remote end definition" in "process a command" which operates on the unparsed scalar value string, and that would fail if the id isn't a CDDL uint. However, it doesn't extract a known-to-be-uint id if something else in the schema didn't match.
Overall, that we both parse the message payload as JSON and interpret it per CDDL seems suspect, and probably not how one would implement this.
If fixing this in this PR is too much, then a follow-up issue would be OK.
There was a problem hiding this comment.
OK, so I moved the JSON parse down into the "respond with an error" algorithm. It seems hard to eliminate the possibility of a second parse entirely at least in the spec. A real implementation could be structured so that parsing and validation were seperate steps or something. But I think this is good enough to ensure interoperable behaviour.
index.bs
Outdated
|
|
||
| 1. Issue: Form a valid JSON |errorObject| given |code|. | ||
| 1. If |command id| is not a number, let |command id| be null. |
There was a problem hiding this comment.
OK, so I moved the JSON parse down into the "respond with an error" algorithm. It seems hard to eliminate the possibility of a second parse entirely at least in the spec. A real implementation could be structured so that parsing and validation were seperate steps or something. But I think this is good enough to ensure interoperable behaviour.
jgraham
force-pushed
the
error_response
branch
from
f4d456e
to
75ce370
Compare
The main difficulty here is that if we get a command with an invalid id, it's unclear what to return in the id field. This chooses to return null in that case.
jgraham
force-pushed
the
error_response
branch
from
75ce370
to
9e8724e
Compare
index.bs
Outdated
| string containing a stack trace report of the active stack frames at the | ||
| time when the error occurred. | ||
|
|
||
| 1. Let |response| be the result of [=serialize a map as JSON=] given |error |
There was a problem hiding this comment.
We can now use https://infra.spec.whatwg.org/#serialize-an-infra-value-to-a-json-string here.
There was a problem hiding this comment.
Presumably we want to use "as bytes" instead.
|
|
||
| 7. Let |response| be a new [=map=] with the following properties: | ||
| 1. Let |command id| and |value| be the two fields |result|'s data. |
There was a problem hiding this comment.
Alright, so this is unpacking the return value from 'Return success with (data parsed["id"], value)' elsewhere.
The main difficulty here is that if we get a command with an invalid id, it's
unclear what to return in the id field. This chooses to return null in that case.
Preview | Diff