From 9491846047859774273faae66f60640dd531fe30 Mon Sep 17 00:00:00 2001 From: Jon Seymour Date: Sat, 20 Feb 2016 22:22:26 +1100 Subject: [PATCH] models: improve handling of points with empty field names or with no fields Influx does not support fields with empty names or points with no fields. NewPoint is changed to validate that all field names are non-empty. AddField is removed because we now require that all fields are specified on construction. NewPointFromByte is changed to return an error if a unmarshaled binary point does not have any fields. newFieldsFromBinary is changed to prevent an infinite loop that can arise while attempting to parse corrupt binary point data. TestNewPointsWithBytesWithCorruptData is changed to reflect the change in the behaviour of NewPointFromByte. Signed-off-by: Jon Seymour --- models/points.go | 67 +++++++++++++++++++++---------------------- models/points_test.go | 18 +++--------- 2 files changed, 37 insertions(+), 48 deletions(-) diff --git a/models/points.go b/models/points.go index 6df1b22184c..336dc4f2836 100644 --- a/models/points.go +++ b/models/points.go @@ -3,6 +3,7 @@ package models import ( "bytes" "encoding/binary" + "errors" "fmt" "hash/fnv" "math" @@ -25,6 +26,8 @@ var ( ' ': []byte(`\ `), '=': []byte(`\=`), } + + ErrPointMustHaveAField = errors.New("point without fields is unsupported") ) // Point defines the values that will be written to the database @@ -37,7 +40,6 @@ type Point interface { SetTags(tags Tags) Fields() Fields - AddField(name string, value interface{}) Time() time.Time SetTime(t time.Time) @@ -1063,7 +1065,7 @@ func unescapeStringField(in string) string { // an unsupported field value (NaN) or out of range time is passed, this function returns an error. func NewPoint(name string, tags Tags, fields Fields, time time.Time) (Point, error) { if len(fields) == 0 { - return nil, fmt.Errorf("Point without fields is unsupported") + return nil, ErrPointMustHaveAField } if !time.IsZero() { if err := CheckTime(time); err != nil { @@ -1078,6 +1080,9 @@ func NewPoint(name string, tags Tags, fields Fields, time time.Time) (Point, err return nil, fmt.Errorf("NaN is an unsupported value for field %s", key) } } + if len(key) == 0 { + return nil, fmt.Errorf("all fields must have non-empty names") + } } return &point{ @@ -1092,6 +1097,9 @@ func NewPointFromBytes(b []byte) (Point, error) { if err := p.UnmarshalBinary(b); err != nil { return nil, err } + if len(p.Fields()) == 0 { + return nil, ErrPointMustHaveAField + } return p, nil } @@ -1212,14 +1220,6 @@ func (p *point) Fields() Fields { return p.cachedFields } -// AddField adds or replaces a field value for a point -func (p *point) AddField(name string, value interface{}) { - fields := p.Fields() - fields[name] = value - p.fields = fields.MarshalBinary() - p.cachedFields = nil -} - // SetPrecision will round a time to the specified precision func (p *point) SetPrecision(precision string) { switch precision { @@ -1394,38 +1394,37 @@ func newFieldsFromBinary(buf []byte) Fields { } i, name = scanTo(buf, i, '=') - if len(name) == 0 { - continue - } name = escape.Unescape(name) i, valueBuf = scanFieldValue(buf, i+1) - if len(valueBuf) == 0 { - fields[string(name)] = nil - continue - } + if len(name) > 0 { + if len(valueBuf) == 0 { + fields[string(name)] = nil + continue + } - // If the first char is a double-quote, then unmarshal as string - if valueBuf[0] == '"' { - value = unescapeStringField(string(valueBuf[1 : len(valueBuf)-1])) - // Check for numeric characters and special NaN or Inf - } else if (valueBuf[0] >= '0' && valueBuf[0] <= '9') || valueBuf[0] == '-' || valueBuf[0] == '+' || valueBuf[0] == '.' || - valueBuf[0] == 'N' || valueBuf[0] == 'n' || // NaN - valueBuf[0] == 'I' || valueBuf[0] == 'i' { // Inf + // If the first char is a double-quote, then unmarshal as string + if valueBuf[0] == '"' { + value = unescapeStringField(string(valueBuf[1 : len(valueBuf)-1])) + // Check for numeric characters and special NaN or Inf + } else if (valueBuf[0] >= '0' && valueBuf[0] <= '9') || valueBuf[0] == '-' || valueBuf[0] == '+' || valueBuf[0] == '.' || + valueBuf[0] == 'N' || valueBuf[0] == 'n' || // NaN + valueBuf[0] == 'I' || valueBuf[0] == 'i' { // Inf - value, err = parseNumber(valueBuf) - if err != nil { - panic(fmt.Sprintf("unable to parse number value '%v': %v", string(valueBuf), err)) - } + value, err = parseNumber(valueBuf) + if err != nil { + panic(fmt.Sprintf("unable to parse number value '%v': %v", string(valueBuf), err)) + } - // Otherwise parse it as bool - } else { - value, err = strconv.ParseBool(string(valueBuf)) - if err != nil { - panic(fmt.Sprintf("unable to parse bool value '%v': %v\n", string(valueBuf), err)) + // Otherwise parse it as bool + } else { + value, err = strconv.ParseBool(string(valueBuf)) + if err != nil { + panic(fmt.Sprintf("unable to parse bool value '%v': %v\n", string(valueBuf), err)) + } } + fields[string(name)] = value } - fields[string(name)] = value i++ } return fields diff --git a/models/points_test.go b/models/points_test.go index b8afae9ee5c..49f6079bfa5 100644 --- a/models/points_test.go +++ b/models/points_test.go @@ -1774,20 +1774,10 @@ t159,label=another a=2i,value=1i 1` } func TestNewPointsWithBytesWithCorruptData(t *testing.T) { - ch := make(chan error) - go func() { - corrupted := []byte{0, 0, 0, 3, 102, 111, 111, 0, 0, 0, 4, 61, 34, 65, 34, 1, 0, 0, 0, 14, 206, 86, 119, 24, 32, 72, 233, 168, 2, 148} - p, err := models.NewPointFromBytes(corrupted) - p.Fields() // this method should always return, even if the data is corrupt. - ch <- err - }() - select { - case err := <-ch: - if err != nil { - t.Fatalf("unexpected error: got: %v, expected: nil", err) - } - case _ = <-time.NewTimer(time.Second).C: - t.Fatalf("probable infite loop. got: timeout, expected: return") + corrupted := []byte{0, 0, 0, 3, 102, 111, 111, 0, 0, 0, 4, 61, 34, 65, 34, 1, 0, 0, 0, 14, 206, 86, 119, 24, 32, 72, 233, 168, 2, 148} + p, err := models.NewPointFromBytes(corrupted) + if p != nil || err == nil { + t.Fatalf("NewPointFromBytes: got: (%v, %v), expected: (nil, error)", p, err) } }