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

feat: Add the api header to the GapicClientTrait #553

Merged
merged 1 commit into from
Apr 20, 2024

Conversation

Hectorhammett
Copy link
Contributor

This adds the API version header to the GapicClientTrait trait for the new api version control.

@@ -135,6 +135,71 @@ public function testHeadersOverwriteBehavior()
);
}

public function testVersionedHeadersOverwriteBehavion()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am just testing for the VersionedStubGapicClient as the expected behaviour is that if there is no Api version, it should not send the header, which is tested already by the other tests. We could add one specific test if we want to make sure that that is the case.

@Hectorhammett Hectorhammett changed the title Add the api header to the GapicClientTrait feat: Add the api header to the GapicClientTrait Apr 18, 2024
@Hectorhammett Hectorhammett marked this pull request as ready for review April 18, 2024 23:05
@Hectorhammett Hectorhammett requested review from a team as code owners April 18, 2024 23:05
@@ -629,6 +629,13 @@ private function createCallStack(array $callConstructionOptions)
'X-Goog-User-Project' => [$quotaProject]
];
}

if (isset($this->apiVersion)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also check !empty?

Copy link
Contributor

@bshaffer bshaffer Apr 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a reminder that isset will return false when $apiVersion is not defined, not initialized, and when it's equal to null. It will return true when $apiVersion is false or an empty string. !empty will return false in all of these cases.

The only valid difference I could see would be when the proto defines google.api.api_version as an empty string:

google.api.api_version = ""

In this case, would we want to include it as an empty header or skip it? If the former, then what we have here is correct. If the latter, then we'd want to use !empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm I can't think of a case where the apiVersion is either a null. It can be an empty string if you write an empty api version on the proto I guess?

If so, I still think there is value in sending the value itself even if is empty. Imagine someone forgets adding an API version and then they test the client. I think seeing the empty value on the header is more valuable than not sending it at all. But is just my train of thought.

Perhaps you have a specific case in mind? I am still not an expert on the generation side of things 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a reminder that isset will return false when $apiVersion is not defined, not initialized, and when it's equal to null. It will return true when $apiVersion is false or an empty string. !empty will return false in all of these cases.

The only valid difference I could see would be when the proto defines google.api.api_version as an empty string:

google.api.api_version = ""

In this case, would we want to skip it or include it as an empty header? If the former, then what we have here is correct. If the latter, then we'd want to use !empty.

I wrote an answer before you pressed "comment" haha.

So yeah in my previous comment, I added a point where I see value in sending an empty string. That empty string is not a correct version, but I think there is value in sending whatever is in there for visibility? Don't think that obfuscating that mistake would be helpful.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, I think isset in here works as well or better than empty 👍

@bshaffer bshaffer merged commit cc102eb into main Apr 20, 2024
13 checks passed
@bshaffer bshaffer deleted the api-versioning-header branch April 20, 2024 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants