RequestOptions usage and mutability

request_options.md

RequestOptions scenarios

These are the scenarios we support today, not necessarily the ones we have to provide in the final design.

1. Users can provide static (for a group of operations) configuration. E.g.: configure custom logger for all requests peformed in scope of this API call

2. Provide dynamic (per-request) configuration. E.g. set request body or operation-specific context

So RequestOptions is a mix of (usually) per-request and (usually) static config.

Code we expect to write (today)

Users:

private static final RequestOptions BLOB_DOWNLOAD_OPTIONS = new RequestOptions()
   .setResponseBodyMode(BUFFER)
   .setProgressReporter(REPORTER);
   // future - setErrorOptions(DONT_FAIL_ON_404);
   
downloadBlob() {
  var content = blobClient.download(BLOB_DOWNLOAD_OPTIONS);
  content.writeTo(networkStream);
}

uploadBlob() {
  blobClient.upload(new RequestOptions()
    .setBody(BinaryData.fromStream(networkStream))
    .setProgressReporter(REPORTER));
}

Libs:

public Response download(RequestOptions options) {
  // context, instrumentation context:
  Span span = startSpan(options);   
  RequestOptions modifiedOptions = copyOptionsModifyInstrContext(options, span);
    
  return serviceClient.download(modifiedOptions);
}

public void createContainerAndUploadBlob(String containerName, String blobName, RequestOptions options) {
  // body is not applicable to this request
  RequestOptions optionsWithoutBody = copyAndRemoveBody(options)
  serviceClient.createContainer(containerName, optionsWithoutBody); 
  
  // body is only applicable here
  serviceClient.upload(containerName, blobName, options);
} 

Problems

• It is never safe to mutate shared instance of RequestOptions. Core/client libs MUST NOT mutate it. Users can only mutate it while it's being configured.

• It's not clear which properties apply to all requests in the scope of a complex operation:

  • should auth policy pass request options to Identity?
  • should client libs pass it to inner libs (e.g. eventhubs checkpoint calling storage)?
  • should probably pass them to redirected requests, individual pages when paging, chunks when downloading in chunks.
  • undefined when complex operations perform multiple calls (e.g. get content length and then parallel-download in chunks)

Proposal

RequestOptions are intended to be shared across different calls. They need to be immutable to support this without becoming a pitfall.

How to do it

Options:

1. RequestOptions contains both - per-request and static config. They are not-modifiable after initial setup and can be copied if modification is necessary. This can be achieved with

   • a. RequestOptions.set* retuning a new instance. Cons: this goes against all other *Options behavior and becomes error-prone.
   • b. RequestOptionsBuilder and similar. Cons: We don't like builder pattern on options
   • c. RequestOptions.lock() and RequestOptions.isLocked(). Cons: depends on users calling lock() after setting options up. Libs/core need to check if options are locked when modification is needed.
   • d. Keep things as they are. Cons: error prone, need to add copy-constructors and style checks for set* methods in the client libs.

   Comparing a, b, c, d the b (builder) is the most robust, the least surprising option.

2. RequestOptions contains static config only. Dynamic config is passed separately and is immutable. E.g.

   private static final RequestOptions BLOB_DOWNLOAD_OPTIONS = new RequestOptions()
      .setResponseBodyMode(BUFFER)
      .setProgressReporter(REPORTER);
      // future - setErrorOptions(DONT_FAIL_ON_404);
   
   private static final RequestOptions BLOB_UPLOAD_OPTIONS = new RequestOptions()
      .setProgressReporter(REPORTER);
      
   downloadBlob() {
     var content = blobClient.download(BLOB_DOWNLOAD_OPTIONS);
     content.writeTo(networkStream);
   }
   
   uploadBlob() {
     blobClient.upload(BinaryData.fromStream(networkStream), BLOB_UPLOAD_OPTIONS);
   }

   public Response download(RequestOptions options, Context context) {
     // context, instrumentation context:
     Context modifiedContext = startSpan(context);
   
     return serviceClient.download(options, modifiedContext);
   }
   
   public void createContainerAndUploadBlob(String containerName, String blobName, BinaryData body, RequestOptions options, Context context) {
     Context modifiedContext = startSpan(context);    
     serviceClient.createContainer(containerName, options, modifiedContext); 
     serviceClient.upload(blobName, body, options, modifiedContext);
   }    

Comparing 1 and 2:

1. Usability: p2 gives clear answer on what RequestOptions are for - static config. It's hard to misuse them. You don't need to repeat static config along with dynamic one (body, context)
2. Correctness: you can't design good complex API like createContainerAndUploadBlob with p1 (RequestOptions containing body)
3. Perf: presumably mostly the same, but cloning all options is slightly more expensive than cloning context when it's necessary

Paraphrasing from what I spoke on in team sync.

I believe a lot of confusion and concern with RequestOptions mutability comes from us carrying over concepts added to azure-core RequestOptions to support low-level clients (ability to set request body, headers, and query parameters conveniently, or to wholesale mutate the request) in a new world which doesn't make as much sense for ClientCore RequestOptions.

My thought here is to split RequestOptions into two types, RequestOptions with the simple request configurations (ProgressReporter, ClientLogger, telemetry, context) and ComplexRequestOptions (better name needed) which allows for the full-fledged, "get out of jail free" APIs that let you fully mutate the HttpRequest. This helps separate out the "old world" azure-core RequestOptions meant for LLCs and "new world" ClientCore RequestOptions which is more meant to be a strongly typed Context bag.