Add Error test cases and simplify duration parsing logic (#41)

* Add Error test cases and simplify duration parsing logic

* Moved regexp building outside of parsing function

* restructure regex declaration to conform with gofmt

Author: thomshutt

mpd/duration.go

@@ -5,13 +5,27 @@ package mpd
 import (
 	"encoding/xml"
 	"errors"
+	"fmt"
+	"regexp"
 	"strconv"
 	"strings"
 	"time"
 )
 
 type Duration time.Duration
 
+var (
+	rStart   = "^P"          // Must start with a 'P'
+	rDays    = "(\\d+D)?"    // We only allow Days for durations, not Months or Years
+	rTime    = "(?:T"        // If there's any 'time' units then they must be preceded by a 'T'
+	rHours   = "(\\d+H)?"    // Hours
+	rMinutes = "(\\d+M)?"    // Minutes
+	rSeconds = "([\\d.]+S)?" // Seconds (Potentially decimal)
+	rEnd     = ")?$"         // end of regex must close "T" capture group
+)
+
+var xmlDurationRegex = regexp.MustCompile(rStart + rDays + rTime + rHours + rMinutes + rSeconds + rEnd)
+
 func (d Duration) MarshalXMLAttr(name xml.Name) (xml.Attr, error) {
 	return xml.Attr{name, d.String()}, nil
 }
@@ -141,77 +155,52 @@ func fmtInt(buf []byte, v uint64) int {
 
 func parseDuration(str string) (time.Duration, error) {
 	if len(str) < 3 {
-		return 0, errors.New("input duration too short")
+		return 0, errors.New("At least one number and designator are required")
 	}
 
-	var minus bool
-	offset := 0
-	if str[offset] == '-' {
-		minus = true
-		offset++
+	if strings.Contains(str, "-") {
+		return 0, errors.New("Duration cannot be negative")
 	}
 
-	if str[offset] != 'P' {
-		return 0, errors.New("input duration does not have a valid prefix")
+	// Check that only the parts we expect exist and that everything's in the correct order
+	if !xmlDurationRegex.Match([]byte(str)) {
+		return 0, errors.New("Duration must be in the format: P[nD][T[nH][nM][nS]]")
 	}
-	offset++
 
-	var dateStr, timeStr string
-	if i := strings.IndexByte(str[offset:], 'T'); i != -1 {
-		dateStr = str[offset : offset+i]
-		timeStr = str[offset+i+1:]
-	} else {
-		dateStr = str[offset:]
-	}
+	var parts = xmlDurationRegex.FindStringSubmatch(str)
+	var total time.Duration
 
-	var sum float64
-	if len(dateStr) > 0 {
-		if i := strings.IndexByte(dateStr, 'Y'); i != -1 {
-			return 0, errors.New("input duration contains Years notation")
+	if parts[1] != "" {
+		days, err := strconv.Atoi(strings.TrimRight(parts[1], "D"))
+		if err != nil {
+			return 0, fmt.Errorf("Error parsing Days: %s", err)
 		}
+		total += time.Duration(days) * time.Hour * 24
+	}
 
-		if i := strings.IndexByte(dateStr, 'M'); i != -1 {
-			return 0, errors.New("input duration contains Months notation")
+	if parts[2] != "" {
+		hours, err := strconv.Atoi(strings.TrimRight(parts[2], "H"))
+		if err != nil {
+			return 0, fmt.Errorf("Error parsing Hours: %s", err)
 		}
+		total += time.Duration(hours) * time.Hour
+	}
 
-		if i := strings.IndexByte(dateStr, 'D'); i != -1 {
-			days, err := strconv.Atoi(dateStr[0:i])
-			if err != nil {
-				return 0, err
-			}
-			sum += float64(days) * 86400
+	if parts[3] != "" {
+		mins, err := strconv.Atoi(strings.TrimRight(parts[3], "M"))
+		if err != nil {
+			return 0, fmt.Errorf("Error parsing Minutes: %s", err)
 		}
+		total += time.Duration(mins) * time.Minute
 	}
 
-	if len(timeStr) > 0 {
-		var pos int
-		if i := strings.IndexByte(timeStr[pos:], 'H'); i != -1 {
-			hours, err := strconv.ParseInt(timeStr[pos:pos+i], 10, 64)
-			if err != nil {
-				return 0, err
-			}
-			sum += float64(hours) * 3600
-			pos += i + 1
-		}
-		if i := strings.IndexByte(timeStr[pos:], 'M'); i != -1 {
-			minutes, err := strconv.ParseInt(timeStr[pos:pos+i], 10, 64)
-			if err != nil {
-				return 0, err
-			}
-			sum += float64(minutes) * 60
-			pos += i + 1
-		}
-		if i := strings.IndexByte(timeStr[pos:], 'S'); i != -1 {
-			seconds, err := strconv.ParseFloat(timeStr[pos:pos+i], 64)
-			if err != nil {
-				return 0, err
-			}
-			sum += seconds
+	if parts[4] != "" {
+		secs, err := strconv.ParseFloat(strings.TrimRight(parts[4], "S"), 64)
+		if err != nil {
+			return 0, fmt.Errorf("Error parsing Seconds: %s", err)
 		}
+		total += time.Duration(secs * float64(time.Second))
 	}
 
-	if minus {
-		sum = -sum
-	}
-	return time.Duration(sum * float64(time.Second)), nil
+	return total, nil
 }

mpd/duration_test.go

@@ -4,7 +4,7 @@ import (
 	"testing"
 	"time"
 
-	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestDuration(t *testing.T) {
@@ -15,28 +15,45 @@ func TestDuration(t *testing.T) {
 	}
 	for ins, ex := range in {
 		timeDur, err := time.ParseDuration(ins)
-		assert.Equal(t, nil, err)
+		require.Equal(t, nil, err)
 		dur := Duration(timeDur)
-		assert.Equal(t, ex, dur.String())
+		require.Equal(t, ex, dur.String())
 	}
 }
 
 func TestParseDuration(t *testing.T) {
 	in := map[string]float64{
-		"PT0S":         0,
-		"PT1M":         60,
-		"PT2H":         7200,
-		"PT6M16S":      376,
-		"PT1.97S":      1.97,
-		"PT1H2M3.456S": 3723.456,
-		"P1DT2H":       (26 * time.Hour).Seconds(),
-		"PT20M":        (20 * time.Minute).Seconds(),
-		"-P60D":        -(60 * 24 * time.Hour).Seconds(),
-		"PT1M30.5S":    (time.Minute + 30*time.Second + 500*time.Millisecond).Seconds(),
+		"PT0S":          0,
+		"PT1M":          60,
+		"PT2H":          7200,
+		"PT6M16S":       376,
+		"PT1.97S":       1.97,
+		"PT1H2M3.456S":  3723.456,
+		"P1DT2H":        (26 * time.Hour).Seconds(),
+		"PT20M":         (20 * time.Minute).Seconds(),
+		"PT1M30.5S":     (time.Minute + 30*time.Second + 500*time.Millisecond).Seconds(),
+		"PT1004199059S": (1004199059 * time.Second).Seconds(),
 	}
 	for ins, ex := range in {
 		act, err := parseDuration(ins)
-		assert.NoError(t, err, ins)
-		assert.Equal(t, ex, act.Seconds(), ins)
+		require.NoError(t, err, ins)
+		require.Equal(t, ex, act.Seconds(), ins)
+	}
+}
+
+func TestParseBadDurations(t *testing.T) {
+	in := map[string]string{
+		"P20M":   `Duration must be in the format: P[nD][T[nH][nM][nS]]`, // We don't allow Months (doesn't make sense when converting to duration)
+		"P20Y":   `Duration must be in the format: P[nD][T[nH][nM][nS]]`, // We don't allow Years (doesn't make sense when converting to duration)
+		"P15.5D": `Duration must be in the format: P[nD][T[nH][nM][nS]]`, // Only seconds can be expressed as a decimal
+		"P2H":    `Duration must be in the format: P[nD][T[nH][nM][nS]]`, // "T" must be present to separate days and hours
+		"2DT1H":  `Duration must be in the format: P[nD][T[nH][nM][nS]]`, // "P" must always be present
+		"PT2M1H": `Duration must be in the format: P[nD][T[nH][nM][nS]]`, // Hours must appear before Minutes
+		"P":      `At least one number and designator are required`,      // At least one number and designator are required
+		"-P20H":  `Duration cannot be negative`,                          // Negative duration doesn't make sense
+	}
+	for ins, msg := range in {
+		_, err := parseDuration(ins)
+		require.EqualError(t, err, msg, "Expected an error for: %s", ins)
 	}
 }

mpd/mpd_test.go

@@ -75,7 +75,7 @@ func (s *MPDSuite) TestNewMPDLive() {
 		MediaPresentationDuration: Strptr(VALID_MEDIA_PRESENTATION_DURATION),
 		MinBufferTime:             Strptr(VALID_MIN_BUFFER_TIME),
 		period:                    &Period{},
-		Periods:                   []*Period{&Period{}},
+		Periods:                   []*Period{{}},
 	}
 	assert.Equal(s.T(), expectedMPD, m)
 }
@@ -115,7 +115,7 @@ func (s *MPDSuite) TestNewMPDLiveWithBaseURLInMPD() {
 		MediaPresentationDuration: Strptr(VALID_MEDIA_PRESENTATION_DURATION),
 		MinBufferTime:             Strptr(VALID_MIN_BUFFER_TIME),
 		period:                    &Period{},
-		Periods:                   []*Period{&Period{}},
+		Periods:                   []*Period{{}},
 		BaseURL:                   VALID_BASE_URL_VIDEO,
 	}
 	assert.Equal(s.T(), expectedMPD, m)
@@ -150,7 +150,7 @@ func (s *MPDSuite) TestNewMPDHbbTV() {
 		MediaPresentationDuration: Strptr(VALID_MEDIA_PRESENTATION_DURATION),
 		MinBufferTime:             Strptr(VALID_MIN_BUFFER_TIME),
 		period:                    &Period{},
-		Periods:                   []*Period{&Period{}},
+		Periods:                   []*Period{{}},
 	}
 	assert.Equal(s.T(), expectedMPD, m)
 }
@@ -165,7 +165,7 @@ func (s *MPDSuite) TestNewMPDOnDemand() {
 		MediaPresentationDuration: Strptr(VALID_MEDIA_PRESENTATION_DURATION),
 		MinBufferTime:             Strptr(VALID_MIN_BUFFER_TIME),
 		period:                    &Period{},
-		Periods:                   []*Period{&Period{}},
+		Periods:                   []*Period{{}},
 	}
 	assert.Equal(s.T(), expectedMPD, m)
 }