Merge lp:~niemeyer/juju-core/schema-field-map-defaults into lp:~juju/juju-core/trunk
- schema-field-map-defaults
- Merge into trunk
Proposed by
Gustavo Niemeyer
Status: | Merged |
---|---|
Merged at revision: | 318 |
Proposed branch: | lp:~niemeyer/juju-core/schema-field-map-defaults |
Merge into: | lp:~juju/juju-core/trunk |
Prerequisite: | lp:~niemeyer/juju-core/schema-string-map |
Diff against target: |
382 lines (+113/-75) 7 files modified
charm/config.go (+4/-1) charm/meta.go (+11/-12) environs/config/config.go (+12/-12) environs/dummy/environs.go (+3/-3) environs/ec2/config.go (+12/-19) schema/schema.go (+39/-18) schema/schema_test.go (+32/-10) |
To merge this branch: | bzr merge lp:~niemeyer/juju-core/schema-field-map-defaults |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
The Go Language Gophers | Pending | ||
Review via email: mp+115863@code.launchpad.net |
Commit message
Description of the change
schema: support defaults in FieldMap
To post a comment you must log in.
Revision history for this message
Gustavo Niemeyer (niemeyer) wrote : | # |
Revision history for this message
Gustavo Niemeyer (niemeyer) wrote : | # |
Please take a look.
Revision history for this message
Roger Peppe (rogpeppe) wrote : | # |
On 2012/07/20 00:32:02, niemeyer wrote:
> Please take a look.
excellent stuff; make schema work for its living!
LGTM
Revision history for this message
Roger Peppe (rogpeppe) wrote : | # |
https:/
File schema/schema.go (right):
https:/
schema/
nice.
Revision history for this message
William Reade (fwereade) wrote : | # |
LGTM, very nice.
Revision history for this message
Gustavo Niemeyer (niemeyer) wrote : | # |
*** Submitted:
schema: support defaults in FieldMap
R=rog, fwereade
CC=
https:/
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'charm/config.go' |
2 | --- charm/config.go 2012-07-19 22:31:15 +0000 |
3 | +++ charm/config.go 2012-07-20 00:33:23 +0000 |
4 | @@ -130,7 +130,10 @@ |
5 | "default": schema.OneOf(schema.String(), schema.Int(), schema.Float(), schema.Bool()), |
6 | "description": schema.String(), |
7 | }, |
8 | - schema.Optional{"default", "description"}, |
9 | + schema.Defaults{ |
10 | + "default": schema.Omit, |
11 | + "description": schema.Omit, |
12 | + }, |
13 | ) |
14 | |
15 | var configSchema = schema.FieldMap( |
16 | |
17 | === modified file 'charm/meta.go' |
18 | --- charm/meta.go 2012-07-19 22:31:15 +0000 |
19 | +++ charm/meta.go 2012-07-20 00:33:23 +0000 |
20 | @@ -155,10 +155,6 @@ |
21 | return |
22 | } |
23 | |
24 | - // Optional values are context-sensitive and/or have |
25 | - // defaults, which is different than what FieldMap can |
26 | - // readily support. So just do it here first, then |
27 | - // coerce to the real schema. |
28 | v, err = mapC.Coerce(v, path) |
29 | if err != nil { |
30 | return |
31 | @@ -167,12 +163,6 @@ |
32 | if _, ok := m["limit"]; !ok { |
33 | m["limit"] = c.limit |
34 | } |
35 | - if _, ok := m["optional"]; !ok { |
36 | - m["optional"] = false |
37 | - } |
38 | - if _, ok := m["scope"]; !ok { |
39 | - m["scope"] = ScopeGlobal |
40 | - } |
41 | return ifaceSchema.Coerce(m, path) |
42 | } |
43 | |
44 | @@ -183,7 +173,10 @@ |
45 | "scope": schema.OneOf(schema.Const(ScopeGlobal), schema.Const(ScopeContainer)), |
46 | "optional": schema.Bool(), |
47 | }, |
48 | - schema.Optional{"scope"}, |
49 | + schema.Defaults{ |
50 | + "scope": ScopeGlobal, |
51 | + "optional": false, |
52 | + }, |
53 | ) |
54 | |
55 | var charmSchema = schema.FieldMap( |
56 | @@ -197,5 +190,11 @@ |
57 | "revision": schema.Int(), // Obsolete |
58 | "subordinate": schema.Bool(), |
59 | }, |
60 | - schema.Optional{"provides", "requires", "peers", "revision", "subordinate"}, |
61 | + schema.Defaults{ |
62 | + "provides": schema.Omit, |
63 | + "requires": schema.Omit, |
64 | + "peers": schema.Omit, |
65 | + "revision": schema.Omit, |
66 | + "subordinate": schema.Omit, |
67 | + }, |
68 | ) |
69 | |
70 | === modified file 'environs/config/config.go' |
71 | --- environs/config/config.go 2012-07-19 22:31:15 +0000 |
72 | +++ environs/config/config.go 2012-07-20 00:33:23 +0000 |
73 | @@ -23,20 +23,21 @@ |
74 | m: m.(map[string]interface{}), |
75 | t: make(map[string]interface{}), |
76 | } |
77 | - if s, _ := c.m["default-series"].(string); s == "" { |
78 | + |
79 | + if c.m["default-series"].(string) == "" { |
80 | c.m["default-series"] = CurrentSeries |
81 | } |
82 | |
83 | // Load authorized-keys-path onto authorized-keys, if necessary. |
84 | - path, _ := c.m["authorized-keys-path"].(string) |
85 | - keys, _ := c.m["authorized-keys"].(string) |
86 | + path := c.m["authorized-keys-path"].(string) |
87 | + keys := c.m["authorized-keys"].(string) |
88 | if path != "" || keys == "" { |
89 | c.m["authorized-keys"], err = authorizedKeys(path) |
90 | if err != nil { |
91 | return nil, err |
92 | } |
93 | - delete(c.m, "authorized-keys-path") |
94 | } |
95 | + delete(c.m, "authorized-keys-path") |
96 | |
97 | // Check if there are any required fields that are empty. |
98 | for _, attr := range []string{"name", "type", "default-series", "authorized-keys"} { |
99 | @@ -103,11 +104,10 @@ |
100 | "authorized-keys-path": schema.String(), |
101 | } |
102 | |
103 | -var checker = schema.FieldMap( |
104 | - fields, |
105 | - []string{ |
106 | - "default-series", |
107 | - "authorized-keys", |
108 | - "authorized-keys-path", |
109 | - }, |
110 | -) |
111 | +var defaults = schema.Defaults{ |
112 | + "default-series": CurrentSeries, |
113 | + "authorized-keys": "", |
114 | + "authorized-keys-path": "", |
115 | +} |
116 | + |
117 | +var checker = schema.FieldMap(fields, defaults) |
118 | |
119 | === modified file 'environs/dummy/environs.go' |
120 | --- environs/dummy/environs.go 2012-07-19 22:31:15 +0000 |
121 | +++ environs/dummy/environs.go 2012-07-20 00:33:23 +0000 |
122 | @@ -218,8 +218,8 @@ |
123 | "zookeeper": schema.Bool(), |
124 | "broken": schema.Bool(), |
125 | }, |
126 | - []string{ |
127 | - "broken", |
128 | + schema.Defaults{ |
129 | + "broken": false, |
130 | }, |
131 | ) |
132 | |
133 | @@ -233,7 +233,7 @@ |
134 | Config: cfg, |
135 | zookeeper: m1["zookeeper"].(bool), |
136 | } |
137 | - ecfg.broken, _ = m1["broken"].(bool) |
138 | + ecfg.broken = m1["broken"].(bool) |
139 | return ecfg, nil |
140 | } |
141 | |
142 | |
143 | === modified file 'environs/ec2/config.go' |
144 | --- environs/ec2/config.go 2012-07-19 22:31:15 +0000 |
145 | +++ environs/ec2/config.go 2012-07-20 00:33:23 +0000 |
146 | @@ -25,11 +25,12 @@ |
147 | "region": schema.String(), |
148 | "control-bucket": schema.String(), |
149 | "public-bucket": schema.String(), |
150 | - }, []string{ |
151 | - "access-key", |
152 | - "secret-key", |
153 | - "region", |
154 | - "public-bucket", |
155 | + }, |
156 | + schema.Defaults{ |
157 | + "access-key": "", |
158 | + "secret-key": "", |
159 | + "region": "us-east-1", |
160 | + "public-bucket": "", |
161 | }, |
162 | ) |
163 | |
164 | @@ -41,9 +42,9 @@ |
165 | m := v.(map[string]interface{}) |
166 | c := &providerConfig{Config: config} |
167 | c.bucket = m["control-bucket"].(string) |
168 | - c.publicBucket = maybeString(m["public-bucket"], "") |
169 | - c.auth.AccessKey = maybeString(m["access-key"], "") |
170 | - c.auth.SecretKey = maybeString(m["secret-key"], "") |
171 | + c.publicBucket = m["public-bucket"].(string) |
172 | + c.auth.AccessKey = m["access-key"].(string) |
173 | + c.auth.SecretKey = m["secret-key"].(string) |
174 | if c.auth.AccessKey == "" || c.auth.SecretKey == "" { |
175 | if c.auth.AccessKey != "" { |
176 | return nil, fmt.Errorf("environment has access-key but no secret-key") |
177 | @@ -57,17 +58,9 @@ |
178 | } |
179 | } |
180 | |
181 | - regionName := maybeString(m["region"], "us-east-1") |
182 | - if _, ok := aws.Regions[regionName]; !ok { |
183 | - return nil, fmt.Errorf("invalid region name %q", regionName) |
184 | + c.region = m["region"].(string) |
185 | + if _, ok := aws.Regions[c.region]; !ok { |
186 | + return nil, fmt.Errorf("invalid region name %q", c.region) |
187 | } |
188 | - c.region = regionName |
189 | return c, nil |
190 | } |
191 | - |
192 | -func maybeString(x interface{}, dflt string) string { |
193 | - if x == nil { |
194 | - return dflt |
195 | - } |
196 | - return x.(string) |
197 | -} |
198 | |
199 | === modified file 'schema/schema.go' |
200 | --- schema/schema.go 2012-07-19 22:31:15 +0000 |
201 | +++ schema/schema.go 2012-07-20 00:33:23 +0000 |
202 | @@ -305,8 +305,14 @@ |
203 | return out, nil |
204 | } |
205 | |
206 | +// Omit is a marker for FieldMap and StructFieldMap defaults parameter. |
207 | +// If a field is not present in the map and defaults to Omit, the missing |
208 | +// field will be ommitted from the coerced map as well. |
209 | +var Omit omit |
210 | +type omit struct{} |
211 | + |
212 | type Fields map[string]Checker |
213 | -type Optional []string |
214 | +type Defaults map[string]interface{} |
215 | |
216 | // FieldMap returns a Checker that accepts a map value with defined |
217 | // string keys. Every key has an independent checker associated, |
218 | @@ -314,32 +320,27 @@ |
219 | // individually. If a field fails to be processed, processing stops |
220 | // and returns with the underlying error. |
221 | // |
222 | +// Fields in defaults will be set to the provided value if not present |
223 | +// in the coerced map. If the default value is schema.Omit, the field |
224 | +// missing field will be ommitted from the coerced map as well. |
225 | +// |
226 | // The coerced output value has type map[string]interface{}. |
227 | -func FieldMap(fields Fields, optional Optional) Checker { |
228 | - return fieldMapC{fields, optional, false} |
229 | +func FieldMap(fields Fields, defaults Defaults) Checker { |
230 | + return fieldMapC{fields, defaults, false} |
231 | } |
232 | |
233 | // StrictFieldMap returns a Checker that acts as the one returned by FieldMap, |
234 | // but the Checker returns an error if it encounters an unknown key. |
235 | -func StrictFieldMap(fields Fields, optional Optional) Checker { |
236 | - return fieldMapC{fields, optional, true} |
237 | +func StrictFieldMap(fields Fields, defaults Defaults) Checker { |
238 | + return fieldMapC{fields, defaults, true} |
239 | } |
240 | |
241 | type fieldMapC struct { |
242 | fields Fields |
243 | - optional []string |
244 | + defaults Defaults |
245 | strict bool |
246 | } |
247 | |
248 | -func (c fieldMapC) isOptional(key string) bool { |
249 | - for _, k := range c.optional { |
250 | - if k == key { |
251 | - return true |
252 | - } |
253 | - } |
254 | - return false |
255 | -} |
256 | - |
257 | func (c fieldMapC) Coerce(v interface{}, path []string) (interface{}, error) { |
258 | rv := reflect.ValueOf(v) |
259 | if rv.Kind() != reflect.Map { |
260 | @@ -362,20 +363,40 @@ |
261 | l := rv.Len() |
262 | out := make(map[string]interface{}, l) |
263 | for k, checker := range c.fields { |
264 | - vpath[len(vpath)-1] = k |
265 | var value interface{} |
266 | valuev := rv.MapIndex(reflect.ValueOf(k)) |
267 | if valuev.IsValid() { |
268 | value = valuev.Interface() |
269 | - } else if c.isOptional(k) { |
270 | - continue |
271 | + } else if dflt, ok := c.defaults[k]; ok { |
272 | + if dflt == Omit { |
273 | + continue |
274 | + } |
275 | + value = dflt |
276 | } |
277 | + vpath[len(vpath)-1] = k |
278 | newv, err := checker.Coerce(value, vpath) |
279 | if err != nil { |
280 | return nil, err |
281 | } |
282 | out[k] = newv |
283 | } |
284 | + for k, v := range c.defaults { |
285 | + if v == Omit { |
286 | + continue |
287 | + } |
288 | + if _, ok := out[k]; !ok { |
289 | + checker, ok := c.fields[k] |
290 | + if !ok { |
291 | + return nil, fmt.Errorf("got default value for unknown field %q", k) |
292 | + } |
293 | + vpath[len(vpath)-1] = k |
294 | + newv, err := checker.Coerce(v, vpath) |
295 | + if err != nil { |
296 | + return nil, err |
297 | + } |
298 | + out[k] = newv |
299 | + } |
300 | + } |
301 | return out, nil |
302 | } |
303 | |
304 | |
305 | === modified file 'schema/schema_test.go' |
306 | --- schema/schema_test.go 2012-07-19 22:31:15 +0000 |
307 | +++ schema/schema_test.go 2012-07-20 00:33:23 +0000 |
308 | @@ -240,7 +240,7 @@ |
309 | out, err := sch.Coerce(map[string]interface{}{"a": "A", "b": "B"}, aPath) |
310 | |
311 | c.Assert(err, IsNil) |
312 | - c.Assert(out, DeepEquals, map[string]interface{}{"a": "A", "b": "B"}) |
313 | + c.Assert(out, DeepEquals, map[string]interface{}{"a": "A", "b": "B", "c": "C"}) |
314 | |
315 | out, err = sch.Coerce(42, aPath) |
316 | c.Assert(out, IsNil) |
317 | @@ -261,7 +261,7 @@ |
318 | // b is optional |
319 | out, err = sch.Coerce(map[string]interface{}{"a": "A"}, aPath) |
320 | c.Assert(err, IsNil) |
321 | - c.Assert(out, DeepEquals, map[string]interface{}{"a": "A"}) |
322 | + c.Assert(out, DeepEquals, map[string]interface{}{"a": "A", "c": "C"}) |
323 | |
324 | // First path entry shouldn't have dots in an error message. |
325 | out, err = sch.Coerce(map[string]bool{"a": true}, nil) |
326 | @@ -273,26 +273,48 @@ |
327 | fields := schema.Fields{ |
328 | "a": schema.Const("A"), |
329 | "b": schema.Const("B"), |
330 | - } |
331 | - sch := schema.FieldMap(fields, schema.Optional{"b"}) |
332 | + "c": schema.Const("C"), |
333 | + } |
334 | + defaults := schema.Defaults{ |
335 | + "b": schema.Omit, |
336 | + "c": "C", |
337 | + } |
338 | + sch := schema.FieldMap(fields, defaults) |
339 | assertFieldMap(c, sch) |
340 | |
341 | - out, err := sch.Coerce(map[string]interface{}{"a": "A", "b": "B", "c": "C"}, aPath) |
342 | + out, err := sch.Coerce(map[string]interface{}{"a": "A", "b": "B", "d": "D"}, aPath) |
343 | c.Assert(err, IsNil) |
344 | - c.Assert(out, DeepEquals, map[string]interface{}{"a": "A", "b": "B"}) |
345 | + c.Assert(out, DeepEquals, map[string]interface{}{"a": "A", "b": "B", "c": "C"}) |
346 | +} |
347 | + |
348 | +func (s *S) TestFieldMapDefaultInvalid(c *C) { |
349 | + fields := schema.Fields{ |
350 | + "a": schema.Const("A"), |
351 | + } |
352 | + defaults := schema.Defaults{ |
353 | + "a": "B", |
354 | + } |
355 | + sch := schema.FieldMap(fields, defaults) |
356 | + _, err := sch.Coerce(map[string]interface{}{}, aPath) |
357 | + c.Assert(err, ErrorMatches, `<path>.a: expected "A", got "B"`) |
358 | } |
359 | |
360 | func (s *S) TestStrictFieldMap(c *C) { |
361 | fields := schema.Fields{ |
362 | "a": schema.Const("A"), |
363 | "b": schema.Const("B"), |
364 | - } |
365 | - sch := schema.StrictFieldMap(fields, schema.Optional{"b"}) |
366 | + "c": schema.Const("C"), |
367 | + } |
368 | + defaults := schema.Defaults{ |
369 | + "b": schema.Omit, |
370 | + "c": "C", |
371 | + } |
372 | + sch := schema.StrictFieldMap(fields, defaults) |
373 | assertFieldMap(c, sch) |
374 | |
375 | - out, err := sch.Coerce(map[string]interface{}{"a": "A", "b": "B", "c": "C"}, aPath) |
376 | + out, err := sch.Coerce(map[string]interface{}{"a": "A", "b": "B", "d": "D"}, aPath) |
377 | c.Assert(out, IsNil) |
378 | - c.Assert(err, ErrorMatches, `<path>.c: expected nothing, got "C"`) |
379 | + c.Assert(err, ErrorMatches, `<path>.d: expected nothing, got "D"`) |
380 | } |
381 | |
382 | func (s *S) TestSchemaMap(c *C) { |
Reviewers: mp+115863_ code.launchpad. net,
Message:
Please take a look.
Description:
schema: support defaults in FieldMap
https:/ /code.launchpad .net/~niemeyer/ juju-core/ schema- field-map- defaults/ +merge/ 115863
Requires: /code.launchpad .net/~niemeyer/ juju-core/ schema- string- map/+merge/ 115849
https:/
(do not edit description out of merge proposal)
Please review this at https:/ /codereview. appspot. com/6419056/
Affected files: config/ config. go dummy/environs. go ec2/config. go schema_ test.go
A [revision details]
M charm/config.go
M charm/meta.go
M environs/
M environs/
M environs/
M schema/schema.go
M schema/