New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix panic about float in storage port, and add a test that verifies it #501
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -60,7 +60,14 @@ func (c *environConfig) storageListenIPAddress() string { | |
} | ||
|
||
func (c *environConfig) storagePort() int { | ||
return c.attrs["storage-port"].(int) | ||
switch val := c.attrs["storage-port"].(type) { | ||
case float64: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do any other types need to be included in this switch? float32? (u)int*? What guarantees do we have on the type to which the value de-serializes? Conceivably it could change in the future. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are comments in the code that says that this comes from deserializing from JSON, and JSON numbers are always a float64 unless they get coerced into other values.... since this is just a map[string]interface{}, it'll never coerce, just use the default (float64). There was actually already a test that supposedly was testing this, but obviously was not actually testing this code..... I left that test in there, but added this one to actually test a float getting in. |
||
return int(val) | ||
case int: | ||
return val | ||
default: | ||
panic(fmt.Sprintf("Unexpected %T in storage-port: %#v. Expected float64 or int.", val, val)) | ||
} | ||
} | ||
|
||
func (c *environConfig) storageAuthKey() string { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -157,3 +157,20 @@ func (s *configSuite) TestValidateConfigWithFloatPort(c *gc.C) { | |
unknownAttrs := valid.UnknownAttrs() | ||
c.Assert(unknownAttrs["storage-port"], gc.Equals, int(8040)) | ||
} | ||
|
||
func (s *configSuite) TestConfigWithFloatPort(c *gc.C) { | ||
// When the config values get serialized through JSON, the integers | ||
// get coerced to float64 values. The parsing needs to handle this. | ||
values := MinimalConfigValues() | ||
values["storage-port"] = float64(8040) | ||
cfg, err := config.New(config.UseDefaults, values) | ||
c.Assert(err, gc.IsNil) | ||
_, err = ProviderInstance.Validate(cfg, nil) | ||
c.Assert(err, gc.IsNil) | ||
|
||
env, err := ProviderInstance.Open(cfg) | ||
c.Assert(err, gc.IsNil) | ||
|
||
// really, we're asserting that this doesn't panic :) | ||
c.Assert(env.(*manualEnviron).cfg.storagePort(), gc.Equals, int(8040)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could also use the gc.Panic/gc.PanicMatches checkers to test that it does panic when expected. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the int cast on int(8040) necessary? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As far as I understand it, at the point gocheck compares the values go has already inferred a type from the literal: int. So the cast is necessary. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It really shouldn't ever be set to something that's not a float or an int.... the panic is basically just there in the cast because something has to happen there, and we can't return anything. The cast to int is not necessary... the constant 8040 will default to becoming an int, but it does sort of make it a little more clear that the constant isn't getting implicitly cast to some other type (like float64). |
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this is the only non-string/bool field, so the others should be fine. FYI, I find it surprising that the whole config schema mechanism didn't take care of the type for us.