Skip to content

Support custom supported/deprecated version headers #875

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

Closed
1 task done
pinkfloydx33 opened this issue Sep 9, 2022 · 5 comments · Fixed by #879
Closed
1 task done

Support custom supported/deprecated version headers #875

pinkfloydx33 opened this issue Sep 9, 2022 · 5 comments · Fixed by #879
Assignees

Comments

@pinkfloydx33
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Is your feature request related to a problem? Please describe the problem.

Our APIs use header versioning with a custom header name, x-ourcompany-api-version. We were able to easily achieve this using the tools available in this library.

This x-ourcompany- header prefix convention applies to many of the headers we emit/consume generally, but particularly to the supported/deprecated API version headers. At present, those header names are hard-coded into the DefaultApiVersionReporter:

private const string ApiSupportedVersions = "api-supported-versions";
private const string ApiDeprecatedVersions = "api-deprecated-versions";

This required us to provide our own custom IReportApiVersions implementation. It's a nearly verbatim copy of the DefaultApiVersionReporter that reads the header names from options instead. I also have to take care to register/replace the service as well.

Describe the solution you'd like

I propose that the api-supported-versions and api-deprecated-versions header names be read from options, defaulting/falling-back to the existing constants.

For example, perhaps something akin to:

builder.Services.AddApiVersioning(options =>
{
    // reporting api versions will return the headers
    // "api-supported-versions" and "api-deprecated-versions"
    options.ReportApiVersions = true;
    options.SupportedVersionsHeader = "x-custom-api-supported-versions";
    options.DeprecatedVersionsHeader = "x-custom-api-deprecated-versions";
});

Additional context

The obvious work around is to continue with our custom implementation. This is really heavy handed considering we only need to customize the header names. It also suffers from the fact that it doesn't automatically receive updates to the source material (suppose the writing of the sunset policy or optimizations to the header output changes).

Perhaps if the class weren't sealed it could have virtual members for either retrieving the header names or writing the headers, though the latter would work best with a change in accessibility of AddApiVersionHeader (which might actually be useful on its own). This would at least make is easier for us to customize the behavior without needing to reimplement the entire class if new options are deemed undesirable.

@commonsensesoftware
Copy link
Collaborator

Seems reasonable. Any supported change here will need the reciprocal in the new client-side features or they'll be mismatched. I'm normally resistant to seal anything, but this something that has virtually never changed nor asked for additional features. It didn't look like there was much more you could customize here without a separate implementation; I guess I was wrong. In taking full audit of all the code for 6.0, types that seemed like they could or should be sealed were for the performance benefit.

While I'm still happy to consider the enhancement, there is another way around the issue without having to reimplement everything using a decorator. For example:

public sealed class CustomApiVersionReporter : IReportApiVersions
{
    // it is possible to decorate over whatever IReportApiVersions was registered via DI, but
    // that likely isn't useful in your scenario so just use the type directly to keep it simple
    private readonly DefaultApiVersionReporter decorated = new();

    public ApiVersionMapping Mapping => decorated.Mapping;

    public void Report( HttpResponse response, ApiVersionModel apiVersionModel )
    {
        decorated.Report( response, apiVersionModel );

        var headers = response.Headers;       

        RenameHeader( "api-supported-versions", "x-custom-api-supported-versions" );
        RenameHeader( "api-deprecated-versions", "x-custom-api-deprecated-versions" );
    }

    private static void RenameHeader( IHeaderDictionary headers, string oldName, string newName )
    {
        if ( !headers.TryGetValue( oldName, out var header ) || headers.ContainsKey( newName ) )
        {
            return;
        }
        
        headers.Remove( oldName );
        headers.Add( newName, header );
    }
}

💡 Side bar: The recommendation to use the x- prefix in headers has been deprecated for over a decade (see RFC 6648). The x- doesn't mean what it was originally supposed to mean and provides little additional value. Prefixes are fine, but the recommendation is to drop the x-; for example ms-api-version or amz-request-id. You'd still need a way to change the headers for this issue, but may you find that bit of information enlightening. 😉

@pinkfloydx33
Copy link
Author

pinkfloydx33 commented Sep 9, 2022

I don't know why I didn't consider the decorator approach. Well.. I did but for some reason it didn't occur to me that I could rename the headers (omg! /brainfart). That's certainly better than reimplementing it all.

Any supported change here will need the reciprocal in the new client-side features or they'll be mismatched

Could you explain what you mean?

Re: x- I know, but stuck with some legacy things and the powers that be didn't want to mix-and-match

@commonsensesoftware
Copy link
Collaborator

Indeed. Talking issues through have a way of working out a solution. It's arguably not the ideal solution, but it works with somewhat minimal effort and supplants having to fork all the logic.

There is now a client library for HttpClient that you can use to automatically write the desired API version into requests as well as query version and policy information from the server, when supported (e.g. advertised). You can also use it to wire up to logging. This has the benefit of logging events when the API version you requested is detected as being deprecated or a sunset policy indicates the API will go away. It can also be informative, such as letting you know a newer API version is available for you to use.

Up until 6.0, the notion of advertising versioning and policy information was interesting, but there's never been any common libraries to make use of it; now there is. This means the headers named to be consistent on the client side too. Since the headers have been constant, there's been no need to expose or share them outside of two locations. I may need to reconsider now.

private const string ApiSupportedVersions = "api-supported-versions";
private const string ApiDeprecatedVersions = "api-deprecated-versions";

If you're curious how it all comes together, you can see some usage examples in ApiVersionHandlerLoggerTTest.cs.

@pinkfloydx33
Copy link
Author

Ah OK I see what you mean. Yeah that wouldn't work for us... at least if consuming our own apis (but we use nswag for that and we'd know anyways) but our consumers would have an issue if leveraging the new libs. I can't imagine we're the only ones out there with custom headers. As you said, up to this point it's been interesting or informational at best... But now it seems a way to customize both sides of it would be worthwhile.

I checked but didn't see... is there an RFC dictating those two header names? I assume not since Google search brings me straight to this repo. If they were then I could argue the need internally to use them. But even still there's likely to be someone, somewhere who needs/wants to customize.

Looking at the file you linked, it looks like that will notify you on the fly (while the app is running) if a new version is detected? How's that handle the case if the deprecation happens while the app is offline? Sorry, perhaps better asked elsewhere.

@commonsensesoftware
Copy link
Collaborator

Most of the core features are based on what is now the Microsoft REST Guidelines. Before that existed, there were internal docs floating around inside Microsoft that defined api-supported-versions and api-deprecated-versions. Beyond header name, not much more was provided it about them. That never transitioned to the public space and is seemingly lost to time now. I thought it was an interesting idea and worth preserving, so I did.

It's fine since it's all along the same line of discussion. You are correct that it can only be done on the fly. If you onboard today, how would you know that the API is deprecated or a new version is available tomorrow? You could have an automated process that runs the checks on a more regular interval, but that is probably overkill. Deprecation doesn't mean it doesn't exist, it just means it will go away at some point (which is what a sunset policy can describe). The idea of the client extensions is to notify clients and their consumers when this happens rather than having to proactively check. It is somewhat assumed that the client regularly contacts the server. If the API is no longer available and then caveat emptor, but that's honestly no better than it is today without any of this information.

Thinking of it as being analogous to the warnings you might setup when your client certificate is about to expire. You want some lead time to adapt. A thoughtful service owner will want to advertise to their clients an API is deprecated and when it will sunset - likely with 6 months or more to update. Of course, if you own both sides, then you are master of your destiny and you know when things change. 😉

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

Successfully merging a pull request may close this issue.

2 participants