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

Add Support for API Explorer #59

Closed
3 tasks done
commonsensesoftware opened this issue Nov 11, 2016 · 24 comments
Closed
3 tasks done

Add Support for API Explorer #59

commonsensesoftware opened this issue Nov 11, 2016 · 24 comments
Assignees
Milestone

Comments

@commonsensesoftware
Copy link
Collaborator

commonsensesoftware commented Nov 11, 2016

Overview

Add out-of-the-box support for the API Explorer. The implementation should enable enumerating controllers and their associated actions, which are grouped by API version .

Requirements

  • Enable enumeration of API-versioned controllers and actions
  • Simplify documenting API-versioned services using Swagger
  • Simplify documenting API-versioned services using Swashbuckle
  • [Optional] Extend support to ASP.NET Web API Help Pages

Tasks

  • Implement IApiExplorer for ASP.NET Web API
  • Implement IApiExplorer for ASP.NET Core
  • Implement IApiExplorer for ASP.NET Web API with OData
@nomttr
Copy link

nomttr commented Nov 23, 2016

I found article which turned out to be very helpful in case of versioning and Web API Help Page. It isn't perfect but in 95% of cases it works perfectly! All of step are explained in the article. I changed implementation of GetFriendlyID method to takes api versioning into the account. In my case i'm using url versioning so RelativePath always conatins {version} string but you should consider another versioning strategies.

ApiDescriptionExtensions

public static string GetFriendlyId(this ApiDescription description)
        {
            if (description.RelativePath.Contains("{version}"))
            {
                var controllerApiVersion = description.ActionDescriptor.ControllerDescriptor.GetImplementedApiVersions().FirstOrDefault(); //consider that each controller can implement only one ApiVersion
                if (controllerApiVersion != null)
                {
                    StringBuilder apiVersionValue = new StringBuilder();

                    if (controllerApiVersion.MinorVersion != null && controllerApiVersion.MinorVersion != 0)
                    {
                        apiVersionValue.AppendFormat("{0}.{1}", controllerApiVersion.MajorVersion, controllerApiVersion.MinorVersion);
                    }
                    else
                    {
                        apiVersionValue.AppendFormat(controllerApiVersion.MajorVersion.ToString());
                    }

                    description.RelativePath = description.RelativePath.Replace("{version}", apiVersionValue.ToString());
                }
            }
                     //omitted
      }

Issue I found in this solution affects identical routes but different HttpMethods. In this case it treats them as matching routes and because of this it creates more ApiDescriptions than actually exist. Workaround for this issue is to distinct Model in the ApiGroup.cshtml but I think there is better solution for this case.

ApiGroup.cshtml

@model IGrouping<HttpControllerDescriptor, ApiDescription>

       //omitted

        @{
            IEnumerable<ApiDescription> apiGroup = Model.ToList().DistinctBy(d => d.GetFriendlyId());

            foreach (var apiDescription in apiGroup.OrderByDescending(r => r.RelativePath))
            {
                <tr>
                    <td class="api-name"><a href="@Url.Action("Api", "Help", new {apiId = apiDescription.GetFriendlyId()})">@apiDescription.HttpMethod.Method @apiDescription.RelativePath</a></td>
                    <td class="api-documentation">
                        @if (apiDescription.Documentation != null || !string.IsNullOrWhiteSpace(apiDescription.Documentation))
                        {
                            <p>@apiDescription.Documentation</p>
                        }
                        else
                        {
                            <p>No documentation available.</p>
                        }
                    </td>
                </tr>
         }

I think you should consider to add this code to your solution. Probably you have to suit it to be more universal but at least it's a great starting point. Hope this gonna be helpful for you and looking forward for changes!

@commonsensesoftware
Copy link
Collaborator Author

Nice. This is will be useful. I've started taking a crack at creating an IApiExplorer that adds support for versioning. The built-in implementation uses a lot of internal types and methods, which makes the port more difficult. This is a great link. It might negate the need to folk the original code and rather adapt over it. I'm hoping to have a beta version of the functionality out during the holidays. Thanks for sharing.

@commonsensesoftware
Copy link
Collaborator Author

The first iteration of packages are available. I'm leaving a little burn-in time with a beta release for issues and/or feedback before the official release. I'll be updating the wiki soon.

@xperiandri
Copy link

@commonsensesoftware, how to remove letter v from group name?

@commonsensesoftware
Copy link
Collaborator Author

commonsensesoftware commented Apr 21, 2017

Good question. I guess I haven't really documented those scenarios yet. This is handled by the IApiVersionGroupNameFormatter service. The default behavior is implemented by the DefaultApiVersionGroupNameFormatter.

This class is sealed for now because there's nothing to extend or override in the base implementation thus far. The single method defined by the interface is responsible for formatting a given ApiVersion as a group name. To change it, just add your own implementation and then replace that service in the services collection as a singleton.

services.TryAddSingleton<IApiVersionGroupNameFormatter, MyApiVersionGroupNameFormatter>();

The default implementation uses a v because that seemed to be what most people use in their Swagger documentation. If you have alternate ideas or suggestions, please share. I hope that helps. Let me know if you have any more questions.

@xperiandri
Copy link

Well I would add that to VersionedApiExplorer options as a delegate instead of a whole class as a service.
I don't know if you considered some other additions to that class.

@xperiandri
Copy link

So if I want plain version, will apiVersion.ToString() be enough?

@commonsensesoftware
Copy link
Collaborator Author

By plain, do you mean no other formatting? I suspect - yes - that's what you're looking for. You can see the ApiVersion.ToString implementation. A limited set of format codes are available. If no format code is provided, then the entire version is formatted. The string result depends on the API version value itself. I considered implementing IFormattable, but there hasn't been a strong need for it up to this point.

@xperiandri
Copy link

Yep.
And it might be simpler than whole class

@commonsensesoftware
Copy link
Collaborator Author

I considered it, but a single delegate won't actually work because the functionality is used in two separate, disconnected places:

  • IApiVersionDescriptionProvider
  • IApiDescriptionProvider

I considered putting on the ApiVersioningOptions, but the API explorer behaviors aren't really associated. There's still a few possible alternatives.

Option 1

Add a DelegatingApiVersionGroupNameFormatter which accepts a delegate. Then you can register the service with:

new DelegatingApiVersionGroupNameFormatter( version => version.ToString() )

Option 2

Add options specific to the API Explorer. Something like:

services.AddMvcCore().AddVersionedApiExplorer(
   o =>
   {
     o.FormatApiVersionAsGroupName = version => version.ToString();
   } );

I think I'd favor the second option. In either case, what should the default format be? Every example I've seen uses a v prefix. Honestly, I'm totally fine with just using ApiVersion.ToString(). My goal was to hit the 80% use case scenario.

@xperiandri
Copy link

xperiandri commented Apr 26, 2017

The second one looks better.
From one point of view this is right however from another default choice just won't be changed by majority.
So that by this decision you make a decision for the 80% of this 80%. Hence 64% will do what you say.
That's why I would leave this version formatting for documentation and sample but not default way.

@commonsensesoftware
Copy link
Collaborator Author

@xperiandri checkout PR #124 if you get a chance. I think you may have some thoughts there. I've added support for formatting ApiVersion and refactored the how group name formatting gets done. By default, there will be no formatting, which will basically be the result of ToString().

Without making you spelunk everything for some basic thoughts, here's what things look like in the updated examples:

services.AddMvcCore().AddVersionedApiExplorer( o => o.GroupNameFormat = "'v'VVV" );

This format translates to: v<Major>[.Minor][-Status]

Not to worry, I'll put up a comprehensive wiki page that will document all of the format speciifiers, their meanings, and examples. If you're curious about how this is done, take a look at the ApiVersionFormatProvider.

I think this sets things up better for new options in the future.

@xperiandri
Copy link

Looks good.
However how would I express version like v1.03 or ver01.25?
I mean if I want that major or minor to be 2 digits always.

@commonsensesoftware
Copy link
Collaborator Author

Here's my cheat sheet of format codes I used and will be the basis of the forthcoming wiki page:

  • G - Group version
  • GG - Group version with status
  • V - Major version
  • VV - Major version with minor version
  • VVV - Major version with optional minor version and status
  • VVVV - Major version with minor version and status
  • v - Minor version
  • S - Status
  • yy - Two-digit year
  • yyyy - Year
  • M - Single-digit month
  • MM - Two-digit month
  • d - Single-digit day
  • dd - Two-digit day

I placed a strong emphasis on providing format specifiers that will deal with handling all of the optional components, which is something that would be difficult to do otherwise without writing code. For example, the period between major and minor version should be omitted when the optional minor version is also omitted. The minor version is optional if it is missing (e.g. null) or exactly equals zero. The major version is never optional (according to spec).

There isn't currently a way to pad the major or minor version numbers, so formatting something like "ver01.25" wouldn't be possible. However, inline with the standard format codes, I could add support for a numeric placeholder.

Something like:

  • V, V0 - Major version
  • V[1+] - Major version padded by zeros. Similar to the 00# format specifiers.
  • v, v0 - Minor version
  • v[1+] - Minor version padded by zeros. Similar to the 00# format specifiers.

If this was supported, then your scenario would be satisfied by the format: 'ver'V1.v1. This then begs the question as to whether the other variants should support this as well. Namely VV, VVV, and VVVV. It looks kind of odd, but I haven't been able to come up with other [simple] options for optional content. If the expanded formats are supported too, then your scenario would be satisfied by format: 'ver'VVV1.

@xperiandri
Copy link

I would propose:

  • G - Group version
  • V - Major version (formatable as VV padded by zeros)
  • v - Minor version (formatable as VV padded by zeros)
  • S - Status
  • GS - Group version with status
  • Vv - Major version with minor version (formatable as VVvv padded by zeros)
  • Vw - Major version with optional minor version and status (formatable as VVww padded by zeros)
  • VvS - Major version with minor version and status (formatable as VVvvS padded by zeros)
  • yy - Two-digit year
  • yyyy - Year
  • M - Single-digit month
  • MM - Two-digit month
  • d - Single-digit day
  • dd - Two-digit day

@commonsensesoftware
Copy link
Collaborator Author

I considered something like that. I certainly won't argue that it makes more logical sense. However, when you consider how format specifiers get used, it doesn't quite hold up. Consider the "M" specifier for month in January:

  • M - 1
  • MM - 01
  • MMM - Jan
  • MMMM - Janurary

When you include time, "m" is minutes. A format like "Mm" would be "15" where "1" is January and "5" is the minutes. I mention this because in your example the following are ambiguous: "V" + "v" and "Vv". In terms of any alternative forms, format specifier rules require that the casing of each code have matching casing.

Using a number at the end does feel a bit wonky, but would be consistent with how other format specifiers work. For example:

  • ( 1 ).ToString( "N" ) -> 1.00
  • ( 1 ).ToString( "N0" ) -> 1
  • ( 1 ).ToString( "N1" ) -> 1.0
  • ( 1 ).ToString( "N2" ) -> 1.00
  • ( 1 ).ToString( "D" ) -> 1
  • ( 1 ).ToString( "D0" ) -> 1
  • ( 1 ).ToString( "D1" ) -> 1
  • ( 1 ).ToString( "D2" ) -> 01
  • ( 1 ).ToString( "D3" ) -> 001

If a different format specifier can be used for the alternate form[s], I'd be onboard with that. Perhaps use "V" and "v" for the normal forms and "P" and "p" for the padded forms?

Value Format Result Description
1.5 v 5 Minor version
1.5 V 1 Major version
1 VV 1.0 Major and minor version
1.0-RC VVV 1-RC Major, optional minor version, and status
1-RC VVV 1.0-RC Major, minor version, and status
1.5 p 05 Padded minor version with default of two digits
1.5 p0 5 Padded minor version with zero digits
1.5 p1 5 Padded minor version with one digit
1.5 p2 05 Padded minor version with two digits
1.5 p[3+] [...]005 Padded minor version with three or more digits
1.5 P 01 Padded major version with default of two digits
1.5 P0 1 Padded major version with zero digits
1.5 P1 1 Padded major version with one digit
1.5 P2 01 Padded major version with two digits
1.5 P[3+] [...]001 Padded major version with three or more digits
1.5 PP 01.05 Padded major and minor version with default of two digits
1-RC PPP 01-RC Padded major, minor version with default of two digits
1-RC PPPP 01.05-RC Padded major, minor version with default of two digits

The PP, PPP, PPP forms could also support a numeric value to indicate the amount of padding which would apply to both the minor and major versions.

If it helps provide additional context, don't forget that all these codes can be used in String.Format too:

var apiVersion = new ApiVersion( 1, 1, "Beta" );
var message = string.Format( "Welcome to version {0:VV} (S)", apiVersion );
Console.WriteLine( message );
// Output: Welcome to version 1.1 (Beta)

@xperiandri
Copy link

xperiandri commented May 4, 2017

Yep, makes sense. And if I want padding there will be no ability to make minor optional.
Looks right.
Can I write P2.p4 or P3-p3?

@commonsensesoftware
Copy link
Collaborator Author

Yes. P2.p4 = 01.0001 and P3-p3 = 001.001

The PPP format would be the only one that wouldn't include an optional minor version; just like VVV. If the default is already two digits, does something like PPP3 even make sense? The number would always reflect both the major and minor versions. In other words, PPP3 = P3.p3. The only difference is that the PPP won't include the . when the minor version is optional. In the P3.p3 format, you end up with 001..

Assuming people will really want padding, I don't know that they'd want more than 2 digits. If I only support PPP for now and people ask for PPP[n] later, that won't be a breaking change. Thoughts?

I'll add the support for padding, update the PR, and push things in.

BTW: Thanks for taking the time to provide input. It helps flush out ideas and get a larger perspective of how things might actually be used.

@xperiandri
Copy link

Cool! Thanks. Your work is highly valuable.
Will you release it as alpha package on MyGet?

@commonsensesoftware
Copy link
Collaborator Author

@xperiandri Sorry for the delay. The 1.1 RC has now been published with all these features. Note that there is breaking, behavioral change for ASP.NET Core. :( You'll now need to call app.UseMvc().UseApiVersioning(). This is called out in the release notes.

Let me know if you run into anything, but I think 1.1 is ready.

@xperiandri
Copy link

So, does it require full MVC to run?

@commonsensesoftware
Copy link
Collaborator Author

No. The UseMvc extension is already part of Mvc.Core. It's kind of a lot to explain, but basically there needs to be a catch-all router at the end of the pipeline because the IActionSelector can be called multiple times. This is the only way to ensure that all candidates have been considered in the current ASP.NET Core routing design. It's similar to why 405 (Method Not Allowed) is not and cannot currently be supported. This is something I'm working with the ASP.NET team to address in ASP.NET Core 2.0.

I'm happy to discuss this further for those that are curious. In most cases, this was not a problem for adopters. In the cases that hit this issue, it was a really design flaw that could only be addressed by changing the way you author your service. One of my key design principles was make sure no one has to learn anything new or different about routing.

@xperiandri
Copy link

Well if it is the only option then fine.
I just thought that scenario with MvcCore only can be covered.

@commonsensesoftware
Copy link
Collaborator Author

I think you might be confusing AddMvc and AddMvcCore with UseMvc. UseMvc adds the middleware. To my knowledge, UseMvcCore and UseMvc must always be called. Both AddMvcCore and UseMvc reside in the core MVC library, while AddMvc lives in the full MVC library. No references have changed, so everything should still work without any of the UI stuff.

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

No branches or pull requests

3 participants