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

Remove NuGet dependencies version upper limits #4642

Closed paulomorgado closed 10 months ago

paulomorgado commented 10 months ago

Some packages have upper version limits on some of the dependencies which result in NU1608 warnings.

Since this does not prevent the usage of upper version packages, this PR removes that limitation.

Closes #4638

olegd-superoffice commented 10 months ago

Upper limits for Asp.Net Core 2.x dependencies should not be removed. 2.2 is deprecated while 2.1 is the only supported version which will receive security updates. Upper limits are introduced in #4561 for this reason. Probably worth adding a comment about why those upper limit exist in NSwag.Commands.csproj

paulomorgado commented 10 months ago

Upper limits for Asp.Net Core 2.x dependencies should not be removed. 2.2 is deprecated while 2.1 is the only supported version which will receive security updates. Upper limits are introduced in #4561 for this reason. Probably worth adding a comment about why those upper limit exist in NSwag.Commands.csproj

@olegd-superoffice, #4561 does nothing to prevent the user from targeting those versions. It can still be forced.

The same would apply to .NET 3.1 and .NET 5.0.

olegd-superoffice commented 10 months ago

Preventing the user of using anything was not the purpose of that PR. The purpose was to change versions of Microsoft.AspNetCore.*.dll bundled inside NSwag.MSBuild package (in tools\Win folder). Without upper limit the package included deprecated 2.2.x versions which are in fact older than 2.1.x versions of the same dlls and are flagged by various security scanners.

paulomorgado commented 10 months ago

Preventing the user of using anything was not the purpose of that PR. The purpose was to change versions of Microsoft.AspNetCore.*.dll bundled inside NSwag.MSBuild package (in tools\Win folder). Without upper limit the package included deprecated 2.2.x versions which are in fact older than 2.1.x versions of the same dlls and are flagged by various security scanners.

The effective change to the dependency was changing 2.2.0 to 2.1.x and not setting an upper limit.

If that could be a really issue, [2.1.x] could be used instead of just 2.1.x.

olegd-superoffice commented 10 months ago

Having upper limit just makes it explicit, Either way comment in NSwag.Commands.csproj is required to explain why versions 2.1 are used and why those references should not be updated to 2.2

paulomorgado commented 10 months ago

Having upper limit just makes it explicit, Either way comment in NSwag.Commands.csproj is required to explain why versions 2.1 are used and why those references should not be updated to 2.2

It does nothing of the sort. The source code will only use one specific version and it can't target a incompatible runtime.

The end user can still force it in a number of ways.

olegd-superoffice commented 10 months ago

Nothing of which sort? The end user cannot force anything about dlls bundled inside NSwag.MSBuild package.

paulomorgado commented 10 months ago

Nothing of which sort? The end user cannot force anything about dlls bundled inside NSwag.MSBuild package.

And nothing on the dependency version declaration changes that.

But that's not the case with NSwag.Commands. The package does not ship with the dependencies. Only declares the references.

olegd-superoffice commented 10 months ago

So what does removing upper limit achieve here?

As you said, end user can override it if they want. But having upper limit prevents end user from using unsupported library versions accidentally. Especially for people who are using paket for dependency management which would just update everything to the latest version on paket update.

paulomorgado commented 10 months ago

So what does removing upper limit achieve here?

As you said, end user can override it if they want. But having upper limit prevents end user from using unsupported library versions accidentally. Especially for people who are using paket for dependency management which would just update everything to the latest version on paket update.

Because I don't use paket I forgot about that. But I expect this package to not be the only one to cause issues.

I'd need a binlog to analyze that.

Lyra2108 commented 10 months ago

I would also highly welcome this change. We updated to net8 and this package is now causing build warnings for our build.

e.g.

Warning NU1608 : Detected package version outside of dependency constraint: NSwag.Generation.AspNetCore 13.20.0 requires Microsoft.Extensions.DependencyInjection.Abstractions (>= 7.0.0 && < 8.0.0) but version Microsoft.Extensions.DependencyInjection.Abstractions 8.0.0 was resolved.
Warning NU1608 : Detected package version outside of dependency constraint: NSwag.Generation.AspNetCore 13.20.0 requires Microsoft.Extensions.Options (>= 7.0.0 && < 8.0.0) but version Microsoft.Extensions.Options 8.0.0 was resolved.

I see the point that you want to prevent accidental issues with too new versions. But like @paulomorgado mentioned, it is also the only package for us which does this and causes the issue. Which now leaves me with the pain to either void the 0 build warnings policy I bound myself to (It is way simpler to notice new bugs if the build is not flooded with warnings) or ignore the warning which as far as I know I can only do in general so it would leave me blind for other packages issues :/

Also with the paket update: I have not experienced so far that this is a real use case long run in a production environment. Any package update I do, I need to test. Even if I blindly update all of them, I still need to go back and check for the updates done. Usually it is not the dependency tree which causes the issues, especially for the MS libs but more changes within the lib.

olegd-superoffice commented 10 months ago

@Lyra2108 I'm only talking about Microsoft.AspNetCore.* packages and only for .Net Framework 4. Do you have use case where having upper limit on these particular packages causes you any pain?

paulomorgado commented 10 months ago

@olegd-superoffice, can you provide a minimal exemple of how this is an issue for you?

olegd-superoffice commented 10 months ago

It is not an issue for me. Because I'm already very familiar with the versioning mess created by Microsoft with Microsoft.AspNetCore.* packages on .NET 4. The reality of this is that lower (2.1.x) versions of packages are newer and supported while higher versions of packages (2.2.x) are older, insecure and not supported. This is exception and do not apply to other packages. In this reality having explicit upper limit on these versions is helping users to not shoot themselves in the foot is a good thing.

I have already described how it can be an issue for someone using paket. Here's a simple script to reproduce it:

dotnet new classlib -f netstandard2.0
dotnet new tool-manifest
dotnet tool install paket
dotnet tool restore
dotnet paket init
echo nuget NSwag.Commands >> paket.dependencies
dotnet paket update | find "AspNetCore"

This will use current release version of NSwag.Commands (13.20) which doesn't have this upper limit. Observe that versions 2.2.x were installed by paket.

paulomorgado commented 10 months ago

The paket.dependencies I ended up with is:

source https://api.nuget.org/v3/index.json

storage: none
nuget
NSwag.Commands

And got this error:

dotnet paket update
Paket version 8.0.0+6bcb14ec191f11e984ff0e58016f5987a5cfa8f6
Total time taken: 0 milliseconds
Paket failed with
-> Error in paket.dependencies line 4
     could not retrieve NuGet package from

I tried with

dotnet new classlib -f netstandard2.0
dotnet add package NSwag.Commands
dotnet build

And got a dependency of Microsoft.AspNetCore/2.1.7:

sls -path .\bin\Debug\netstandard2.0\nuget.deps.json -s -patt '"Microsoft.AspNetCore/'

D:\Temp\Projects\nswag#4642\nuget\bin\Debug\netstandard2.0\nuget.deps.json:34:      "Microsoft.AspNetCore/2.1.7": {
D:\Temp\Projects\nswag#4642\nuget\bin\Debug\netstandard2.0\nuget.deps.json:2034:    "Microsoft.AspNetCore/2.1.7": {
D:\Temp\Projects\nswag#4642\nuget\bin\Debug\netstandard2.0\nuget.deps.json:2038:      "path": "microsoft.aspnetcore/2.1.7",

However I can force it to reference Microsoft.AspNetCore/2.2.0, even though it's outside the interval:

dotnet add package Microsoft.AspNetCore --version 2.2.0
dotnet build

Now it uses 2.2.0:

sls -path .\bin\Debug\netstandard2.0\nuget.deps.json -s -patt '"Microsoft.AspNetCore/'

D:\Temp\Projects\nswag#4642\nuget\bin\Debug\netstandard2.0\nuget.deps.json:34:      "Microsoft.AspNetCore/2.2.0": {
D:\Temp\Projects\nswag#4642\nuget\bin\Debug\netstandard2.0\nuget.deps.json:2034:    "Microsoft.AspNetCore/2.2.0": {
D:\Temp\Projects\nswag#4642\nuget\bin\Debug\netstandard2.0\nuget.deps.json:2038:      "path": "microsoft.aspnetcore/2.2.0",

paket chooses to make its own interpretation of how NuGet should work and it's paket's responsability to make it work.

olegd-superoffice commented 10 months ago

Are you getting new line in paket.dependencies between nuget and NSwag.Commands? It should look like this (nuget NSwag.Commands in the same line):

source https://api.nuget.org/v3/index.json

storage: none
nuget NSwag.Commands 

Also, you can replace target framework in generated .csproj file with net48, I just used netstandard2.0 because it is supported by classlib template.

paulomorgado commented 10 months ago

Are you getting new line in paket.dependencies between nuget and NSwag.Commands? It should look like this (nuget NSwag.Commands in the same line):

source https://api.nuget.org/v3/index.json

storage: none
nuget NSwag.Commands 

That's what echo nuget NSwag.Commands does on PowerShell.

Also, you can replace target framework in generated .csproj file with net48, I just used netstandard2.0 because it is supported by classlib template.

That will cause errors with dotnet paket update

You can use dotnet paket add Microsoft.AspNetCore --version 2.1.7 to pin the version of Microsoft.AspNetCore.

Sonic198 commented 10 months ago

Hi, any idea when this fix could be released?

olegd-superoffice commented 10 months ago

That will cause errors with dotnet paket update

No, I just tested it. No errors here.

You can use dotnet paket add Microsoft.AspNetCore --version 2.1.7 to pin the version of Microsoft.AspNetCore.

Thanks, I already know how to pin dependencies versions, but my point is that the defaults should be safe to use and that's what upper limit provides here. But can you provide a minimal example of how this existing upper limit is an issue for you?

paulomorgado commented 10 months ago

Setting upper version limits is for when the referencing library does not work with versions above a specific version, which is not the case. It has nothing to do with supportability of the dependencies.

As far as I know, the whole NuGet system was built on the premise that each package should reference the lowest dependencies that match the usage. Upper limits are in place because there might be breaking changes in the dependencies.

Paket, on the other end, chose to opt for the highiest version, knowing the issues that would bring.

I'll leave the final decision to @RicoSuter .

olegd-superoffice commented 10 months ago

Setting upper version limits is for when the referencing library does not work with versions above a specific version, which is not the case. It has nothing to do with supportability of the dependencies.

This is just your opinion, but there are other opinions what upper limit is for. For example Microsoft Kiota developers use different approach. Also in case of Microsoft.Extensions.* packages it is actually the case. See the answer here - versions of these packages should match the major/minor of the ASP.NET Core version you are running.

Anyway, you cannot provide any example of how this limit on .NET 4 is an issue for you?

olegd-superoffice commented 10 months ago

@Lyra2108 BTW, can you try the latest preview of NSwag v14 (14.0.0-preview012 as of today) and see if you still have this warning?

Lyra2108 commented 10 months ago

Well... Yes, the warnings are fixed with the latest preview...

grafik

But I need to do a lot of commented out lines to not have build errors any more and for some e.g. the TypeMappers I could not see quick replacements. So I will need an update guide and quite some time to really use the v14. Therefore, an update for the v13 would be preferred.

RicoSuter commented 10 months ago

@Lyra2108 all these settings are now under SchemaSettings and not on the generator settings itself anymore, should be quick to fix.

Lyra2108 commented 10 months ago

Thanks, that was simpler and faster to get running then expected. :) I am happy to wait for the v14 Release.

paulomorgado commented 10 months ago

@RicoSuter, because it's a, somewhat, unexpected breaking change, this could be attributed as obsolete with warning and proxied for the new API.

In a few releases, it should be an error and removed only on version 15.

I can submit a PR for that.

RicoSuter commented 10 months ago

@paulomorgado what do you mean? Can you create a PR?

paulomorgado commented 10 months ago

@RicoSuter, I just created #4648 for discussion.