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

Skip null fields in ProtoBuf serialization #588

Closed
wants to merge 2 commits into from

Conversation

bezmax
Copy link

@bezmax bezmax commented Oct 30, 2019

Defaults to skipping null fields during Protobuf serialization. Resolves issue #71 .

The issue blocks any real usage of kotlinx.serialization, as there is no way to serialize an optional field as of right now. I suggest this as an easy and mostly-compatible fix until a long-term solution is devised.

The only real problem I can see with this approach is this:

data class Message(val a: String? = "foo bar")
...
val msgIn = Message(null)
val bytes = ProtoBuf.dump(Message.serializer(), msgIn )
val msgOut = ProtoBuf.load(Message.serializer(), bytes)
msgIn shouldBe msgOut //Fails, as it will become a specified default string.

@sandwwraith
Copy link
Member

There's also a bigger problem with primitive types:

data class Message(val a: Int? = 42)
...
val msgIn = Message(null)

If we drop a here, protobuf spec says: when a property of type int is missing, the parser should substitute 0, so a client reading our message won't get null. On the other hand, if someone omits an a value when it is 0, we'll incorrectly insert 42.

However, I completely agree with you that this is a pain point. Your solution works with non-primitive types when default values for properties are null, so it covers plenty of cases, but it is still a workaround.

So I suggest to move it under a configuration flag, like with JsonConfiguration.allowStructuredMapKeys. Since we don't have much configuration flags, separate ProtobufConfiguration seems unnecessary to me, an additional boolean parameter in Protobuf constructor should be okay. Also, it would useful to have documentation for that parameter which describes its shortcomings (basically, our examples where this setting produces an incorrect result).

Also, please re-open this PR against the dev branch.

@bezmax
Copy link
Author

bezmax commented Oct 31, 2019

@sandwwraith Yes, I know about that issue. Did not mention it as it was already there even before this suggested change. The suggestion actually makes it more consistent now in (incorrect) behaviour.

For example, right now you already can do this:

data class Message(val a: Int = 42) //Not even nullable
print(ProtoBuf.loads(Message.serializer(), "")) //Will be 42, while should have been 0

I see that there's a shouldWriteElement on TaggedEncoder, instead of adding a property I could just implement it correctly just for classes by overriding this method. In that case for scalar types you could still define your fields as val a: Int = 0 and be mostly proto compatible, and for classes it will allow nulls by default - which is also compatible. What do you think about this approach? I'll try to modify my PR for a concrete example of how this would look like.

@bezmax
Copy link
Author

bezmax commented Oct 31, 2019

Created a new PR against dev with a slightly different (and fully Proto compatible) approach: #591

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.

2 participants