Use SHA384 and make key more explicit #6237
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The point of this is to make it easier and more reliable for people overriding the logic to obtain specific values from messages/options to include in cache key without having to search through an opaque list of `object` values.
stephentoub
approved these changes
Apr 3, 2025
peterwald
approved these changes
Apr 3, 2025
SteveSandersonMS
deleted the
stevesa/use-sha384-and-make-key-more-explicit
branch
joperezr
pushed a commit
to joperezr/extensions
that referenced
this pull request
Apr 8, 2025
* Minimal change in AIJsonUtilities from SHA256 to SHA384 * Update GetCacheKey API to have explicit messages/options params The point of this is to make it easier and more reliable for people overriding the logic to obtain specific values from messages/options to include in cache key without having to search through an opaque list of `object` values.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR includes the changes from #6236 and also changes the
GetCacheKeymethod signature so that it explicitly takesmessages/optionsparams:We don't strictly have to do this. However it can be beneficial for people who want to customize the cache key computation to include other data (e.g., from
AdditionalPropertiesfrom within messages or options). Previously they would have to just somehow know that one of the incomingvalueswill be the data they need, and would need to do runtime type checking to find the relevant one, and would have to hope that we don't change the incoming data shape in the future (e.g., pre-JSON-serializing the values).The more explicit signature in this PR would be a guarantee that we will in fact pass in these specific
IEnumerable<ChatMessage>/ChatOptions?types. I don't think there's much drawback to us offering that guarantee, since this is really baked into theIChatClientcontract anyway - those are exactly the data items that middleware has access to.Microsoft Reviewers: Open in CodeFlow
Hash algorithm
I considered also making the hash algorithm configurable (defaulting to SHA384). However that complicates things because currently we use a thread-static
IncrementalHashStreamthat holds theIncrementalHashinstance. We'd have to create new instances when different hash algorithms are specified.Since we've been told that in this case, SHA384 is sufficient without being configurable, I'd be happy to leave it at that.