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

Enums in proto files fail. #611

Closed
maurogonzalez opened this issue May 8, 2019 · 9 comments
Closed

Enums in proto files fail. #611

maurogonzalez opened this issue May 8, 2019 · 9 comments
Labels
bug Something isn't working

Comments

@maurogonzalez
Copy link

maurogonzalez commented May 8, 2019

Testing enum types in proto messages I got the following errors:

https://github.com/maurogonzalez/rpctest/tree/enums

Using protocol.proto
And srcGen to generate scala code.

  • grpc client returns:
ERROR:
  Code: Internal
  Message: grpc: failed to unmarshal the received message type_enum field HelloResponse.message is not compatible with nil value
  • mu client logs:
// ClientApp
 ...
client.sayHello(Test.HELLO)

// Server Log: INFO GreeterServiceHandler - Request: HelloRequest(GOODBYE)
// Client Log: INFO GreeterServiceClient - Request: HELLO - Result: HelloResponse(GOODBYE)
  • Not sure if it is related with Scala objects and Java's io.grpc.
@toddburnside
Copy link

@maurogonzalez, could you clarify where you are seeing this error? It looks like the mu client is succeeding. I also successfully tested the mu client with your code.

Are you seeing the error in your go client? Or elsewhere?

@maurogonzalez
Copy link
Author

@toddburnside Sorry for the late response. I didn't update the README but to get the behavior:

Current behavior:

@toddburnside
Copy link

@maurogonzalez OK, I see what you mean, now. It appears that no matter what the client sends to the server, the server sees GOODBYE. Not very friendly behavior on the part of the server. 😬

@toddburnside
Copy link

It turns out the server is also seeing the first enum in alphabetic order. So, if you have enum values of ACK, CHECK and 'QUACK, the server will always see ACK` no matter which order they are defined in the enum. I'll keep looking into this.

@maurogonzalez
Copy link
Author

I haven't had much time to investigate deeply, sorry.

Thanks @toddburnside !

@juanpedromoreno juanpedromoreno added the bug Something isn't working label Jun 25, 2019
@toddburnside
Copy link

toddburnside commented Jun 26, 2019

I did some more investigating. It turns out PBDirect doesn't work properly with the code generated for the enums.

Given this definition in the .proto file:

enum Test {
  LATER = 0;
  HELLO = 1;
  GOODBYE = 2;
  HI = 5;
}

The following scala code is generated:

sealed trait Test
object Test {
  case object LATER extends Test
  case object HELLO extends Test
  case object GOODBYE extends Test
  case object HI extends Test
}

Although PBDirect has code for handling coproducts via shapeless, and doesn't complain about this encoding, it appears to always send an empty field over the wire for them. I'm uncertain why this results in them being deserialized into the cast object with the smallest value in alphabetic order (GOODBYE in this case), but that is what is happening.

Looking at the tests for enums in PBDirect here, it appears that PBDirect uses some extra traits when encoding protobuf enums. If I change the scala code to the following, everything works as expected.

sealed trait Test extends Pos
object Test {
  case object LATER extends Test with Pos._0
  case object HELLO extends Test with Pos._1
  case object GOODBYE extends Test with Pos._2
  case object HI extends Test with Pos._5
}

I think that extending Pos, Pos._0, etc. is required in order to support the fact that protobuf assigns integral values to enum members, and that those numbers do not need to be contiguous. So, it seems like in order to allow the use of enums with protobuf, our generated code will need to include the above.

Anyone have any thoughts on this?

@toddburnside
Copy link

higherkindness/skeuomorph#100 was created to continue discussion on this issue.

@dzanot
Copy link
Contributor

dzanot commented Jul 16, 2019

It's worth noting that #606 depends on us being able to pick the "Zero" element for enum defaults. Per the protobuf docs:

every enum definition must contain a constant that maps to zero as its first element. This is because:

There must be a zero value, so that we can use 0 as a numeric default value.

@cb372
Copy link
Member

cb372 commented Jan 17, 2020

This issue should be fixed in the latest version (v0.20.1) of mu-scala. We've largely rewritten the protobuf serialization library and fixed a lot of bugs, as well as improving support for protobuf enums. The mu-scala codegen will now encode enums in Scala as enumeratum IntEnumEntrys, meaning the serialization library can serialize them to protobuf with the correct ordinal.

If you have any more problems with enums, please open a new issue.

@cb372 cb372 closed this as completed Jan 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants