Handle nils in filesystem #50
Conversation
| coerced, err := checker.Coerce(source, nil) | ||
| if err != nil { | ||
| return nil, WrapWithDeserializationError(err, "filesystem 2.0 schema check failed") | ||
| } | ||
| valid := coerced.(map[string]interface{}) | ||
| // From here we know that the map returned from the schema coercion | ||
| // contains fields of the right type. | ||
|
|
||
| mount_point, ok := valid["mount_point"].(string) |
There was a problem hiding this comment.
no need for the if !ok check, as when not present mount_point will be "".
mount_point, _ := valid["mount_point"].(string)
There was a problem hiding this comment.
@dimitern This won't work if they're present but null in the JSON.
There was a problem hiding this comment.
Duh, misunderstood what you're saying, sorry.
There was a problem hiding this comment.
It works, but it can be misleading at first glance:
value, ok := someMapStringString["missing-key"] -> value = "", ok = false
value, _ := someMapStringString["missing-key"] -> value = "", no panic
value := someMapStringString["missing-key"] -> value = "", no panic
On the other hand:
value, ok := someNonStringInterfaceValue.(string) -> value = "", ok = false
value, _ := someNonStringInterfaceValue.(string) -> value = "", no panic
value := someNonStringInterfaceValue.(string) -> panics
We have both cases in one, so:
value, _ := mapStringInterface["missing-key"].(string) -> value = "" (i.e. we got a non-nil interface{} value with a wrapped nil in it due to the map lookup, but then trying to type assert that value to a string still returns "" and will panic without _ on the second return)
|
LGTM |
| if !ok { | ||
| mount_point = "" | ||
| } | ||
| label, ok := valid["label"].(string) |
There was a problem hiding this comment.
Same as above: label, _ := valid["label"].(string) is enough.
|
LGTM with a couple of suggestions for simplification. |
| @@ -39,24 +39,36 @@ func (f *filesystem) UUID() string { | |||
| func filesystem2_0(source map[string]interface{}) (*filesystem, error) { | |||
| fields := schema.Fields{ | |||
| "fstype": schema.String(), | |||
| "mount_point": schema.String(), | |||
| "label": schema.String(), | |||
| "mount_point": schema.OneOf(schema.Nil(""), schema.String()), | |||
There was a problem hiding this comment.
Do you need the schema.OneOfs here if you're putting in defaults below? Or is it that they're sometimes null in the JSON?
There was a problem hiding this comment.
It is needed if the value is nil it fails the schema check - even with a default. The default handles it being missing.
There was a problem hiding this comment.
Yes, the defaults are applied only after coercing the json blob to the field map.
|
LGTM |
|
|
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju-gomaasapi |
Fixes https://bugs.launchpad.net/juju-core/+bug/1575808