-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Adding type in EncryptionKeyWrapMetadata and performing validation on partition key #21407
Adding type in EncryptionKeyWrapMetadata and performing validation on partition key #21407
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@simplynaveen20 in similar scenario, does non Cosmos Exception type reach public surface area in the DotNet SDK?
void validatePartitionKeyPathsAreNotEncrypted(List<List<String>> partitionKeyPathTokens) { | ||
checkNotNull(partitionKeyPathTokens, "partitionKeyPathTokens cannot be null"); | ||
List<String> propertiesToEncrypt = | ||
this.includedPaths.stream().map(clientEncryptionIncludedPath -> clientEncryptionIncludedPath.getPath().substring(1)).collect(Collectors.toList()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice usage of java8 stream API. :-)
@@ -82,6 +82,13 @@ public CosmosAsyncClientEncryptionKey getClientEncryptionKey(String id) { | |||
|
|||
EncryptionKeyStoreProvider encryptionKeyStoreProvider = | |||
this.cosmosEncryptionAsyncClient.getEncryptionKeyStoreProvider(); | |||
|
|||
if (!encryptionKeyStoreProvider.getProviderName().equals(encryptionKeyWrapMetadata.getType())) { | |||
throw new IllegalArgumentException("The EncryptionKeyWrapMetadata Type value does not match with the " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in similar scenario, does non Cosmos Exception type reach public surface area in the DotNet SDK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes .NET has ArgumentException as well. To me this is correct and it should be non cosmos as it is validation on argument passed by user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how would it work on the async API? do we do validation upfront? or do we return a Mono.of(IllegalArgException)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This validation is on async API itself which is doing validation upfront .
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/models/ClientEncryptionPolicy.java
Outdated
Show resolved
Hide resolved
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/models/EncryptionKeyWrapMetadata.java
Outdated
Show resolved
Hide resolved
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/models/EncryptionKeyWrapMetadata.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @simplynaveen20
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. except the question on the async API and how validation and throwing IllegalArgException
from async api works.
@@ -82,6 +82,13 @@ public CosmosAsyncClientEncryptionKey getClientEncryptionKey(String id) { | |||
|
|||
EncryptionKeyStoreProvider encryptionKeyStoreProvider = | |||
this.cosmosEncryptionAsyncClient.getEncryptionKeyStoreProvider(); | |||
|
|||
if (!encryptionKeyStoreProvider.getProviderName().equals(encryptionKeyWrapMetadata.getType())) { | |||
throw new IllegalArgumentException("The EncryptionKeyWrapMetadata Type value does not match with the " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how would it work on the async API? do we do validation upfront? or do we return a Mono.of(IllegalArgException)
This PR contains 3 parts
closes #20376