Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use jsonpb for json unmarshalling and interface for oneof #13

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

GeertJohan
Copy link
Contributor

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
Copy link
Contributor Author

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

@awalterschulze
Copy link
Member

  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
Copy link
Member

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

@awalterschulze
Copy link
Member

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

@GeertJohan
Copy link
Contributor Author

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.

@awalterschulze
Copy link
Member

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

@GeertJohan
Copy link
Contributor Author

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
Copy link
Member

Excellent :)

@GeertJohan
Copy link
Contributor Author

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

@@ -685,6 +711,9 @@ func Builder(visited map[string]struct{}, root bool, fieldname string, help stri
s = append(s, `s += '<a href="#" class="del-child btn btn-danger btn-xs" role="button" fieldname="`+fieldname+`">Remove</a>'`)
s = append(s, `s += '</div><div class="col-sm-10">'`)
s = append(s, `s += '<label class="heading">`+fieldname+`</label>'`)
if len(msg.OneofDecl) > 0 {
s = append(s, `s += '<br/>This message has `+fmt.Sprintf("%d", len(msg.OneofDecl))+` oneof declarations.<br/>'`)
Copy link
Member

Choose a reason for hiding this comment

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

I guess this is just for debugging, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure, I wanted to include an explanation. But maybe it's better to put some time in the layout itself so the user can easily identify which oneof fields belong together.

@awalterschulze
Copy link
Member

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
Copy link
Contributor Author

Good idea, will do.

@GeertJohan
Copy link
Contributor Author

Added an example to /Label/Loop

@GeertJohan
Copy link
Contributor Author

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
Copy link
Member

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

@awalterschulze
Copy link
Member

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

@GeertJohan
Copy link
Contributor Author

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

@awalterschulze
Copy link
Member

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

@awalterschulze
Copy link
Member

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

@awalterschulze
Copy link
Member

fixed :)

@awalterschulze
Copy link
Member

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.

@GeertJohan
Copy link
Contributor Author

@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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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

@GeertJohan GeertJohan changed the title Use jsonpb for json unmarshalling Use jsonpb for json unmarshalling and interface for oneof Nov 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants