Support nullable=false on a map to mean embedded map value #121

smarterclayton · 2015-11-26T05:28:42Z

Builds upon #119 and #120

Allow Go maps that use type aliases for their key or value.

Add a casttype test to casttype.proto

map[string]Message does not allow reflect.Value#Addr() on the reflect.Value retrieved by accessing the map at an index, because the map value is not accessable as a pointer. Create a copy of the value if CanAddr() is false before invoking proto.Text and jsonpb.Marshal. Required to support nullable=false map values

Instead of map<string,Message> becoming map[string]*Message, use nullable=false to indicate the resulting map should be map[string]Message, which can be used to avoid indirection in the maps.

awalterschulze · 2015-11-26T08:21:37Z

test/casttype/combos/both/casttype.proto

We could probably have two Wilson tests, one with nullable=false and one without?

awalterschulze · 2015-11-26T08:25:23Z

Almost there :)
This is really amazing work.
Thank you so much.

Unmarshaller should explicitly include that.

Add a test.

awalterschulze · 2015-11-27T07:35:53Z

gogoproto/doc.go

We should probably mention that this is not supported on message types?

Hrm - I can add a test, I thought it would be supported. Let me try that.

I forgot, this is hard to test with the structure of test cast type. It will work when the alias type is in the same package and points to the generated struct (we have an example of it in Kube). Will think on how to deliver a better test here - is a follow up fine?

Yes I think just say we don't support messages for castvalue.
If you can figure out how to support and test it, then we just update the doc at that point.

awalterschulze · 2015-11-27T07:44:54Z

I am only nit picking now, because everything looks correct and pretty much ready to be merged.

I also think I need to ask again, since this is a new pull request.
Are you ok with giving over your copyright on the code in pull request 121?

awalterschulze · 2015-11-27T07:45:21Z

Are you ready for me to merge it?

smarterclayton · 2015-11-28T22:05:28Z

I'll go through and address those comments - just thinking of the next person to come in here :) Yes, I am fine with transferring copyright for this pull request.

jsonpb doesn't have access to the original type of the cast map value from Proto, which means the generic struct reflection marshalling fails if the cast type is not proto.Message. Add a struct tag annotation that indicates the original message type so we can look it up in the proto registry.

smarterclayton · 2015-11-29T01:22:48Z

Fixed more edge cases and applied review feedback. See the jsonpb commit - I'm not sure this is the best solution, but without access to the proto.Properties map key and value data, it's a harder problem.

awalterschulze · 2015-11-30T08:27:21Z

plugin/stringer/stringer.go

We can just reuse the values again instead of assigning new ones.

awalterschulze · 2015-11-30T08:34:04Z

Maybe we can get to a point where we can merge before supporting castvalue of message types.
And then think about casttype and castvalue for message types, what do you think?

smarterclayton · 2015-11-30T16:32:04Z

Well, I believe support is complete for that - it's just whether the jsonpb
changes worry you or not.

On Mon, Nov 30, 2015 at 3:34 AM, Walter Schulze [email protected]
wrote:

Maybe we can get to a point where we can merge before supporting castvalue
of message types.
And then think about casttype and castvalue for message types, what do you
think?

—
Reply to this email directly or view it on GitHub
#121 (comment).

awalterschulze · 2015-11-30T18:30:21Z

Unfortunately they do worry me if they are there and not properly tested.
Lets rather drop the feature until it can properly be supported?

The other thing is that the tests are breaking for protoc 2.5 and 2.6.1, see the travis build.
I have made another comment about how to fix that.

smarterclayton · 2015-11-30T19:46:31Z

By not properly tested - I believe the castvalue test case exercises all the code paths (but I could be wrong). The problem of castvalue I believe is a general problem, not specific to nullable message vs non-nullable message.

The use case we had was for map[string]MessageAlias - so I need to solve castvalue of a message one way or another.

smarterclayton · 2015-11-30T20:58:13Z

The proto error was fixed - castvalue copied and pasted something that did not support maps.

awalterschulze · 2015-12-01T07:41:19Z

I think castvalue and casttype for message types are very hard.
I think we could merge everything without the changes made for castvalue message types and then try to tackle that separately, just because I think it is a hard problem.
If you want to continue in this pull request, you are welcome.
I just think splitting the work here will make this a little easier.

smarterclayton · 2015-12-03T05:35:19Z

I may be missing the areas that you're most concerned with. Is there generated code that combo doesn't touch that I may be missing?

Two more commits added that cover castvalue more thoroughly (with fixes)

awalterschulze · 2015-12-04T18:01:25Z

Sorry about the delay.
I was on holiday yesterday.
I'll get to this ASAP

awalterschulze · 2015-12-07T10:40:03Z

I can't remember, but I think you mentioned a problem with imported casttype messages?
I think the generated code will cause a circular dependancy. So its a easy case not to support even though messages that are in the same package actually do seem to work.

awalterschulze · 2015-12-07T11:20:38Z

plugin/equal/equal.go

This could just be a normal if and else?

Don't worry I fixed this.

Support nullable=false on a map to mean embedded map value

awalterschulze · 2015-12-07T11:31:56Z

Thank you very much smartest clayton.
This is some excellent work. I really appreciate your efforts.

smarterclayton added 8 commits November 23, 2015 22:13

Honor gogoproto.casttype when generating map field

362b202

Generated .proto and .pb.go for map casttype

b6fc28e

Add castkey and castvalue to support map casting

3074ef7

Allow Go maps that use type aliases for their key or value.

Support castkey and castvalue in plugins

90a5a8d

Add a casttype test to casttype.proto

Generated tests for castkey changes

d7adba1

Interpret nullable=false on a map mean an embedded map value

d493e5c

Instead of map<string,Message> becoming map[string]*Message, use nullable=false to indicate the resulting map should be map[string]Message, which can be used to avoid indirection in the maps.

Add generated test cases for a map with nullable value

3d18b66

awalterschulze reviewed Nov 26, 2015
View reviewed changes

smarterclayton added 6 commits November 26, 2015 12:50

Map casttype can hide use of castkey and castvalue

7264a6b

Unmarshaller should explicitly include that.

Review comments

f45f3f2

Review comments 2 - don't double cast populate

a1b2a13

Add a test.

Generated test output

5039eb2

Review comments 3 - incorrect sprintf flag

ae720f7

Generated tests for review comments 3

a299828

smarterclayton mentioned this pull request Nov 27, 2015

Set up generation of protobuf from our Go structs for use as a serialization format kubernetes/kubernetes#17854

Merged

9 tasks

awalterschulze reviewed Nov 27, 2015
View reviewed changes

smarterclayton added 4 commits November 28, 2015 20:14

Add castvalue test case for map[prim]Message and *Message

893806a

Handle edge cases related to casting map values

0a99494

Generated test types

f249c9b

awalterschulze reviewed Nov 30, 2015
View reviewed changes

plugin/stringer/stringer.go

Copy link

Member

awalterschulze Nov 30, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can just reuse the values again instead of assigning new ones.

castvalue requires proto 3.0.0

304d882

smarterclayton added 2 commits December 3, 2015 00:33

Expand tests on castvalue, fix issues

748cece

More generated test cases

c454bc3

awalterschulze reviewed Dec 7, 2015
View reviewed changes

awalterschulze added a commit that referenced this pull request Dec 7, 2015

Merge pull request #121 from smarterclayton/nullable_map_values

89243af

Support nullable=false on a map to mean embedded map value

awalterschulze merged commit 89243af into gogo:master Dec 7, 2015

tamird mentioned this pull request Dec 26, 2015

Add tightening and culling functions to gossip networks. cockroachdb/cockroach#3501

Merged

Support nullable=false on a map to mean embedded map value #121

Support nullable=false on a map to mean embedded map value #121

Uh oh!

Conversation

smarterclayton commented Nov 26, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awalterschulze commented Nov 26, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awalterschulze commented Nov 27, 2015

Uh oh!

awalterschulze commented Nov 27, 2015

Uh oh!

smarterclayton commented Nov 28, 2015

Uh oh!

smarterclayton commented Nov 29, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awalterschulze commented Nov 30, 2015

Uh oh!

smarterclayton commented Nov 30, 2015

Uh oh!

awalterschulze commented Nov 30, 2015

Uh oh!

smarterclayton commented Nov 30, 2015

Uh oh!

smarterclayton commented Nov 30, 2015

Uh oh!

awalterschulze commented Dec 1, 2015

Uh oh!

smarterclayton commented Dec 3, 2015

Uh oh!

awalterschulze commented Dec 4, 2015

Uh oh!

awalterschulze commented Dec 7, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awalterschulze commented Dec 7, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants