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

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

Open
null-d3v opened this issue Oct 31, 2022 · 15 comments
Open

Comments

@null-d3v
Copy link

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
Copy link

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

@hmoratopcs
Copy link

@paulomorgado

@paulomorgado
Copy link
Contributor

paulomorgado commented Dec 28, 2023

.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
Copy link

hmoratopcs commented Dec 28, 2023

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:

  • At Nswag.Msbuild 13.20.0, it is ignored. I found this old commit (996b61f) that implies that the last flag is used, but it's the other way around, the FIRST one is used in 13.20.0.
  • At Nswag.Msbuild 14.0.0-preview012, an exception is thrown complaining about the duplicated GenerateClientInterfaces flag. Unrecognised arguments are present: [/GenerateExceptionClasses:true]

I think the problem could be solved by tweaking the logic in

<Command Condition="!%(FirstForGenerator) OR !%(NSwagGenerateExceptionClasses)">%(Command) /GenerateExceptionClasses:false</Command>
to not add the false flag if the true flag has been set explicitly.

@paulomorgado
Copy link
Contributor

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
Copy link

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 (996b61f) I mentioned.

@paulomorgado
Copy link
Contributor

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
Copy link
Contributor

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

Can someone test if the changes solve this issue?

@paulomorgado
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

Have you tried /ExceptionClass:ServiceException?

@janrhansen
Copy link

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
Copy link
Contributor

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

@janrhansen
Copy link

@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
Copy link
Contributor

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants