Add json (un)marshalling methods on NullUUID #44

blastrock · 2017-03-15T15:49:13Z

Without these methods, NullUUID is marshalled as an object with an UUID and a Valid properties. This allows to marshal it as null or a string.

coveralls · 2017-03-15T15:53:07Z

Coverage decreased (-0.5%) to 96.057% when pulling 6b5babb on blastrock:json_marshalling into b061729 on satori:master.

satori · 2017-03-16T13:15:46Z

@blastrock could you please reimplement it with encoding.TextMarshaler and encoding.TextUnmarshaler interfaces?

blastrock · 2017-03-16T14:05:39Z

I'm new to go, so maybe I got something wrong, but if I implement MarshalText like this:

func (u NullUUID) MarshalText() ([]byte, error) {
	if u.Valid == false {
		return []byte("null"), nil
	}
	return []byte(u.UUID.String()), nil
}

A null UUID is marshalled in JSON as a string "null" instead of a null value. If I return nil instead of []byte("null"), then it gets marshalled as an empty string "".

dmitshur · 2017-03-18T16:39:25Z

@blastrock Documentation of json.Marshal at https://godoc.org/encoding/json#Marshal states:

If an encountered value implements the Marshaler interface and is not a nil pointer, Marshal calls its MarshalJSON method to produce JSON. If no MarshalJSON method is present but the value implements encoding.TextMarshaler instead, Marshal calls its MarshalText method and encodes the result as a JSON string.

The second sentence explains the behavior you're seeing.

blastrock · 2017-03-18T19:11:12Z

Indeed, but this is not the behavior one would expect, as a null UUID is better represented by a null JSON value than an empty string.

In my use-case at least, I need a null JSON value when a NullUUID is not Valid, and I think only MarshalJSON can provide me with this behavior.

dmitshur · 2017-03-18T19:12:36Z

Yes, I agree. I don't think you'll be able to use just encoding.TextMarshaler to achieve that, you'll need json.Marshaler.

satori · 2017-03-22T00:38:52Z

uuid.go

+		u.Valid = false
+	}
+	return nil
+}


Would something like that make UnmarshalJSON a bit cleaner?

func (u *NullUUID) UnmarshalJSON(b []byte) error { if bytes.Equal(b, []byte("null")) { return nil } if err := json.Unmarshal(b, &u.UUID); err != nil { return err } u.Valid = true return nil }

I highly prefer being explicit about return values, I think it's much more readable to see return err or return nil than just seeing return and trying to infer what's being returned.

According to Go style:

Naked returns are okay if the function is a handful of lines. Once it's a medium sized function, be explicit with your return values.

I'd say this is a medium sized function.

Also:

Corollary: it's not worth it to name result parameters just because it enables you to use naked returns. Clarity of docs is always more important than saving a line or two in your function.

@shurcooL thanks for your valuable comment, I've edited my code quirk

So, you removed u.Valid = false part. I thought it would be better to explicitly initialize all the fields and not rely on default initialization. I don't mind changing that if you want.

As for the comparison with "null", it injects json logic in the function and that should be the responsibility of the json package. Maybe the contents of the buffer is not "null" but " null " and that comparison would fail, but the json package will parse it correctly.

So, you removed u.Valid = false part. I thought it would be better to explicitly initialize all the fields and not rely on default initialization. I don't mind changing that if you want.

I think you need to explicitly initialize all fields, you cannot rely on default initialization. The value being unmarshaled into may be zero, or it may not be zero. Removing u.Valid = false would make it fail to operate correctly on &NullUUID{Valid: true} starting value.

@blastrock JSON decoder tokenizes input and it will pass "null" to UnmarshalJSON in any way.

If you look closer besides stylistic changes I removed intermediate UUID allocation by passing &u.UUID to json.Unmarshal.
Simple benchmark of original UnmarshalJSON and patched one:

BenchmarkUnmarshalJSON-4 2000000 879 ns/op 312 B/op 3 allocs/op

vs

BenchmarkUnmarshalJSON-4 2000000 770 ns/op 288 B/op 1 allocs/op

Benchmark itself:

func BenchmarkUnmarshalJSON(b *testing.B) { bytes := []byte("\"886313e1-3b8a-5372-9b90-0c9aee199e5d\"") u := NullUUID{} for i := 0; i < b.N; i++ { u.UnmarshalJSON(bytes) } }

Just for comparison UnmarshalText benchmark does 0 allocations and 10x faster:

BenchmarkUnmarshalText-4 20000000 88.9 ns/op 0 B/op 0 allocs/op

I agree with @shurcooL's comment about initializing all fields but it might be Scan method does it the most correct way by initializing UUID struct member to be Nil too.

@blastrock JSON decoder tokenizes input and it will pass "null" to UnmarshalJSON in any way.

Indeed, it seems so. But then u.UnmarshalJson(" null ") wouldn't work, but it may be acceptable. Just tell me your decision on that and I'll update the PR.

If you look closer besides stylistic changes I removed intermediate UUID allocation by passing &u.UUID to json.Unmarshal.

Right, I didn't notice that, and I don't see any other way to remove that allocation.

I agree with @shurcooL's comment about initializing all fields but it might be Scan method does it the most correct way by initializing UUID struct member to be Nil too.

Will do.

coveralls · 2017-03-30T11:40:40Z

Coverage decreased (-0.5%) to 96.029% when pulling d39caff on blastrock:json_marshalling into 5bf94b6 on satori:master.

dmitshur · 2017-03-30T18:02:51Z

uuid.go

+		return nil
+	}
+
+	u.Valid = true


Wouldn't it make more sense to do u.Valid = true after if err := json.Unmarshal(b, &u.UUID); err != nil { ... }? What's the reason you decided to put it on top, instead of below?

You are right, putting it after the if makes more sense.

coveralls · 2017-03-31T13:03:05Z

Coverage decreased (-0.5%) to 96.043% when pulling b99e53e on blastrock:json_marshalling into 5bf94b6 on satori:master.

coveralls · 2017-03-31T13:03:05Z

Coverage decreased (-0.5%) to 96.043% when pulling b99e53e on blastrock:json_marshalling into 5bf94b6 on satori:master.

prettymuchbryce · 2017-04-30T19:35:28Z

This seems like a good change. Any updates here?

RangelReale · 2017-11-15T21:58:23Z

I am having the same problem, I think this PR is good too.

prettymuchbryce · 2017-11-19T18:03:16Z

I have created this small package as a workaround for now. I hope this PR can be merged so I can use satori/go.uuid directly.

jpfluger · 2018-02-11T03:35:18Z

👍 The latest commits work for me in conjunction with go-pg. NullUUID has json marshalled using the json encoder, as appropriate. Here's the quick links to MarshallJSON and UnmarshallJSON.

rstrlcpy · 2018-05-03T11:26:31Z

Any updates? When this PR will be merged?

theckman · 2018-07-19T06:11:20Z

@blastrock Some of us in the Go community have forked this repository with the intent of maintaining it long-term. It seems like this proposed change could be a good addition to the new fork. Would you be interested in raising this change against our repo?

https://github.com/gofrs/uuid

theckman · 2018-07-19T06:31:32Z

uuid.go

+
+// UnmarshalJSON unmarshalls a NullUUID
+func (u *NullUUID) UnmarshalJSON(b []byte) error {
+	if bytes.Equal(b, []byte("null")) {


Should we mark this as an invalid UUID? It seems like sending null could be a very explicit way to do the same thing, without sending 16 NULL bytes.

I am not sure what you mean here, we must fill the NullUUID struct. Unless we change it to contain a pointer, we can't set u.UUID to nil, can we?

blastrock · 2018-07-21T19:18:33Z

@theckman thanks for forking and maintaining this package! I'll do that when I have some time. To tell the truth, we are not using this package anymore in our project. Of course, I wouldn't mind if you just take this code and put it in your fork, and I don't care about getting my name in the commit either ^^

This commit adapts satori/go.uuid#44 for our fork of the original project. This brings JSON Marshaling and Unmarshaling to the `uuid.NullUUID` type used for use with SQL databases. This is needed because `uuid.NullUUID` is a shim around `uuid.UUID`, providing the information needed by the `database/sql` package to support storing the value in a nullable column. Without these methods, the type gets converted unnecessarily to a different format than the standard `uuid.UUID` type. Signed-off-by: Tim Heckman <t@heckman.io>

theckman · 2018-07-23T00:43:41Z

@blastrock I took a swing adapting this PR for our fork, and was able to keep a commit attributing it to you. gofrs/uuid#38

This commit adapts satori/go.uuid#44 for our fork of the original project. This brings JSON Marshaling and Unmarshaling to the `uuid.NullUUID` type used for use with SQL databases. This is needed because `uuid.NullUUID` is a shim around `uuid.UUID`, providing the information needed by the `database/sql` package to support storing the value in a nullable column. Without these methods, the type gets converted unnecessarily to a different format than the standard `uuid.UUID` type. Signed-off-by: Tim Heckman <t@heckman.io>

blastrock · 2018-07-29T11:43:59Z

Great. I'm a bit late but I just reviewed it, seems good to me :)

Thank you!

satori reviewed Mar 22, 2017

View reviewed changes

dmitshur reviewed Mar 30, 2017

View reviewed changes

Add json (un)marshalling methods on NullUUID

b99e53e

acln0 mentioned this pull request Jul 19, 2018

Outstanding open issues / PRs in satori/go.uuid gofrs/uuid#32

Closed

theckman mentioned this pull request Jul 19, 2018

Add deprecation notice in favor of github.com/gofrs/uuid and archive this repo #84

Open

theckman reviewed Jul 19, 2018

View reviewed changes

theckman mentioned this pull request Jul 23, 2018

Add JSON (Un)marshaling methods on NullUUID type gofrs/uuid#38

Merged

blastrock closed this Jul 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add json (un)marshalling methods on NullUUID #44

Add json (un)marshalling methods on NullUUID #44

blastrock commented Mar 15, 2017

coveralls commented Mar 15, 2017 •

edited

Loading

satori commented Mar 16, 2017

blastrock commented Mar 16, 2017

dmitshur commented Mar 18, 2017 •

edited

Loading

blastrock commented Mar 18, 2017

dmitshur commented Mar 18, 2017 •

edited

Loading

satori Mar 22, 2017 •

edited

Loading

dmitshur Mar 22, 2017 •

edited

Loading

satori Mar 22, 2017

blastrock Mar 22, 2017

dmitshur Mar 22, 2017

satori Mar 22, 2017

blastrock Mar 23, 2017

coveralls commented Mar 30, 2017 •

edited

Loading

dmitshur Mar 30, 2017

blastrock Mar 31, 2017

coveralls commented Mar 31, 2017

coveralls commented Mar 31, 2017 •

edited

Loading

prettymuchbryce commented Apr 30, 2017

RangelReale commented Nov 15, 2017

prettymuchbryce commented Nov 19, 2017

jpfluger commented Feb 11, 2018

rstrlcpy commented May 3, 2018

theckman commented Jul 19, 2018

theckman Jul 19, 2018

blastrock Jul 21, 2018

blastrock commented Jul 21, 2018

theckman commented Jul 23, 2018

blastrock commented Jul 29, 2018

Add json (un)marshalling methods on NullUUID #44

Add json (un)marshalling methods on NullUUID #44

Conversation

blastrock commented Mar 15, 2017

coveralls commented Mar 15, 2017 • edited Loading

satori commented Mar 16, 2017

blastrock commented Mar 16, 2017

dmitshur commented Mar 18, 2017 • edited Loading

blastrock commented Mar 18, 2017

dmitshur commented Mar 18, 2017 • edited Loading

satori Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

dmitshur Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 31, 2017

coveralls commented Mar 31, 2017 • edited Loading

prettymuchbryce commented Apr 30, 2017

RangelReale commented Nov 15, 2017

prettymuchbryce commented Nov 19, 2017

jpfluger commented Feb 11, 2018

rstrlcpy commented May 3, 2018

theckman commented Jul 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blastrock commented Jul 21, 2018

theckman commented Jul 23, 2018

blastrock commented Jul 29, 2018

coveralls commented Mar 15, 2017 •

edited

Loading

dmitshur commented Mar 18, 2017 •

edited

Loading

dmitshur commented Mar 18, 2017 •

edited

Loading

satori Mar 22, 2017 •

edited

Loading

dmitshur Mar 22, 2017 •

edited

Loading

coveralls commented Mar 30, 2017 •

edited

Loading

coveralls commented Mar 31, 2017 •

edited

Loading