[LLC] Set "default response classifier" on message

[annelo-msft]

In order for us to allow Response.IsError to reflect default behavior (i.e. which status codes are defined in the OpenAPI spec as success vs. error), we need to pass this information to the pipeline so it can be set on the response.

We achieve this via setting the ResponseClassifier on the message when the request is created.

In the LLC generator, we will need to:

1. Generate properties on the client for each group of successful status codes in the OpenAPI spec
2. Generate classifier type definitions
3. Set the classifier on the message in CreateXxRequest

In addition, we will:
4. Remove the RequestOptions allocation from the default path

[annelo-msft]

@AlexanderSher, can you confirm this is the design we want to go with before we assign it?

[AlexanderSher]

There is one limitation with this approach: users won't be able to customize how exceptions are created. I'd prefer classifier to be internal (rather than private) with method ThrowOnErrorResponse.

[annelo-msft]

Hi Alexander, the user will still be able to customize exception creation because we plan to move the customization to ResponseClassifier in Azure/azure-sdk-for-net#24031

I'd prefer classifier to be internal (rather than private) with method ThrowOnErrorResponse.
We've actually discussed this a lot with @KrzysztofCwalina, @ellismg, and @pakrym, and decided that the following is a preferred API as it's more idiomatic to .NET

      if (response.IsError())
      {
          throw new RequestFailedException(response);
      }

I am working on this as part of Azure/azure-sdk-for-net#23372

[AlexanderSher]

because we plan to move the customization to ResponseClassifier

Ok, let's say a user needs custom error code handling and custom exception types. How this will be customized?

[annelo-msft]

Custom error code handling was previously exposed via the RequestOptions API, but removed in this PR because we wanted to accelerate the timeline for releasing RC1:
https://github.com/Azure/azure-sdk-for-net/pull/24083/files#diff-62f3bc8e067ac5ebd48d94a2f93253252b108bf6dbe6baa9c7563943efe03f8dL8

Customizing exceptions currently happens in the internal shared source class ClientDiagnostics, and we are planning to move this functionality into ResponseClassifier as part of this change: Azure/azure-sdk-for-net#24031

[annelo-msft]

Regarding the API we selected instead of ThrowOnErrorResponse, I can share the recording of the meeting where this was decided if you'd like. Thanks!

[annelo-msft]

To close the loop here - @AlexanderSher and I chatted, and we'll move forward with this approach. There is a feature request from another team for error customization and @AlexanderSher is going to file an issue to track that -- once we have that we can decide on a timeline for addressing it and whether or not it should be part of LLC RC1.

[AlexanderSher]

@annelo-msft @pakrym
Am I right that the following chunk of code would be the same for all methods:

                await Pipeline.SendAsync(message, options?.CancellationToken ?? default).ConfigureAwait(false);
                var statusOption = options?.StatusOption ?? ResponseStatusOption.Default;

                if (statusOption == ResponseStatusOption.NoThrow)
                {
                    return message.Response;
                }
                else
                {
                    if (!message.ResponseClassifier.IsErrorResponse(message))
                    {
                        return message.Response;
                    }
                    else
                    {
                        throw await _clientDiagnostics.CreateRequestFailedExceptionAsync(message.Response).ConfigureAwait(false);
                    }
                }

[pakrym]

@AlexanderSher that's a great observation! Wonder if we can move this code into some shared file and not codegen it.

[AlexanderSher]

This is the code we generate right now for the HEAD as bool case:

switch (message.Response.Status)
{
    case int s when s >= 200 && s < 300:
        return Response.FromValue(true, message.Response);
    case int s when s >= 400 && s < 500:
        return Response.FromValue(false, message.Response);
    default:
        throw await _clientDiagnostics.CreateRequestFailedExceptionAsync(message.Response).ConfigureAwait(false);
}

Which part of it should go into ResponseClassifier?

[annelo-msft]

Great question, @AlexanderSher!

In this case, we wouldn't use the standard code you noted in #1535 (comment). Instead, since we need to return Response<bool> we would have something like:

if (!message.ResponseClassifier.IsErrorResponse(message))
{
    return Response.FromValue(message.Response.Status >= 200 && message.Response.Status < 300, message.Response);
}

if (options.StatusOption == ResponseStatusOption.NoThrow)
{
    return new ErrorResponse<bool>(message.Response, _clientDiagnostics.CreateRequestFailedException(message.Response));
}
else
{
    throw await _clientDiagnostics.CreateRequestFailedExceptionAsync(message.Response).ConfigureAwait(false);
}

With the ResponseClassifier being:

private class ResponseClassifier200204404 : ResponseClassifier
{
    public override bool IsErrorResponse(HttpMessage message)
    {
        switch (message.Response.Status)
        {
            case 200:
            case 204:
            case 404:
                return false;
            default:
                return true;
        }
    }
}

What are your thoughts on this proposal?

[AlexanderSher]

ResponseClassifier in this case doesn't incapsulate all the logic related to the codes. Since HEAD as bool is a special case by itself, I think we should skip generating classifier for it for now.

[annelo-msft]

I agree it's a special case, and I think it's interesting -- thanks for calling this out @AlexanderSher. ResponseClassifier isn't intended to encapsulate all the logic related to status codes -- its responsibility is to say whether a given status code is expected (has a response defined in the swagger file) or an error (e.g. IsErrorResponse). The complication here is that we're also returning a convenience value related to the status code -- i.e. once we've determined the status code is an expected one, we still need to decide whether the expected status code represents true or false for "Exists".

Given this, I'd suggest we continue generating classifiers in the same way as other methods so we handle IsErrorResponse consistently across LLC methods. I agree there may be an opportunity to rewrite what happens in the !message.ResponseClassifier.IsErrorResponse(message)) case for clarity. What about something like:

if (!message.ResponseClassifier.IsErrorResponse(message))
{
   bool resourceExists = message.Response.Status == 200 || message.Response.Status ==204;
    return Response.FromValue(resourceExists, message.Response);
}

I'm not tied to this specific implementation, though, so happy to discuss alternatives...