Schema for Juju RPC messages

[John Meinel]

FWIW, its a ~1 line change to encoding/json to be strict instead of not (at
least for clients adding extra keys, I'm not sure about clients omitting
keys, but we can sometimes tell because the struct would have the 0-value).
There was some discussion about a way forward was to "fork encoding/json if
you want to be strict". We could, I was trying to evaluate how hard doing
something like explicit schema validation on top, instead of just stricter
Unmarshal.

There is some extra stuff that you get from Schema, must be an integer
greater than 10 isn't really encoded into Unmarshal, or must be a field
referencing some other object in the same document. Though the latter is
not a huge ask of our API.

John
=:->


On Thu, Jul 28, 2016 at 5:30 AM, Tim Penhey <tim.penhey at canonical.com>
wrote:

> While at the London sprint I was toying with the idea of adding the
> ability of the rpc package to do some rudimentary initial validation.
>
> We could get a good part of the way with relatively little effort IMO.
>
> Using the reflect package, we can interrogate the public attributes of the
> structure we are attempting to serialize into. This gives access to the
> structure serialization tags. We could then use these tags to reject
> incoming messages if they provide any keys that are unexpected.
>
> For example, lets look at the ApplicationDeploy params:
> (had to munge some of the struct tags for the email)
> type ApplicationDeploy struct {
>   ApplicationName  string       `json:"application"`
>   Series           string       `json:"series"`
>   CharmUrl         string       `json:"charm-url"`
>   Channel          string       `json:"channel"`
>   NumUnits         int          `json:"num-units"`
>   Config           map[string]string `json:"config,omitempty"`
>   ConfigYAML       string            `json:"config-yaml"`
>   Constraints      constraints.Value  `json:"constraints"`
>   Placement        []*instance.Placement
>      `json:"placement,omitempty"`
>   Storage          map[string]storage.Constraints
>      `json:"storage,omitempty"`
>   EndpointBindings map[string]string
>      `json:"endpoint-bindings,omitempty"`
>   Resources        map[string]string
>      `json:"resources,omitempty"`
> }
>
> This would give us a set of valid keys:
>   ("application", "series", "charm-url", "channel", "num-units",
>    "config", "config-yaml", "constraints", "placement", "storage",
>    "endpoint-bindings", "resources")
>
> There would be overhead in doing the check though, because instead of
> deserializing directly into the structure, we'd deserialize into something
> like map[string]interface{} first, and do a key check, then only
> deserialize into the structure if it passed.
>
> We could use the "omitempty" tags to mark optional args that can be
> missing.
>
> Just doing this would catch the typos of keys, and would also make us be
> more strict in the "I'll just add this extra field" to existing facade
> versions, as they could then fail with older versions of the server with
> "unknown field" type errors.
>
> Thoughts?
> Tim
>
>
> On 28/07/16 04:29, John A Meinel wrote:
>
>> We've had some requests from people trying to drive Juju that it would
>> actually be really nice if we were stricter with the messages that we
>> accept on our API. Specifically, as we've changed the API methods, they
>> had several hard-to-debug problems because they were passing a parameter
>> that was named incorrectly, and Juju was not giving them any indication
>> that something was wrong.
>>
>> I put together a (very hackish) test branch, to see if I could use
>> JSONSchema to define our request and response messages, and give nicer
>> error messages (rather than silent acceptance). As I was working with
>> JSON, I realized the extra " and { characters really got in the way of
>> making a document that was readable, so I leveraged our existing YAML to
>> JSON conversion mechanisms to write the description in YAML and then
>> load the object tree into JSONSchema to validate our requests.
>>
>> I did end up getting basic validation of the outer structure (just the
>> Request/Response message, not the contents of Parameters) functioning
>> here:
>>
>> https://github.com/jameinel/juju/blob/yaml-schema-rpc/rpc/jsoncodec/codec.go
>> You can see what some of the error messages look like here:
>>
>> https://github.com/jameinel/juju/blob/yaml-schema-rpc/rpc/jsoncodec/codec_test.go
>>
>> The actual code itself isn't useful as is, because it needs to have the
>> schema validation stuff pulled out into its own file, etc. But it does
>> give a proof-of-concept of basic message validation. I'm not super
>> excited by some of the error messages
>> (gojsonschema.ResultError.Description is very nice by itself for missing
>> keys, extra properties, etc, but not enough information for invalid
>> values, while ResultError.String() is overly verbose in the opposite way.)
>>
>> I'd like to get a bit of feedback on whether people would find this
>> interesting, especially if we brought that all the way into the Param
>> structs as well. I haven't done any benchmarking to determine if this is
>> a large overhead or not. But the golfing community seems determined to
>> not do a Strict Unmarshal function, and seems to be recommending using a
>> Schema instead.
>>
>> I'm not completely sold either way, though I do find the YAML format for
>> the schema description to be far more human readable than the JSON format.
>>
>> Thoughts?
>>
>> John
>> =:->
>>
>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/juju-dev/attachments/20160728/ba455880/attachment-0001.html>