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

[LLC] [Synapse] Enable cast from Response to a hand-written model #23372

Closed
annelo-msft opened this issue Aug 17, 2021 · 6 comments
Closed

[LLC] [Synapse] Enable cast from Response to a hand-written model #23372

annelo-msft opened this issue Aug 17, 2021 · 6 comments
Assignees
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library. needs-design Synapse

Comments

@annelo-msft
Copy link
Member

annelo-msft commented Aug 17, 2021

Problem

As we add high-level methods to the Synapse Access Control low-level client, we need to make it possible to pass a CancellationToken to any HLC method by calling the LLC version.

LLC Considerations
Per @KrzysztofCwalina, we would like the models to be designed such that even if they are in the main package, they can be used with the protocol methods, i.e. given the following protocol method and helper method:

public virtual Response Foo(RequestContent content, RequestOptions options);
public virtual OutputModel Foo(InputModel model);

We should allow:

var input = new InputModel();
OutputModel output = client.Foo(input, new RequestOptions());

To make this work for the input is easy. We just need a cast from InputModel to RequestContent.

Similarly, we could add a cast from Response to OutputModel to make the output work, but in addition it would be great if the error behavior was exactly the same whether the protocol method or the grow up method was called. This means the cast from Response to OutputModel would have to have access to ClientDiagnostics.

Implementation
To accomplish this, we’ll add the following to Core, as specified by @pakrym:

Add Response.Throw() method that produces the same exception as the full data-plane client would for the same response.

So these two examples produce the same exception when the service returns 404:

var response = llc.GetSomething();
response.Throw();

dataPlane.GetSomething();

The call to response.Throw() can replace any line throw _clientDiagnostics.CreateRequestFailedException(message.Response); in generated/HLC code.

@ghost ghost added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Aug 17, 2021
@annelo-msft annelo-msft added Azure.Core Client This issue points to a problem in the data-plane of the library. Synapse labels Aug 17, 2021
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Aug 17, 2021
@annelo-msft annelo-msft self-assigned this Aug 17, 2021
@annelo-msft annelo-msft changed the title [Synapse][LLC] Enable cast from hand-written model to Response [LLC] [Synapse] Enable cast from hand-written model to Response Sep 15, 2021
@annelo-msft
Copy link
Member Author

annelo-msft commented Sep 15, 2021

Per recent discussion, we'll do this as:

  • Adding a property to Response to indicate if an exception should be thrown (for now, I’m going to call this IsError)
  • Adding a constructor to RequestFailedException that reads internal properties of Response to format the exception in default or custom ways
  • Using the “default classifier” – i.e. the one generated from the REST API spec – to inform the semantics of Response.IsError

Relevant PR making API additions to ResponseClassifier: https://github.com/Azure/azure-sdk-for-net/pull/22923/files

@annelo-msft
Copy link
Member Author

We will not resolve API questions as part of this change.

@annelo-msft
Copy link
Member Author

annelo-msft commented Sep 15, 2021

@KrzysztofCwalina KrzysztofCwalina changed the title [LLC] [Synapse] Enable cast from hand-written model to Response [LLC] [Synapse] Enable cast from Response to a hand-written model Sep 15, 2021
@annelo-msft
Copy link
Member Author

Depends on: #24051

@annelo-msft
Copy link
Member Author

Design issues currently being discussed here: #24164

@annelo-msft
Copy link
Member Author

Relevant PRs have merged.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library. needs-design Synapse
Projects
None yet
Development

No branches or pull requests

1 participant