Skip to content

Rename apiKey() to credential() in FormRecognizer and TextAnalytics ClientBuilder#10738

Merged
samvaity merged 5 commits intoAzure:masterfrom
samvaity:update-to-cred
May 6, 2020
Merged

Rename apiKey() to credential() in FormRecognizer and TextAnalytics ClientBuilder#10738
samvaity merged 5 commits intoAzure:masterfrom
samvaity:update-to-cred

Conversation

@samvaity
Copy link
Member

@samvaity samvaity commented May 5, 2020

Fixes #10737

@samvaity samvaity requested review from mssfang and sima-zhu as code owners May 5, 2020 22:42
@samvaity samvaity requested a review from JonathanGiles May 5, 2020 22:42
@samvaity samvaity self-assigned this May 5, 2020
/**
* Test for null API key
*/
@Test
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are the tests removed?

Copy link
Member Author

@samvaity samvaity May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we have two overloads for the credential in the builder, it cannot resolve at compile time to pass in null object.

Copy link
Contributor

@mssfang mssfang May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assigning the null value to a type value, such as AzureKeyCredential credential = null, and replace null value with credential will resolve it. `=

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also do cast in API.
.credential((AzureKeyCredential) null)

*/
public FormRecognizerClientBuilder apiKey(AzureKeyCredential apiKeyCredential) {
public FormRecognizerClientBuilder credential(AzureKeyCredential apiKeyCredential) {
this.credential = Objects.requireNonNull(apiKeyCredential, "'apiKeyCredential' cannot be null.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Search is validating apiKeyCredential.getKey empty or null case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moreover, if the credential is required, we can move the validation up to buildClient()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public SearchServiceClientBuilder credential(AzureKeyCredential keyCredential) {
        if (keyCredential == null) {
            throw logger.logExceptionAsError(new NullPointerException("'keyCredential' cannot be null."));
        }
        if (CoreUtils.isNullOrEmpty(keyCredential.getKey())) {
            throw logger.logExceptionAsError(
                new IllegalArgumentException("'keyCredential' cannot have a null or empty API key."));
        }
        this.keyCredential = keyCredential;
        return this;
    }

Here is how search do validation for your reference.

Copy link
Member Author

@samvaity samvaity May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sima-zhu Could this

if (keyCredential == null) {
throw logger.logExceptionAsError(new NullPointerException("'keyCredential' cannot be null."));
}

be replaced with

   this.credential = Objects.requireNonNull(keyCredential, "'keyCredential' cannot be null.");

And also for the rest of the code, I see that we are already doing the empty checks here and here. It would be redundant?

Copy link
Contributor

@sima-zhu sima-zhu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@@ -54,7 +54,7 @@ public void invalidProtocol() {
public void nullApiKey() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method name should be renamed too.

AzureKeyCredential credential = new AzureKeyCredential("{api_key}");
TextAnalyticsClient client = new TextAnalyticsClientBuilder()
.apiKey(credential)
.credential(credential)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the name of the class to RotateAzureKeyCredential

// BEGIN: com.azure.ai.textanalytics.TextAnalyticsAsyncClient.instantiation
TextAnalyticsAsyncClient textAnalyticsAsyncClient = new TextAnalyticsClientBuilder()
.apiKey(new AzureKeyCredential("{api_key}"))
.credential(new AzureKeyCredential("{api_key}"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use azure_key_credential instead of api_key everywhere to be consistent with terminology.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated to say key as that is what it is called in Portal.

@samvaity
Copy link
Member Author

samvaity commented May 6, 2020

/azp run java - textanalytics - ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@samvaity samvaity merged commit 1956e6b into Azure:master May 6, 2020
@samvaity samvaity deleted the update-to-cred branch September 17, 2020 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update ClientBuilder method apiKey to credential in Form Recognizer and Text Analytics

4 participants