Skip to content

Produce more descriptive error for GenerateSasUri when using TokenCredential#23388

Merged
christothes merged 2 commits intoAzure:mainfrom
christothes:chriss/tablesSasTokenCred
Aug 18, 2021
Merged

Produce more descriptive error for GenerateSasUri when using TokenCredential#23388
christothes merged 2 commits intoAzure:mainfrom
christothes:chriss/tablesSasTokenCred

Conversation

@christothes
Copy link
Copy Markdown
Member

The current exception message thrown when attempting to call GenerateSasUri with a client constructed with a TokenCredential could be more descriptive.

@christothes christothes self-assigned this Aug 18, 2021
@ghost ghost added the Tables label Aug 18, 2021
@christothes christothes requested a review from jsquire August 18, 2021 13:53
Argument.AssertNotNull(builder, nameof(builder));
if (SharedKeyCredential == null)
{
throw new InvalidOperationException($"{nameof(GenerateSasUri)} requires a credential other than {nameof(TokenCredential)} in order to sign the SAS token.");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
throw new InvalidOperationException($"{nameof(GenerateSasUri)} requires a credential other than {nameof(TokenCredential)} in order to sign the SAS token.");
throw new InvalidOperationException($"{nameof(GenerateSasUri)} requires a credential that is not a {nameof(TokenCredential)} type in order to sign the SAS token.");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not in love with the suggestion, honestly... but it took me a couple of reads of the original to understand that you're looking for a shared key-based credential.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, I am not in love with what I originally wrote either. I wanted to be less specific than Shared Key, since Connection String auth also works. Although the shared key is also present there, it's not explicitly a SharedKeyCredential.

What I was trying to convey with the current message is roughly - "this will fail if you used the TokenCredential ctor"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

What do you think of this?

throw new InvalidOperationException($"{nameof(GenerateSasUri)} requires that this client be constructed with a credential type other than {nameof(TokenCredential)} in order to sign the SAS token.");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice.... I like that!

@christothes christothes merged commit e132ef7 into Azure:main Aug 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants