Schema for Juju RPC messages

[Tim Penhey]

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
> =:->
>
>
>