encoding/json: Marshal of nil net.IP fails

[gopherbot]

by liviobs:

What steps will reproduce the problem?
If possible, include a link to a program on play.golang.org.
1. Marshalling an empty net.IP used to work. For example:
http://play.golang.org/p/JQS_wUkDNr
2. With 'tip' version of golang, it fails.

What is the expected output?

null <nil>

What do you see instead?

 json: error calling MarshalJSON for type net.IP: invalid IP address


Which compiler are you using (5g, 6g, 8g, gccgo)?

6g

Which operating system are you using?

Linux

Which version are you using?  (run 'go version')

go version devel +a71616f65cb1 Tue Sep 03 21:23:52 2013 -0700 linux/amd64

Please provide any additional information below.

It looks like the following check is what changed the behavior (from changeset
17745:7c4368941249)

src/pkg/net/ip.go:

+// MarshalText implements the encoding.TextMarshaler interface.
+// The encoding is the same as returned by String.
+func (ip IP) MarshalText() ([]byte, error) {
+       if len(ip) != IPv4len && len(ip) != IPv6len {
+               return nil, errors.New("invalid IP address")
+       }

With an empty net.IP len(ip) is 0.

[ianlancetaylor]

Comment 1:

Check whether this needs to be fixed in 1.2.

Labels changed: added go1.2maybe.

[gopherbot]

Comment 2 by liviobs:

Two follow up comments about this issue (this issue is important to me):
1) I rely heavily on the fact that certain operations work correctly for non-initialized
values in Go. It is useful everywhere, but particularly useful for complex structs. I
hit this problem exactly on a struct with members being structs themselves. I would
rather avoid having to code struct initializers for things with net.IP inside, specially
given that no other struct in my code needs it.
I would plead to the Go team to keep something like json.Marshal() to "just work" out of
the box without me having to understand whether certain types from the standard package
(such as net.IP) is implemented using slice, struct, or etc. 
2) I was able to hack at this problem with the following patch. I'm not happy with it
since there is a awkward conversion between the MarshalText nil ("<nil>") and the
JSON nil ("null"). I don't know of a better strategy, since an implementer of
MarshalText for a slice type may want to return a custom value  for empty slices that is
different than "<nil>". It seems that the json encoder will *have* to call the
TextMarshaler even for empty slices.
diff -r 11b2294dca61 src/pkg/encoding/json/encode.go
--- a/src/pkg/encoding/json/encode.go   Wed Sep 04 17:02:08 2013 -0700
+++ b/src/pkg/encoding/json/encode.go   Thu Sep 05 16:01:24 2013 -0400
@@ -460,6 +460,7 @@
    m := v.Interface().(encoding.TextMarshaler)
    b, err := m.MarshalText()
    if err == nil {
+       if bytes.Equal(b, []byte("<nil>")) { b = []byte("null") }
        _, err = e.stringBytes(b)
    }
    if err != nil {
diff -r 11b2294dca61 src/pkg/net/ip.go
--- a/src/pkg/net/ip.go Wed Sep 04 17:02:08 2013 -0700
+++ b/src/pkg/net/ip.go Thu Sep 05 16:01:24 2013 -0400
@@ -315,7 +315,7 @@
 // MarshalText implements the encoding.TextMarshaler interface.
 // The encoding is the same as returned by String.
 func (ip IP) MarshalText() ([]byte, error) {
-   if len(ip) != IPv4len && len(ip) != IPv6len {
+   if len(ip) != 0 && len(ip) != IPv4len && len(ip) != IPv6len {
        return nil, errors.New("invalid IP address")
    }
    return []byte(ip.String()), nil
diff -r 11b2294dca61 src/pkg/net/ip_test.go
--- a/src/pkg/net/ip_test.go    Wed Sep 04 17:02:08 2013 -0700
+++ b/src/pkg/net/ip_test.go    Thu Sep 05 16:01:24 2013 -0400
@@ -65,10 +65,6 @@
            if out, err := tt.in.MarshalText(); string(out) != tt.out || err != nil {
                t.Errorf("IP.MarshalText(%v) = %q, %v, want %q, nil", out, err, tt.out)
            }
-       } else {
-           if _, err := tt.in.MarshalText(); err == nil {
-               t.Errorf("IP.MarshalText(nil) succeeded, want failure")
-           }
        }
    }
 }

[rsc]

Comment 3:

I don't know what the fix is, but I do think we should probably support this somehow. 
I am leaning toward encoding the nil IP as "". It formerly encoded as JSON null (not a
string), but you couldn't unmarshal it from that, and other addresses encoded as base64,
so it is very difficult to believe that anyone cares about the earlier forms.
It is easy to believe that people care about whether Marshal succeeds or fails. We must
find a way for it to succeed.

Labels changed: added priority-later, go1.2, removed priority-triage, go1.2maybe.

Owner changed to @rsc.

Status changed to Accepted.

[gopherbot]

Comment 4 by liviobs:

Hi Russ,
Thanks for the suggestion. I tried your suggestion (i.e., encoding nil IP as ""), and it
works better than what I had previously tried (in my previous attempt, unmarshal would
get confused, and my attempts to fix the unmarshal in json/decode.go by special-casing
the 'null' identifier was failing).
If I use the following example: http://play.golang.org/p/Iz_EhNcaAC , and apply the
changes below to tip, I get (what I believe is) sane behavior for both marshal and
unmarshal:
{192.168.0.1}
{"IP":"192.168.0.1"} <nil>
{192.168.0.1} <nil>
{<nil>}
{"IP":""} <nil>
{<nil>} <nil>
I just had to tweak the tests in ip_test.go ever so slightly to get the build to succeed.
Also, I agree with your reasoning about earlier forms of JSON-ized net.IP not being a
concern given that unmarshal of the base64 version did not work "out of the box" (I had
to work around the problem by implementing a UnmarshalJSON for the parent struct and
special casing the net.IP field member).
diff -r 58e5ed6b5029 src/pkg/net/ip.go
--- a/src/pkg/net/ip.go Thu Sep 05 23:06:34 2013 -0400
+++ b/src/pkg/net/ip.go Fri Sep 06 00:19:18 2013 -0400
@@ -315,6 +315,9 @@
 // MarshalText implements the encoding.TextMarshaler interface.
 // The encoding is the same as returned by String.
 func (ip IP) MarshalText() ([]byte, error) {
+   if len(ip) == 0 {
+       return []byte(""), nil
+   }
    if len(ip) != IPv4len && len(ip) != IPv6len {
        return nil, errors.New("invalid IP address")
    }
@@ -324,6 +327,10 @@
 // UnmarshalText implements the encoding.TextUnmarshaler interface.
 // The IP address is expected in a form accepted by ParseIP.
 func (ip *IP) UnmarshalText(text []byte) error {
+   if len(text) == 0 {
+       ip = nil
+       return nil
+   }
    s := string(text)
    x := ParseIP(s)
    if x == nil {
diff -r 58e5ed6b5029 src/pkg/net/ip_test.go
--- a/src/pkg/net/ip_test.go    Thu Sep 05 23:06:34 2013 -0400
+++ b/src/pkg/net/ip_test.go    Fri Sep 06 00:19:18 2013 -0400
@@ -13,18 +13,19 @@
 var parseIPTests = []struct {
    in  string
    out IP
+   err error
 }{
-   {"127.0.1.2", IPv4(127, 0, 1, 2)},
-   {"127.0.0.1", IPv4(127, 0, 0, 1)},
-   {"127.0.0.256", nil},
-   {"abc", nil},
-   {"123:", nil},
-   {"::ffff:127.0.0.1", IPv4(127, 0, 0, 1)},
-   {"2001:4860:0:2001::68", IP{0x20, 0x01, 0x48, 0x60, 0, 0, 0x20, 0x01, 0, 0, 0, 0, 0,
0, 0x00, 0x68}},
-   {"::ffff:4a7d:1363", IPv4(74, 125, 19, 99)},
-   {"fe80::1%lo0", nil},
-   {"fe80::1%911", nil},
-   {"", nil},
+   {"127.0.1.2", IPv4(127, 0, 1, 2), nil},
+   {"127.0.0.1", IPv4(127, 0, 0, 1), nil},
+   {"127.0.0.256", nil, &ParseError{"IP address", "127.0.0.256"}},
+   {"abc", nil, &ParseError{"IP address", "abc"}},
+   {"123:", nil, &ParseError{"IP address", "123:"}},
+   {"::ffff:127.0.0.1", IPv4(127, 0, 0, 1), nil},
+   {"2001:4860:0:2001::68", IP{0x20, 0x01, 0x48, 0x60, 0, 0, 0x20, 0x01, 0, 0, 0, 0, 0,
0, 0x00, 0x68}, nil},
+   {"::ffff:4a7d:1363", IPv4(74, 125, 19, 99), nil},
+   {"fe80::1%lo0", nil, &ParseError{"IP address", "fe80::1%lo0"}},
+   {"fe80::1%911", nil, &ParseError{"IP address", "fe80::1%911"}},
+   {"", nil, nil},
 }
 
 func TestParseIP(t *testing.T) {
@@ -34,8 +35,8 @@
        }
        var out IP
 
-       if err := out.UnmarshalText([]byte(tt.in)); !reflect.DeepEqual(out, tt.out) ||
(tt.out == nil) != (err != nil) {
-           t.Errorf("IP.UnmarshalText(%q) = %v, %v, want %v", tt.in, out, err, tt.out)
+       if err := out.UnmarshalText([]byte(tt.in)) ; !reflect.DeepEqual(out, tt.out) ||
!reflect.DeepEqual(err, tt.err) {
+           t.Errorf("IP.UnmarshalText(%q) = %v, %v, want %v, %v", tt.in, out, err, tt.out,
tt.err)
        }
    }
 }
@@ -65,10 +66,6 @@
            if out, err := tt.in.MarshalText(); string(out) != tt.out || err != nil {
                t.Errorf("IP.MarshalText(%v) = %q, %v, want %q, nil", out, err, tt.out)
            }
-       } else {
-           if _, err := tt.in.MarshalText(); err == nil {
-               t.Errorf("IP.MarshalText(nil) succeeded, want failure")
-           }
        }
    }
 }

[bradfitz]

Comment 5:

This issue was closed by revision da7a51d.

Status changed to Fixed.