Repository: fxamacker/cbor (go)

Issue:
Deprecate Valid() and add Wellformed() to replace it
RFC 8949 distinctly defines what is "Valid" and what is "Well-formed".

Although the old docs for Valid() correctly said it checks for well-formedness, Valid() is not an appropriate function name because it can lead to confusion.

### Proposed Solution

- Add Wellformed() as a replacement for Valid().
- For now, deprecate Valid() and make it call Wellformed().  This makes Valid() behave the same in v2.5.0 as prior releases.

Wellformed() checks whether data is a well-formed encoded CBOR data item and that it complies with additional restrictions such as MaxNestedLevels, MaxArrayElements, MaxMapPairs, etc.  If there are any remaining bytes after the encoded CBOR data item, then ExtraneousDataError is returned.

:warning: Projects should begin replacing calls to Valid() with calls to WellFormed().

Pull request:
Deprecate Valid() and add Wellformed() to replace it
Valid() is deprecated and only calls WellFormed(), so it will behave the same way between current v2.4 and upcoming v2.5 release.

Projects should begin replacing calls to Valid() with calls to WellFormed().

Although the old docs for Valid() correctly said it checks for well-formedness, this refactor was done because Valid() is not an appropriate function name.

RFC 8949 distinctly defines what is "Valid" and what is "Well-formed".

Wellformed() checks whether data is a well-formed encoded CBOR data item and that it complies with additional restrictions such as MaxNestedLevels, MaxArrayElements, MaxMapPairs, etc.  If there are any remaining bytes after the encoded CBOR data item, then ExtraneousDataError is returned.

While at it, also updated comments to more closely match terminology used in RFC 8949.

Closes #399

Patch:
diff --git a/decode.go b/decode.go
index 343da9e..5bc9e86 100644
--- a/decode.go
+++ b/decode.go
@@ -99,11 +99,32 @@ func Unmarshal(data []byte, v interface{}) error {
 	return defaultDecMode.Unmarshal(data, v)
 }
 
-// Valid checks whether the CBOR data is complete and well-formed.
+// Valid checks whether data is a well-formed encoded CBOR data item and
+// that it complies with default restrictions such as MaxNestedLevels,
+// MaxArrayElements, MaxMapPairs, etc.
+//
+// If there are any remaining bytes after the CBOR data item,
+// an ExtraneousDataError is returned.
+//
+// WARNING: Valid doesn't check if encoded CBOR data item is valid (i.e. validity)
+// and RFC 8949 distinctly defines what is "Valid" and what is "Well-formed".
+//
+// Deprecated: Valid is kept for compatibility and should not be used.
+// Use Wellformed instead because it has a more appropriate name.
 func Valid(data []byte) error {
 	return defaultDecMode.Valid(data)
 }
 
+// Wellformed checks whether data is a well-formed encoded CBOR data item and
+// that it complies with default restrictions such as MaxNestedLevels,
+// MaxArrayElements, MaxMapPairs, etc.
+//
+// If there are any remaining bytes after the CBOR data item,
+// an ExtraneousDataError is returned.
+func Wellformed(data []byte) error {
+	return defaultDecMode.Wellformed(data)
+}
+
 // Unmarshaler is the interface implemented by types that wish to unmarshal
 // CBOR data themselves.  The input is a valid CBOR value. UnmarshalCBOR
 // must copy the CBOR data if it needs to use it after returning.
@@ -499,10 +520,32 @@ type DecMode interface {
 	//
 	// See the documentation for Unmarshal for details.
 	Unmarshal(data []byte, v interface{}) error
-	// Valid checks whether the CBOR data is complete and well-formed.
+
+	// Valid checks whether data is a well-formed encoded CBOR data item and
+	// that it complies with configurable restrictions such as MaxNestedLevels,
+	// MaxArrayElements, MaxMapPairs, etc.
+	//
+	// If there are any remaining bytes after the CBOR data item,
+	// an ExtraneousDataError is returned.
+	//
+	// WARNING: Valid doesn't check if encoded CBOR data item is valid (i.e. validity)
+	// and RFC 8949 distinctly defines what is "Valid" and what is "Well-formed".
+	//
+	// Deprecated: Valid is kept for compatibility and should not be used.
+	// Use Wellformed instead because it has a more appropriate name.
 	Valid(data []byte) error
+
+	// Wellformed checks whether data is a well-formed encoded CBOR data item and
+	// that it complies with configurable restrictions such as MaxNestedLevels,
+	// MaxArrayElements, MaxMapPairs, etc.
+	//
+	// If there are any remaining bytes after the CBOR data item,
+	// an ExtraneousDataError is returned.
+	Wellformed(data []byte) error
+
 	// NewDecoder returns a new decoder that reads from r using dm DecMode.
 	NewDecoder(r io.Reader) *Decoder
+
 	// DecOptions returns user specified options used to create this DecMode.
 	DecOptions() DecOptions
 }
@@ -550,10 +593,10 @@ func (dm *decMode) DecOptions() DecOptions {
 func (dm *decMode) Unmarshal(data []byte, v interface{}) error {
 	d := decoder{data: data, dm: dm}
 
-	// check valid
-	off := d.off          // Save offset before data validation
-	err := d.valid(false) // don't allow any extra data after valid data item.
-	d.off = off           // Restore offset
+	// Check well-formedness.
+	off := d.off               // Save offset before data validation
+	err := d.wellformed(false) // don't allow any extra data after valid data item.
+	d.off = off                // Restore offset
 	if err != nil {
 		return err
 	}
@@ -561,10 +604,31 @@ func (dm *decMode) Unmarshal(data []byte, v interface{}) error {
 	return d.value(v)
 }
 
-// Valid checks whether the CBOR data is complete and well-formed.
+// Valid checks whether data is a well-formed encoded CBOR data item and
+// that it complies with configurable restrictions such as MaxNestedLevels,
+// MaxArrayElements, MaxMapPairs, etc.
+//
+// If there are any remaining bytes after the CBOR data item,
+// an ExtraneousDataError is returned.
+//
+// WARNING: Valid doesn't check if encoded CBOR data item is valid (i.e. validity)
+// and RFC 8949 distinctly defines what is "Valid" and what is "Well-formed".
+//
+// Deprecated: Valid is kept for compatibility and should not be used.
+// Use Wellformed instead because it has a more appropriate name.
 func (dm *decMode) Valid(data []byte) error {
+	return dm.Wellformed(data)
+}
+
+// Wellformed checks whether data is a well-formed encoded CBOR data item and
+// that it complies with configurable restrictions such as MaxNestedLevels,
+// MaxArrayElements, MaxMapPairs, etc.
+//
+// If there are any remaining bytes after the CBOR data item,
+// an ExtraneousDataError is returned.
+func (dm *decMode) Wellformed(data []byte) error {
 	d := decoder{data: data, dm: dm}
-	return d.valid(false)
+	return d.wellformed(false)
 }
 
 // NewDecoder returns a new decoder that reads from r using dm DecMode.
@@ -581,7 +645,7 @@ type decoder struct {
 // value decodes CBOR data item into the value pointed to by v.
 // If CBOR data item fails to be decoded into v,
 // error is returned and offset is moved to the next CBOR data item.
-// Precondition: d.data contains at least one valid CBOR data item.
+// Precondition: d.data contains at least one well-formed CBOR data item.
 func (d *decoder) value(v interface{}) error {
 	// v can't be nil, non-pointer, or nil pointer value.
 	if v == nil {
diff --git a/stream.go b/stream.go
index 4e8c1e8..f60f129 100644
--- a/stream.go
+++ b/stream.go
@@ -77,7 +77,7 @@ func (dec *Decoder) readNext() (int, error) {
 		if dec.off < len(dec.buf) {
 			dec.d.reset(dec.buf[dec.off:])
 			off := dec.off // Save offset before data validation
-			validErr = dec.d.valid(true)
+			validErr = dec.d.wellformed(true)
 			dec.off = off // Restore offset
 
 			if validErr == nil {
diff --git a/valid.go b/valid.go
index 9bf5fdb..a5213d0 100644
--- a/valid.go
+++ b/valid.go
@@ -68,7 +68,7 @@ func (e *TagsMdError) Error() string {
 	return "cbor: CBOR tag isn't allowed"
 }
 
-// ExtraneousDataError indicates found extraneous data following valid CBOR message.
+// ExtraneousDataError indicates found extraneous data following well-formed CBOR data item.
 type ExtraneousDataError struct {
 	numOfBytes int // number of bytes of extraneous data
 	index      int // location of extraneous data
@@ -78,15 +78,15 @@ func (e *ExtraneousDataError) Error() string {
 	return "cbor: " + strconv.Itoa(e.numOfBytes) + " bytes of extraneous data starting at index " + strconv.Itoa(e.index)
 }
 
-// valid checks whether the CBOR data is complete and well-formed.
+// wellformed checks whether the CBOR data item is well-formed.
 // allowExtraData indicates if extraneous data is allowed after the CBOR data item.
 // - use allowExtraData = true when using Decoder.Decode()
 // - use allowExtraData = false when using Unmarshal()
-func (d *decoder) valid(allowExtraData bool) error {
+func (d *decoder) wellformed(allowExtraData bool) error {
 	if len(d.data) == d.off {
 		return io.EOF
 	}
-	_, err := d.validInternal(0)
+	_, err := d.wellformedInternal(0)
 	if err == nil {
 		if !allowExtraData && d.off != len(d.data) {
 			err = &ExtraneousDataError{len(d.data) - d.off, d.off}
@@ -95,9 +95,9 @@ func (d *decoder) valid(allowExtraData bool) error {
 	return err
 }
 
-// validInternal checks data's well-formedness and returns max depth and error.
-func (d *decoder) validInternal(depth int) (int, error) {
-	t, ai, val, err := d.validHead()
+// wellformedInternal checks data's well-formedness and returns max depth and error.
+func (d *decoder) wellformedInternal(depth int) (int, error) {
+	t, ai, val, err := d.wellformedHead()
 	if err != nil {
 		return 0, err
 	}
@@ -108,7 +108,7 @@ func (d *decoder) validInternal(depth int) (int, error) {
 			if d.dm.indefLength == IndefLengthForbidden {
 				return 0, &IndefiniteLengthError{t}
 			}
-			return d.validIndefiniteString(t, depth)
+			return d.wellformedIndefiniteString(t, depth)
 		}
 		valInt := int(val)
 		if valInt < 0 {
@@ -129,7 +129,7 @@ func (d *decoder) validInternal(depth int) (int, error) {
 			if d.dm.indefLength == IndefLengthForbidden {
 				return 0, &IndefiniteLengthError{t}
 			}
-			return d.validIndefiniteArrayOrMap(t, depth)
+			return d.wellformedIndefiniteArrayOrMap(t, depth)
 		}
 
 		valInt := int(val)
@@ -156,7 +156,7 @@ func (d *decoder) validInternal(depth int) (int, error) {
 		for j := 0; j < count; j++ {
 			for i := 0; i < valInt; i++ {
 				var dpt int
-				if dpt, err = d.validInternal(depth); err != nil {
+				if dpt, err = d.wellformedInternal(depth); err != nil {
 					return 0, err
 				}
 				if dpt > maxDepth {
@@ -178,7 +178,7 @@ func (d *decoder) validInternal(depth int) (int, error) {
 			if cborType(d.data[d.off]&0xe0) != cborTypeTag {
 				break
 			}
-			if _, _, _, err = d.validHead(); err != nil {
+			if _, _, _, err = d.wellformedHead(); err != nil {
 				return 0, err
 			}
 			depth++
@@ -187,13 +187,13 @@ func (d *decoder) validInternal(depth int) (int, error) {
 			}
 		}
 		// Check tag content.
-		return d.validInternal(depth)
+		return d.wellformedInternal(depth)
 	}
 	return depth, nil
 }
 
-// validIndefiniteString checks indefinite length byte/text string's well-formedness and returns max depth and error.
-func (d *decoder) validIndefiniteString(t cborType, depth int) (int, error) {
+// wellformedIndefiniteString checks indefinite length byte/text string's well-formedness and returns max depth and error.
+func (d *decoder) wellformedIndefiniteString(t cborType, depth int) (int, error) {
 	var err error
 	for {
 		if len(d.data) == d.off {
@@ -211,15 +211,15 @@ func (d *decoder) validIndefiniteString(t cborType, depth int) (int, error) {
 		if (d.data[d.off] & 0x1f) == 31 {
 			return 0, &SyntaxError{"cbor: indefinite-length " + t.String() + " chunk is not definite-length"}
 		}
-		if depth, err = d.validInternal(depth); err != nil {
+		if depth, err = d.wellformedInternal(depth); err != nil {
 			return 0, err
 		}
 	}
 	return depth, nil
 }
 
-// validIndefiniteArrayOrMap checks indefinite length array/map's well-formedness and returns max depth and error.
-func (d *decoder) validIndefiniteArrayOrMap(t cborType, depth int) (int, error) {
+// wellformedIndefiniteArrayOrMap checks indefinite length array/map's well-formedness and returns max depth and error.
+func (d *decoder) wellformedIndefiniteArrayOrMap(t cborType, depth int) (int, error) {
 	var err error
 	maxDepth := depth
 	i := 0
@@ -232,7 +232,7 @@ func (d *decoder) validIndefiniteArrayOrMap(t cborType, depth int) (int, error)
 			break
 		}
 		var dpt int
-		if dpt, err = d.validInternal(depth); err != nil {
+		if dpt, err = d.wellformedInternal(depth); err != nil {
 			return 0, err
 		}
 		if dpt > maxDepth {
@@ -255,7 +255,7 @@ func (d *decoder) validIndefiniteArrayOrMap(t cborType, depth int) (int, error)
 	return maxDepth, nil
 }
 
-func (d *decoder) validHead() (t cborType, ai byte, val uint64, err error) {
+func (d *decoder) wellformedHead() (t cborType, ai byte, val uint64, err error) {
 	dataLen := len(d.data) - d.off
 	if dataLen == 0 {
 		return 0, 0, 0, io.ErrUnexpectedEOF
@@ -308,7 +308,7 @@ func (d *decoder) validHead() (t cborType, ai byte, val uint64, err error) {
 		switch t {
 		case cborTypePositiveInt, cborTypeNegativeInt, cborTypeTag:
 			return 0, 0, 0, &SyntaxError{"cbor: invalid additional information " + strconv.Itoa(int(ai)) + " for type " + t.String()}
-		case cborTypePrimitives: // 0xff (break code) should not be outside validIndefinite().
+		case cborTypePrimitives: // 0xff (break code) should not be outside wellformedIndefinite().
 			return 0, 0, 0, &SyntaxError{"cbor: unexpected \"break\" code"}
 		}
 		return t, ai, val, nil


Test patch:
diff --git a/valid_test.go b/valid_test.go
index 74fddc0..39e9e4d 100644
--- a/valid_test.go
+++ b/valid_test.go
@@ -10,8 +10,8 @@ import (
 
 func TestValid1(t *testing.T) {
 	for _, mt := range marshalTests {
-		if err := Valid(mt.cborData); err != nil {
-			t.Errorf("Valid() returned error %v", err)
+		if err := Wellformed(mt.cborData); err != nil {
+			t.Errorf("Wellformed() returned error %v", err)
 		}
 	}
 }
@@ -19,8 +19,8 @@ func TestValid1(t *testing.T) {
 func TestValid2(t *testing.T) {
 	for _, mt := range marshalTests {
 		dm, _ := DecOptions{DupMapKey: DupMapKeyEnforcedAPF}.DecMode()
-		if err := dm.Valid(mt.cborData); err != nil {
-			t.Errorf("Valid() returned error %v", err)
+		if err := dm.Wellformed(mt.cborData); err != nil {
+			t.Errorf("Wellformed() returned error %v", err)
 		}
 	}
 }
@@ -39,17 +39,17 @@ func TestValidExtraneousData(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			err := Valid(tc.cborData)
+			err := Wellformed(tc.cborData)
 			if err == nil {
-				t.Errorf("Valid(0x%x) didn't return an error", tc.cborData)
+				t.Errorf("Wellformed(0x%x) didn't return an error", tc.cborData)
 			} else {
 				ederr, ok := err.(*ExtraneousDataError)
 				if !ok {
-					t.Errorf("Valid(0x%x) error type %T, want *ExtraneousDataError", tc.cborData, err)
+					t.Errorf("Wellformed(0x%x) error type %T, want *ExtraneousDataError", tc.cborData, err)
 				} else if ederr.numOfBytes != tc.extraneousDataNumOfBytes {
-					t.Errorf("Valid(0x%x) returned %d bytes of extraneous data, want %d", tc.cborData, ederr.numOfBytes, tc.extraneousDataNumOfBytes)
+					t.Errorf("Wellformed(0x%x) returned %d bytes of extraneous data, want %d", tc.cborData, ederr.numOfBytes, tc.extraneousDataNumOfBytes)
 				} else if ederr.index != tc.extraneousDataIndex {
-					t.Errorf("Valid(0x%x) returned extraneous data index %d, want %d", tc.cborData, ederr.index, tc.extraneousDataIndex)
+					t.Errorf("Wellformed(0x%x) returned extraneous data index %d, want %d", tc.cborData, ederr.index, tc.extraneousDataIndex)
 				}
 			}
 		})
@@ -63,8 +63,8 @@ func TestValidOnStreamingData(t *testing.T) {
 	}
 	d := decoder{data: buf.Bytes(), dm: defaultDecMode}
 	for i := 0; i < len(marshalTests); i++ {
-		if err := d.valid(true); err != nil {
-			t.Errorf("valid() returned error %v", err)
+		if err := d.wellformed(true); err != nil {
+			t.Errorf("wellformed() returned error %v", err)
 		}
 	}
 }
@@ -111,12 +111,12 @@ func TestDepth(t *testing.T) {
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
 			d := decoder{data: tc.cborData, dm: defaultDecMode}
-			depth, err := d.validInternal(0)
+			depth, err := d.wellformedInternal(0)
 			if err != nil {
-				t.Errorf("valid(0x%x) returned error %v", tc.cborData, err)
+				t.Errorf("wellformed(0x%x) returned error %v", tc.cborData, err)
 			}
 			if depth != tc.wantDepth {
-				t.Errorf("valid(0x%x) returned depth %d, want %d", tc.cborData, depth, tc.wantDepth)
+				t.Errorf("wellformed(0x%x) returned depth %d, want %d", tc.cborData, depth, tc.wantDepth)
 			}
 		})
 	}
@@ -176,12 +176,12 @@ func TestDepthError(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			dm, _ := tc.opts.decMode()
 			d := decoder{data: tc.cborData, dm: dm}
-			if _, err := d.validInternal(0); err == nil {
-				t.Errorf("valid(0x%x) didn't return an error", tc.cborData)
+			if _, err := d.wellformedInternal(0); err == nil {
+				t.Errorf("wellformed(0x%x) didn't return an error", tc.cborData)
 			} else if _, ok := err.(*MaxNestedLevelError); !ok {
-				t.Errorf("valid(0x%x) returned wrong error type %T, want (*MaxNestedLevelError)", tc.cborData, err)
+				t.Errorf("wellformed(0x%x) returned wrong error type %T, want (*MaxNestedLevelError)", tc.cborData, err)
 			} else if err.Error() != tc.wantErrorMsg {
-				t.Errorf("valid(0x%x) returned error %q, want error %q", tc.cborData, err.Error(), tc.wantErrorMsg)
+				t.Errorf("wellformed(0x%x) returned error %q, want error %q", tc.cborData, err.Error(), tc.wantErrorMsg)
 			}
 		})
 	}