Skip to content

Commit 53b147b

Browse files
EmyrkEmyrk
committed
Fix unmarshal json for optionsets
1 parent dc67bb7 commit 53b147b

File tree

3 files changed

+137
-0
lines changed

3 files changed

+137
-0
lines changed

‎cli/clibase/option.go

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
package clibase
22

33
import (
4+
"bytes"
5+
"encoding/json"
46
"os"
57
"strings"
68

@@ -65,6 +67,20 @@ type Option struct {
6567
ValueSource ValueSource `json:"value_source,omitempty"`
6668
}
6769

70+
// optionNoMethods is just a wrapper around Option so we can defer to the
71+
// default json.Unmarshaler behavior.
72+
type optionNoMethods Option
73+
74+
func (o *Option) UnmarshalJSON(data []byte) error {
75+
// If an option has no values, we have no idea how to unmarshal it.
76+
// So just discard the json data.
77+
if o.Value == nil {
78+
o.Value = &DiscardValue
79+
}
80+
81+
return json.Unmarshal(data, (*optionNoMethods)(o))
82+
}
83+
6884
func (o Option) YAMLPath() string {
6985
if o.YAML == "" {
7086
return ""
@@ -79,6 +95,85 @@ func (o Option) YAMLPath() string {
7995
// OptionSet is a group of options that can be applied to a command.
8096
type OptionSet []Option
8197

98+
// UnmarshalJSON implements json.Unmarshaler for OptionSets. Options have an
99+
// interface Value type that cannot handle unmarshalling because the types cannot
100+
// be inferred. Since it is a slice, instantiating the Options first does not
101+
// help.
102+
//
103+
// However, we typically do instantiate the slice to have the correct types.
104+
// So this unmarshaller will attempt to find the named option in the existing
105+
// set, if it cannot, the value is discarded. If the option exists, the value
106+
// is unmarshalled into the existing option, and replaces the existing option.
107+
//
108+
// The value is discarded if it's type cannot be inferred. This behavior just
109+
// feels "safer", although it should never happen if the correct option set
110+
// is passed in.
111+
func (os *OptionSet) UnmarshalJSON(data []byte) error {
112+
dec := json.NewDecoder(bytes.NewBuffer(data))
113+
// Should be a json array, so consume the starting open bracket.
114+
t, err := dec.Token()
115+
if err != nil {
116+
return xerrors.Errorf("read array open bracket: %w", err)
117+
}
118+
if t != json.Delim('[') {
119+
return xerrors.Errorf("expected array open bracket, got %q", t)
120+
}
121+
122+
// As long as json elements exist, consume them. The counter is used for
123+
// better errors.
124+
var i int
125+
OptionSetDecodeLoop:
126+
for dec.More() {
127+
var opt Option
128+
// jValue is a placeholder value that allows us to capture the
129+
// raw json for the value to attempt to unmarshal later.
130+
var jValue jsonValue
131+
opt.Value = &jValue
132+
err := dec.Decode(&opt)
133+
if err != nil {
134+
return xerrors.Errorf("decode %d option: %w", i, err)
135+
}
136+
137+
// Try to see if the option already exists in the option set.
138+
// If it does, just update the existing option.
139+
for i, have := range *os {
140+
if have.Name == opt.Name {
141+
if jValue != nil {
142+
err := json.Unmarshal(jValue, &(*os)[i].Value)
143+
if err != nil {
144+
return xerrors.Errorf("decode option %q value: %w", have.Name, err)
145+
}
146+
// Set the opt's value
147+
opt.Value = (*os)[i].Value
148+
} else {
149+
// Discard the value if it's nil.
150+
opt.Value = DiscardValue
151+
}
152+
// Override the existing.
153+
(*os)[i] = opt
154+
// Go to the next option to decode.
155+
continue OptionSetDecodeLoop
156+
}
157+
}
158+
159+
// If the option doesn't exist, the value will be discarded.
160+
// We do this because we cannot infer the type of the value.
161+
opt.Value = DiscardValue
162+
*os = append(*os, opt)
163+
i++
164+
}
165+
166+
t, err = dec.Token()
167+
if err != nil {
168+
return xerrors.Errorf("read array close bracket: %w", err)
169+
}
170+
if t != json.Delim(']') {
171+
return xerrors.Errorf("expected array close bracket, got %q", t)
172+
}
173+
174+
return nil
175+
}
176+
82177
// Add adds the given Options to the OptionSet.
83178
func (s *OptionSet) Add(opts ...Option) {
84179
*s = append(*s, opts...)

‎cli/clibase/values.go

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -430,6 +430,32 @@ func (discardValue) Type() string {
430430
return "discard"
431431
}
432432

433+
func (discardValue) UnmarshalJSON(data []byte) error {
434+
return nil
435+
}
436+
437+
type jsonValue json.RawMessage
438+
439+
func (jsonValue) Set(string) error {
440+
return xerrors.Errorf("json value is read-only")
441+
}
442+
443+
func (jsonValue) String() string {
444+
return ""
445+
}
446+
447+
func (jsonValue) Type() string {
448+
return "json"
449+
}
450+
451+
func (j *jsonValue) UnmarshalJSON(data []byte) error {
452+
if j == nil {
453+
return xerrors.New("json.RawMessage: UnmarshalJSON on nil pointer")
454+
}
455+
*j = append((*j)[0:0], data...)
456+
return nil
457+
}
458+
433459
var _ pflag.Value = (*Enum)(nil)
434460

435461
type Enum struct {

‎cli/server_test.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1188,6 +1188,22 @@ func TestServer(t *testing.T) {
11881188
require.Equal(t, map[string]string{"serious_business_unit": "serious_business_unit"}, deploymentConfig.Values.OIDC.GroupMapping.Value)
11891189
require.Equal(t, "Sign In With Coder", deploymentConfig.Values.OIDC.SignInText.Value())
11901190
require.Equal(t, "https://example.com/icon.png", deploymentConfig.Values.OIDC.IconURL.Value().String())
1191+
1192+
// Verify the option values
1193+
for _, opt := range deploymentConfig.Options {
1194+
switch opt.Flag {
1195+
case "access-url":
1196+
require.Equal(t, "http://example.com", opt.Value.String())
1197+
case "oidc-icon-url":
1198+
require.Equal(t, "https://example.com/icon.png", opt.Value.String())
1199+
case "oidc-sign-in-text":
1200+
require.Equal(t, "Sign In With Coder", opt.Value.String())
1201+
case "redirect-to-access-url":
1202+
require.Equal(t, "false", opt.Value.String())
1203+
case "derp-server-region-id":
1204+
require.Equal(t, "999", opt.Value.String())
1205+
}
1206+
}
11911207
})
11921208
})
11931209

0 commit comments

Comments
 (0)