Introduce Async API counterparts for AE base class#3673
Introduce Async API counterparts for AE base class#3673cheenamalhotra wants to merge 2 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
Adds asynchronous counterparts to key store provider APIs in SqlColumnEncryptionKeyStoreProvider to enable async implementations for Always Encrypted key operations.
Key changes:
- Introduced async virtual methods (Decrypt/Encrypt column encryption key, Sign/Verify CMK metadata) that currently throw NotImplementedException.
- Added corresponding XML documentation entries for the new async methods.
- Minor wording adjustments in existing XML return documentation.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlColumnEncryptionKeyStoreProvider.cs | Adds async virtual method stubs for encryption key and metadata operations. |
| doc/snippets/Microsoft.Data.SqlClient/SqlColumnEncryptionKeyStoreProvider.xml | Documents newly added async methods and adjusts some existing return descriptions. |
...Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlColumnEncryptionKeyStoreProvider.cs
Outdated
Show resolved
Hide resolved
...Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlColumnEncryptionKeyStoreProvider.cs
Outdated
Show resolved
Hide resolved
...Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlColumnEncryptionKeyStoreProvider.cs
Outdated
Show resolved
Hide resolved
...Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlColumnEncryptionKeyStoreProvider.cs
Outdated
Show resolved
Hide resolved
doc/snippets/Microsoft.Data.SqlClient/SqlColumnEncryptionKeyStoreProvider.xml
Show resolved
Hide resolved
doc/snippets/Microsoft.Data.SqlClient/SqlColumnEncryptionKeyStoreProvider.xml
Outdated
Show resolved
Hide resolved
doc/snippets/Microsoft.Data.SqlClient/SqlColumnEncryptionKeyStoreProvider.xml
Outdated
Show resolved
Hide resolved
| public abstract byte[] DecryptColumnEncryptionKey(string masterKeyPath, string encryptionAlgorithm, byte[] encryptedColumnEncryptionKey); | ||
|
|
||
| /// <include file='../../../../../../doc/snippets/Microsoft.Data.SqlClient/SqlColumnEncryptionKeyStoreProvider.xml' path='docs/members[@name="SqlColumnEncryptionKeyStoreProvider"]/DecryptColumnEncryptionKeyAsync/*'/> | ||
| public virtual Task<byte[]> DecryptColumnEncryptionKeyAsync(string masterKeyPath, string encryptionAlgorithm, byte[] encryptedColumnEncryptionKey) |
There was a problem hiding this comment.
Cryptographic tasks will normally complete synchronously; Azure Key Vault is the exception here. What about returning ValueTask<byte[]>, to remove an allocation in the synchronous case?
There was a problem hiding this comment.
Since these 4 new async methods are all implemented by calling their synchronous counterparts, I would agree that the operations will definitely complete synchronously.
Do we plan to re-implement these in terms of some other underlying async methods later?
There was a problem hiding this comment.
Yes, these implementations will be re-implemented in all Key Vault providers, as issue #3672 covers. Both sync and async will be made truly sync and truly async flows - with external calls separated.
Also, in the codebase, we want to make sure we call sync v/s async APIs of AE that are separated based on caller.
This, here, is a fallback for any consumer of AE implementation without async implementation.
paulmedynski
left a comment
paulmedynski
left a comment
There was a problem hiding this comment.
Asking for clarification on return types and synchronous implementation.
benrr101
left a comment
benrr101
left a comment
There was a problem hiding this comment.
For new APIs we should have some discussion on the signatures. Definitely won't be able to get this in before preview2.
|
This pull request has been marked as stale due to inactivity for more than 30 days. If you would like to keep this pull request open, please provide an update or respond to any comments. Otherwise, it will be closed automatically in 7 days. |
github-actions
bot
added
the
Stale
|
Wait, wait 🙈 |
github-actions
bot
removed
the
Stale
|
This pull request has been marked as stale due to inactivity for more than 30 days. If you would like to keep this pull request open, please provide an update or respond to any comments. Otherwise, it will be closed automatically in 7 days. |
github-actions
bot
added
the
Stale
|
Stay |
github-actions
bot
removed
the
Stale
cheenamalhotra
added
the
Public API 🆕
5 participants
For #3672:
Introduces async API counterparts in base class 'SqlColumnEncryptionKeyStoreProvider'