Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix panic about float in storage port, and add a test that verifies it #501

Merged
merged 1 commit into from Aug 12, 2014
Merged

fix panic about float in storage port, and add a test that verifies it #501

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 8 additions & 1 deletion provider/manual/config.go
View file
Open in desktop
Expand Up @@ -60,7 +60,14 @@ func (c *environConfig) storageListenIPAddress() string {
}

func (c *environConfig) storagePort() int {
Copy link
Contributor

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.

return c.attrs["storage-port"].(int)
switch val := c.attrs["storage-port"].(type) {
case float64:
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 {
Expand Down
17 changes: 17 additions & 0 deletions provider/manual/config_test.go
View file
Open in desktop
Expand Up @@ -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))
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the int cast on int(8040) necessary?

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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).

}