Azure / autorest.java

Extension for AutoRest (https://github.com/Azure/autorest) that generates Java code
MIT License
32 stars 80 forks source link

client, add validateClient to ClientBuilder #2783

Closed weidongxu-microsoft closed 1 month ago

weidongxu-microsoft commented 1 month ago

fix https://github.com/Azure/autorest.java/issues/2719

code change mainly in ServiceClientBuilderTemplate.java https://github.com/Azure/autorest.java/pull/2783/files#diff-011f78c981243301193df6b93ff9aa9373f5486e722793f3ded47bd5425fcbf8

example of generated ClientBuilder https://github.com/weidongxu-microsoft/autorest.java/blob/client_dpg-validate-builder/typespec-tests/src/main/java/com/resiliency/servicedriven/ResiliencyServiceDrivenClientBuilder.java#L258-L279


Would like advice on where we invoke the validateBuilder(). Context: https://github.com/Azure/autorest.java/pull/2783#discussion_r1612764904 Basically, some variable like credential is on HttpPipeline, that should not be validated, if user provide their own HttpPipeline. And some variable like endpoint is not on HttpPipeline, that can always be validated when creating the client. However, I don't want to make it more complex by providing a flag or 2 validate method (say validatePipelineParameter, validateNonPipelineParameter). ^^^ Updated to validateClient alone. If dev need to do similar of validatePipeline for credential, they may do such

if (this.pipeline == null) {
  // validate credential
}
haolingdong-msft commented 1 month ago

validateClient() looks good to me. Just think of below:

  1. wonder is there any reason to remove validatePipeline()
  2. Later, we could collect the use case of validating clients, e.g. validate non-null fields. Provide examples in our docs to recommend user how to write good validation. e.g. use Ojects.requireNonNull to validate non-null fields
weidongxu-microsoft commented 1 month ago
  1. this is discussed in yesterday's scrum, and in this PR description -- it probably can be done within validateClient, and we can always "add" later (but "remove" likely causes problem if anyone already uses it)