gogo / letmegrpc

[maintainer wanted] generates a web form gui from a grpc specification
BSD 3-Clause "New" or "Revised" License
421 stars 48 forks source link

Use jsonpb for json unmarshalling and interface for oneof #13

Open GeertJohan opened 9 years ago

GeertJohan commented 9 years ago

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 commented 9 years ago

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

awalterschulze commented 9 years ago

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 commented 9 years ago

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

awalterschulze commented 9 years ago

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

GeertJohan commented 9 years ago

update gogoprotobuf

Ok cool!

3) 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.

awalterschulze commented 9 years ago

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

GeertJohan commented 9 years ago

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.

awalterschulze commented 9 years ago

Excellent :)

GeertJohan commented 9 years ago

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

awalterschulze commented 9 years ago

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?

GeertJohan commented 9 years ago

Good idea, will do.

GeertJohan commented 9 years ago

Added an example to /Label/Loop

GeertJohan commented 9 years ago

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.

awalterschulze commented 9 years ago

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 commented 9 years ago

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

GeertJohan commented 9 years ago

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

awalterschulze commented 9 years ago

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

awalterschulze commented 9 years ago

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

awalterschulze commented 9 years ago

fixed :)

awalterschulze commented 9 years ago

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.

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.

GeertJohan commented 9 years ago

@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?

awalterschulze commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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        |
---------------------