Added token credential for queue #4673
Conversation
sima-zhu
requested review from
alzimmermsft,
jaschrep-msft and
rickle-msft
as code owners
kurtzeborn
added
Client
| import java.util.TreeMap; | ||
|
|
||
|
|
||
| public final class Utility { |
There was a problem hiding this comment.
If this is being copied from my PR to add a common package for storage make this package private as it will be replaced in the future.
Same comment for the public methods in this class.
| @@ -86,7 +86,8 @@ public QueueClient createClientWithCredential() { | |||
| QueueClient queueClient = new QueueClientBuilder() | |||
| .endpoint("https://${accountName}.queue.core.windows.net") | |||
| .queueName("myqueue") | |||
| .credential(SASTokenCredential.fromQuery("{SASTokenQueryParams}")) | |||
| .credential(SASTokenCredential.fromQueryParameters( | |||
There was a problem hiding this comment.
Let's simplify these examples and assume that SASTokenCredential.fromSASTokenString is used. There will be more changes to SASTokenCredential creation once Files and Queues have the ability to generate their own SAS tokens.
| SASTokenCredential credential = SASTokenCredential.fromQuery(fullURL.getQuery()); | ||
| if (credential != null) { | ||
| this.sasTokenCredential = credential; | ||
| this.sasTokenCredential = SASTokenCredential.fromQueryParameters(Utility.parseQueryString(fullURL.getQuery())); |
There was a problem hiding this comment.
Why are we parsing a query string into a map just to pass it into a method that turns it back into a query string?
There was a problem hiding this comment.
I agreed with take the string directly. Moved it logic to SasTokenCredential for current use. Alan will have another common module PR which will take care the token as a whole.
There was a problem hiding this comment.
One possible reason this was happening is for validation. We do something similar with permissions (parse the permissions string passed to validate it and then immediately toString it). If there is any sort of validation logic in either parseQueryString or fromQueryString, I'd recommend keeping this as is and adding a comment. If there's nothing of value and it's just a complete waste of cpu time, then I'm fine changing this.
There was a problem hiding this comment.
This line has been changed from Alan's common module PR.
I don't think it is good to address this only for queue.
If it is necessary, then we need to file a new issue to address this overall.
| @@ -83,6 +86,7 @@ | |||
| private HttpLogDetailLevel logLevel; | |||
| private RetryPolicy retryPolicy; | |||
| private Configuration configuration; | |||
| private TokenCredential tokenCredential; | |||
There was a problem hiding this comment.
Can you put this up with the other credential declarations so they're all together? Also, can we name the field bearerTokenCredential? That way, compared with the name sasTokenCredential, they both have qualifiers and it doesn't sound like one's a generic version of the other.
There was a problem hiding this comment.
Renamed the tokenCred as suggested.
| * @return a mapping of query string pieces as key-value pairs. | ||
| */ | ||
| public static TreeMap<String, String> parseQueryString(final String queryString) { | ||
| TreeMap<String, String> pieces = new TreeMap<>(String::compareTo); |
There was a problem hiding this comment.
Declare as Map instead of TreeMap. I don't see the justification for declaring implementation details.
There was a problem hiding this comment.
Discussed offline with @alzimmermsft , this is introduced by previous change.
I would like to keep it as it is, as there is no hurt to the functionality. We will make decision in common module PR.
| import java.util.TreeMap; | ||
|
|
||
|
|
||
| public final class Utility { |
There was a problem hiding this comment.
This class feels like it should be in common package, not queue.
There was a problem hiding this comment.
Alan has another PR to extract the common module.
I copied over some of the necessary one to support the Preview 2 release.
* Renamed StorageClient to BlobServiceClient * Cleanup README * Fix README example links * Fixed XML serialization issue
* Added links REST API documentation
Add @hemanttanwar to /sdk/identity/
No description provided.