[PHP] Adding a new enum value results in a breaking change?

[fiboknacky]

What version of protobuf and what language are you using?
Version: 3.x
Language: PHP

What operating system (Linux, Windows, ...) and version?
Probably all OS. I tested on Linux

What did you do?
After releasing a new version of the Google Ads API (v16_1), the Google Ads API PHP library is updated to accommodate this change.
In the release, we added one more new enum value to CriterionType.
But when a user uses that enum class to parse a new value (LIFE_EVENT), it failed with Enum Google\Ads\GoogleAds\V16\Enums\CriterionTypeEnum\CriterionType has no name defined for value 41

What did you expect to see
Based on this doc , it should just parses the value without any issues.
I'd expect it to represent a new value with UNKNOWN and all methods in that enum class should just work.

What did you see instead?
it failed with Enum Google\Ads\GoogleAds\V16\Enums\CriterionTypeEnum\CriterionType has no name defined for value 41 when calling name().

[bshaffer]

I believe the problem is that the user hasn't updated their library to receive the new enums. This is a problem we have seen happen when APIs add an enum value - the value can be sent before the client library of the user is updated to understand the new value. When that happens, errors like this can arise.

The way to fix this error in the short term is for the user to update to the latest version of the client library. The way to prevent this in the long term is to surround each call to name or value with a try/catch for UnexpectedValueException and gracefully handle unknown values.

[fiboknacky]

Protobuf libs are supposed to be tolerant to this and treat new values as UNKNOWN as also described here, no?
So, I wonder if it's just the name and value functions that are not conformant here?

[bshaffer]

@fiboknacky that's correct - protobuf and the client libraries are tolerant when they receive unknown enum values, but name and value are convenience methods which are never called internally. So to make them tolerant, the customer must define how to handle unknown values. For example:

try {
    $name = CriterionType::name($value); // throws exception when new values are added
} catch (UnexpectedValueException $e) {
    // If the value is unknown, set values to some enum default (in this case, "UNKNOWN")
    $name = 'UNKNOWN';
    $value = 1; 
}

It would be possible in Protobuf to have name and value return null instead of throwing exceptions when the enum doesn't exist. But this would be a breaking change at this point.

[fiboknacky]

I think it should return UNKNOWN or something, but anyway, I wonder if this is a breaking change? Sounds like a bug fix to the correct behavior. Anyway, if we're going to really release this change, do you mean the major version of the protobuf should be bumped?

[bshaffer]

if we're going to really release this change, do you mean the major version of the protobuf should be bumped?

I think there's a misunderstanding. There is no breaking change, the methods name and value are just not FORWARDS-compatible with new enum values unless surrounded by a try/catch. I don't know what you mean, there's no plan to bump protobuf, and AFAIK, no breaking change being discussed.

We can't return UNKNOWN because that enum value may not exist - UNKNOWN is defined in the enum itself. In the case of this enum, it's 1, but in others it could be 0, or not exist at all.

We could have done something like return null instead of a value, but at this point that WOULD be a breaking change, so I don't see that happening.

[fiboknacky]

We can't return UNKNOWN because that enum value may not exist - UNKNOWN is defined in the enum itself. In the case of this enum, it's 1, but in others it could be 0, or not exist at all.

I may misunderstood the spec (some are defaults to Ads protos only), but let's get back to a basic question (putting the name and value methods aside for now): If a new unknown enum value is parsed, what will happen?
Will it always be decoded to 0? I ask because the spec for this say that PHP is conformant, so it should at least decode the value to something deterministically, right?

[bshaffer]

for PHP enums are OPEN. The unknown value is saved in the protobuf field even if it's unknown. There is no breaking change for new enums.

[fiboknacky]

Thanks. That reminds me that PHP doesn't have a real support for enum (at least for the currently supported PHP versions that we use), so it can be open like you said. The generated enum classes are just a convenient class representing the proto enums.

We could have done something like return null instead of a value, but at this point that WOULD be a breaking change, so I don't see that happening.

That's what I meant to ask about "breaking change" in this comment. Sorry that my misunderstanding made it more confusing by mentioning things about releasing new versions of protobuf.

Let's say if we want to make a change for name and value. How would this be possible (at least for Ads)?
We could just change this in the GAPIC generator without disturbing other products, correct?
From what I see, it's like when we roll out GAPIC v2. We just need to do a good communication with our users, and bump the major version of the Ads lib.

It's not a high priority now, but I wish to understand the situation first, so we can prioritize it in the future. Thanks!

[bshaffer]

We could just change this in the GAPIC generator without disturbing other products, correct?

name and value are generated by the protobuf library, not by the GAPIC library. Right now, there isn't really a way to extend protobuf message classes. Ideally, behavioral changes should be made to the PHP protobuf generator, as long as we could make them in a non-breaking way.

One way this could potentially be done in GAPIC is to overwrite/modify the generated protobuf classes. This is generally frowned upon, however.

What changes would you want to make?

[fiboknacky]

I see. Personally, I think name shouldn't throw an exception since it goes against the spec. It should just have gracefully returned something.
Since the AIP standard says the first value should be either XXX_UNSPECIFIED or UNKNOWN, I think it should just have returned the string representation of value 0.
Likewise, for value, if the unknown name is passed, it could just return 0 or null like you mentioned.
But I understand that would be debatable.

One way this could potentially be done in GAPIC is to overwrite/modify the generated protobuf classes. This is generally frowned upon, however.

For Ads, this would be much more predictable, since we have an Ads extended spec to always have two first values as UNSPECIFIED and UNKNOWN.
So, we can be sure that 0 represents UNSPECIFIED in Ads.

[bshaffer]

@fiboknacky just a reminder that this library doesn't go against any spec because protobuf is agnostic of any API. It does not follow AIPs (which are specific to Google API standards). It can't return the value 0 because it's possible that 0 doesn't exist in the enum. Likewise for UNSPECIFIED or UNKNOWN.

As for overwriting/modifying, again that would happen outside this library, so I can't really speak to that. If you want to write up a design or file a big against https://github.com/googleapis/gapic-generator-php, we can discuss it there. I think for now we can close this issue here. Do you agree?

[fiboknacky]

@fiboknacky just a reminder that this library doesn't go against any spec because protobuf is agnostic of any API. It does not follow AIPs (which are specific to Google API standards). It can't return the value 0 because it's possible that 0 doesn't exist in the enum. Likewise for UNSPECIFIED or UNKNOWN.

I didn't know that it doesn't follow AIPs. Thanks for the information.

I think for now we can close this issue here. Do you agree?

Yes, I think this is more complicated than I thought and it's probably not about the protobuf spec itself. We should close this for now. Thanks.