Skip to content

Conversation

@KrzysztofCwalina
Copy link
Member

@KrzysztofCwalina KrzysztofCwalina commented Jun 29, 2021

cc: @jkotas

At some point we might want to cache the whole token, but that is a bit more tricky as the JWT audience changes from call to call (it's the absolute URI).

@KrzysztofCwalina KrzysztofCwalina marked this pull request as ready for review June 29, 2021 17:09
Copy link
Member

@jsquire jsquire left a comment

Choose a reason for hiding this comment

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

Should we consider adding a test or two for validation of the behavior?

public KeyBytesCache(string key)
{
Key = key;
KeyBytes = Encoding.UTF8.GetBytes(key);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
KeyBytes = Encoding.UTF8.GetBytes(key);
KeyBytes = string.IsNullOrEmpty(key)
? Array.Empty<byte>()
: Encoding.UTF8.GetBytes(key);

Probably a tiny efficiency, but should we consider avoiding GetBytes when the string is empty? It looks like there's no special case for it and it'll do some stack allocations that we can avoid. (ref) Maybe a factory method for an empty instance, instead?

@KrzysztofCwalina
Copy link
Member Author

Should we consider adding a test or two for validation of the behavior?

We will be adding test before GA but it's a non trivial workitem. The problem is that this component is inherently difficult to test. There are not APIs to observe changes to the service.

I was also thinking about unit testing this policy with fake messages. I need to talk to Pavel about adding fake Transport/Request/Response for that. He wanted to do something like that in Azure.Core, i.e. add test only Request/Response.

@KrzysztofCwalina KrzysztofCwalina merged commit 6b98ba0 into Azure:main Jun 29, 2021
@KrzysztofCwalina KrzysztofCwalina deleted the WPS_cache branch July 8, 2021 16:19
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