Azure / autorest.csharp

Extension for AutoRest (https://github.com/Azure/autorest) that generates C# code
MIT License
143 stars 165 forks source link

[TypeSpec] Add an option to force System.Text.Json converter to be applied to models #3469

Closed JoshLove-msft closed 3 weeks ago

JoshLove-msft commented 1 year ago

For swagger, we can apply the value of converter to x-csharp-usage, e.g. https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/eventgrid/Azure.Messaging.EventGrid/src/autorest.md#apply-converters-and-update-namespace-for-system-event-data-models.

This is not possible in Typespec and is a blocker for migrating the .NET Event Grid system events from swagger to Typespec.

ArcturusZhang commented 1 year ago

Well we could do it by using the CodeGenModel attribute to append usages to a model. But this feature is not implemented for DPG yet, to be honest I am not quite sure if the Usage and Formats here are working properly for mgmt and HLC. But my point here is that in order to support this, we do not need anything updated on typespec side or TCGC side, we could do it all on the dotnet side.

https://github.com/Azure/autorest.csharp/blob/128ee903c26da989ae46d829187d43acd6474b60/src/assets/Generator.Shared/CodeGenModelAttribute.cs#L16 Despite the comment says the allowed values are input, output, model and error, converter should also be an acceptable value since we have the usage of converter.

ArcturusZhang commented 1 year ago

We now believes that converter should not belong to usage therefore in the typespec world we no longer support the converter usage. But in the meantime, we will create a decorator in TCGC's csharp namespace to let you mark a specific model needs a JSON converter. On the other hand, since we are going to introduce the public serialization story, the converter should no longer be introduced for new models - if we are trying to solve a backward compatibility issue, we could always do that using customized code, I believe it is the case here.

ArcturusZhang commented 1 year ago

Hi @JoshLove-msft considering the existence of this new public serialization feature, new libraries never need the converters any more. And in your case, if you would like to migrate old libraries with converters (because you cannot remove the attribute otherwise breaking change), we could use customization code to add back the converters.

Therefore the generator will not implement it any more, and we plan to remove the usage converter when it is possible.

Close this as a "wont fix"

m-nash commented 1 year ago

We need this for backwards compatibility on libraries that convert from swagger to typespec. Removing an attribute is a breaking change. We plan to do this via a decorator in the tcgc.csharp namespace.

Our recommendation is that this is not used going forward at all but only in the case of back compat.

live1206 commented 4 months ago

we need to implement this in both autorest.csharp and microsoft/typespec

JoshLove-msft commented 1 month ago

Any update on this? This is needed for the Event Grid migration. /cc @m-nash @lmazuel

live1206 commented 1 month ago

I think https://github.com/Azure/autorest.csharp/issues/4887 will resolve this

jsquire commented 1 month ago

@ArthurMa1978, @m-nash: Can we please prioritize this for delivery no later than the second week of August?

live1206 commented 1 month ago

@hasJsonConverter in csharp namespace

live1206 commented 3 weeks ago

This can be enabled by adding @useSystemTextJsonConverter to the model, example can be found.