AdvertiseApiVersionsAttribute causes stackoverflow when enumarated

[marius-w-nilsen]

Is there an existing issue for this?

• I have searched the existing issues

Describe the bug

Adding [AdvertiseApiVersions] attribute to either Controller or Method causes stack overflow exception when enumarted.

Because AdvertiseApiVersionsAttribute.GetHashCode() doesn't call base.GetHashCode() it causes a stackoverflow when enumerated. I.e when Swagger generates docs (Swashbuckle.AspNetCore.SwaggerGen.SwaggerGenerator.GenerateResponse)

AdvertiseApiVersionsAttribute

Expected Behavior

Adding [AdvertiseApiVersions] does not cause stack overflow when enumerated.

Steps To Reproduce

Add [AdvertiseApiVersions(3.0)] attribute to a controller V2/Controllers/OrdersController.cs in your example (AspNetCore/OpenApiExample)

Exceptions (if any)

If I completely misunderstand the reason for [AdvertiseApiVersions]

.NET Version

7.0.100

Anything else?

I was unable to get the FormatProviderTest to run locally, otherwise this would've been a PR 😢

[commonsensesoftware]

Confirmed. This is a 🐞 . I'm surprised this has never been caught by code analysis or a test.

I just discovered a different issue related to reference assembly package. I don't understand why it ever worked, but that's probably why the test is failing. The problem happens in Visual Studio, but not the CLI. You might try dotnet build or just dotnet test and see if it works.

I work on get a patch out. Thanks for reporting it.

[marius-w-nilsen]

Thanks!
Yep, tests run from CLI,
Noticed potential culture problems (:scream:) in the formatting tests though.
Expected status to be "Major = 1, Minor = 0, Iteration = 1.0", but "Major = 1, Minor = 0, Iteration = 1,0" differs near ",0" (index 35).

Console.WriteLine(string.Format(new CultureInfo("nb-no"), "{0:N1}", 1));  //1,0 <-- not expected in test
Console.WriteLine(string.Format(new CultureInfo("en-us"), "{0:N1}", 1));  //1.0
Console.WriteLine(string.Format( CultureInfo.InvariantCulture, "{0:N1}", 1)); //1.0

From MultipleFormatParameterData

  yield return new object[] { provider, "Major = {0:V}, Minor = {0:v}, Iteration = {1:N1}", 1, "Major = 1, Minor = 0, Iteration = 1.0" };

In Culture "nb-no" a comma is used as decimal seperator which doesn't follow Microsoft API versioning guidelines as far as I can tell, but I must admit I don't know what the "Iteration" argument is used for.

Looking around if I can spot why, but if it's an automatic for you, at least the information is shared 😄

[commonsensesoftware]

Whoah! That is a good find. I guess I never thought about that (yay en-us only 🤣). Supporting IFormatProvider is being compliant with the API design, but - honestly - the only option that makes sense is CultureInfo.InvariantCulture. I mean, an API version is not culture-specific. The , is the standard decimal character in Arabic numerals, which is what is used by the majority of the world. I know en-us and en-gb are a few of cultures that use Arabic numerals, but with . instead of ,.

Aside: I've always wondered how you can tell the mantissa component with ,. Is 100,000 the same as 100.000? Maybe it's supposed to be 100000 versus 100,000 == 100.000 🤔 .

This is a fixable problem. I'll update the test harness so that it explicitly sets the CultureInfo.CurrentCulture to en-us. The code won't know that's happening, but that will be provide deterministic results - that will work.

Thanks for taking the time to try and sharing the results.

[commonsensesoftware]

6.2.1 of Asp.Versioning.Abstractions has been published with the fix. The fix will be merged into the final 7.0 version too.

[marius-w-nilsen]

Using 6.2.1 of Abstractions fixes the problem.
About the culture, i had to [AssumeCulture] in one more test before i got all tests in .sln to run.
MMM generates aug and not expected Aug

I tried to create a PR for this, excuse my novice OSS contribution knowledge: #936

[commonsensesoftware]

No problem. Looks good for a simple change. Are you using Visual Studio, VS Code, or some other editor? I did notice a nitpick on the format spacing. I curious because the build is supposed to fail for violations and VS or VS Code should have been able to detect and apply a Quick Fix from .editorconfig. I enforce it upon myself, but it supposed be easy for other people to pick up and run with. There aren't many contributors so I'm looking for feedback.

[marius-w-nilsen]

VS Code, but i am not sure why the settings.json in the repo was not respected? Cannot speak for the builds however.
I didn't have the editorconfig extension beforehand, maybe thats why?

[commonsensesoftware]

I believe VS Code has native .editorconfig support. You might need an extension for fancy, UI modifications of the file. It should have been caught by StyleCop and resulted in an error during the build. Failing on that, yeah, the settings.json should have kicked in. That was the whole reason why I added it. 🤷🏽 Oh well, that's for the putting in the effort. We got it in. 😄