search

RicoSuter / NSwag

The Swagger/OpenAPI toolchain for .NET, ASP.NET Core and TypeScript.
http://NSwag.org
MIT License
6.78k stars 1.29k forks source link

C# generated code: exception classes are not created in subsequent references #4205

Open null-d3v opened 2 years ago

null-d3v commented 2 years ago

When using multiple OpenApiReferences with NSwagCSharp generation, exception classes are only generated for the first listed dependency.

Example

<ItemGroup>
  <OpenApiReference Include="OpenApiReference/Service1_openapi.json" SourceUrl="http://localhost:5050/openapi.json">
    <CodeGenerator>NSwagCSharp</CodeGenerator>
    <ClassName>{controller}Service</ClassName>
    <Namespace>Service1</Namespace>
    <Options>/ExceptionClass:ServiceException /GenerateClientInterfaces:true /GenerateOptionalParameters:true</Options>
  </OpenApiReference>
  <OpenApiReference Include="OpenApiReference/Service2_openapi.json" SourceUrl="http://localhost:5051/openapi.json">
    <CodeGenerator>NSwagCSharp</CodeGenerator>
    <ClassName>{controller}Service</ClassName>
    <Namespace>Service2</Namespace>
    <Options>/ExceptionClass:ServiceException /GenerateClientInterfaces:true /GenerateOptionalParameters:true</Options>
  </OpenApiReference>
  <OpenApiReference Include="OpenApiReference/Service3_openapi.json" SourceUrl="http://localhost:5052/openapi.json">
    <CodeGenerator>NSwagCSharp</CodeGenerator>
    <ClassName>{controller}Service</ClassName>
    <Namespace>Service3</Namespace>
    <Options>/ExceptionClass:ServiceException /GenerateClientInterfaces:true /GenerateOptionalParameters:true</Options>
  </OpenApiReference>
  </ItemGroup>
<ItemGroup>

ServiceException definitions in the Service2 and Service3 generated files are absent, resulting in compiler failures for those two references. This cannot be rectified by explicitly using GenerateExceptionClasses; the exceptions will continue to fail to generate. Using {controller} placeholders for the exception class also has the same result.

janrhansen commented 1 year ago

Bump - same issue here. Did you find a workaround?

hmoratopcs commented 10 months ago

@paulomorgado

paulomorgado commented 10 months ago

.NET's OpenAPI client generation has a concept of FirstForGenerator and NSwagCSharp uses that to generate exception classes only on the first execution of the generator.

That makes sense to me, as I don't want a different type of exception for all client classes. In fact, I use my own exception class and never generate.

You might try something like this:

<ItemGroup>
  <OpenApiReference Include="OpenApiReference/Service1_openapi.json" SourceUrl="http://localhost:5050/openapi.json">
    <CodeGenerator>NSwagCSharp</CodeGenerator>
    <ClassName>{controller}Service</ClassName>
    <Namespace>Service1</Namespace>
    <FirstForGenerator>true</FirstForGenerator>
    <Options>/ExceptionClass:ServiceException /GenerateClientInterfaces:true /GenerateOptionalParameters:true</Options>
  </OpenApiReference>
  <OpenApiReference Include="OpenApiReference/Service2_openapi.json" SourceUrl="http://localhost:5051/openapi.json">
    <CodeGenerator>NSwagCSharp</CodeGenerator>
    <ClassName>{controller}Service</ClassName>
    <Namespace>Service2</Namespace>
    <Options>/ExceptionClass:ServiceException /GenerateClientInterfaces:true /GenerateOptionalParameters:true</Options>
    <FirstForGenerator>true</FirstForGenerator>
  </OpenApiReference>
  <OpenApiReference Include="OpenApiReference/Service3_openapi.json" SourceUrl="http://localhost:5052/openapi.json">
    <CodeGenerator>NSwagCSharp</CodeGenerator>
    <ClassName>{controller}Service</ClassName>
    <Namespace>Service3</Namespace>
    <Options>/ExceptionClass:ServiceException /GenerateClientInterfaces:true /GenerateOptionalParameters:true</Options>
    <FirstForGenerator>true</FirstForGenerator>
  </OpenApiReference>
  </ItemGroup>
<ItemGroup>

But I'm not sure it would work.

You have a very particular and odd use case here. It will work if you split your clients into different projects.

Either way, I would recommend creating your own compatible exception and using that one in all clients.

Use https://aka.ms/msbuildlog to debug your build.

hmoratopcs commented 10 months ago

Thank you very much, @paulomorgado for your quick response.

Adding <FirstForGenerator>true</FirstForGenerator> didn't have any effect for me, tested using Nswag.Msbuild/NSwag.ApiDescription.Client 13.20.0 and 14.0.0-preview012 (and Microsoft.Extensions.ApiDescription.Client 7.0.2).

I tried adding /GenerateExceptionClasses:true to Options, the problem is:

I think the problem could be solved by tweaking the logic in https://github.com/RicoSuter/NSwag/blob/b7190d9f64a73f5054c8e2cae984758d4dcba11c/src/NSwag.ApiDescription.Client/NSwag.ApiDescription.Client.targets#L22 to not add the false flag if the true flag has been set explicitly.

paulomorgado commented 10 months ago

Have you tried FirstForGenerator like I mentioned?

How were you doing this before?

Looks like you are trying to use OpenApiReference in a way that was never intended to.

hmoratopcs commented 10 months ago

Yes, adding <FirstForGenerator>true</FirstForGenerator> didn't have any effect for me.

Before, we where launching the nswag command from the console.

What was never intended? I think the use case is pretty starightforward: add two references with different namespaces for each. This should "just work", but it doesn't compile without flags because the second misses the exception class. I agree with you that it's not best practice to have two exception classes, and the best practice would be to add /GenerateExceptionClasses:false to Options and provide your own exception class, but the simpler case without flags should work.

If you mean that /GenerateExceptionClasses:true was never intended to be added to Options, I think it was, according to the commit (https://github.com/RicoSuter/NSwag/commit/996b61f21682b447a17616e2a0bf34ba340057bf) I mentioned.

paulomorgado commented 10 months ago

Yes, adding <FirstForGenerator>true</FirstForGenerator> didn't have any effect for me.

It was a long shot.

Before, we where launching the nswag command from the console.

For the common use case of OpenApiReference, that is the equivalent of having multiple projects.

What was never intended? I think the use case is pretty starightforward: add two references with different namespaces for each. This should "just work", but it doesn't compile without flags because the second misses the exception class. I agree with you that it's not best practice to have two exception classes, and the best practice would be to add /GenerateExceptionClasses:false to Options and provide your own exception class, but the simpler case without flags should work.

Have you found any .NET documentation supporting your claims that this particular case should just work?

If you mean that /GenerateExceptionClasses:true was never intended to be added to Options, I think it was, according to the commit (996b61f) I mentioned.

OpenApiReference provides only information about the first execution in each project. Because the most common use case is to generate everything in the same namespace for the same project, NSwagCSharp uses FirstForGenerator to generate the exception class only for the first execution. Otherwise, it would generate the same class multiple times and the code would fail to compile.

paulomorgado commented 10 months ago

I've created #4668, but haven't tested it.

Can someone test if the changes solve this issue?

paulomorgado commented 10 months ago

This has proven to be a hard to resolve issue, given the infrastructure provided by the .NET SDK and NSwag tooling.

On the other hand, it is easy to customize and you can build your own for your own needs.

When everything else fails: Exec task

janrhansen commented 8 months ago

So, as far as I understand it - the conclusion is that 1) we cannot force nswag to generate one exception-class per client, and 2) we cannot force it to use our own custom class either (I'm unsure if the /ExceptionClass:ServiceException parameter works?) So we have to create each client in separate projects?

I may misunderstand something general about OpenApiReference - but on a "logic" level, I agree with @hmoratopcs: "I think the use case is pretty starightforward: add two references with different namespaces for each. This should "just work" I don't know if it HAS to work (or not) due to some specification, but I surely would like to be able to generate more than one API client in the same project, when consuming APIs in our microservice architecture. So just add the <OpenApiReference ...>...</OpenApiReference> for each client I want, and I can live with the "same" exception class being generated as many times as I have references. Is that a "bad" approach? Especially compared to the alternatives of a) create a project for each API client I want generated. The exception class will then be repeated across multiple projects instead, or b) call the toolchain otherwise. Right now we do it much like exec task but conditionally so only in debug, i.e. locally when developing and hence changes to the generated class will only occur once we update the downloaded swagger.json. The exception class is again repeated but within the same project under different namespaces, like Product.Integration.Clients.FirstClient. and Product.Integration.Clients.SecondClient.

Comment and ideas are welcome - as well as support for "just generating the exception class every single time". If possible, of course. I can't really decide if what @paulomorgado is saying is that this simply cannot be done due to other limitations outside of nswag. I hope not :)

paulomorgado commented 8 months ago

Have you tried /ExceptionClass:ServiceException?

janrhansen commented 8 months ago

Have you tried /ExceptionClass:ServiceException?

No, I have not. Based on the original post from @null-d3v it doesn't look like it works:

ServiceException definitions in the Service2 and Service3 generated files are absent, resulting in compiler failures for those two references. Is it intented to work with multiple generated clients, or just for customizing the name of the class you will get for your first and only client?

I believe that part of my post was the "least interesting" :) What I find more relevant is the part of "how could it be fixed". Is it somehow "wrong" to just have the exception classes generated each and every time? It's inside the clients namespace anyway, so I don't really see issue. It would be precisely the same if I moved every client to its own project, but would cause more overhead.

Maybe you could explain to me (since I obviously don't get it):

1) Is there a reason for not generating the exception class every time 2) "should" it be possible to have it generated the first time, and then in other generated clients, refer to that single, first class?

paulomorgado commented 8 months ago

@janrhansen, it could be possible. After all, it's all software. Feel free to submit a PR.

janrhansen commented 8 months ago

@paulomorgado Thanks.. The problem is (or may be) that I might be mistaken here. If the current behavior is in fact "correct", then submitting a PR with a change will probably break it for other users, which nobody wants. What I'm seeking is a confirmation that this is actually a "bug" (or, inconvenience) and as such someone could fix it - or a better understanding of how the tool is supposed to work, if it is not a "bug". Since I don't know the first thing about this, but only use the nswag tools to avoid trivial http client generation, I reach out to you guys who have built the tools with (from my perspective) a simple question:

Is there a reason for not generating the exception class every time.

If there is, I probably shouldn't bother figuring out how to change this behavior :) If not, I will see if I can find time to help, but with two kids etc. this might not be anytime soon :)

paulomorgado commented 8 months ago

@janrhansen, nothing like a proof of concept to clear everyone's doubts.

You have to take into account the development and maintenance costs and the percentage of users that require this and are blocked from doing it in any other way.