Refactor the way we use CDDL #50
Conversation
jgraham
force-pushed
the
cddl_refactor
branch
2 times, most recently
from
eba286a
to
119cc6d
Compare
| "invalid argument" | ||
| ), | ||
| message: text, | ||
| stacktrace: text, |
There was a problem hiding this comment.
Did you mean to introduce stacktrace here? https://www.jsonrpc.org/specification#error_object doesn't have it, and maybe we should, but it'd be good to introduce in a separate PR for visibility.
There was a problem hiding this comment.
I was basing it on what we provide in WebDriver 1.0 rather than on JSON-RPC.
There was a problem hiding this comment.
Sure, we won't be using JSON-RPC error objects, but this is doing more than refactoring. If you mention the addition in the eventual commit message, that's OK I think.
Previously the spec only provided fragments of CDDL to validate specific kinds of messages. But it makes sense from an implementation point of view to have full schema that can be used directly. In practice we require two seperate schema, one for validation of messages going from the local end (i.e. client) to the remote end (i.e. browser) and one for messages going in the opposite direction. This change adds those schema to the spec. The modelling adopted here is likely not the only approach, but the basic idea is that each kind of message is modelled as a choice between several variants, representing each possible message in the protocol. Because the messages have unique names (e.g. in the `method` field) we know that zero or one will match. However the names are not represented directly. In terms of organistaion, this currently defines a schema prelude with each message type specified by a set of choices, one per module. Then each module defines a choice per command (or other message) and finally the messages themselves define the actual schema. I expect this organisation to change somewhat over time. In particular using the `//=` oeprator in CDDL seems like it might avoid the need to define so many terms upfront, but it also seemed to be poorly supported in the CDDL library I tried.
Co-authored-by: Philip Jägenstedt <philip@foolip.org>
jgraham
force-pushed
the
cddl_refactor
branch
from
0448ff5
to
199dd08
Compare
index.bs
Outdated
| - A <dfn export for=command>return type</dfn>, which may be null. | ||
| Each [=command=] is defined by: | ||
| - A <dfn export for=command>command id</dfn>. This is an integer id | ||
| provided by the [=local end=] for each command. The value of the id is not |
There was a problem hiding this comment.
I think this section should at least briefly explain what the command ID is for. The remote end does in fact use it when sending a command response back to the local end. So I think it's fair to say the ID can be treated as opaque, but it is not entirely accurate to say the remote end doesn't use it.
There was a problem hiding this comment.
Thinking some more about this, I'm not sure the command ID should be mentioned here at all. It's a transport-layer concept that has no bearing on the definition of the command itself. Maybe the definition and requirements for the command ID should go in the transport section instead?
There was a problem hiding this comment.
The requirements are already there. I've changed things so that this section is only stating facts about the command id; I couldn't see anywhere in the transport section that would make it clearer.
There was a problem hiding this comment.
Thanks. The new description is more clear.
Also use remote-cddl for remote end definitions, and local-cddl for local end definitions
Previously the spec only provided fragments of CDDL to validate
specific kinds of messages. But it makes sense from an implementation
point of view to have full schema that can be used directly.
In practice we require two seperate schema, one for validation of
messages going from the local end (i.e. client) to the remote
end (i.e. browser) and one for messages going in the opposite
direction. This change adds those schema to the spec.
The modelling adopted here is likely not the only approach, but the
basic idea is that each kind of message is modelled as a choice
between several variants, representing each possible message in the
protocol. Because the messages have unique names (e.g. in the
methodfield) we know that zero or one will match. However the names are not
represented directly.
In terms of organistaion, this currently defines a schema prelude with
each message type specified by a set of choices, one per module. Then
each module defines a choice per command (or other message) and
finally the messages themselves define the actual schema. I expect
this organisation to change somewhat over time. In particular using
the
//=oeprator in CDDL seems like it might avoid the need todefine so many terms upfront, but it also seemed to be poorly
supported in the CDDL library I tried.
Preview | Diff