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

Upgrading to v1.11.2 breaks the unit test of our (lonely) streaming method #363

Closed
PierrickVoulet opened this issue Jan 21, 2022 · 7 comments · Fixed by #364
Closed

Upgrading to v1.11.2 breaks the unit test of our (lonely) streaming method #363

PierrickVoulet opened this issue Jan 21, 2022 · 7 comments · Fixed by #364
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

@PierrickVoulet
Copy link
Contributor

PierrickVoulet commented Jan 21, 2022

Environment details

  • OS: Linux 64
  • PHP version: 8.0.1
  • Package name and version: gax-php, 1.11.2

Steps to reproduce

  1. Checkout the source code of google-ads-php.
  2. Change the version of the dependency gax-php to 1.10.0 in the composer.json
  3. Run composer update
  4. Run the PHPUnit test that was generated based on google-ads protos Google\\Ads\\GoogleAds\\V9\\Services\\GoogleAdsServiceClientTest::searchStreamExceptionTest.
  5. It passes
  6. Change the version of the dependency gax-php to 1.11.2 in the composer.json
  7. Run composer update
  8. Run the PHPUnit test again.
  9. It fails with the following error
TypeError : Google\Protobuf\Internal\RepeatedField::offsetExists(): Argument #1 ($index) must be of type int, string given
 /usr/local/google/home/pierrick/git/google-ads-php/src/Google/Ads/GoogleAds/Lib/V9/GoogleAdsExceptionTrait.php:47
 /usr/local/google/home/pierrick/git/google-ads-php/src/Google/Ads/GoogleAds/Lib/V9/ServerStreamingGoogleAdsExceptionMiddleware.php:96
 /usr/local/google/home/pierrick/git/google-ads-php/src/Google/Ads/GoogleAds/Lib/V9/ServerStreamingGoogleAdsResponseMetadataCallable.php:63
 /usr/local/google/home/pierrick/git/google-ads-php/tests/Google/Ads/GoogleAds/V9/Services/GoogleAdsServiceClientTest.php:290

In this particular case an ApiException is thrown with the metadata field typed as RepeatedField with 1.11.2 which was not the case before (I believe it was a keyed array in the version 1.10.0).

@noahdietz noahdietz added 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. labels Jan 21, 2022
@PierrickVoulet
Copy link
Contributor Author

PierrickVoulet commented Jan 21, 2022

FYI @fiboknacky @aohren

@PierrickVoulet
Copy link
Contributor Author

PierrickVoulet commented Jan 21, 2022

Not sure if that helps but debugging the unit test that fail, I found that the ApiException instance is not created in the same way depending on the version of gax-php being used.

1.10.0: It uses the method createFromStdClass and the metadata is NULL.

image

1.11.2: It uses the method createFromRpcStatus and the metadata is a RepeatedField got from status.

image

@noahdietz
Copy link
Contributor

noahdietz commented Jan 21, 2022

That definitely helps! That looks like the root cause.

@noahdietz
Copy link
Contributor

Actually, in looking at the create documentation, the $metadata may either be a PHP array OR a RepeatedField. So the fact that this is a RepeatedField is allowed.

@noahdietz
Copy link
Contributor

However, the documentation for ApiException constructor indicates $metadata should be an array.

hmm.

@noahdietz
Copy link
Contributor

Ok so we are thinking that the this is a bug in gax-php that has kind of been lurking.

ApiException::create is documented as accepting []mixed|RepeatedField for $metadata. That $metadata value is passed to ApiException::_construct untouched. ApiException::_construct documents that it expects $metadata to be an array, and ApiException::getMetadata documents the return type i.e. $metadata is []mixed.

So the bug is that ApiException::create needs to convert $metadata that's instanceof RepeatedField into an array prior to constructing the ApiException.

The problem was introduced because ServerStream::readAll was changed to construct an exception via ApiException::createFromRpcStatus whereas it was ApiException::createFromStdClass (a gRPC specific type) before. The former (the code recently released) supplies the Google/Rpc/Status::getDetails() directly to ApiException::create. Since it isn't "massaged" to the array type as I mentioned earlier, this whole issue popped up.

@bshaffer

@noahdietz
Copy link
Contributor

Ok we've got a fix planned. It will come early next week. Sorry about the breakage, please continue to use 1.10 for now.

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

Successfully merging a pull request may close this issue.

2 participants