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

Fix some issues with API version handling in new getClient #2738

Merged
merged 7 commits into from
Aug 14, 2024

Conversation

timovv
Copy link
Member

@timovv timovv commented Aug 8, 2024

A couple of bug fixes as a follow up to #2728:

  • For API version endpoint parameters, if TCGC does not provide a default API version (e.g. due to TypeSpec versioning not being used), respect the default value provided in the @server decorator, like we do for other params. This fixes the "parametrizedHost" smoke test's apiVersion param not having a default value set.
  • Destructure the options to remove the apiVersion option when it is present before passing the options to getClient. This prevents the Core policy from picking it up.

Copy link
Member

@qiaozha qiaozha left a comment

Choose a reason for hiding this comment

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

If the purpose of the following change

- const updatedOptions = {
+ const { apiVersion: _, ...updatedOptions } = {

is just remove apiVersion from the option, we should always do that, otherwise, for cases like this one apiVersion is required parameter

or even we don't have apiVersion in the options, it will get assigned to updatedOptions and passed into core-client-rest

@timovv
Copy link
Member Author

timovv commented Aug 9, 2024

If apiVersion is a required parameter or not present in the options, why would the user ever pass apiVersion in a way that requires it to be removed? If they do, should we really be handling that?

Copy link
Member

@joheredi joheredi left a comment

Choose a reason for hiding this comment

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

I think the changes look good. I don't think there are problems when apiVersion is required. If user ends up passing it also in the options, it is good they get the warning from core.

@qiaozha
Copy link
Member

qiaozha commented Aug 12, 2024

I think the changes look good. I don't think there are problems when apiVersion is required. If user ends up passing it also in the options, it is good they get the warning from core.

We will only get warning from core after Azure/azure-sdk-for-js#29904 is merged and we bump the core version, otherwise the current logic will just append a api-version query parameter as the ApiVersionPolicy is in core no matter if the client side has an api version or not. But, in order to get Azure/azure-sdk-for-js#29904 merged, we need codegen to be correctly and explicitly not pass the api version to core if it's not supported and refresh all the existing RLCs as suggested by Azure/azure-sdk-for-js#29883 (comment) step 3, the order is tricky, but I feel like we have to remove it until @xirzec 's PR get merged 😞

@timovv timovv force-pushed the fix-some-api-version-things branch from ff6b4b8 to d9a97e8 Compare August 14, 2024 00:04
@timovv
Copy link
Member Author

timovv commented Aug 14, 2024

Updated to always destructure the API version, regardless of whether the apiVersion parameter is required, optional, or present at all

@timovv timovv force-pushed the fix-some-api-version-things branch from 5bc89ee to 451a9cc Compare August 14, 2024 19:41
@timovv timovv merged commit a77ad85 into Azure:main Aug 14, 2024
15 checks passed
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