Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for associating XML Comments with custom tags defined using TagsAttribute #2565

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

yingxinj
Copy link

@yingxinj yingxinj commented Dec 5, 2022

Description & reference issues

As of .NET 6.0, Swagger/OpenAPI supports using TagsAttribute to rename each group of endpoints, overriding the default value of the controller name. (See also e.g. the top voted answer by Raul on this SO question)

The existing implementation of Swashbuckle's XmlCommentsDocumentFilter associates the controller's <summary> XML comment with the controller name. It currently has two limitations:

  1. It does not support using TagsAttribute - it only links the summary node with the controller name. See the last four comments on TagActionsBy custom tag or controller name #467, these last comments refer to the new TagsAttribute instead of TagActionsBy.

  2. It only adds tags for controllers with a summary node. If the user has summary nodes for some controllers but not others, then the root tags object will only include a partial list of tags. This results in the side-effect that tags are not displayed in alphabetical order. Instead, tags with descriptions are displayed first, followed by tags without descriptons. Related issues:

Another linked issue #2530 is a result of both these limitations.

This PR fixes both these issues. If IncludeXmlComments is true:

  • then the resulting tag names & descriptions will support the new TagsAttribute, falling back to the default (controller name) if TagsAttribute is not used.
  • then the resulting root tags object will contain all controller-level tags, regardless of whether they have an XML summary node or not. Those without a summary node will not have any "description" field.

Tests

Tests are included for the target framework .NET 6.0, which supports TagsAttribute. The SwaggerGen.Tests project currently only targets .NET 6.0, so I did not add unit tests for earlier frameworks. I built the project in .NET 5.0 and did a manual test of this case to check it works.

Potential improvements

This PR still has some limitations:

  • TagsAttribute theoretically allows you to specify more than one custom tag e.g. by [Tags("first tag", "second tag")] however I don't see this as a primary use case in generating the swagger file, and there is no way of providing separate XML summaries for the two tags, so this is not supported in this PR - only the first tag is supported.
  • TagsAttribute can be specified at the method level, if so, such tags would still be omitted from the root tags object.

If it's actually useful, I would be happy to add support for these cases.

@NilkOne
Copy link

NilkOne commented Jan 5, 2023

Great !
When will this fix added on NuGet?

@silkfire
Copy link

@domaindrivendev Any ETA on this?

@Skintkingle
Copy link

We could really do with this change.

@sirmeepington
Copy link

This would be a lifesaver 👍

@DenKn
Copy link

DenKn commented Nov 28, 2023

This fix does not cover all cases. For example it won't work

 public abstract class ConfigurationControllerBase : ControllerBase
{
}

[Tags("ConfigurationController ")]
public class ConfigurationController : ConfigurationControllerBase 
{
}

@martincostello
Copy link
Collaborator

Thanks for contributing - if you'd like to continue with this pull request, please rebase against the default branch to pick up our new CI.

@yingxinj yingxinj force-pushed the support-tagsattribute-with-xml-comments branch from 9843dd8 to 51e095f Compare April 15, 2024 19:39
@yingxinj
Copy link
Author

yingxinj commented Apr 15, 2024

Thanks @martincostello, I've rebased against your latest master branch.

Edit: apologies, I've realised that the build is failing, I'll look into it when I have another chance.

Comment on lines +425 to +427
/// If customizing the default tag for operations via TagsAttribute, only the first Tag per controller will be
/// associated with the description.
/// However, don't set this flag if you're customizing the default tag for operations via TagActionsBy.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make TagsAttribute and TagActionsBy <see .... /> references in these two comments.

Comment on lines +5 to +13
public static class KeyValuePairExtensions
{
// Explicit deconstruct required for older .NET frameworks
public static void Deconstruct<TKey, TValue>(this KeyValuePair<TKey, TValue> kvp, out TKey key, out TValue value)
{
key = kvp.Key;
value = kvp.Value;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to only add this polyfill for the TFMs that need it. Also, can it be internal?

swaggerDoc.Tags = swaggerDoc.Tags.OrderBy(x => x.Name).ToList();
}

private class ControllerInfo
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private class ControllerInfo
private sealed class ControllerInfo


#if NET6_0_OR_GREATER
controllerInfo.CustomTagName =
group.First().MethodInfo.DeclaringType?.GetCustomAttribute<TagsAttribute>()?.Tags[0];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it'll throw an exception if Tags is empty or null.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants