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

TypeSpec C#: @@include can't override inherited @@internal #3519

Closed
trrwilson opened this issue Jun 22, 2023 · 2 comments · Fixed by #3671
Closed

TypeSpec C#: @@include can't override inherited @@internal #3519

trrwilson opened this issue Jun 22, 2023 · 2 comments · Fixed by #3671
Assignees
Labels
customer-reported Issues that are reported by GitHub users external to the Azure organization. DPG/RLC v2.0 GA DPG needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team Not-GA-Required v3 Version 3 of AutoRest C# generator.

Comments

@trrwilson
Copy link
Member

trrwilson commented Jun 22, 2023

We're using the @internal decorator (applied in a client.tsp file via @@) to hide a long-running operation route from the public surface of our client libraries, as we'd like to wrap the presentation of it with custom code.

This works well and produces the desired effect of directing the emission of the C# method and its response model hierarchy to be internal instead of public. It also applies to the options model used for the operation -- and that's the one thing we'd still like to retain in the public surface for reuse.

We tried to use the @include decorator for the options model in conjunction with the @internal used on the route that consumes that options model, but it unfortunately doesn't appear that @include can override the serialization change effected by @internal.

Issue/request: conditionalize model emission visibility to retain public if @include is used, even if the parsing hierarchy has been made @internal.

Example client.tsp body (where @internal works, but @include doesn't take effect in .NET):

// Azure-specific long-running operations should be treated as implementation details
@@internal(Azure.OpenAI.startGenerateImage);

// Some models from client-internal LROs are still desired for custom public surface
@@include(Azure.OpenAI.ImageGenerationOptions)

Edit: adding link for our original impacted .tsp:

This is intended to make the long-running operations internal while still preserving the public visibility of the options and response payload models involved (as those are then wrapped in a custom implementation).

@trrwilson trrwilson added the v3 Version 3 of AutoRest C# generator. label Jun 22, 2023
@chunyu3
Copy link
Member

chunyu3 commented Jun 25, 2023

Hello @trrwilson would you please provide the typespec files you uses and we want to check how you conjunction @include and @internal?

And do you mean that the model will be used in more than one operations, one is decorated with @internal, the other is not?

@chunyu3 chunyu3 added the DPG label Jun 25, 2023
@lirenhe lirenhe added the customer-reported Issues that are reported by GitHub users external to the Azure organization. label Jun 26, 2023
@ghost ghost added the needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team label Jun 26, 2023
@tadelesh
Copy link
Member

tadelesh commented Jun 29, 2023

When a model is internal but include, we should not treat it as internal model.

I think this should be done in TCGC. When doing transitive closure detection for internal models, we should not label one model with internal if it has @include decoration.

@iscai-msft what do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customer-reported Issues that are reported by GitHub users external to the Azure organization. DPG/RLC v2.0 GA DPG needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team Not-GA-Required v3 Version 3 of AutoRest C# generator.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants