Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

time: time.Time's UnmarshalJSON produces bad error message when given "null" #9037

Closed
gopherbot opened this issue Nov 1, 2014 · 13 comments
Closed

Comments

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 1, 2014

by dannyyoo@google.com:

The following demonstrates the problem:

---------------
package main

import (
    "encoding/json"
    "fmt"
    "bytes"
    "time"
)

type RegisterStructure struct {
    SignupTimestamp time.Time `json:"signuptimestamp,omitempty"`
}

func main() {
    requestBody := []byte(`{"signuptimestamp" : null}`)
    dec := json.NewDecoder(bytes.NewReader(requestBody))
    registerInstance := RegisterStructure{}
    err := dec.Decode(&registerInstance)
    if err != nil {
        fmt.Println(err)
    }
}
---------------

This reports the following error:

----------------------------
parsing time "null" as ""2006-01-02T15:04:05Z07:00"":
cannot parse "null" as """
----------------------------


Instead, what should probably happen is to set to the time.Time zero value.  This came
up in the course of answering on Stack Overflow:

http://stackoverflow.com/questions/26684752/json-decode-cannot-parse-timestamp-in-json-into-go-struct
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 1, 2014

Comment 1:

Labels changed: added repo-main, release-go1.5.

@gopherbot gopherbot added new labels Nov 1, 2014
@bradfitz bradfitz modified the milestone: Go1.5 Dec 16, 2014
@rsc rsc removed the repo-main label Apr 14, 2015
@dspezia
Copy link
Contributor

@dspezia dspezia commented Apr 27, 2015

The documentation says:

"The JSON null value unmarshals into an interface, map, pointer, or slice by setting that Go value to nil. Because null is often used in JSON to mean “not present,” unmarshaling a JSON null into any other Go type has no effect on the value and produces no error."

My understanding is a null value bound to a time.Time should leave it unchanged (since a time.Time is a struct). Now, time.Time implements the json.Unmarshaller interface, and the implementation just parses a RFC3339 formatted string, without checking for JSON null values. In the JSON decoding engine, the call to the Unmarshaller interface takes precedence over null value management, which I guess is the origin of the problem.

There are two ways to fix it:

  1. A minor change in decode.go can prevent an eventual Unmarshaller method to be called in case of null value, fully leading to the "null for non reference type, no change" semantic.

  2. Proper management of null values in the time.Time unmarshaller method can be added.

I have a fix which seems to work fine for the first case, but I'm not 100% sure it is really the expected behavior. Can someone please advise?

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Apr 27, 2015

JSON null probably decode to the zero value of time.Time{}

@dspezia
Copy link
Contributor

@dspezia dspezia commented Apr 27, 2015

Currently, it returns an error. See http://play.golang.org/p/4hwb51LBPG

The question is: should it decode to a zero value (making it a special case), or leave the value unchanged (as specified in the encoding/json package documentation)? And, in that case, should it be managed in encoding/json, or delegated to the unmarshaller implementation of time.Time?

Note that this is actually not specific to time.Time, I can produce a similar issue with any type implementing the Unmarshaller interface - see http://play.golang.org/p/Q6_XQjYIbn

@dspezia
Copy link
Contributor

@dspezia dspezia commented May 14, 2015

I think we are in the freeze now, so I suppose we will not change the behavior described in the documentation. Mailing a CL implementing my first solution.

@gopherbot
Copy link
Author

@gopherbot gopherbot commented May 14, 2015

CL https://golang.org/cl/9376 mentions this issue.

@rsc rsc closed this in 1a99ba5 Jul 22, 2015
@dspezia
Copy link
Contributor

@dspezia dspezia commented Jul 31, 2015

This issue has to be reopened, and set to Go1.6 (because the fix has been reverted).

@mikioh mikioh modified the milestones: Go1.6, Go1.5 Jul 31, 2015
@mikioh mikioh reopened this Jul 31, 2015
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Sep 13, 2015

CL https://golang.org/cl/14552 mentions this issue.

@rsc rsc modified the milestones: Go1.7, Go1.6 Dec 5, 2015
@rsc
Copy link
Contributor

@rsc rsc commented Dec 5, 2015

Putting off again. The change is too intricate, and we didn't get to it in time. Sorry.

@quentinmit quentinmit modified the milestones: Go1.8, Go1.7 May 19, 2016
@quentinmit quentinmit added the NeedsFix label May 19, 2016
@quentinmit
Copy link
Contributor

@quentinmit quentinmit commented May 19, 2016

@rsc moved the CL to 1.8; sadly we won't get to this for 1.7.

@gopherbot
Copy link
Author

@gopherbot gopherbot commented Oct 12, 2016

CL https://golang.org/cl/30944 mentions this issue.

@gopherbot gopherbot closed this in f444b48 Oct 17, 2016
@segeschecho
Copy link

@segeschecho segeschecho commented Jan 6, 2017

I made an example of a Workaround for versions before 1.8 that uses this patch, solving the time.Time problem:

https://play.golang.org/p/fpjg9hRb7l

@invisiblethreat
Copy link

@invisiblethreat invisiblethreat commented Jan 26, 2017

@segeschecho FWIW, it only solves the null case. If it's not null, it's a recursion bomb.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
9 participants
You can’t perform that action at this time.