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

Protobuf repeated uint, fixed and enum serialization not working #2401

Closed
Dogacel opened this issue Aug 5, 2023 · 5 comments
Closed

Protobuf repeated uint, fixed and enum serialization not working #2401

Dogacel opened this issue Aug 5, 2023 · 5 comments

Comments

@Dogacel
Copy link
Contributor

Dogacel commented Aug 5, 2023

Describe the bug

Any field in the following message is not being able to serialize:

syntax = "proto3";

package repeateds;

message RepeatedsMessage {
  repeated uint32 repeated_uint32 = 3;
  repeated uint64 repeated_uint64 = 4;
  repeated fixed32 repeated_fixed32 = 7;
  repeated fixed64 repeated_fixed64 = 8;
  repeated sfixed32 repeated_sfixed32 = 9;
  repeated sfixed64 repeated_sfixed64 = 10;

  repeated NestedEnum repeated_nested_enum = 18;
  repeated ForeignEnum repeated_foreign_enum = 19;

  enum NestedEnum {
    FOO = 0;
    BAR = 1;
    BAZ = 2;
    NEG = -1;  // Intentionally negative.
  }
}

enum ForeignEnum {
  FOREIGN_FOO = 0;
  FOREIGN_BAR = 1;
  FOREIGN_BAZ = 2;
}

I think the documentation has some conflicting into, according to my tests uints and sfixed's work fine.

I just added SIGNED to sfixed and it turned out OK. But when it comes to repeated values, I think just writing

  @ProtoNumber(number = 5)
  @ProtoPacked
  @ProtoType(type = SIGNED)
  public val repeatedSint32: List<Int> = emptyList(),

does not work. I haven't tested Map completely yet but I think there is gonna be a similar issue there too.

To Reproduce

As I am running conformance tests from my public project and it contains lots of lines, here is the link to tests, you can find every offender in tests as commented in source code.

Link to project tests

Expected behavior

All types should serialize / deserialize correctly.

Environment

  • Kotlin version: 1.9.0
  • Library version: 1.9.0
  • Kotlin platforms: JVM
  • Gradle version: 7
@sandwwraith
Copy link
Member

sandwwraith commented Aug 7, 2023

I guess it's because

@ProtoType(type = SIGNED)
public val repeatedSint32: List<Int> = emptyList()

makes @prototype applied to List and not Int. Correct way to use it would be val repeatedSint32: List<@ProtoType(SIGNED) Int> = emptyList(), but current annotations target make it impossible.

@Dogacel
Copy link
Contributor Author

Dogacel commented Aug 7, 2023

I guess it's because

@ProtoType(type = SIGNED)
public val repeatedSint32: List<Int> = emptyList()

makes @prototype applied to List and not Int. Correct way to use it would be val repeatedSint32: List<@ProtoType(SIGNED) Int> = emptyList(), but current annotations target make it impossible.

OK it makes sense, but it seems like that @ProtoType is just ignored when used against a List according to my tests.

For UintXX and Enum repeateds, I see this error for some reason.

Expected wire type 0, but found 2

I think sint is deserialized but incorrectly due to format (value = 2*value + 1).

Is there anything I can do to help? What is the preferred way to fix this? Maybe we can create a custom serializer for those types temporarily?

@sandwwraith
Copy link
Member

I'm not sure custom serializer would help, because it doesn't have a place to pass @ProtoType annotation. It should be fixed in the runtime somehow.

@Dogacel
Copy link
Contributor Author

Dogacel commented Aug 7, 2023

I'm not sure custom serializer would help, because it doesn't have a place to pass @ProtoType annotation. It should be fixed in the runtime somehow.

I don't have much context on how to resolve those issues but I am able to successfully reproduce them so I opened a PR here:

This PR should include a very extensive test suite that can be run. I hope solutions to the bugs can be written on top of this suite so we can verify them as they are fixed.

I also think having those conformance tests is a great way to track the completeness and maturity of the library. What are your thoughts @sandwwraith ? I am hoping that this is useful.

@Dogacel Dogacel closed this as not planned Won't fix, can't repro, duplicate, stale Aug 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants