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

feature request: add a way to differentiate V1 and V2 clients #640

Closed
bshaffer opened this issue Jul 5, 2023 · 4 comments
Closed

feature request: add a way to differentiate V1 and V2 clients #640

bshaffer opened this issue Jul 5, 2023 · 4 comments
Labels
priority: p3 Desirable enhancement or fix. May not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@bshaffer
Copy link
Contributor

bshaffer commented Jul 5, 2023

We don't have a clean way to typehint V2 clients, or to tell a difference between the two. This would be handy simply for typehinting in GAX or other functions if we wanted to accept any client, or if we'd like to have conditional logic depending on the client type (see googleapis/gax-php#470 and googleapis/gax-php#450).

NOTE: With union types in PHP 8.0, typehinting any client may not be as important, but there would still be cases where it'd be nice to accept any API client.

Some ways I can see us doing this:

  1. Implement an interface on BaseClient or Client:
namespace Google\Cloud\Vision\Client;

use Google\Cloud\Vision\Client\BaseClient;
use Google\ApiCore\ClientInterface;

class ImageAnnotatorClient extends ImageAnnotatorBaseClient implements ClientInterface
{
}

The client interface could be empty:

namespace Google\ApiCore;
/**
 * @internal
 */
interface ClientIterface
{
}
  1. Add a private constant to all base clients
namespace Google\Cloud\Vision\Client\BaseClient;
class ImageAnnotatorClient extends ImageAnnotatorBaseClient implements ClientInterface
{
    private constant SURFACE_V2=true;
}

This would not be typehint-able, but it would be able to be usable in GapicClientTrait.

  1. Do nothing, use substr(__CLASS__, -10) === 'BaseClient' if we need to differentiate

This is the easiest solution, but also the hackiest / least reliable.

... Thoughts?

@bshaffer bshaffer added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. priority: p3 Desirable enhancement or fix. May not be included in next release. labels Jul 5, 2023
@noahdietz
Copy link
Collaborator

noahdietz commented Jul 5, 2023

Do nothing, use substr(CLASS, -10) === 'BaseClient' if we need to differentiate

What you have in googleapis/gax-php#470 for this solution seems fine to me.

My only questions are:

  1. Will we eventually remove the === 'BaseClient' check from gax? e.g. after we bump the gapic major versions and drop v1 surface
  2. If answer to the above is "yes", then reducing the amount of generated code (focusing on a gax-based solution, not options 1 or 2) seems like the exact right solution
  3. If answer to the above is "no", then we need to consider the cost of executing this check indefinitely, and would we rather have something more robust like an interface (solution 1), for long term use
  4. Assuming we keep future 1.x versions of GAX compliant with gapic SURFACE_V1 clients, then we will always need to have a differentiator like this until a 2.x of GAX is cut, right?

@bshaffer
Copy link
Contributor Author

bshaffer commented Jul 5, 2023

  1. Will we eventually remove the === 'BaseClient' check from gax?

Yes

  1. If answer to the above is "yes", then reducing the amount of generated code seems like the exact right solution

I agree with this only if we don't see value in the Interface option (or option 2) outside of this requirement

  1. Assuming we keep future 1.x versions of GAX compliant with gapic SURFACE_V1 clients, then we will always need to have a differentiator like this until a 2.x of GAX is cut, right?

Yes, as the currently published composer requirements for "google/gax": "^1.x" would always be able to pull it in for the old versions unless we cut a new release.

@noahdietz
Copy link
Collaborator

I agree with this only if we don't see value in the Interface option (or option 2) outside of this requirement

I think we can always add this later if we find a use case for it. I think it would perhaps be the better solution long term. Let's keep it in our back pocket?

If our only use case atm is for client info headers, I think the simple, gax-based class check will do the trick.

@bshaffer
Copy link
Contributor Author

bshaffer commented Jul 5, 2023

I agree, there's not really a request for it and we can always add it later. Closing this as the substr(__CLASS__, -10) === 'BaseClient' works for now!

@bshaffer bshaffer closed this as completed Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p3 Desirable enhancement or fix. May not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

No branches or pull requests

2 participants