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

[text analytics] regen TA with GA autorest #14215

Merged
merged 5 commits into from
Oct 2, 2020

Conversation

iscai-msft
Copy link
Contributor

With GA autorest, we raise a ValueError instead of a NotImplementedError for api version and functionality mismatch

@@ -47,7 +47,9 @@ async def entities_recognition_general(
:raises: ~azure.core.exceptions.HttpResponseError
"""
cls = kwargs.pop('cls', None) # type: ClsType["models.EntitiesResult"]
error_map = {404: ResourceNotFoundError, 409: ResourceExistsError}
error_map = {
401: ClientAuthenticationError, 404: ResourceNotFoundError, 409: ResourceExistsError
Copy link
Member

Choose a reason for hiding this comment

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

I don't believe that a 401 response should map to ClientAuthenticationError - that is for when we fail to get a token from AAD etc.

@chlowell , can you please confirm/deny?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@johanste in my experience, this has been the mapping, @lmazuel and I GAd autorest python with this mapping.

401 also corresponds to ClientAuthenticationError in TA before this change (code here

Copy link
Member

Choose a reason for hiding this comment

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

This seems reasonable to me. The service responding 401 indicates an authentication problem. I'm uncertain there's great value in raising the more specific error though. If the client got a 401, that means the credential provided a token the service rejected, e.g. because a service principal doesn't have permission to access the resource. What action would an application take to recover from that?

Copy link
Contributor Author

@iscai-msft iscai-msft Oct 2, 2020

Choose a reason for hiding this comment

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

ClientAuthenticationError was already what TA was throwing before I joined, but my thinking for this is a more descriptive error. We also already GAd with this behavior, so we also can't change it in the library, @johanste do you know more about the decision to throw ClientAuthenticationError?

Copy link
Member

Choose a reason for hiding this comment

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

Didn't we distinguish between "I couldn't get a token to present to the service" and "I got a token that I could present to the service, but the service still doesn't like me"? I may have gotten things mixed up here....

Copy link
Member

Choose a reason for hiding this comment

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

Not to my knowledge. You might be thinking of the way credentials distinguish between "the authority rejected my token request" and "I don't have what I need to request a token".

@iscai-msft iscai-msft requested a review from johanste October 2, 2020 20:24
@iscai-msft iscai-msft merged commit 0895b0c into Azure:master Oct 2, 2020
@iscai-msft iscai-msft deleted the regen_ta branch October 2, 2020 21:40
iscai-msft added a commit that referenced this pull request Oct 7, 2020
…into fr-business-cards

* 'master' of https://github.com/Azure/azure-sdk-for-python: (71 commits)
  move the environment prep above the tooling that needs it (#14246)
  Increment version for appconfiguration releases (#14245)
  Azure Communication Service - Phone Number Administration (#14237)
  [text analytics] fix query param in cli call to get endpoint (#14243)
  Resolve Failing Documentation Build for azure-mgmt-core (#14239)
  Add code reviewers (#14229)
  [ServiceBus] make amqp_message properties read-only (#14095)
  [ServiceBus]remove topic parameter object settability (#14116)
  app config owner (#12986)
  [KeyVault] Handle Role Definition UUID Name Internally (#14218)
  Increment version for storage releases (#14224)
  Update Key Vault changelogs for October release (#14226)
  [ServiceBus] CI Test hotfixes (#14195)
  [text analytics] regen TA with GA autorest (#14215)
  [Storage][STG74]ChangeLog (#14192)
  fixes python 2.7 issue with unicode and strings again! (#14216)
  Feature/storage stg74 (#14175)
  Update communication pacakges to version b2 (#14209)
  [KeyVault] Add Status Methods to Query Backup and Restore Operations (#14158)
  Update buffered sender (#13851)
  ...
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-python that referenced this pull request May 18, 2021
add AVS 2021-06-01 API (Azure#14215)

* add AVS 2021-06-01 API

* add cmdlets custom word

* rename ScriptCmdlet cmdletDescription to description (Azure#14218)

* Adding optional to the script parameter (Azure#14437)

Co-authored-by: David Becher <dabecher@microsoft.com>

Co-authored-by: david becher <dab4au@virginia.edu>
Co-authored-by: David Becher <dabecher@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants