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.

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

Derive DPG ResponseClassifiers from CoreResponseClassifier #2006

Closed
annelo-msft opened this issue Feb 22, 2022 · 3 comments · Fixed by #2023
Closed

Derive DPG ResponseClassifiers from CoreResponseClassifier #2006

annelo-msft opened this issue Feb 22, 2022 · 3 comments · Fixed by #2023
Assignees
Labels
v3 Version 3 of AutoRest C# generator.

Comments

@annelo-msft
Copy link
Member

Problem

The DPG implementation of per-operation response classifiers currently derives from the ResponseClassifier type. But, the base ResponseClassifier cannot be directly modified in a way that allows us to efficiently compose a chain of ResponseClassificationHandlers with a ResponseClassifier at the end of the chain.

In PR 26547, we introduced a new CoreResponseClassifier type that allows us to both add status code classifiers and evaluate responses against them efficiently.

What we'll do

We'll replace the old DPG per-operation ResponseClassifier implementation with the new one (below) deriving from CoreResponseClassifier.

Old DPG Per-Operation ResponseClassifier

private sealed class ResponseClassifier200404 : ResponseClassifier
{
    private static ResponseClassifier _instance;
    public static ResponseClassifier Instance => _instance ??= new ResponseClassifier200404();
    public override bool IsErrorResponse(HttpMessage message)
    {
        return message.Response.Status switch
        {
            200 => false,
            404 => false,
            _ => true
        };
    }
}

New DPG Per-Operation ResponseClassifier

private sealed class ResponseClassifier200404 : CoreResponseClassifier
{
    private static CoreResponseClassifier _instance;
    public static CoreResponseClassifier Instance => _instance ??= new ResponseClassifier200404();

    public ResponseClassifier200404() : base(stackalloc int[]{ 200, 404 })
    {
    }
}
@annelo-msft annelo-msft added the v3 Version 3 of AutoRest C# generator. label Feb 22, 2022
@annelo-msft
Copy link
Member Author

annelo-msft commented Mar 1, 2022

In addition, we'll want to create the HttpMessage as follows:

var message = _pipeline.CreateMessage(context, ResponseClassifier200404.Instance); // passing the appropriate classifier

This should work for classifiers that don't derive from CoreResponseClassifier as well (like the one for the head request test case). They are just much more efficient when they derive from CoreResponseClassifier, so we want to use it as much as possible.

@AlexanderSher
Copy link
Contributor

AlexanderSher commented Mar 1, 2022

private static ResponseClassifier _responseClassifier200404;
private static ResponseClassifier ResponseClassifier200404 => _responseClassifier200404 ??= new CoreResponseClassifier(stackalloc int[]{ 200, 404 });

Usage:

var message = _pipeline.CreateMessage(context, ResponseClassifier200404); 

@AlexanderSher
Copy link
Contributor

AlexanderSher commented Mar 1, 2022

    private sealed class ResponseClassifierOverride200To300400To500 : ResponseClassifier
    {
        public override bool IsErrorResponse(HttpMessage message)
        {
            return message.Response.Status switch
            {
                >= 200 and < 300 => false,
                >= 400 and < 500 => false,
                _ => true
            };
        }
    }
    
    private static ResponseClassifier _responseClassifier200To300400To500;
    private static ResponseClassifier ResponseClassifier200To300400To500 => _responseClassifier200To300400To500 ??= new ResponseClassifierOverride200To300400To500();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v3 Version 3 of AutoRest C# generator.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants