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

Unmarshall numbers in options to number not float64 #308

Merged
merged 3 commits into from
Mar 13, 2019
Merged

Unmarshall numbers in options to number not float64 #308

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2186b1e
Unmarshall numbers in options to number not float64
pavolloffay Mar 13, 2019
eebadde
Add mising tests
pavolloffay Mar 13, 2019
475c34d
call unmarshall directly
pavolloffay Mar 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion pkg/apis/jaegertracing/v1/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package v1

import (
"bytes"
"encoding/json"
"fmt"
"strings"
Expand Down Expand Up @@ -38,7 +39,11 @@ func (o *Options) Filter(prefix string) Options {
// UnmarshalJSON implements an alternative parser for this field
func (o *Options) UnmarshalJSON(b []byte) error {
var entries map[string]interface{}
json.Unmarshal(b, &entries)
d := json.NewDecoder(bytes.NewReader(b))
d.UseNumber()
Copy link
Member Author

@pavolloffay pavolloffay Mar 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

A bit of context: JSON spec defines numbers as float64 so all number are by default unmarshalled to float64. Our Options.Args() method uses fmt.Sprintf(%v, val) to convert the value to string - the %v formats the float into exponent notation e.g. 5e+06

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the details - would it be possible to provide a test?

Copy link
Contributor

Choose a reason for hiding this comment

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

Including one that fails the decode, just to improve the code coverage.

Copy link
Member Author

Choose a reason for hiding this comment

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

Apart from it is it ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes

if err := d.Decode(&entries); err != nil {
return err
}

return o.parse(entries)
}
Expand Down
28 changes: 28 additions & 0 deletions pkg/apis/jaegertracing/v1/options_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,34 @@ func TestMultipleSubValues(t *testing.T) {
assert.Len(t, args, 3)
}

func TestUnmarshalToArgs(t *testing.T) {
tests := []struct {
in string
args []string
err string
}{
{in: `^`, err: "invalid character '^' looking for beginning of value"},
{
in: `{"a": 5000000000, "b": 15.222, "c":true, "d": "foo"}`,
args: []string{"--a=5000000000", "--b=15.222", "--c=true", "--d=foo"},
},
}
for _, test := range tests {
opts := Options{}
err := opts.UnmarshalJSON([]byte(test.in))
if test.err != "" {
assert.EqualError(t, err, test.err)
} else {
assert.NoError(t, err)
args := opts.ToArgs()
sort.SliceStable(args, func(i, j int) bool {
return args[i] < args[j]
})
assert.Equal(t, test.args, args)
}
}
}

func TestMultipleSubValuesWithFilter(t *testing.T) {
o := NewOptions(nil)
o.UnmarshalJSON([]byte(`{"memory": {"max-traces": "50000"}, "es": {"server-urls": "http://elasticsearch:9200", "username": "elastic", "password": "changeme"}}`))
Expand Down