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

One machine generates an open API document with descriptions for ProblemDetails and another machine generates them without #3465

Closed sandord closed 3 years ago

sandord commented 3 years ago

I've got this weird case where one machine generates an open API document with descriptions for ProblemDetails (from Microsoft.AspNetCore.Mvc) and another machine generates them without.

Here's an excerpt:

"ProblemDetails": {
    "type": "object",
    "description": "A machine-readable format for specifying errors in HTTP API responses based on https://tools.ietf.org/html/rfc7807.",
    "additionalProperties": {
      "nullable": true
    },
    "properties": {
      "type": {
        "type": "string",
        "description": "A URI reference [RFC3986] that identifies the problem type. This specification encourages that, when\ndereferenced, it provide human-readable documentation for the problem type\n(e.g., using HTML [W3C.REC-html5-20141028]).  When this member is not present, its value is assumed to be\n\"about:blank\".",
        "nullable": true
      },
      "title": {
        "type": "string",
        "description": "A short, human-readable summary of the problem type.It SHOULD NOT change from occurrence to occurrence\nof the problem, except for purposes of localization(e.g., using proactive content negotiation;\nsee[RFC7231], Section 3.4).",
        "nullable": true
      },

And the same document but now without descriptions:

"ProblemDetails": {
    "type": "object",
    "additionalProperties": {
      "nullable": true
    },
    "properties": {
      "type": {
        "type": "string",
        "nullable": true
      },
      "title": {
        "type": "string",
        "nullable": true
      },

I'm using the exact same code on both machines, calling AspNetCoreOpenApiDocumentGenerator.

I've checked the generation configuration for anything referring to the inclusion of descriptions/summaries/documentation but I didn't find it.

The only other thing I can think of right now is the absence of an Microsoft.AspNetCore.Mvc.XML file on the machine that generates the document without the descriptions. But that file, nor its DLL counterpart, is not in my bin directory. That's probably due to the fact that my project is configured with the Microsoft.NET.Sdk.Web SDK and the Mvc DLL is referenced implicitly.

jeremyVignelles commented 3 years ago

Do both machines have the same NSwag version ? I think something changed recently around that, but I'm not sure.

RicoSuter commented 3 years ago

It's the new xml docs file look up via nuget cache: https://github.com/RicoSuter/Namotion.Reflection/blob/master/src/Namotion.Reflection/XmlDocsExtensions.cs#L801

The main problem is that with .net core 3.1 transitive nuget dependencies only output dlls but no xml docs so we need to search them in all possible locations (nuget cache, asp.net core installation), it's super hacky...

Probably this now works only on Windows and on Linux it fails to find the xml docs files from the dotnet core installation (it's part of the asp.net core sdk which is somewhere in the dotnet installation).

RicoSuter commented 3 years ago

Maybe you can clone Namotion.Reflection and run the tests on the failing machine? There is a test specifically for ProblemDetails :-)

sandord commented 3 years ago

Do both machines have the same NSwag version ? I think something changed recently around that, but I'm not sure.

Yes, exactly the same version (13.11.1). The machines are practically identical. Except for that little detail that's causing the issue of course ;-)

sandord commented 3 years ago

It's the new xml docs file look up via nuget cache: https://github.com/RicoSuter/Namotion.Reflection/blob/master/src/Namotion.Reflection/XmlDocsExtensions.cs#L801

The main problem is that with .net core 3.1 transitive nuget dependencies only output dlls but no xml docs so we need to search them in all possible locations (nuget cache, asp.net core installation), it's super hacky...

Probably this now works only on Windows and on Linux it fails to find the xml docs files from the dotnet core installation (it's part of the asp.net core sdk which is somewhere in the dotnet installation).

Ah, I see. It would be nice if we could opt-in to this new feature somehow, given its current instability.

sandord commented 3 years ago

Maybe you can clone Namotion.Reflection and run the tests on the failing machine? There is a test specifically for ProblemDetails :-)

Thanks, that sounds like a plan. I'm on a few days leave now so I'll check it out when I get back.

sandord commented 3 years ago

Maybe you can clone Namotion.Reflection and run the tests on the failing machine? There is a test specifically for ProblemDetails :-)

I ran all 80 tests on the failing machine and they all succeeded 🤔 This was on the machine that did not produce any descriptions/summaries for ProblemDetails.

scottsauber commented 3 years ago

We are hitting this same problem with a team that has some on macOS and some on Windows. This is causing our autogenerated TypeScript file to generate differences depending on who compiles it. Is there any workaround for this to turn this off or generate deterministically regardless of OS?

mikenunan commented 3 years ago

I've just run into this one too, it's particularly strange as colleagues with the same versions of everything and the exact same packs (confirmed with machine-wide searches of Microsoft.AspNetCore.Mvc.Core.xml which contains the xmldoc for ProblemDetails) don't pick up the docs whereas I do. Count me as another vote for an option to disable or otherwise ensure deterministic behaviour please.

mikenunan commented 3 years ago

I've just spent a chunk of time figuring out a workaround to this so I thought I'd share details here. Turns out that the significant difference on my workstation versus all my teammates' is that my SDK is putting "C:\\Program Files\\dotnet\\sdk\\NuGetFallbackFolder" in additionalProbingPaths in the MyProject.runtimeconfig.dev.json that it generates if you don't supply one explicitly for the project.

If I supply my own MyProject.runtimeconfig.dev.json with an empty path list in the JSON it will be used and the end result is the same generated code output as everyone else, without the additional xmldoc for ProblemDetails. Example:

{
  "runtimeOptions": {
    "additionalProbingPaths": [
    ]
  }
}

However, this only works (in VS2019 16.10.4 and SDK 5.0.302 anyway) if the explicty JSON file is more recent than the copy in bin\Debug\net5.0. In the end I resorted to adding a PostBuild step to the csproj as follows:

  <Target Name="PostBuild" AfterTargets="PostBuildEvent">
    <Exec Command="copy /b MyProject.API.runtimeconfig.dev.json+" />
  </Target>

(That just does the equivalent of a Linux touch on the JSON and means that the build will do the right thing second time around, although not on the first or after a clean.)

And note that in .NET 6 runtime config will no longer be generated by default anyway, which will automatically sidestep this problem (see https://github.com/dotnet/sdk/issues/16818 and https://github.com/dotnet/sdk/pull/17014)

Stepping back, the root cause of this is actually in the Namotion code I think. Using procmon to compare what was happening on a colleague's machine relative to mine revealed some strange behaviour. Although I do have a copy of Microsoft.AspNetCore.Mvc.Core.xml in the NuGetFallbackFolder structure, NSwag wasn't using that one, it carried on searching and used:

C:\Program Files\dotnet\packs\Microsoft.AspNetCore.App.Ref\5.0.0\ref\net5.0\Microsoft.AspNetCore.Mvc.Core.xml

It seems very strange that getting removing additionalProbingPath entries unrelated to any parent of that path changed the behaviour for me, but that's what happened. My colleague does also have the same file at that same path, but in his case NSwag is crashing out with an exception before it gets to it (can see a Windows Error Reporting dump report for it).

@RicoSuter if you want more info about any of this please reach out and I'll be happy to provide.

RicoSuter commented 3 years ago

@mikenunan

The big problem here is that the xml docs dlls in nuget packages (any external nuget and also asp.net core) are not put into the output folder so we need to look them up in the nuget cache folder, etc. this is super hacky at the moment. You can find the code here:

https://github.com/RicoSuter/Namotion.Reflection/blob/master/src/Namotion.Reflection/XmlDocsExtensions.cs#L801

There are also unit tests for this here to test or debug this on different machines: https://github.com/RicoSuter/Namotion.Reflection/blob/master/src/Namotion.Reflection.Tests/XmlDocsExtensionsTests.cs#L598 https://github.com/RicoSuter/Namotion.Reflection/blob/master/src/Namotion.Reflection.Tests/XmlDocsExtensionsTests.cs#L611

The problem is that this works on some machines/OSes and does not on other, so the output is not stable... Any better idea how to solve this?

sandord commented 3 years ago

It would be something if we could disable the scanning altogether so we can at least have consistent results across machines.

RicoSuter commented 3 years ago

It would be something if we could disable the scanning altogether so we can at least have consistent results across machines.

In next version this can be disabled with XmlDocs.ResolveFromNuGetCacheOrDotNetSdk = false; - just call this before generation, e.g. in Program.cs

RicoSuter commented 3 years ago

Created a new issue in NR: https://github.com/RicoSuter/Namotion.Reflection/issues/88

scottsauber commented 3 years ago

@RicoSuter - is that property exposed in the nswag.json as well? We just use msbuild calling nswag.exe to auto generate our TS models

mikenunan commented 3 years ago

@RicoSuter

Can't suggest any better idea as yet, but if time allows I will pull down a local build and see if I can at least figure out what's blowing up with that WER fault. I'll let you know if I succeed with that.

Ref @scottsauber's comment we are in that same boat too, running dotnet-nswag from msbuild so it'd be great if we can have control of that option in the json or on the command line pretty please!

RicoSuter commented 3 years ago

is that property exposed in the nswag.json as well? We just use msbuild calling nswag.exe to auto generate our TS models

This needs to be done in the spec generator and not code generator, and for this you already need code in your project (ie the AddOpenApiDocument()), so the CLI anyways calls into this code, adding this directly before AddOpenApiDocument() shouldnt be a problem, no?

scottsauber commented 3 years ago

That should work. Thanks @RicoSuter for adding this! Looking forward to trying this out on the next version.

RicoSuter commented 3 years ago

Soon: https://github.com/RicoSuter/NSwag/pull/3582 (v13.13.0) However, I really want to find a fix for this - not just disable it :-)

scottsauber commented 3 years ago

@RicoSuter - if you want I can take a look if you point me in a direction (other than what’s mentioned above) or if you wanna hop on a call and pair on it. I have both a Mac and a Windows machine available.

RicoSuter commented 3 years ago

@scottsauber that would be nice, what additional info do you need on top of the stuff in https://github.com/RicoSuter/Namotion.Reflection/issues/88 I'd say that you should be able to run and debug the mentioned unit test and if it's not working on your machine, try to find a solution how to get the xml docs path :-)