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

Protobuf Upgrade to 25.2 has broken V1 GAPIC Generator #683

Open
bshaffer opened this issue Feb 2, 2024 · 0 comments
Open

Protobuf Upgrade to 25.2 has broken V1 GAPIC Generator #683

bshaffer opened this issue Feb 2, 2024 · 0 comments
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@bshaffer
Copy link
Contributor

bshaffer commented Feb 2, 2024

cl/603226138 has broken the GAPIC generation in the V1 clients (see googleapis/google-cloud-php#7023). Some of the changes that look wrong are:

  • All RPC method arguments have been removed
  • All sample arguments have been removed
  • All setters used in tests have been removed
  • References to resource name helpers have been removed in comments

This may be due to changes in the way the GAPIC generator locates google.api.field_behavior, which is done in FieldDetails::determineIsRequired, and is read by ProtoHelpers::getCustomOptionRaw:

$values = [];
if ($message->hasOptions()) {
$opts = $message->getOptions();
$unknown = $messageUnknown->getValue($opts);
if ($unknown) {
$unknownStream = new CodedInputStream($unknown);
// Read through the stream of all custom options, looking for
// the requested option-id. If it's repeated, then all options
// must be parsed, otherwise return the first found.
while (($tag = $unknownStream->readTag()) !== 0) {
$value = 0;
// TODO: Handle extra option types as required.
switch (GPBWire::getTagWireType($tag)) {
case GPBWire::WIRETYPE_VARINT:
$unknownStream->readVarint32($value);
break;
case GPBWire::WIRETYPE_LENGTH_DELIMITED:
$len = 0;
$unknownStream->readVarintSizeAsInt($len);
$unknownStream->readRaw($len, $value);
break;
default:
throw new \Exception('Cannot read option tag');
}
if (GPBWire::getTagFieldNumber($tag) === $optionId) {
if ($repeated) {
$values[] = $value;
} else {
return $value;
}
}
}
}
}

Note: There may be a way to get options from UPB. However, the APIs would need to be implemented for both the pure-PHP and upb backends. The upb library itself supports it, but there is currently no glue.

UPDATE: We've been able to duplicate the error (which seems to be affecting all "repeated" custom options) by updating the protoc binary to 25.2 and running the following test:

vendor/bin/phpunit tests/Unit/Utils/ --filter testProtoCustomOptions

UPDATE 2 - If we add [packed = false] to the custom option, the previous custom option logic works as expected.

@bshaffer bshaffer added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p2 Moderately-important priority. Fix may not be included in next release. labels Feb 2, 2024
@bshaffer bshaffer changed the title Protobuf Upgrade has broken V1 GAPIC Generator Protobuf Upgrade to 25.2 has broken V1 GAPIC Generator Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

No branches or pull requests

1 participant