letmegrpc icon indicating copy to clipboard operation
letmegrpc copied to clipboard

Use jsonpb for json unmarshalling and interface for oneof

Open GeertJohan opened this issue 9 years ago • 24 comments

I've found a way to get oneof working.

Fixes #10.

  1. Move to jsonpb (godoc)
  2. Use standard protobuf go generator instead of gogo, otherwise jsonpb won't detect the oneof fields. I'm not sure if using go_out instead of gogo_out breaks other things?? So far it seems to be working fine.
  3. Send only one field in the oneof object in json.

GeertJohan avatar Nov 16 '15 10:11 GeertJohan

PS @awalterschulze did you see the email I sent you?

GeertJohan avatar Nov 16 '15 10:11 GeertJohan

  1. Cool
  2. Maybe I should just update gogoprotobuf first, since there have been quite a few minor changes in golang/protobuf.
  3. Is it really just a one line change?

awalterschulze avatar Nov 16 '15 10:11 awalterschulze

Yes I received your email, I will be replying to it tonight :) Thank you very much for the reply.

awalterschulze avatar Nov 16 '15 10:11 awalterschulze

I will let you know once I have completed the update of gogoprotobuf

awalterschulze avatar Nov 16 '15 11:11 awalterschulze

update gogoprotobuf

Ok cool!

  1. Is it really just a one line change?

No, 3 is actually the largest part. The interface should reflect the fact that only one field can be set. The json creation should include only the selected field.

Currently when using a oneof definition like this

oneof foo {
  int64 a = 1;
  int64 b = 2;
}

And in the interface a is set to 42, then the following json is created by the javascript:

{"a": 42, "b": 0}

jsonpb doesn't accept that, it must be {"a": 42}.

So there are some modifications to the generated html/javascript that I'm currently working on. I'll add it to this PR when it's done. Together with your fix for 2 we'll have oneof support working.

GeertJohan avatar Nov 16 '15 12:11 GeertJohan

Ok now I understand this is only the first part of multiple pull requests, my bad.

awalterschulze avatar Nov 16 '15 12:11 awalterschulze

Yes I'll add the commit for html/javascript to this PR. Opened this as a work-in-progress PR so you could see what direction I'm going and give help/feedback.

GeertJohan avatar Nov 16 '15 12:11 GeertJohan

Excellent :)

awalterschulze avatar Nov 16 '15 13:11 awalterschulze

0dd2fe6 adds support for oneof in the generated go/javascript.

GeertJohan avatar Nov 16 '15 14:11 GeertJohan

Maybe you could edit https://github.com/gogo/letmegrpc/blob/master/letmetestserver/serve/serve.proto to include a oneof field so I could run it and get an idea of what it looks like?

awalterschulze avatar Nov 16 '15 14:11 awalterschulze

Good idea, will do.

GeertJohan avatar Nov 16 '15 15:11 GeertJohan

Added an example to /Label/Loop

GeertJohan avatar Nov 16 '15 15:11 GeertJohan

I've detected that this will only work with basic types, not yet with a oneof where one of the fields is a message. That will probably increase the complexity quite a bit.

GeertJohan avatar Nov 16 '15 15:11 GeertJohan

gogoprotobuf is updated again, but only up to https://github.com/golang/protobuf/commit/4a63085a886242d4e41ab91fbff2ef27775defba, since I have found some issues with this commit. It shouldn't have any effect on what you are doing.

awalterschulze avatar Nov 16 '15 15:11 awalterschulze

Can you run your version? Mine seems to generate empty *.letmegrpc.go files

awalterschulze avatar Nov 16 '15 15:11 awalterschulze

Yes, since I updated to the latest version of gogoprotobuf mine does too.. Maybe something changed upstream causing it to fail?

GeertJohan avatar Nov 16 '15 16:11 GeertJohan

It doesn't seem to have anything to do with your changes. I'll take a deeper look.

awalterschulze avatar Nov 16 '15 16:11 awalterschulze

I have an idea it has to do with the writeOutput optimization when I only use the GeneratePlugin method.

awalterschulze avatar Nov 16 '15 16:11 awalterschulze

fixed :)

awalterschulze avatar Nov 16 '15 16:11 awalterschulze

I finally took a look at your gui :) The graying looks very cool, I really like it, but I don't know how well it will work for multiple oneofs in the same message? Also as you mentioned it is unclear how well this will work with message fields.

I think there are quite a few ways to do oneof, but I don't know which will be the best.

  • A dropdown menu with which the user first chooses the field and then fills in the value
  • Graying fields where oneof fields are grouped together, maybe under a sub heading.
  • etc

I think the best way would make it very obvious to the user that there is one field to fill in. Maybe a usecase would help with a design.

awalterschulze avatar Nov 16 '15 16:11 awalterschulze

@awalterschulze I want to add zebra colors (odd/even) to oneof's belonging together. That should make it clear when there are multiple oneof groups. I think showing multiple fields is a better way to represent oneof, its closer to the underlying encoding. As scope is quite difficult to maintain in the generate javscript, I've made the oneof purely based on domnode classes. This means that supporting messages as a oneof field will be quite difficult, it wasn't part of my original plan to add oneof. Maybe note that as a missing feature?

GeertJohan avatar Nov 16 '15 18:11 GeertJohan

I really like the idea of zebra colors and showing the multiple fields does keep things easier :) If we don't add messages now I am afraid that it might not fit into this model and then we might need to redo everything :(

awalterschulze avatar Nov 17 '15 07:11 awalterschulze

Here is an example message.

message Form {
  string Name = 1;
  oneof Origin {
    State State = 2;
    Country OtherCountry = 3;  
  }
  string Lastname = 4;
  oneof Contact {
    string email = 5;
    string telephone = 6;
    string cellphone = 7;
  }
}

enum State {
    Alabama = 1;
    ...
}

message Country {
  CountryCode CountryCode = 1;
  string Province = 2;
}

enum CountryCode {
  ...
}

awalterschulze avatar Nov 17 '15 07:11 awalterschulze

I got the idea from a colleague, but maybe we could use tabs Please imagine the pipes (|) are aligned, * is active, [] is a button and ___ is a text box

Form
====

Name: ______

                         |   Origin
-----------------------------------------------------
State                |
---------------------  [Set Country]
*OtherCountry* |
---------------------

Lastname: ________________

                         |   Contact
-----------------------------------------------------
Email                |
---------------------  
*Telephone*     |  _______________
---------------------
Cellphone        |
---------------------

awalterschulze avatar Nov 17 '15 07:11 awalterschulze