Only one tag is returned on operations when OpenApiTagReference is used

[martincostello]

Describe the bug

Having updated Swashbuckle.AspNetCore to 2.0.0-preview.11 and ASP.NET Core 10 preview 3 in domaindrivendev/Swashbuckle.AspNetCore#3283, I've identified an issue where only one tag is returned when a larger set of tag referencess is assigned to an OpenAPI operation.

In creating a self-contained repro for the issue, I find that this is also an issue in 2.0.0-preview.16.

My guess is that there's an issue with how the custom OpenApiTagComparer class is implemented, which is used when assigning a value to the property here:

OpenAPI.NET/src/Microsoft.OpenApi/Models/OpenApiOperation.cs

Lines 29 to 45 in 63a8a34

	public HashSet<OpenApiTagReference>? Tags
	{
	get
	{
	return _tags;
	}
	set
	{
	if (value is null)
	{
	return;
	}
	_tags = value is HashSet<OpenApiTagReference> tags && tags.Comparer is OpenApiTagComparer ?
	tags :
	new HashSet<OpenApiTagReference>(value, OpenApiTagComparer.Instance);
	}
	}

Specifically, it looks like it doesn't cater for an OpenApiReference where Name resolves to null:

OpenAPI.NET/src/Microsoft.OpenApi/OpenApiTagComparer.cs

Lines 20 to 35 in 63a8a34

	public bool Equals(IOpenApiTag? x, IOpenApiTag? y)
	{
	if (x is null && y is null)
	{
	return true;
	}
	if (x is null || y is null)
	{
	return false;
	}
	if (ReferenceEquals(x, y))
	{
	return true;
	}
	return StringComparer.Equals(x.Name, y.Name);
	}

Code To Reproduce

using Microsoft.OpenApi.Models;
using Microsoft.OpenApi.Models.References;

var document = new OpenApiDocument();

var tags = new HashSet<OpenApiTagReference>();

tags.Add(new OpenApiTagReference("one", document));
tags.Add(new OpenApiTagReference("two", document));
tags.Add(new OpenApiTagReference("three", document));

Console.WriteLine("There are {0} original tags.", tags.Count);
Console.WriteLine("The original tags are: {0}", string.Join(", ", tags.Select(t => t.Reference.Id)));

var operation = new OpenApiOperation() { Tags = tags };

Console.WriteLine("There are {0} tags in the operation.", operation.Tags.Count);
Console.WriteLine("The operation tags are: {0}", string.Join(", ", operation.Tags.Select(t => t.Reference.Id)));

<Project Sdk="Microsoft.NET.Sdk">
 <PropertyGroup>
 <OutputType>Exe</OutputType>
 <TargetFramework>net9.0</TargetFramework>
 <ImplicitUsings>enable</ImplicitUsings>
 <Nullable>enable</Nullable>
 </PropertyGroup>
 <ItemGroup>
 <PackageReference Include="Microsoft.OpenApi" Version="2.0.0-preview.16" />
 </ItemGroup>
</Project>

Expected behavior

There are 3 original tags.
The original tags are: one, two, three
There are 3 tags in the operation.
The operation tags are: one, two, three

Actual behaviour

There are 3 original tags.
The original tags are: one, two, three
There are 1 tags in the operation.
The operation tags are: one

[baywet]

Hi Martin Costello (@martincostello),
Thank you for using the SDK and for reaching out.

Are you able to reproduce the behaviour when the document contains the tag definitions? (from your description, I'd assume that no)

[martincostello]

There were just two tests failing in Swashbuckle from this difference, and I've not seen it in any other projects, so I guess not.

Regardless of them being "real" tags or not, they're all equivalently "not real", and it doesn't make any sense for the code to treat N tags as all equal to each other when they aren't the same object and all refer to different tags.

All three being returned makes the most sense to me, but if you wanted to argue they should be ignored because they aren't in the document, then there should be 0 tags, not 1.

[baywet]

I guess we could work this around by type testing to a tag reference, and if both pass the test, comparing the reference Id instead.

The behaviour for failing to resolve tag references is undefined at this point, I guess it makes sense to keep them in memory in the object model so the library user gets a chance to resolve the situation (i.e. things to disappear magically).

What happens during serialization time is a different story, serialization resulting in an invalid document is bad, so we should probably filter out the values which can't be resolved.

What do you think?

[martincostello]

Yeah the current behaviour is definitely confusing. You create a HashSet<T> containing N items, but then once you assign it and read it back there's only 1.

If you want to enforce that only tag references that exist at the document level are present, it would probably be much less confusing for the consumer to do that at the point of serialization, then things aren't confusing in the debugger and code and different places can inspect things without objects disappearing.

I've changed Swashbuckle to create the document tags before it adds the operation tags so they don't disappear (if you do it the other way around you still get the original problem) (domaindrivendev/Swashbuckle.AspNetCore@a0bf6d1), but I think something needs changing here. I only worked out what was going on by digging into the source here and finding the custom comparer - I'm not sure many people would bother doing that, and would just raise an issue saying there was a bug and be very confused.

[baywet]

Thank you for the additional information.

Let's do this then:

1. update the comparer to compare reference ids when items are references.
2. add a validation to throw on unresolved references during serialization of tag references which do not resolve.

Is this something you'd like to submit a pull request for provided some guidance?

[martincostello]

Number 1 should be trivial to pick up and fix.

Number 2 less so - you've probably got a better idea of how you want that to work so maybe it's better suited for the team to pick up and do.

[baywet]

These are the places where the tag references serialization is happening.

OpenAPI.NET/src/Microsoft.OpenApi/Models/OpenApiOperation.cs

Line 183 in 25136af

writer.WriteOptionalCollection(

OpenAPI.NET/src/Microsoft.OpenApi/Models/OpenApiOperation.cs

Line 237 in 25136af

writer.WriteOptionalCollection(

And unit tests should be added here.

OpenAPI.NET/test/Microsoft.OpenApi.Tests/Models/OpenApiOperationTests.cs

Line 17 in 25136af

{

Let us know if you have any additional comments or questions.