Add an option to ignore unknown enum cases when decoding from JSON (instead of failing)

[Cozmonat]

The issue was already described in the past but looks like no PR was uploaded: https://github.com/apple/swift-protobuf/issues/481#issuecomment-295800326

I know it is unwanted behaviour but in real life scenarios backend teams sometimes coming to the necessity of extending existing enums by adding new cases. This leads to backward incompatibility in the mobile clients using swift-protobuf generated models and when backend encodes enum cases with case names.

To avoid the issue, an option added to pass unknown enum cases and use first enum case as default case following the best practice in proto programming. Backend teams usually define first enum case as "unknown" or "initial" or any other with the intention that it will be used as a default case, or to indicate the problem.

[thomasvl]

This still feels sorta wrong to add a specific feature for this just to the Swift implementation. Normally to get better support for a proto definition to expand with time, I believe the binary form is suggested because of these sorta issues with JSON.

If you do feel strongly about this, It seems like it might be worth bring up the discussion on the main proto repo (or maybe better on their mailing list) as if the functionality is useful, it shouldn't be limited to just one language.

[tbkka]

If you can point to other protobuf implementations that support this, that would certainly help. As you've noticed, we're very conservative about adding features.

[Cozmonat]

I haven't found any other networking libraries which are "wrapping" SwiftProtobuf. I think the developers should be using Protobufs in conjunction with their own networking layer implementation.

I am agree with you and I hope I understand why it doesn't look right ideologically. But my reasons is practical. I have already the system working and clients are using REST via gRPC bridging to speak with the backend. Some clients can use Swagger, some other handle requests by manually parsing and building JSONs. In that case I can't force other teams to change their already well-working implementation so my decoding will not fail in my iOS client. I don't see real alternative but fix my implementation.

This is an option for those who can't force their backend to change existing encoding for REST clients.

[thomasvl]

Sorry, not following the wrapping comment. I believe we were both asking from the POV of other Protobuf implementations that support something like this. Hopefully the ones in Google's official distro.

[Cozmonat]

One of google/protobuf implementations contains already an option called 'ignore_unknown_enum_values' inside 'json_util.cc'.

Actual implementation is inside "src/google/protobuf/util/internal/datapiece.cc", function DataPiece::ToEnum:

 // If ignore_unknown_enum_values is true an unknown enum value is ignored.
      
if (ignore_unknown_enum_values) {
  *is_unknown_enum_value = true;
  return enum_type->enumvalue(0).number();
}

[thomasvl]

One of google/protobuf implementations contains already an option called 'ignore_unknown_enum_values' inside 'json_util.cc'.

That appears to be an internal detail for some of the rewriting support between json/binary formats, and isn't directly exposed, if you look at the exposed options, there doesn't seem to be anything: https://github.com/protocolbuffers/protobuf/blob/master/src/google/protobuf/util/json_util.h

[Cozmonat]

@thomasvl, I am not sure I understand your point.

There is an issue with current SwiftProtobuf implementation so it can't be used in production if your backend produces JSON with enum values encoded as names and not numbers.

[thomasvl]

My point is no other proto implementation seems to provide the option you are suggesting and we prefer to stay inline with the other implementation on abilities.

The Swift impl is to spec and in line with the other libraries. This is one of the downsides of using JSON (vs. binary) and it is usually handled by doing something like early in your communications having clients identify a "version" they are using and then the server ensure it only sends (and expects) JSON with the constructs known that that time.

The generally problem with the "ignore" options is they are lossy and there is no way for for clients to really know when things aren't known. The binary format has unknown fields support (and unknown enum value handing with proto3 syntax), so it is more robust and thus better handle these things.

[thomasvl]

ps - if you think there is general need/interest, please bring it up on https://github.com/protocolbuffers/protobuf or their discussion group (https://groups.google.com/forum/#!forum/protobuf) to see about getting something like this generally added to multiple languages. Having the broader feedback is the best way to help figure out if there are likely to be issues/downsides when adding support for something like this and then using it.

[tbkka]

Sorry for how long this took. Apparently, the C++ implementation has supported an equivalent feature for a while, but we missed it. If you could take a few minutes to clean this up a bit, I think we can go forward with it.

[Cozmonat]

Good news! This should help us a lot. @tbkka, I've updated the commit as you requested.

[thomasvl]

I think map decoding also comes thru here, so the value will be nil, and the pair won't end up in the map. We'll need to confirm that is correct vs. using the default value. (we can correct this on the map handling side since single field might want to say unset in this case).

[thomasvl]

we'll need to check map<*, Enum>, and when the single field is within a oneof.

[tbkka]

@thomasvl has asked for some additional test cases. Will you be able to implement those?

[tbkka]

If you'd like to continue pursuing this, please rebase against the new main branch and re-submit. Thank you!