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
=:->
--
Juju-dev mailing list
[email protected]
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/juju-dev
