[7392] [cpp] Fix IgnoreUnknownEnumStringValueInMap conformance tests

[antongrbin]

Motivation

This PR fixes failing JSON conformance tests for cpp with name IgnoreUnknownEnumStringValueInMap. Specifically, for the following input of type TestMapOfEnums:

{
  "enum_map": {
    "key1": "PROTOCOL",
    "key2": "UNKNOWN_ENUM_STRING_VALUE",
    "key3": "BUFFER"
  }
}

The JSON parser with ignore_unknown_fields=true should produce a map with 2 entries, skipping the unknown enum value under key2. The implementation before this PR outputs key2 with 0-th enum value (PROTOCOL) instead.

The JSON parsing spec was discussed in #7392.

Recent similar changes in other languages:

• Python: 86abf35
• Swift: Use JSONDecodingOptions.ignoreUnknownFields to ignore unknown enums. apple/swift-protobuf#1345
• C#: C#: Ignore invalid enum string values in JSON, when invalid fields are being ignored. #15758

Changes

The main difficulty in implementing the fix is the fact that parser will first allocate a new map entry message and then consume the value. In this special case (unknown enum string value), we need to do it the other way around: first consume the value, then allocate the map entry if needed.

The PR initially contains 5 commits, with first 4 being noop unit tests and refactors:

1. unit test that documents the mainline behavior (this test will be changed in the 5th commit)
2. noop refactoring that simplifies the signature of ParseEnumFromStr
3. noop refactoring that extract map key parsing into a separate function (we'll call it twice in 5th commit)
4. noop refactoring that extracts parse map entry into a separate function (since it will get more logic)
5. the commit that actually implements the fix and adjusts tests

[antongrbin]

@haberman friendly ping to take a look :)

[antongrbin]

@sbenzaquen can you help me ping @haberman through internal channels :) I bet this is a GitHub notifications recall miss.

I am reaching out to you since you recently reviewed #16567

[antongrbin]

@jskeet, since you worked with me on the linked issue #7392, can you help me get reviewer's attention here through internal channels? I suspect that they are not seeing GitHub notifications.

I am not blocked on this, just wanted to eventually remove the failing tests that I added.

[jskeet]

I've pinged internally.

[googleberg]

@esrauchg Can you take a look at this?

[esrauchg]

Sorry @antongrbin for the delays in getting someone to look at this! I can help review/push this forward here from the protobuf team.

[Edit: removed questions that I realized weren't relevant after looking closer]

[esrauchg]

Thanks for the contribution! This is a great start, just one structuring comment and a few nits.

[esrauchg]

Nit: I think slightly cleaner to just use the WithFieldType status like this:

absl::optional<int32_t>> enum_value;

RETURN_IF_ERROR(Traits::WithFieldType(
    map_field, [&lex, &enum_value](const Desc<Traits>& map_entry_desc) {
  ASSIGN_OR_RETURN(enum_value, ParseEnum<Traits>(lex, Traits::ValueField(map_entry_desc));
  return absl::OkStatus();
}

[esrauchg]

Nit: this should just be *enum_value (or enum_value->value()), you already checked that has_value is true above so it will be guaranteed to succeed.

It's academic in this case because the 0 value would never be used, but under proto2 semantics (where enums are closed, which is the case you are handling here) the default value for an enum is whatever is listed first in the definition, which isn't necessarily 0 value for the enum; there could either be a 0 value that is not listed first (then 0 is a legal value but not the default) or no listed 0 value at all (then its not a legal value at all).

Only under proto3 / open enums is it mandatory that the enum has a 0 value and that it is listed first (exactly to allow every enum to have a 0 default in proto3).

[esrauchg]

Can we structure this to be less drastically special cased? I'd rather it 'infect' the happy case if that avoids such a large diversion to handle the edge case.

Is it possible to structure this to always parse map key, parse map value, decide whether to err or ok if the value can't be parsed due to it being unknown enum, then construct the message?