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

Implemented encoding null in key and value of a map in Protobuf #2910

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

shanshin
Copy link
Contributor

Also improved null encoding error messages.

Resolves #2760

Also improved `null` encoding error messages.

Resolves #2760
@shanshin shanshin requested a review from sandwwraith January 23, 2025 13:37
@shanshin
Copy link
Contributor Author

The open question is whether we want to allow encoding null in the key.

This change may also make the encoded messages incompatible with some decoders created using protoc.

@sandwwraith
Copy link
Member

The open question is whether we want to allow encoding null in the key.

How will it look like? A value without a key? In that case, we possibly can decode it automatically into a null: value map pair

@@ -37,7 +38,8 @@ internal abstract class ProtobufTaggedEncoder : ProtobufTaggedBase(), Encoder, C
if (nullableMode != NullableMode.ACCEPTABLE) {
val message = when (nullableMode) {
NullableMode.OPTIONAL -> "'null' is not supported for optional properties in ProtoBuf"
NullableMode.COLLECTION -> "'null' is not supported for collection types in ProtoBuf"
NullableMode.COLLECTION -> "'null' is not supported as the value of collection types in ProtoBuf"
NullableMode.LIST_ELEMENT -> "'null' is not supported as the value of a list element in ProtoBuf"
Copy link
Member

Choose a reason for hiding this comment

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

Is LIST_ELEMENT added for the purpose of proper error message only? I do not see this string being tested anywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, only the accepted or not flag is sufficient to throw an error. The enumeration is created only for printong different messages

Copy link
Member

Choose a reason for hiding this comment

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

Then add a test for this message pls

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests added

@shanshin
Copy link
Contributor Author

shanshin commented Jan 23, 2025

How will it look like? A value without a key?

Each map declaration in protobuf structually is repeated field with Entry message

message NullableMap {
    map<string, int32> m = 1;
}

is the same as

message NullableMap {
    message m_Entry {
        optional string key = 1;
        optional int32 value = 2;
    }

    repeated m_Entry m = 1;
}

see the protobuf doc

Therefore, we can skip either the key or the value.

In that case, we possibly can decode it automatically into a null: value map pair

It's already implemented in this PR, see encodedKeyNull in testNullMap.
Question: should we leave this opportunity

@sandwwraith
Copy link
Member

I think it is OK to have it if the message itself is not malformed

@shanshin shanshin requested a review from sandwwraith January 23, 2025 17:50
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