Resource token fix - 1 #19547
Resource token fix - 1 #19547
Conversation
xinlian12
requested review from
FabianMeiswinkel,
kirankumarkolli,
kushagraThapar,
mbhaskar,
milismsft,
moderakh and
simplynaveen20
as code owners
ghost
added
the
|
/azp run java - cosmos - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
LGTM. thank you.
please ask
@simplynaveen20 @xinlian12 to review as well, as they are familiar with permission
| Constants.Properties.RESOURCE_PARTITION_KEY, | ||
| BridgeInternal.getPartitionKeyInternal(partitionkey) | ||
| .toJson()); | ||
| if(ModelBridgeInternal.getPartitionKeyObject(partitionkey) != null) { |
There was a problem hiding this comment.
why the null check is needed?
can the user pass null partition key to the setter? is that a valid scenario?
if we decide to do arg validation here shouldn't we throw NPE?
There was a problem hiding this comment.
I agree, good point, having null checks prevent bugs, but they also hide the possible buggy code paths. We should avoid it if this scenario can be validated through the generic code pass.
There was a problem hiding this comment.
Thanks @xinlian12 @simplynaveen20 for these changes, looks good to me overall, just some minor comments.
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/CosmosAsyncClient.java
Outdated
Show resolved
Hide resolved
| Constants.Properties.RESOURCE_PARTITION_KEY, | ||
| BridgeInternal.getPartitionKeyInternal(partitionkey) | ||
| .toJson()); | ||
| if(ModelBridgeInternal.getPartitionKeyObject(partitionkey) != null) { |
There was a problem hiding this comment.
I agree, good point, having null checks prevent bugs, but they also hide the possible buggy code paths. We should avoid it if this scenario can be validated through the generic code pass.
sdk/cosmos/azure-cosmos/src/test/java/com/azure/cosmos/rx/ResourceTokenTest.java
Show resolved
Hide resolved
ResourceTokenTests is for v4 model. ResourceTokenTest is for old model. I think the naming might confusing a little. Will change that. Refactored from ResourceTokenTests to ResourceTokenTestForV4 |
|
/azp run java - cosmos - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| collectionRid, | ||
| ResourceType.Document, | ||
| RequestVerb.GET, | ||
| headers, | ||
| AuthorizationTokenType.PrimaryMasterKey, | ||
| request.properties); | ||
| } catch (UnauthorizedException e) { | ||
| // User doesn't have rid based resource token. Maybe user has name based. |
There was a problem hiding this comment.
We are swallowing the exception here. If there is no active action that we can take, we should at least log it for our own purposes.
There was a problem hiding this comment.
Make sense, since we will retry using name based link next, so will add a debug level message to not confuse the customer
There was a problem hiding this comment.
seems to me we are throwing UnauthorizedException as part of the logic rather that catching a problem
Why can't we do this with a if/else condition where you check it is rid based use that else use name based.
Why do we need to throw exception can't rely on if/else?
exception throwing is expensive!
There was a problem hiding this comment.
The intention here seems like: first always try resouceId, then using named based link as fallback. so it make sure we will try both ways.
If we changed to if else, then we will only try one of them.
Currently, the main issue(query does not work with permissions) is that we are mixing using the name based link or the resouceId based link: how we construct the tokenMap and how we consuming it. There could be two possible ways to fix it:
a. using the namedBasedLink/ResourceIdBasedLink consistently
b. try one, if not found, then fallback to another one.
Depending which way we want to choose, this part of logic can be simplified or should keep some similar logic.
Fix included:
Still need to work on:
Query with permissions