Cannot use a custom sort for OpenApiDocument.Tags in v2

[spanglerco]

Describe the bug
In the v1 API, OpenApiDocument.Tags was an IList<OpenApiTag>, enabling us to order the tags however we want (e.g., in a Swashbuckle document filter). In the v2 API, the property is an ISet<OpenApiTag>, which, while more difficult, would let us use a custom ordering via SortedSet<OpenApiTag> with a custom comparer. However, the OpenApiDocument.Tags setter doesn't actually preserve the collection you give it, unless it's specifically a HashSet<OpenApiTag> that uses OpenApiTagComparer.

Technically, fully populating a SortedSet<OpenApiTag> before assigning it to OpenApiDocument.Tags often "works", because the current HashSet implementation will enumerate in the same order as insertion in many cases (I think it's if there aren't any hash code collisions and you don't remove any elements and add more). But initializing OpenApiDocument.Tags to a SortedSet<OpenApiTag> and then populating it doesn't produce the desired result.

OpenApi File To Reproduce
Any document with multiple tags. For example:

{
  "openapi": "3.0.4",
  "tags": [
    {
      "name": "Data",
      "description": "..."
    },
    {
      "name": "Export",
      "description": "..."
    },
    {
      "name": "Tables",
      "description": "..."
    },
    {
      "name": "Versioning",
      "description": "..."
    }
  ]
}

Expected behavior
I can order the document's tags such that the serialized document maintains the order I want instead of alphabetic:

{
  "openapi": "3.0.4",
  "tags": [
    {
      "name": "Versioning",
      "description": "..."
    },
    {
      "name": "Tables",
      "description": "..."
    },
    {
      "name": "Data",
      "description": "..."
    },
    {
      "name": "Export",
      "description": "..."
    }
  ]
}

Screenshots/Code Snippets
Swashbuckle document filter that works with the v1 API:

public void Apply(OpenApiDocument swaggerDoc, DocumentFilterContext context)
{
    var tagList = new SortedList<int, OpenApiTag>();
    // The actual code discovers the tags using the ApiExplorer
    tagList.Add(2, new OpenApiTag { Name = "Data" });
    tagList.Add(3, new OpenApiTag { Name = "Export" });
    tagList.Add(1, new OpenApiTag { Name = "Tables" });
    tagList.Add(0, new OpenApiTag { Name = "Versioning" });
    swaggerDoc.Tags = tagList.Values;
}

Code that "works" in v2, assuming the HashSet cooperates:

public void Apply(OpenApiDocument swaggerDoc, DocumentFilterContext context)
{
    var tagSet = new SortedSet<OpenApiTag>(new OrderedOpenApiTagComparer());
    tagSet.Add(new OrderedOpenApiTag(2) { Name = "Data" });
    tagSet.Add(new OrderedOpenApiTag(3) { Name = "Export" });
    tagSet.Add(new OrderedOpenApiTag(1) { Name = "Tables" });
    tagSet.Add(new OrderedOpenApiTag(0) { Name = "Versioning" });
    swaggerDoc.Tags = tagSet; // The setter copies the tags into a new HashSet
}

private sealed class OrderedOpenApiTag(int order) : OpenApiTag
{
    public int Order { get; } = order;
}

private sealed class OrderedOpenApiTagComparer : IComparer<OpenApiTag>
{
    public int Compare(OpenApiTag? x, OpenApiTag? y)
    {
        var xOrder = (x as OrderedOpenApiTag)?.Order ?? -1;
        var yOrder = (y as OrderedOpenApiTag)?.Order ?? -1;
        return xOrder.CompareTo(yOrder);
    }
}

Additional context
Perhaps the property setter should only override the ISet implementation if given a HashSet that uses the default comparer and/or an empty set?

[baywet]

Hi @spanglerco
Thank you for using the SDK and for reaching out.

I think that should be easily fixable with the following change in the setter

+_tags = value is HashSet<OpenApiTag> tags && tags.Comparer != EqualityComparer<OpenApiTag>.Default ?
-_tags = value is HashSet<OpenApiTag> tags && tags.Comparer is OpenApiTagComparer ?

And corresponding unit test.

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

Start your work from support/v2 and target support/v2 so we can easily propagate the changes to v3 as well.

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

[spanglerco]

Hi @baywet, I think that would still require the passed in set be a HashSet<T>. Perhaps it could be an OR:

+_tags = value is not HashSet<OpenApiTags> hashSet || hashSet.Comparer != EqualityComparer<OpenApiTag>.Default ?
+    value :
-_tags = value is HashSet<OpenApiTag> tags && tags.Comparer is OpenApiTagComparer ?
-    tags :

I'll see if I have any time to submit something.

[baywet]

Thank you for the additional information.

I think the main order of that change when I made it was to have documents that are compliant with the specification when duplicated tags are being passed to the object model.

Each tag name in the list MUST be unique.

source

Exiting on a different type will lead to most other ISet implementations NOT to have the deduplication implementation, which is most likely a regression we don't want. And with the collection expressions, it's likely that most people "won't know" which implementation they are using.

The only ISet implementations that maintain order are SortedSet and ImmutableSortedSet (unless somebody is rolling out a custom implementation, but that's probably a very limited number of people...). In that case it might be preferable to assumed that people passing a sorted set like you, know what they are doing, and took care of the duplication themselves.

So maybe something like that would be better in the end

+_tags = value is HashSet<OpenApiTag> tags && tags.Comparer != EqualityComparer<OpenApiTag>.Default || value is SortedSet<OpenApiTag> || value is ImmutableSortedSet<OpenApiTag> ?
-_tags = value is HashSet<OpenApiTag> tags && tags.Comparer is OpenApiTagComparer ?

thoughts?

[spanglerco]

@baywet Yeah, I think a whitelist is reasonable. For what it's worth, ISet<T> can't be targeted by a collection expression (currently), so callers have to choose something.

[baywet]

Thank you for the additional information.

I was under the impression that the dotnet team was working on "default implementations for collections expressions targeting abstract and interface types mappings" for net 10. This would have been awesome and saved some more "ceremony" when initializing things. But this probably got bumped to net 11 I guess.