Skip to content
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.

Sign up for GitHub

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

Jump to bottom

design, validate parameters in Builder #39889

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

design, validate parameters in Builder #39889

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d398ada
proposed changes in builder
weidongxu-microsoft Apr 24, 2024
020a2a6
validate
weidongxu-microsoft Apr 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions ...ai-contentsafety/src/main/java/com/azure/ai/contentsafety/ContentSafetyClientBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,7 @@ public ContentSafetyClientBuilder credential(KeyCredential keyCredential) {
@Generated
@Override
public ContentSafetyClientBuilder endpoint(String endpoint) {
Objects.requireNonNull(endpoint, "'endpoint' cannot be null.");
this.endpoint = endpoint;
return this;
}
Expand Down Expand Up @@ -283,8 +284,17 @@ private ContentSafetyClientImpl buildInnerClient() {
return client;
}

@Generated
private void validateBuilderParameters() {
Objects.requireNonNull(endpoint, "'endpoint' cannot be null.");
Copy link
Member Author

Choose a reason for hiding this comment

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

The change on endpoint would apply to any required client properties.

discussion

  1. Should we use a different exception, e.g. InvalidStateException?
  2. Do we put these "default validation" here, or in createHttpPipeline (so that the whole method is generated as empty and intended for customization)?

Copy link
Contributor

Choose a reason for hiding this comment

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

InvalidStateException seems most correct to me.

I think I'm in favor of leaving all of the validation in this method. Perhaps we generate a comment that these are the default requirements and others can be added? "Magic" other validation seems hard to understand.


// dev may customize and add more validation here
// in this module, maybe check either keyCredential or tokenCredential exists
}

@Generated
private HttpPipeline createHttpPipeline() {
validateBuilderParameters();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why here and not in buildInnerClient? It doesn't matter much in this example; are there others where it might matter more?

Copy link
Member Author

Choose a reason for hiding this comment

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

An alternative code flow would be user setting the pipeline via

azure-sdk-for-java/sdk/contentsafety/azure-ai-contentsafety/src/main/java/com/azure/ai/contentsafety/ContentSafetyClientBuilder.java

Line 88 in 020a2a6

public ContentSafetyClientBuilder pipeline(HttpPipeline pipeline) {

And builderInnerClient has this

azure-sdk-for-java/sdk/contentsafety/azure-ai-contentsafety/src/main/java/com/azure/ai/contentsafety/ContentSafetyClientBuilder.java

Lines 278 to 279 in 020a2a6

private ContentSafetyClientImpl buildInnerClient() {
HttpPipeline localPipeline = (pipeline != null) ? pipeline : createHttpPipeline();

(createHttpPipeline won't be called, if user provides the pipeline)

I think in that code flow, there is no need to verify anything (as user provides a pipeline and we cannot inspect it anyway).

Configuration buildConfiguration
= (configuration == null) ? Configuration.getGlobalConfiguration() : configuration;
HttpLogOptions localHttpLogOptions = this.httpLogOptions == null ? new HttpLogOptions() : this.httpLogOptions;
Expand Down