Merge lp:~dimitern/juju-core/148-apiuniter-fix-configsettings into lp:~go-bot/juju-core/trunk

[Dimiter Naydenov]

apiuniter: Fix lp:1231457

This changes several params types, while keeping
the field names for backward compatibility.
And changes the result of ConfigSettings() call
to return map[string]interface{}, because charm
settings are different from relation settings
(which are always strings), and coercing charm
settings to stings discards non-string config
settings, so this is fixed here.

https://codereview.appspot.com/13908044/

R=fwereade

apiuniter: Fix lp:1231457

This changes several params types, while keeping
the field names for backward compatibility.
And changes the result of ConfigSettings() call
to return map[string]interface{}, because charm
settings are different from relation settings
(which are always strings), and coercing charm
settings to stings discards non-string config
settings, so this is fixed here.

https://codereview.appspot.com/13908044/

[Dimiter Naydenov (dimitern)]

Reviewers: mp+187824_code.launchpad.net,

Message:
Please take a look.

Description:
apiuniter: Fix lp:1231457

This changes several params types, while keeping
the field names for backward compatibility.
And changes the result of ConfigSettings() call
to return map[string]interface{}, because charm
settings are different from relation settings
(which are always strings), and coercing charm
settings to stings discards non-string config
settings, so this is fixed here.

https://code.launchpad.net/~dimitern/juju-core/148-apiuniter-fix-configsettings/+merge/187824

Requires:
https://code.launchpad.net/~dimitern/juju-core/147-apiprovisioner-blank-env-secrets/+merge/187738

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/13908044/

Affected files (+121, -111 lines):
   A [revision details]
   M state/api/params/internal.go
   M state/api/provisioner/provisioner.go
   M state/api/uniter/relationunit.go
   M state/api/uniter/relationunit_test.go
   M state/api/uniter/settings.go
   M state/api/uniter/settings_test.go
   M state/api/uniter/unit.go
   M state/apiserver/provisioner/provisioner.go
   M state/apiserver/provisioner/provisioner_test.go
   M state/apiserver/uniter/uniter.go
   M state/apiserver/uniter/uniter_test.go
   M worker/uniter/context.go
   M worker/uniter/context_test.go
   M worker/uniter/jujuc/context.go
   M worker/uniter/jujuc/relation-get.go
   M worker/uniter/jujuc/util_test.go

[William Reade (fwereade)]

LGTM with an error return on unexpected relation settings.

https://codereview.appspot.com/13908044/diff/1/state/api/params/internal.go
File state/api/params/internal.go (right):

https://codereview.appspot.com/13908044/diff/1/state/api/params/internal.go#newcode138
state/api/params/internal.go:138: // EnvironConfig holds an environment
configuration.
Man, our vocabulary is still kinda screwed up here... "Config" means too
many things. Not much we can do about it right now though.

https://codereview.appspot.com/13908044/diff/1/state/api/uniter/unit.go
File state/api/uniter/unit.go (right):

https://codereview.appspot.com/13908044/diff/1/state/api/uniter/unit.go#newcode143
state/api/uniter/unit.go:143: return
map[string]interface{}(result.Settings), nil
can you not do charm.Settings(result.Settings)?

https://codereview.appspot.com/13908044/diff/1/state/apiserver/uniter/uniter.go
File state/apiserver/uniter/uniter.go (left):

https://codereview.appspot.com/13908044/diff/1/state/apiserver/uniter/uniter.go#oldcode797
state/apiserver/uniter/uniter.go:797: // All relation settings should be
strings.
Hmm. This is almost legitimate-panic territory. Best not, since we're
running in the API server and will cause a lot of collateral damage, but
I think we should at least error out -- if it happens, we're clearly not
in Kansas any more.

https://codereview.appspot.com/13908044/

[Dimiter Naydenov (dimitern)]

Please take a look.

https://codereview.appspot.com/13908044/diff/1/state/api/uniter/unit.go
File state/api/uniter/unit.go (right):

https://codereview.appspot.com/13908044/diff/1/state/api/uniter/unit.go#newcode143
state/api/uniter/unit.go:143: return
map[string]interface{}(result.Settings), nil
On 2013/09/26 16:11:52, fwereade wrote:
> can you not do charm.Settings(result.Settings)?

Done.

https://codereview.appspot.com/13908044/diff/1/state/apiserver/uniter/uniter.go
File state/apiserver/uniter/uniter.go (left):

https://codereview.appspot.com/13908044/diff/1/state/apiserver/uniter/uniter.go#oldcode797
state/apiserver/uniter/uniter.go:797: // All relation settings should be
strings.
On 2013/09/26 16:11:52, fwereade wrote:
> Hmm. This is almost legitimate-panic territory. Best not, since we're
running in
> the API server and will cause a lot of collateral damage, but I think
we should
> at least error out -- if it happens, we're clearly not in Kansas any
more.

Done. Also added a couple of tests for the error.

https://codereview.appspot.com/13908044/

=== modified file 'state/api/params/internal.go'
--- state/api/params/internal.go	2013-09-24 08:55:01 +0000
+++ state/api/params/internal.go	2013-09-26 17:24:28 +0000
@@ -105,28 +105,43 @@
 	Results []BoolResult
 }
 
-// Settings holds charm config options names and values.
-type Settings map[string]string
-
-// SettingsResult holds a charm settings map or an error.
-type SettingsResult struct {
-	Error    *Error
-	Settings Settings
-}
-
-// SettingsResults holds the result of an API calls that returns
-// settings for multiple entities.
-type SettingsResults struct {
-	Results []SettingsResult
-}
-
-// Config holds configuration with string keys and arbitrary values.
-type Config map[string]interface{}
-
-// ConfigResult holds a configuration map or an error.
-type ConfigResult struct {
+// RelationSettings holds relation settings names and values.
+type RelationSettings map[string]string
+
+// RelationSettingsResult holds a relation settings map or an error.
+type RelationSettingsResult struct {
+	Error    *Error
+	Settings RelationSettings
+}
+
+// RelationSettingsResults holds the result of an API calls that
+// returns settings for multiple relations.
+type RelationSettingsResults struct {
+	Results []RelationSettingsResult
+}
+
+// ConfigSettings holds unit, service or cham configuration settings
+// with string keys and arbitrary values.
+type ConfigSettings map[string]interface{}
+
+// ConfigSettingsResult holds a configuration map or an error.
+type ConfigSettingsResult struct {
+	Error    *Error
+	Settings ConfigSettings
+}
+
+// ConfigSettingsResults holds multiple configuration maps or errors.
+type ConfigSettingsResults struct {
+	Results []ConfigSettingsResult
+}
+
+// EnvironConfig holds an environment configuration.
+type EnvironConfig map[string]interface{}
+
+// EnvironConfigResult holds environment configuration or an error.
+type EnvironConfigResult struct {
 	Error  *Error
-	Config Config
+	Config EnvironConfig
 }
 
 // RelationUnit holds a relation and a unit tag.
@@ -164,7 +179,7 @@
 type RelationUnitSettings struct {
 	Relation string
 	Unit     string
-	Settings Settings
+	Settings RelationSettings
 }
 
 // RelationUnitsSettings holds the arguments for making a EnterScope
 
=== modified file 'state/api/provisioner/provisioner.go'
--- state/api/provisioner/provisioner.go	2013-09-26 10:38:59 +0000
+++ state/api/provisioner/provisioner.go	2013-09-26 17:24:28 +0000
@@ -71,7 +71,7 @@
 
 // EnvironConfig returns the current environment configuration.
 func (st *State) EnvironConfig() (*config.Config, error) {
-	var result params.ConfigResult
+	var result params.EnvironConfigResult
 	err := st.caller.Call("Provisioner", "", "EnvironConfig", nil, &result)
 	if err != nil {
 		return nil, err
 
=== modified file 'state/api/uniter/relationunit.go'
--- state/api/uniter/relationunit.go	2013-09-10 17:00:19 +0000
+++ state/api/uniter/relationunit.go	2013-09-26 17:24:28 +0000
@@ -105,7 +105,7 @@
 // Settings returns a Settings which allows access to the unit's settings
 // within the relation.
 func (ru *RelationUnit) Settings() (*Settings, error) {
-	var results params.SettingsResults
+	var results params.RelationSettingsResults
 	args := params.RelationUnits{
 		RelationUnits: []params.RelationUnit{{
 			Relation: ru.relation.tag,
@@ -133,9 +133,9 @@
 // unit is not grounds for an error, because the unit settings are
 // guaranteed to persist for the lifetime of the relation, regardless
 // of the lifetime of the unit.
-func (ru *RelationUnit) ReadSettings(uname string) (params.Settings, error) {
+func (ru *RelationUnit) ReadSettings(uname string) (params.RelationSettings, error) {
 	tag := names.UnitTag(uname)
-	var results params.SettingsResults
+	var results params.RelationSettingsResults
 	args := params.RelationUnitPairs{
 		RelationUnitPairs: []params.RelationUnitPair{{
 			Relation:   ru.relation.tag,
 
=== modified file 'state/api/uniter/relationunit_test.go'
--- state/api/uniter/relationunit_test.go	2013-09-13 11:35:33 +0000
+++ state/api/uniter/relationunit_test.go	2013-09-26 17:24:28 +0000
@@ -196,7 +196,7 @@
 
 	gotSettings, err := apiRelUnit.Settings()
 	c.Assert(err, gc.IsNil)
-	c.Assert(gotSettings.Map(), gc.DeepEquals, params.Settings{
+	c.Assert(gotSettings.Map(), gc.DeepEquals, params.RelationSettings{
 		"some":  "settings",
 		"other": "things",
 	})
@@ -230,7 +230,7 @@
 	s.assertInScope(c, myRelUnit, true)
 	gotSettings, err = apiRelUnit.ReadSettings("mysql/0")
 	c.Assert(err, gc.IsNil)
-	c.Assert(gotSettings, gc.DeepEquals, params.Settings{
+	c.Assert(gotSettings, gc.DeepEquals, params.RelationSettings{
 		"some":  "settings",
 		"other": "things",
 	})
 
=== modified file 'state/api/uniter/settings.go'
--- state/api/uniter/settings.go	2013-09-06 16:22:34 +0000
+++ state/api/uniter/settings.go	2013-09-26 17:24:28 +0000
@@ -15,12 +15,12 @@
 	st          *State
 	relationTag string
 	unitTag     string
-	settings    params.Settings
+	settings    params.RelationSettings
 }
 
-func newSettings(st *State, relationTag, unitTag string, settings params.Settings) *Settings {
+func newSettings(st *State, relationTag, unitTag string, settings params.RelationSettings) *Settings {
 	if settings == nil {
-		settings = make(params.Settings)
+		settings = make(params.RelationSettings)
 	}
 	return &Settings{
 		st:          st,
@@ -36,8 +36,8 @@
 // not return map[string]interface{}, but since all values are
 // expected to be strings anyway, we need to fix the uniter code
 // accordingly when migrating to the API.
-func (s *Settings) Map() params.Settings {
-	settingsCopy := make(params.Settings)
+func (s *Settings) Map() params.RelationSettings {
+	settingsCopy := make(params.RelationSettings)
 	for k, v := range s.settings {
 		if v != "" {
 			// Skip deleted keys.
@@ -75,7 +75,7 @@
 // without overwritting.
 func (s *Settings) Write() error {
 	// First make a copy of the map, including deleted keys.
-	settingsCopy := make(params.Settings)
+	settingsCopy := make(params.RelationSettings)
 	for k, v := range s.settings {
 		settingsCopy[k] = v
 	}
 
=== modified file 'state/api/uniter/settings_test.go'
--- state/api/uniter/settings_test.go	2013-09-06 15:39:38 +0000
+++ state/api/uniter/settings_test.go	2013-09-26 17:24:28 +0000
@@ -34,7 +34,7 @@
 	c.Assert(theMap, gc.HasLen, 0)
 
 	// And also accepts a populated map, and returns a converted map.
-	rawSettings := params.Settings{
+	rawSettings := params.RelationSettings{
 		"some":  "settings",
 		"other": "stuff",
 	}
@@ -47,15 +47,15 @@
 	settings := uniter.NewSettings(s.uniter, "blah", "foo", nil)
 
 	settings.Set("foo", "bar")
-	c.Assert(settings.Map(), gc.DeepEquals, params.Settings{
+	c.Assert(settings.Map(), gc.DeepEquals, params.RelationSettings{
 		"foo": "bar",
 	})
 	settings.Set("foo", "qaz")
-	c.Assert(settings.Map(), gc.DeepEquals, params.Settings{
+	c.Assert(settings.Map(), gc.DeepEquals, params.RelationSettings{
 		"foo": "qaz",
 	})
 	settings.Set("bar", "Cheers")
-	c.Assert(settings.Map(), gc.DeepEquals, params.Settings{
+	c.Assert(settings.Map(), gc.DeepEquals, params.RelationSettings{
 		"foo": "qaz",
 		"bar": "Cheers",
 	})
@@ -67,27 +67,27 @@
 	settings.Set("foo", "qaz")
 	settings.Set("abc", "tink")
 	settings.Set("bar", "tonk")
-	c.Assert(settings.Map(), gc.DeepEquals, params.Settings{
+	c.Assert(settings.Map(), gc.DeepEquals, params.RelationSettings{
 		"foo": "qaz",
 		"abc": "tink",
 		"bar": "tonk",
 	})
 	settings.Delete("abc")
-	c.Assert(settings.Map(), gc.DeepEquals, params.Settings{
+	c.Assert(settings.Map(), gc.DeepEquals, params.RelationSettings{
 		"foo": "qaz",
 		"bar": "tonk",
 	})
 	settings.Delete("bar")
-	c.Assert(settings.Map(), gc.DeepEquals, params.Settings{
+	c.Assert(settings.Map(), gc.DeepEquals, params.RelationSettings{
 		"foo": "qaz",
 	})
 	settings.Set("abc", "123")
-	c.Assert(settings.Map(), gc.DeepEquals, params.Settings{
+	c.Assert(settings.Map(), gc.DeepEquals, params.RelationSettings{
 		"foo": "qaz",
 		"abc": "123",
 	})
 	settings.Delete("missing")
-	c.Assert(settings.Map(), gc.DeepEquals, params.Settings{
+	c.Assert(settings.Map(), gc.DeepEquals, params.RelationSettings{
 		"foo": "qaz",
 		"abc": "123",
 	})
@@ -112,7 +112,7 @@
 	c.Assert(err, gc.IsNil)
 	settings, err := apiRelUnit.Settings()
 	c.Assert(err, gc.IsNil)
-	c.Assert(settings.Map(), gc.DeepEquals, params.Settings{
+	c.Assert(settings.Map(), gc.DeepEquals, params.RelationSettings{
 		"some":  "stuff",
 		"other": "things",
 	})
@@ -126,7 +126,7 @@
 	c.Assert(err, gc.IsNil)
 	settings, err = apiRelUnit.Settings()
 	c.Assert(err, gc.IsNil)
-	c.Assert(settings.Map(), gc.DeepEquals, params.Settings{
+	c.Assert(settings.Map(), gc.DeepEquals, params.RelationSettings{
 		"foo":   "qaz",
 		"other": "days",
 	})
 
=== modified file 'state/api/uniter/unit.go'
--- state/api/uniter/unit.go	2013-09-17 18:18:44 +0000
+++ state/api/uniter/unit.go	2013-09-26 17:24:28 +0000
@@ -120,20 +120,12 @@
 	return service, nil
 }
 
-func convertSettings(input params.Settings) charm.Settings {
-	result := make(charm.Settings)
-	for k, v := range input {
-		result[k] = v
-	}
-	return result
-}
-
 // ConfigSettings returns the complete set of service charm config settings
 // available to the unit. Unset values will be replaced with the default
 // value for the associated option, and may thus be nil when no default is
 // specified.
 func (u *Unit) ConfigSettings() (charm.Settings, error) {
-	var results params.SettingsResults
+	var results params.ConfigSettingsResults
 	args := params.Entities{
 		Entities: []params.Entity{{Tag: u.tag}},
 	}
@@ -148,7 +140,7 @@
 	if result.Error != nil {
 		return nil, result.Error
 	}
-	return convertSettings(result.Settings), nil
+	return charm.Settings(result.Settings), nil
 }
 
 // ServiceName returns the service name.
 
=== modified file 'state/apiserver/provisioner/provisioner.go'
--- state/apiserver/provisioner/provisioner.go	2013-09-26 16:01:47 +0000
+++ state/apiserver/provisioner/provisioner.go	2013-09-26 17:24:28 +0000
@@ -152,8 +152,8 @@
 }
 
 // EnvironConfig returns the current environment's configuration.
-func (p *ProvisionerAPI) EnvironConfig() (params.ConfigResult, error) {
-	result := params.ConfigResult{}
+func (p *ProvisionerAPI) EnvironConfig() (params.EnvironConfigResult, error) {
+	result := params.EnvironConfigResult{}
 	config, err := p.st.EnvironConfig()
 	if err != nil {
 		return result, err
 
=== modified file 'state/apiserver/provisioner/provisioner_test.go'
--- state/apiserver/provisioner/provisioner_test.go	2013-09-26 11:15:31 +0000
+++ state/apiserver/provisioner/provisioner_test.go	2013-09-26 17:24:28 +0000
@@ -421,7 +421,7 @@
 	result, err := s.provisioner.EnvironConfig()
 	c.Assert(err, gc.IsNil)
 	c.Assert(result.Error, gc.IsNil)
-	c.Assert(result.Config, gc.DeepEquals, params.Config(envConfig.AllAttrs()))
+	c.Assert(result.Config, gc.DeepEquals, params.EnvironConfig(envConfig.AllAttrs()))
 
 	// Now test it with a non-environment manager and make sure
 	// the secret attributes are masked.
 
=== modified file 'state/apiserver/uniter/uniter.go'
--- state/apiserver/uniter/uniter.go	2013-09-12 14:58:40 +0000
+++ state/apiserver/uniter/uniter.go	2013-09-26 17:24:28 +0000
@@ -6,6 +6,8 @@
 package uniter
 
 import (
+	"fmt"
+
 	"launchpad.net/juju-core/charm"
 	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/errors"
@@ -489,13 +491,13 @@
 
 // ConfigSettings returns the complete set of service charm config
 // settings available to each given unit.
-func (u *UniterAPI) ConfigSettings(args params.Entities) (params.SettingsResults, error) {
-	result := params.SettingsResults{
-		Results: make([]params.SettingsResult, len(args.Entities)),
+func (u *UniterAPI) ConfigSettings(args params.Entities) (params.ConfigSettingsResults, error) {
+	result := params.ConfigSettingsResults{
+		Results: make([]params.ConfigSettingsResult, len(args.Entities)),
 	}
 	canAccess, err := u.accessUnit()
 	if err != nil {
-		return params.SettingsResults{}, err
+		return params.ConfigSettingsResults{}, err
 	}
 	for i, entity := range args.Entities {
 		err := common.ErrPerm
@@ -506,7 +508,7 @@
 				var settings charm.Settings
 				settings, err = unit.ConfigSettings()
 				if err == nil {
-					result.Results[i].Settings = convertSettings(settings)
+					result.Results[i].Settings = params.ConfigSettings(settings)
 				}
 			}
 		}
@@ -791,25 +793,28 @@
 	return result, nil
 }
 
-func convertSettings(settings map[string]interface{}) params.Settings {
-	result := make(params.Settings)
+func convertRelationSettings(settings map[string]interface{}) (params.RelationSettings, error) {
+	result := make(params.RelationSettings)
 	for k, v := range settings {
 		// All relation settings should be strings.
-		sval, _ := v.(string)
+		sval, ok := v.(string)
+		if !ok {
+			return nil, fmt.Errorf("unexpected relation setting %q: expected string, got %T", k, v)
+		}
 		result[k] = sval
 	}
-	return result
+	return result, nil
 }
 
 // ReadSettings returns the local settings of each given set of
 // relation/unit.
-func (u *UniterAPI) ReadSettings(args params.RelationUnits) (params.SettingsResults, error) {
-	result := params.SettingsResults{
-		Results: make([]params.SettingsResult, len(args.RelationUnits)),
+func (u *UniterAPI) ReadSettings(args params.RelationUnits) (params.RelationSettingsResults, error) {
+	result := params.RelationSettingsResults{
+		Results: make([]params.RelationSettingsResult, len(args.RelationUnits)),
 	}
 	canAccess, err := u.accessUnit()
 	if err != nil {
-		return params.SettingsResults{}, err
+		return params.RelationSettingsResults{}, err
 	}
 	for i, arg := range args.RelationUnits {
 		relUnit, err := u.getRelationUnit(canAccess, arg.Relation, arg.Unit)
@@ -817,7 +822,7 @@
 			var settings *state.Settings
 			settings, err = relUnit.Settings()
 			if err == nil {
-				result.Results[i].Settings = convertSettings(settings.Map())
+				result.Results[i].Settings, err = convertRelationSettings(settings.Map())
 			}
 		}
 		result.Results[i].Error = common.ServerError(err)
@@ -845,13 +850,13 @@
 
 // ReadRemoteSettings returns the remote settings of each given set of
 // relation/local unit/remote unit.
-func (u *UniterAPI) ReadRemoteSettings(args params.RelationUnitPairs) (params.SettingsResults, error) {
-	result := params.SettingsResults{
-		Results: make([]params.SettingsResult, len(args.RelationUnitPairs)),
+func (u *UniterAPI) ReadRemoteSettings(args params.RelationUnitPairs) (params.RelationSettingsResults, error) {
+	result := params.RelationSettingsResults{
+		Results: make([]params.RelationSettingsResult, len(args.RelationUnitPairs)),
 	}
 	canAccess, err := u.accessUnit()
 	if err != nil {
-		return params.SettingsResults{}, err
+		return params.RelationSettingsResults{}, err
 	}
 	for i, arg := range args.RelationUnitPairs {
 		relUnit, err := u.getRelationUnit(canAccess, arg.Relation, arg.LocalUnit)
@@ -862,7 +867,7 @@
 				var settings map[string]interface{}
 				settings, err = relUnit.ReadSettings(remoteUnit)
 				if err == nil {
-					result.Results[i].Settings = convertSettings(settings)
+					result.Results[i].Settings, err = convertRelationSettings(settings)
 				}
 			}
 		}
 
=== modified file 'state/apiserver/uniter/uniter_test.go'
--- state/apiserver/uniter/uniter_test.go	2013-09-18 15:57:30 +0000
+++ state/apiserver/uniter/uniter_test.go	2013-09-26 17:24:28 +0000
@@ -808,10 +808,10 @@
 	}}
 	result, err := s.uniter.ConfigSettings(args)
 	c.Assert(err, gc.IsNil)
-	c.Assert(result, gc.DeepEquals, params.SettingsResults{
-		Results: []params.SettingsResult{
+	c.Assert(result, gc.DeepEquals, params.ConfigSettingsResults{
+		Results: []params.ConfigSettingsResult{
 			{Error: apiservertesting.ErrUnauthorized},
-			{Settings: params.Settings{"blog-title": "My Title"}},
+			{Settings: params.ConfigSettings{"blog-title": "My Title"}},
 			{Error: apiservertesting.ErrUnauthorized},
 		},
 	})
@@ -1097,10 +1097,10 @@
 	}}
 	result, err := s.uniter.ReadSettings(args)
 	c.Assert(err, gc.IsNil)
-	c.Assert(result, gc.DeepEquals, params.SettingsResults{
-		Results: []params.SettingsResult{
+	c.Assert(result, gc.DeepEquals, params.RelationSettingsResults{
+		Results: []params.RelationSettingsResult{
 			{Error: apiservertesting.ErrUnauthorized},
-			{Settings: params.Settings{
+			{Settings: params.RelationSettings{
 				"some": "settings",
 			}},
 			{Error: apiservertesting.ErrUnauthorized},
@@ -1116,6 +1116,31 @@
 	})
 }
 
+func (s *uniterSuite) TestReadSettingsWithNonStringValuesFails(c *gc.C) {
+	rel := s.addRelation(c, "wordpress", "mysql")
+	relUnit, err := rel.Unit(s.wordpressUnit)
+	c.Assert(err, gc.IsNil)
+	settings := map[string]interface{}{
+		"other":        "things",
+		"invalid-bool": false,
+	}
+	err = relUnit.EnterScope(settings)
+	c.Assert(err, gc.IsNil)
+	s.assertInScope(c, relUnit, true)
+
+	args := params.RelationUnits{RelationUnits: []params.RelationUnit{
+		{Relation: rel.Tag(), Unit: "unit-wordpress-0"},
+	}}
+	expectErr := `unexpected relation setting "invalid-bool": expected string, got bool`
+	result, err := s.uniter.ReadSettings(args)
+	c.Assert(err, gc.IsNil)
+	c.Assert(result, gc.DeepEquals, params.RelationSettingsResults{
+		Results: []params.RelationSettingsResult{
+			{Error: &params.Error{Message: expectErr}},
+		},
+	})
+}
+
 func (s *uniterSuite) TestReadRemoteSettings(c *gc.C) {
 	rel := s.addRelation(c, "wordpress", "mysql")
 	relUnit, err := rel.Unit(s.wordpressUnit)
@@ -1146,8 +1171,8 @@
 	// We don't set the remote unit settings on purpose to test the error.
 	expectErr := `cannot read settings for unit "mysql/0" in relation "wordpress:db mysql:server": settings not found`
 	c.Assert(err, gc.IsNil)
-	c.Assert(result, gc.DeepEquals, params.SettingsResults{
-		Results: []params.SettingsResult{
+	c.Assert(result, gc.DeepEquals, params.RelationSettingsResults{
+		Results: []params.RelationSettingsResult{
 			{Error: apiservertesting.ErrUnauthorized},
 			{Error: apiservertesting.ErrUnauthorized},
 			{Error: &params.Error{Message: expectErr}},
@@ -1182,15 +1207,42 @@
 	}}}
 	result, err = s.uniter.ReadRemoteSettings(args)
 	c.Assert(err, gc.IsNil)
-	c.Assert(result, gc.DeepEquals, params.SettingsResults{
-		Results: []params.SettingsResult{
-			{Settings: params.Settings{
+	c.Assert(result, gc.DeepEquals, params.RelationSettingsResults{
+		Results: []params.RelationSettingsResult{
+			{Settings: params.RelationSettings{
 				"other": "things",
 			}},
 		},
 	})
 }
 
+func (s *uniterSuite) TestReadRemoteSettingsWithNonStringValuesFails(c *gc.C) {
+	rel := s.addRelation(c, "wordpress", "mysql")
+	relUnit, err := rel.Unit(s.mysqlUnit)
+	c.Assert(err, gc.IsNil)
+	settings := map[string]interface{}{
+		"other":        "things",
+		"invalid-bool": false,
+	}
+	err = relUnit.EnterScope(settings)
+	c.Assert(err, gc.IsNil)
+	s.assertInScope(c, relUnit, true)
+
+	args := params.RelationUnitPairs{RelationUnitPairs: []params.RelationUnitPair{{
+		Relation:   rel.Tag(),
+		LocalUnit:  "unit-wordpress-0",
+		RemoteUnit: "unit-mysql-0",
+	}}}
+	expectErr := `unexpected relation setting "invalid-bool": expected string, got bool`
+	result, err := s.uniter.ReadRemoteSettings(args)
+	c.Assert(err, gc.IsNil)
+	c.Assert(result, gc.DeepEquals, params.RelationSettingsResults{
+		Results: []params.RelationSettingsResult{
+			{Error: &params.Error{Message: expectErr}},
+		},
+	})
+}
+
 func (s *uniterSuite) TestUpdateSettings(c *gc.C) {
 	rel := s.addRelation(c, "wordpress", "mysql")
 	relUnit, err := rel.Unit(s.wordpressUnit)
@@ -1202,7 +1254,7 @@
 	err = relUnit.EnterScope(settings)
 	s.assertInScope(c, relUnit, true)
 
-	newSettings := params.Settings{
+	newSettings := params.RelationSettings{
 		"some":  "different",
 		"other": "",
 	}
 
=== modified file 'worker/uniter/context.go'
--- worker/uniter/context.go	2013-09-13 10:38:02 +0000
+++ worker/uniter/context.go	2013-09-26 17:24:28 +0000
@@ -275,7 +275,7 @@
 }
 
 // SettingsMap is a map from unit name to relation settings.
-type SettingsMap map[string]params.Settings
+type SettingsMap map[string]params.RelationSettings
 
 // ContextRelation is the implementation of jujuc.ContextRelation.
 type ContextRelation struct {
@@ -367,7 +367,7 @@
 	return ctx.settings, nil
 }
 
-func (ctx *ContextRelation) ReadSettings(unit string) (settings params.Settings, err error) {
+func (ctx *ContextRelation) ReadSettings(unit string) (settings params.RelationSettings, err error) {
 	settings, member := ctx.members[unit]
 	if settings == nil {
 		if settings = ctx.cache[unit]; settings == nil {
 
=== modified file 'worker/uniter/context_test.go'
--- worker/uniter/context_test.go	2013-09-17 03:12:12 +0000
+++ worker/uniter/context_test.go	2013-09-26 17:24:28 +0000
@@ -275,10 +275,10 @@
 	// Check that the changes to the local settings nodes have been discarded.
 	node0, err = s.relctxs[0].Settings()
 	c.Assert(err, gc.IsNil)
-	c.Assert(node0.Map(), gc.DeepEquals, params.Settings{"relation-name": "db0"})
+	c.Assert(node0.Map(), gc.DeepEquals, params.RelationSettings{"relation-name": "db0"})
 	node1, err = s.relctxs[1].Settings()
 	c.Assert(err, gc.IsNil)
-	c.Assert(node1.Map(), gc.DeepEquals, params.Settings{"relation-name": "db1"})
+	c.Assert(node1.Map(), gc.DeepEquals, params.RelationSettings{"relation-name": "db1"})
 
 	// Check that the changes have been written to state.
 	settings0, err := s.relunits[0].ReadSettings("u/0")
@@ -303,13 +303,13 @@
 	// Check that the changes to the local settings nodes are still there.
 	node0, err = s.relctxs[0].Settings()
 	c.Assert(err, gc.IsNil)
-	c.Assert(node0.Map(), gc.DeepEquals, params.Settings{
+	c.Assert(node0.Map(), gc.DeepEquals, params.RelationSettings{
 		"relation-name": "db0",
 		"baz":           "3",
 	})
 	node1, err = s.relctxs[1].Settings()
 	c.Assert(err, gc.IsNil)
-	c.Assert(node1.Map(), gc.DeepEquals, params.Settings{
+	c.Assert(node1.Map(), gc.DeepEquals, params.RelationSettings{
 		"relation-name": "db1",
 		"qux":           "4",
 	})
@@ -383,13 +383,13 @@
 		"u/4": {"qux": "4"},
 	})
 	c.Assert(ctx.UnitNames(), gc.DeepEquals, []string{"u/2", "u/4"})
-	assertSettings := func(unit string, expect params.Settings) {
+	assertSettings := func(unit string, expect params.RelationSettings) {
 		actual, err := ctx.ReadSettings(unit)
 		c.Assert(err, gc.IsNil)
 		c.Assert(actual, gc.DeepEquals, expect)
 	}
-	assertSettings("u/2", params.Settings{"baz": "2"})
-	assertSettings("u/4", params.Settings{"qux": "4"})
+	assertSettings("u/2", params.RelationSettings{"baz": "2"})
+	assertSettings("u/4", params.RelationSettings{"qux": "4"})
 
 	// Send a second update; check that members are only added, not removed.
 	ctx.UpdateMembers(uniter.SettingsMap{
@@ -400,10 +400,10 @@
 	c.Assert(ctx.UnitNames(), gc.DeepEquals, []string{"u/1", "u/2", "u/3", "u/4"})
 
 	// Check that all settings remain cached.
-	assertSettings("u/1", params.Settings{"foo": "1"})
-	assertSettings("u/2", params.Settings{"abc": "2"})
-	assertSettings("u/3", params.Settings{"bar": "3"})
-	assertSettings("u/4", params.Settings{"qux": "4"})
+	assertSettings("u/1", params.RelationSettings{"foo": "1"})
+	assertSettings("u/2", params.RelationSettings{"abc": "2"})
+	assertSettings("u/3", params.RelationSettings{"bar": "3"})
+	assertSettings("u/4", params.RelationSettings{"qux": "4"})
 
 	// Delete a member, and check that it is no longer a member...
 	ctx.DeleteMember("u/2")
@@ -454,7 +454,7 @@
 	ctx.UpdateMembers(uniter.SettingsMap{"u/1": {"entirely": "different"}})
 	m, err = ctx.ReadSettings("u/1")
 	c.Assert(err, gc.IsNil)
-	c.Assert(m, gc.DeepEquals, params.Settings{"entirely": "different"})
+	c.Assert(m, gc.DeepEquals, params.RelationSettings{"entirely": "different"})
 }
 
 func (s *ContextRelationSuite) TestNonMemberCaching(c *gc.C) {
@@ -700,7 +700,7 @@
 	return context
 }
 
-func convertSettings(settings params.Settings) map[string]interface{} {
+func convertSettings(settings params.RelationSettings) map[string]interface{} {
 	result := make(map[string]interface{})
 	for k, v := range settings {
 		result[k] = v
@@ -708,8 +708,8 @@
 	return result
 }
 
-func convertMap(settingsMap map[string]interface{}) params.Settings {
-	result := make(params.Settings)
+func convertMap(settingsMap map[string]interface{}) params.RelationSettings {
+	result := make(params.RelationSettings)
 	for k, v := range settingsMap {
 		result[k] = v.(string)
 	}
 
=== modified file 'worker/uniter/jujuc/context.go'
--- worker/uniter/jujuc/context.go	2013-09-12 16:29:52 +0000
+++ worker/uniter/jujuc/context.go	2013-09-26 17:24:28 +0000
@@ -77,12 +77,12 @@
 	UnitNames() []string
 
 	// ReadSettings returns the settings of any remote unit in the relation.
-	ReadSettings(unit string) (params.Settings, error)
+	ReadSettings(unit string) (params.RelationSettings, error)
 }
 
 // Settings is implemented by types that manipulate unit settings.
 type Settings interface {
-	Map() params.Settings
+	Map() params.RelationSettings
 	Set(string, string)
 	Delete(string)
 }
 
=== modified file 'worker/uniter/jujuc/relation-get.go'
--- worker/uniter/jujuc/relation-get.go	2013-09-12 16:29:52 +0000
+++ worker/uniter/jujuc/relation-get.go	2013-09-26 17:24:28 +0000
@@ -78,7 +78,7 @@
 	if !found {
 		return fmt.Errorf("unknown relation id")
 	}
-	var settings params.Settings
+	var settings params.RelationSettings
 	if c.UnitName == c.ctx.UnitName() {
 		node, err := r.Settings()
 		if err != nil {
 
=== modified file 'worker/uniter/jujuc/util_test.go'
--- worker/uniter/jujuc/util_test.go	2013-09-12 16:29:52 +0000
+++ worker/uniter/jujuc/util_test.go	2013-09-26 17:24:28 +0000
@@ -166,7 +166,7 @@
 	return s
 }
 
-func (r *ContextRelation) ReadSettings(name string) (params.Settings, error) {
+func (r *ContextRelation) ReadSettings(name string) (params.RelationSettings, error) {
 	s, found := r.units[name]
 	if !found {
 		return nil, fmt.Errorf("unknown unit %s", name)
@@ -174,7 +174,7 @@
 	return s.Map(), nil
 }
 
-type Settings params.Settings
+type Settings params.RelationSettings
 
 func (s Settings) Get(k string) (interface{}, bool) {
 	v, f := s[k]
@@ -189,8 +189,8 @@
 	delete(s, k)
 }
 
-func (s Settings) Map() params.Settings {
-	r := params.Settings{}
+func (s Settings) Map() params.RelationSettings {
+	r := params.RelationSettings{}
 	for k, v := range s {
 		r[k] = v
 	}