encoding/jsonpb: improve and fix unmarshaling of Duration

Change use of regular expression to manually parsing the value.

Allow value with + symbol in front, e.g. "+3s". Previous regex missed
this.

Do not allow values without numbers, e.g. "-s". Previous regex missed
this as well.

name                  old time/op    new time/op    delta
Unmarshal_Duration-4    1.96µs ± 0%    1.24µs ± 0%   ~     (p=1.000 n=1+1)

name                  old alloc/op   new alloc/op   delta
Unmarshal_Duration-4      703B ± 0%      512B ± 0%   ~     (p=1.000 n=1+1)

name                  old allocs/op  new allocs/op  delta
Unmarshal_Duration-4      20.0 ± 0%      17.0 ± 0%   ~     (p=1.000 n=1+1)

Change-Id: I4db58d70f55607213631c49d698ee6a048b5e094
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/170012
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>

encoding/jsonpb/bench_test.go

@@ -0,0 +1,23 @@
+// Copyright 2019 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package jsonpb_test
+
+import (
+	"testing"
+
+	"github.com/golang/protobuf/v2/encoding/jsonpb"
+	knownpb "github.com/golang/protobuf/v2/types/known"
+)
+
+func BenchmarkUnmarshal_Duration(b *testing.B) {
+	input := []byte(`"-123456789.123456789s"`)
+
+	for i := 0; i < b.N; i++ {
+		err := jsonpb.Unmarshal(&knownpb.Duration{}, input)
+		if err != nil {
+			b.Fatal(err)
+		}
+	}
+}

encoding/jsonpb/decode_test.go

@@ -1756,6 +1756,11 @@ func TestUnmarshal(t *testing.T) {
 		inputMessage: &knownpb.Duration{},
 		inputText:    `"-3s"`,
 		wantMessage:  &knownpb.Duration{Seconds: -3},
+	}, {
+		desc:         "Duration with plus sign",
+		inputMessage: &knownpb.Duration{},
+		inputText:    `"+3s"`,
+		wantMessage:  &knownpb.Duration{Seconds: 3},
 	}, {
 		desc:         "Duration with nanos",
 		inputMessage: &knownpb.Duration{},
@@ -1766,6 +1771,16 @@ func TestUnmarshal(t *testing.T) {
 		inputMessage: &knownpb.Duration{},
 		inputText:    `"-0.001s"`,
 		wantMessage:  &knownpb.Duration{Nanos: -1e6},
+	}, {
+		desc:         "Duration with -nanos",
+		inputMessage: &knownpb.Duration{},
+		inputText:    `"-.001s"`,
+		wantMessage:  &knownpb.Duration{Nanos: -1e6},
+	}, {
+		desc:         "Duration with +nanos",
+		inputMessage: &knownpb.Duration{},
+		inputText:    `"+.001s"`,
+		wantMessage:  &knownpb.Duration{Nanos: 1e6},
 	}, {
 		desc:         "Duration with -secs -nanos",
 		inputMessage: &knownpb.Duration{},
@@ -1799,13 +1814,28 @@ func TestUnmarshal(t *testing.T) {
 	}, {
 		desc:         "Duration with nanos beyond 9 digits",
 		inputMessage: &knownpb.Duration{},
-		inputText:    `"0.9999999990s"`,
+		inputText:    `"0.1000000000s"`,
 		wantErr:      true,
 	}, {
 		desc:         "Duration without suffix s",
 		inputMessage: &knownpb.Duration{},
 		inputText:    `"123"`,
 		wantErr:      true,
+	}, {
+		desc:         "Duration invalid signed fraction",
+		inputMessage: &knownpb.Duration{},
+		inputText:    `"123.+123s"`,
+		wantErr:      true,
+	}, {
+		desc:         "Duration invalid multiple .",
+		inputMessage: &knownpb.Duration{},
+		inputText:    `"123.123.s"`,
+		wantErr:      true,
+	}, {
+		desc:         "Duration invalid integer",
+		inputMessage: &knownpb.Duration{},
+		inputText:    `"01s"`,
+		wantErr:      true,
 	}, {
 		desc:         "Timestamp zero",
 		inputMessage: &knownpb.Timestamp{},

encoding/jsonpb/well_known_types.go

@@ -7,7 +7,6 @@ package jsonpb
 import (
 	"bytes"
 	"fmt"
-	"regexp"
 	"strconv"
 	"strings"
 	"time"
@@ -699,50 +698,103 @@ func (o UnmarshalOptions) unmarshalDuration(m pref.Message) error {
 	return nerr.E
 }
 
-// Regular expression for Duration type in JSON format. This allows for values
-// like 1s, 0.1s, 1.s, .1s. It limits fractional part to 9 digits only for
+// parseDuration parses the given input string for seconds and nanoseconds value
+// for the Duration JSON format. The format is a decimal number with a suffix
+// 's'. It can have optional plus/minus sign. There needs to be at least an
+// integer or fractional part. Fractional part is limited to 9 digits only for
 // nanoseconds precision, regardless of whether there are trailing zero digits.
-var durationRE = regexp.MustCompile(`^-?([0-9]|[1-9][0-9]+)?(\.[0-9]{0,9})?s$`)
-
+// Example values are 1s, 0.1s, 1.s, .1s, +1s, -1s, -.1s.
 func parseDuration(input string) (int64, int32, bool) {
 	b := []byte(input)
-	// TODO: Parse input directly instead of using a regular expression.
-	matched := durationRE.FindSubmatch(b)
-	if len(matched) != 3 {
+	size := len(b)
+	if size < 2 {
+		return 0, 0, false
+	}
+	if b[size-1] != 's' {
 		return 0, 0, false
 	}
+	b = b[:size-1]
 
+	// Read optional plus/minus symbol.
 	var neg bool
-	if b[0] == '-' {
+	switch b[0] {
+	case '-':
 		neg = true
+		b = b[1:]
+	case '+':
+		b = b[1:]
 	}
-	var secb []byte
-	if len(matched[1]) == 0 {
-		secb = []byte{'0'}
-	} else {
-		secb = matched[1]
+	if len(b) == 0 {
+		return 0, 0, false
 	}
-	var nanob []byte
-	if len(matched[2]) <= 1 {
-		nanob = []byte{'0'}
-	} else {
-		nanob = matched[2][1:]
-		// Right-pad with 0s for nanosecond-precision.
-		for i := len(nanob); i < 9; i++ {
-			nanob = append(nanob, '0')
+
+	// Read the integer part.
+	var intp []byte
+	switch {
+	case b[0] == '0':
+		b = b[1:]
+
+	case '1' <= b[0] && b[0] <= '9':
+		intp = b[0:]
+		b = b[1:]
+		n := 1
+		for len(b) > 0 && '0' <= b[0] && b[0] <= '9' {
+			n++
+			b = b[1:]
 		}
-		// Remove unnecessary 0s in the left.
-		nanob = bytes.TrimLeft(nanob, "0")
-	}
+		intp = intp[:n]
 
-	secs, err := strconv.ParseInt(string(secb), 10, 64)
-	if err != nil {
+	case b[0] == '.':
+		// Continue below.
+
+	default:
 		return 0, 0, false
 	}
 
-	nanos, err := strconv.ParseInt(string(nanob), 10, 32)
-	if err != nil {
-		return 0, 0, false
+	hasFrac := false
+	var frac [9]byte
+	if len(b) > 0 {
+		if b[0] != '.' {
+			return 0, 0, false
+		}
+		// Read the fractional part.
+		b = b[1:]
+		n := 0
+		for len(b) > 0 && n < 9 && '0' <= b[0] && b[0] <= '9' {
+			frac[n] = b[0]
+			n++
+			b = b[1:]
+		}
+		// It is not valid if there are more bytes left.
+		if len(b) > 0 {
+			return 0, 0, false
+		}
+		// Pad fractional part with 0s.
+		for i := n; i < 9; i++ {
+			frac[i] = '0'
+		}
+		hasFrac = true
+	}
+
+	var secs int64
+	if len(intp) > 0 {
+		var err error
+		secs, err = strconv.ParseInt(string(intp), 10, 64)
+		if err != nil {
+			return 0, 0, false
+		}
+	}
+
+	var nanos int64
+	if hasFrac {
+		nanob := bytes.TrimLeft(frac[:], "0")
+		if len(nanob) > 0 {
+			var err error
+			nanos, err = strconv.ParseInt(string(nanob), 10, 32)
+			if err != nil {
+				return 0, 0, false
+			}
+		}
 	}
 
 	if neg {