Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Stop various JSON field from being sent with null values #4260
Conversation
|
Travis job failed: Not sure what to make of it. I've restarted the build but it might as well fail again. |
| @@ -1418,7 +1418,7 @@ func (s *apiSuite) TestFind(c *check.C) { | ||
| c.Assert(snaps, check.HasLen, 1) | ||
| c.Assert(snaps[0]["name"], check.Equals, "store") | ||
| c.Check(snaps[0]["prices"], check.IsNil) | ||
| - c.Check(snaps[0]["screenshots"], check.HasLen, 0) | ||
| + c.Check(snaps[0]["screenshots"], check.IsNil) |
bboozzoo
Nov 21, 2017
Contributor
That's interesting. I would suspect the checker to behave the same for var foo []string and foo := []string{}. Both have len(..) == 0.
codecov-io
commented
Nov 21, 2017
•
Edited 2 times
-
codecov-io
Nov 23, 2017
-
codecov-io
Nov 21, 2017
Edited 2 times
-
Nov 23, 2017 -
Nov 21, 2017
Codecov Report
@@ Coverage Diff @@
## master #4260 +/- ##
=========================================
+ Coverage 75.97% 76.3% +0.32%
=========================================
Files 442 442
Lines 38483 38804 +321
=========================================
+ Hits 29239 29610 +371
+ Misses 7229 7161 -68
- Partials 2015 2033 +18
Continue to review full report at Codecov.
|
| +} | ||
| + | ||
| +// MarshalJSON makes Snap a json.Marshaller | ||
| +func (s *Snap) MarshalJSON() ([]byte, error) { |
bboozzoo
Nov 22, 2017
Contributor
The usual way to handle such case this in Go is to do the following:
func (s *Snap) MarshalJSON() ([]byte, error) {
type auxSnap Snap // use auxiliary type so that Go does not call Snap.MarshalJSON()
// separate type just for marshalling
m := struct {
auxSnap
InstallDate *time.Time `json:"install-date,omitempty"`
}{
auxSnap: auxSnap(*s),
}
if !s.InstallDate.IsZero() {
m.InstallDate = &s.InstallDate
}
return json.Marshal(&m)
}
// and you don't need to add unmarshaller anymore
See for an example: https://play.golang.org/p/nHK9bjfd_c
robert-ancell
Nov 22, 2017
Contributor
Oh, that's much nicer. I copied the existing cases in snapd - they should be update too then.
|
CC @pedronis - there are lots of interesting failures here, is this a store fluke?
|
|
no, that's auto-import stuff, no sure what that file is/comes from though |
robert-ancell
and others
added some commits
Nov 20, 2017
|
I'm not going to say this fails because of the PR but the error pattern seems to repeat itself quite often. Something we should look into. I'll run this locally to see what happens. EDIT: This failed with the weird EOF bug the same way. |
zyga
requested changes
Nov 23, 2017
Someone needs to jump into this to debug the test suite. This is clearly caused by the changes here though not sure how.
| @@ -79,7 +79,7 @@ type respJSON struct { | ||
| Type ResponseType `json:"type"` | ||
| Status int `json:"status-code"` | ||
| StatusText string `json:"status"` | ||
| - Result interface{} `json:"result"` | ||
| + Result interface{} `json:"result,omitempty"` |
zyga
Nov 23, 2017
Contributor
<zyga-solus> pedronis: can you please look at http://paste.ubuntu.com/26029227
<zyga-solus> pedronis: is that a sane assertion (syntax wise)
<zyga-solus> pedronis: this is what we fail on that EOF bug
<pedronis> zyga-solus: it looks like one, I don't the problem is assertion
<pedronis> zyga-solus: my guess would be the change in response, but I'm not sure why it would trigger problems
<pedronis> zyga-solus: easiest would be to get the branch, try one of the failing tests, try to undo that change, see if it changes something, then figure out what's happening
<zyga-solus> pedronis: right, I'm looking at the diff again but nothing stands out
<zyga-solus> pedronis: the only new thing is Tracks
<zyga-solus> pedronis: it didn't have any annotation before
<pedronis> but that struct is not involved with ack at all
<pedronis> as I said afaict the only logical candidate
<zyga-solus> hmm
<pedronis> is the change in response.go
<pedronis> it would be good to verify that theory
<zyga-solus> pedronis: sure, that's easy
<zyga-solus> thank you
<zyga-solus> pedronis: so, it's not that
<zyga-solus> pedronis: rebuilt snap, snapd, reinstalled, restarted snapd
So unless I missed something, it is not this line.
zyga
Nov 23, 2017
Contributor
Heh, so obviously re-exec bit me.
<pedronis> zyga-solus: actually I'm quite sure it is
<zyga-solus> pedronis: ok, let me re-check
<pedronis> I mean I�see the now the code
<pedronis> why that would matter
<pedronis> we do things like this in client: err := jsonutil.DecodeWithNumber(bytes.NewReader(rsp.Result), v)
<pedronis> json.Unmarshal(rsp.Result, &resultErr)
<zyga-solus> ok
<zyga-solus> I definitely removed that change
<pedronis> and this is the field in the client: Result json.RawMessage `json:"result"`
<zyga-solus> and rebuilt
* zyga-solus wonders
<pedronis> so if the field is not there (instead of null), I suspect those to break with EOF
<zyga-solus> and restarted
<zyga-solus> Chipaca: woot, thank you!
<zyga-solus> pedronis: could it be a combination, I'm sure I removed this aspect of the patch
<zyga-solus> pedronis: and new snapd is runing
<zyga-solus> *running
<zyga-solus> ah
<zyga-solus> sorry, obviously
<zyga-solus> reexec
<pedronis> heh
<zyga-solus> thank you
<zyga-solus> :)
* pstolowski rozłączył/a się (Quit: Leaving)
<pedronis> now we need to decide whether we want to fix client or we consider this an incompatible change
<pedronis> maybe Chipaca or niemeyer have opinions on that
<zyga-solus> yes, confirmed
<zyga-solus> funny outcome
<pedronis> with how client is written we cannot omit result
| @@ -79,7 +79,7 @@ type respJSON struct { | ||
| Type ResponseType `json:"type"` | ||
| Status int `json:"status-code"` | ||
| StatusText string `json:"status"` | ||
| - Result interface{} `json:"result"` | ||
| + Result interface{} `json:"result,omitempty"` |
zyga
Nov 23, 2017
Contributor
<zyga-solus> pedronis: can you please look at http://paste.ubuntu.com/26029227
<zyga-solus> pedronis: is that a sane assertion (syntax wise)
<zyga-solus> pedronis: this is what we fail on that EOF bug
<pedronis> zyga-solus: it looks like one, I don't the problem is assertion
<pedronis> zyga-solus: my guess would be the change in response, but I'm not sure why it would trigger problems
<pedronis> zyga-solus: easiest would be to get the branch, try one of the failing tests, try to undo that change, see if it changes something, then figure out what's happening
<zyga-solus> pedronis: right, I'm looking at the diff again but nothing stands out
<zyga-solus> pedronis: the only new thing is Tracks
<zyga-solus> pedronis: it didn't have any annotation before
<pedronis> but that struct is not involved with ack at all
<pedronis> as I said afaict the only logical candidate
<zyga-solus> hmm
<pedronis> is the change in response.go
<pedronis> it would be good to verify that theory
<zyga-solus> pedronis: sure, that's easy
<zyga-solus> thank you
<zyga-solus> pedronis: so, it's not that
<zyga-solus> pedronis: rebuilt snap, snapd, reinstalled, restarted snapd
So unless I missed something, it is not this line.
zyga
Nov 23, 2017
Contributor
Heh, so obviously re-exec bit me.
<pedronis> zyga-solus: actually I'm quite sure it is
<zyga-solus> pedronis: ok, let me re-check
<pedronis> I mean I�see the now the code
<pedronis> why that would matter
<pedronis> we do things like this in client: err := jsonutil.DecodeWithNumber(bytes.NewReader(rsp.Result), v)
<pedronis> json.Unmarshal(rsp.Result, &resultErr)
<zyga-solus> ok
<zyga-solus> I definitely removed that change
<pedronis> and this is the field in the client: Result json.RawMessage `json:"result"`
<zyga-solus> and rebuilt
* zyga-solus wonders
<pedronis> so if the field is not there (instead of null), I suspect those to break with EOF
<zyga-solus> and restarted
<zyga-solus> Chipaca: woot, thank you!
<zyga-solus> pedronis: could it be a combination, I'm sure I removed this aspect of the patch
<zyga-solus> pedronis: and new snapd is runing
<zyga-solus> *running
<zyga-solus> ah
<zyga-solus> sorry, obviously
<zyga-solus> reexec
<pedronis> heh
<zyga-solus> thank you
<zyga-solus> :)
* pstolowski rozłączył/a się (Quit: Leaving)
<pedronis> now we need to decide whether we want to fix client or we consider this an incompatible change
<pedronis> maybe Chipaca or niemeyer have opinions on that
<zyga-solus> yes, confirmed
<zyga-solus> funny outcome
<pedronis> with how client is written we cannot omit result
| @@ -79,7 +79,7 @@ type respJSON struct { | ||
| Type ResponseType `json:"type"` | ||
| Status int `json:"status-code"` | ||
| StatusText string `json:"status"` | ||
| - Result interface{} `json:"result,omitempty"` | ||
| + Result interface{} `json:"result"` |
bboozzoo
Nov 23, 2017
•
Edited 1 time
-
bboozzoo
Nov 23, 2017
Contributor
Edited 1 time
-
Nov 23, 2017
@zyga can you leave a comment that this field must not be tagged omitempty in the future because of backward compatibilty?
zyga
merged commit 7727023
into
snapcore:master
Nov 23, 2017
1 check passed
continuous-integration/travis-ci/pr
The Travis CI build passed
Details
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
robert-ancell commentedNov 20, 2017
•
Edited 1 time
-
robert-ancell
Nov 20, 2017
This makes the JSON easier to debug and less traffic to send.
Note that the 'tracks' field seemed to be missing the JSON annotation and is currently being sent as "Tracks". I actually implemented this in snapd-glib as "tracks" so while it is a change to the API I doubt anyone was consuming it apart from snapd-glib at this point.