encoding/asn1: only omit optional elements matching default value.

ASN.1 elements can be optional, and can have a default value.
Traditionally, Go has omitted elements that are optional and that have
the zero value. I believe that's a bug (see [1]).

This change causes an optional element with a default value to only be
omitted when it has that default value. The previous behaviour of
omitting optional, zero elements with no default is retained because
it's used quite a lot and will break things if changed.

[1] https://groups.google.com/d/msg/Golang-nuts/9Ss6o9CW-Yo/KL_V7hFlyOAJ

Fixes #7780.

R=bradfitz

LGTM=bradfitz
R=golang-codereviews, bradfitz, rsc
CC=golang-codereviews, r
https://golang.org/cl/86960045

diff --git a/src/pkg/encoding/asn1/asn1.go b/src/pkg/encoding/asn1/asn1.go
index ec7f91c1bba5ebd8909aecabd24e7e6078b977a2..b06aec3e408335283dc84df93bf1cac32ef33583 100644 (file)
--- a/src/pkg/encoding/asn1/asn1.go
+++ b/src/pkg/encoding/asn1/asn1.go
@@ -822,8 +822,19 @@ func parseField(v reflect.Value, bytes []byte, initOffset int, params fieldParam
        return
 }
 
+// canHaveDefaultValue reports whether k is a Kind that we will set a default
+// value for. (A signed integer, essentially.)
+func canHaveDefaultValue(k reflect.Kind) bool {
+       switch k {
+       case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
+               return true
+       }
+
+       return false
+}
+
 // setDefaultValue is used to install a default value, from a tag string, into
-// a Value. It is successful is the field was optional, even if a default value
+// a Value. It is successful if the field was optional, even if a default value
 // wasn't provided or it failed to install it into the Value.
 func setDefaultValue(v reflect.Value, params fieldParameters) (ok bool) {
        if !params.optional {
@@ -833,9 +844,8 @@ func setDefaultValue(v reflect.Value, params fieldParameters) (ok bool) {
        if params.defaultValue == nil {
                return
        }
-       switch val := v; val.Kind() {
-       case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
-               val.SetInt(*params.defaultValue)
+       if canHaveDefaultValue(v.Kind()) {
+               v.SetInt(*params.defaultValue)
        }
        return
 }

diff --git a/src/pkg/encoding/asn1/marshal.go b/src/pkg/encoding/asn1/marshal.go
index e26fe59b305b29e1c8f81a06a61b150e3adb7ddb..b2f104b4cbe56817a5342038b13d31e3b5a80969 100644 (file)
--- a/src/pkg/encoding/asn1/marshal.go
+++ b/src/pkg/encoding/asn1/marshal.go
@@ -513,8 +513,22 @@ func marshalField(out *forkableWriter, v reflect.Value, params fieldParameters)
                return
        }
 
-       if params.optional && reflect.DeepEqual(v.Interface(), reflect.Zero(v.Type()).Interface()) {
-               return
+       if params.optional && params.defaultValue != nil && canHaveDefaultValue(v.Kind()) {
+               defaultValue := reflect.New(v.Type()).Elem()
+               defaultValue.SetInt(*params.defaultValue)
+
+               if reflect.DeepEqual(v.Interface(), defaultValue.Interface()) {
+                       return
+               }
+       }
+
+       // If no default value is given then the zero value for the type is
+       // assumed to be the default value. This isn't obviously the correct
+       // behaviour, but it's what Go has traditionally done.
+       if params.optional && params.defaultValue == nil {
+               if reflect.DeepEqual(v.Interface(), reflect.Zero(v.Type()).Interface()) {
+                       return
+               }
        }
 
        if v.Type() == rawValueType {

diff --git a/src/pkg/encoding/asn1/marshal_test.go b/src/pkg/encoding/asn1/marshal_test.go
index a15acbed012d15e4b74d47a261ffa289b22635d9..5b0115f28c58f5f99c938503a519fbe44065428d 100644 (file)
--- a/src/pkg/encoding/asn1/marshal_test.go
+++ b/src/pkg/encoding/asn1/marshal_test.go
@@ -58,6 +58,10 @@ type omitEmptyTest struct {
        A []string `asn1:"omitempty"`
 }
 
+type defaultTest struct {
+       A int `asn1:"optional,default:1"`
+}
+
 type testSET []int
 
 var PST = time.FixedZone("PST", -8*60*60)
@@ -133,6 +137,9 @@ var marshalTests = []marshalTest{
        {omitEmptyTest{[]string{}}, "3000"},
        {omitEmptyTest{[]string{"1"}}, "30053003130131"},
        {"Σ", "0c02cea3"},
+       {defaultTest{0}, "3003020100"},
+       {defaultTest{1}, "3000"},
+       {defaultTest{2}, "3003020102"},
 }
 
 func TestMarshal(t *testing.T) {