Support Deprecation header

[MGessinger]

Add new policy to configure Deprecation header field

Add a new DeprecationPolicy which emits a Deprecation header when a matching API version is used.

Description

An API may emit a Deprecation header to inform clients that a resource is/will become deprecated (but may still be available past that point). The new DeprecationPolicy allows users to configure this header, as well as the (optional) associated links. This is analogous to the existing SunsetPolicy.

The implementation follows the existing SunsetPolicy (and associated classes like SunsetPolicyBuilder, SunsetPolicyManager etc) as closely as possible. In an attempt to minimize code duplication between the two, common functionality was extracted into shared base classes/interfaces. There is still some duplication left over, but I chose what seemed like a reasonable a middle ground between streamlining the two policies, and keeping the code clear.

At the time of writing this, no tests have been added yet. Since this is a rather large PR with many changed files, I want a second opinion on the implementation before I start pinning everything down in tests. I did at least verify that the solution compiles.

Design choices

This summarizes the linked issue #1128

The new policy is configured like this:

options.Policies.Deprecate( 0.9 )
                .Effective( DateTimeOffset.Now.AddDays( 60 ) )
                .Link( "deprecation.html" )
                    .Title( "Deprecation Policy" )
                    .Type( "text/html" );

• When a deprecation policy is configured, then it will alwys be omitted, regardless whether the deprecation date is in the future or in the past.
• The RelationType of the configured link must be deprecation as per the spec.
• There is no constraint on the provided media type of the link.

Fixes #1128

[MGessinger]

@dotnet-policy-service agree

[commonsensesoftware]

Thanks for this. Just acknowledging that I've seen it. I'll try to put eyes on it ASAP. FYI, I'll be on vacation next week. I'm not ignoring it. 😉

[MGessinger]

Have you had any chance to look at this yet?

[commonsensesoftware]

Overall, it looks great. I'd estimate that 90%+ of the work is there. There are a few open questions and minor tweaks to address, but this looks close to landing.

[MGessinger]

Thanks for the thorough review! I went through everything and implemented all your suggestions. Hopefully this is in line with what you had in mind. Two or three comments are still open because it wasn't totally clear to me what the best course of action is.

[commonsensesoftware]

Looking good. I'll be traveling this weekend. With the holiday and traveling, it might be next week before I get to take a serious look. I just wanted you to know that am looking and your efforts are appreciated. 😉

[MGessinger]

Bump. Are you still traveling?

From your previous comment, it seemed that this was at least going in the right direction, so in the meantime I'm going to get started on getting the tests green again and adding any tests that may be missing.

[commonsensesoftware]

I am traveling again, but I'm working remote. I'll get eyes on this tonight or tomorrow. Thanks for poking the bear. ;)

[commonsensesoftware]

It's looking great. I think there are just a few niche cases and then this should be ready to go.

[commonsensesoftware]

@MGessinger One other important design aspect I wanted to think about and discuss is what we think the intersection of the new deprecation policy should be the existing APIs and metadata. As an example, the IApiVersionProvider and ApiVersionProviderOptions allow specifying an API version is deprecated.

A user can specify:

[ApiController]
[ApiVersion(0.9, Deprecated = true)]
[Route("[controller]")]
public class ExampleController
{
    [HttpGet]
    public string Get() => Ok("Demo");
}

This controller expresses that it supports version 0.9, but that version is now deprecated. What it does not say is when it was deprecated.

Conversely,

options.Policies.Deprecate( 0.9 )
                .Effective( DateTimeOffset.Now.AddDays( 60 ) )

indicates that all 0.9 APIs will be deprecated 60 days from now. The mere configuration of this date implies that all API versions will be deprecated after a specific date. This would hold true even if APIs do not specify Deprecated = true.

The question becomes "How do we think about precedence?".

I think the precedence would be ranked as:

1. A deprecation policy has been specified for all APIs of a version
   a. This has the advantage of never requiring developers to apply Deprecated = true to endpoints
   b. It might make it more difficult in reporting
2. A deprecation policy with a specific name
3. A deprecation policy with a specific version and name combination
4. An endpoint decorated with a deprecated API version
5. All API endpoints of the same endpoint have been annotated as deprecated

I suspect there's some checks in the implementation that have been missed. Today, things are solely based on #4 and #5. Those cases would now have to take into account the other conditions. The goal should be to make sure the two methods of expressing deprecation are as congruent as possible with a low chance of making a mistake.

[MGessinger]

I think that precedence makes sense. Although I don't think it matters too much, since the different ways of declaring deprecation aren't particularly conflicting: You can declare deprecation one of various ways, but as soon as one of them is active, your endpoint is deprecated. You can't configure it deprecated one way, and simultaneously configure it "not deprecated" another way.
Nonetheless, the order you proposed seems right and I'll get that implemented. Could you point me in the right direction, where I can find this code?

[commonsensesoftware]

The place you're looking for is effectively here:

aspnet-api-versioning/src/AspNetCore/WebApi/src/Asp.Versioning.Mvc.ApiExplorer/Internal/DescriptionProvider.cs

Line 72 in a679121

deprecated.ExceptWith( supported.Concat( advertisedDeprecated ) );

After all deprecated versions are collated and they aren't supported anywhere, they become deprecated in their entirety. This flows down this path:

aspnet-api-versioning/src/AspNetCore/WebApi/src/Asp.Versioning.Mvc.ApiExplorer/Internal/DescriptionProvider.cs

Line 22 in a679121

AppendDescriptions( descriptions, deprecated, sunsetPolicyManager, options, deprecated: true );

Now that I'm explaining it thorough, I'm thinking this is a non-issue. The deprecation policy doesn't say you're deprecated; it just says the policy. This is the same as the sunset policy. The policy doesn't mean you're sunset, it's just the policy. Deprecation can be in the past (e.g. since) or future (e.g. will be). In fact, it doesn't even matter if the API deprecated to apply the policy.

The more I write, the more I think I may have over-thought it.

[commonsensesoftware]

I think this is all solid. I have the new OpenAPI stuff done; it's a lot. I didn't want to make it painful for you so I've been working in a parallel branch. I'd like to get this merged so I can rebase. I'd like to get an initial preview release this week. I'm happy to continue the discussion or do some more tweaking and polishing before the final release.

I'm about ready to push the button now, but I see some tests appear to be failing. Is that expected? I'll hold off until you confirm, but otherwise I think we're at a good checkpoint.

[MGessinger]

Failing tests are definitely not expected. I'll look into those as well as whatever the bot found some time later today and then I'll give you the go once I'm happy!

[MGessinger]

Tests should be fixed now!

The deprecation policy doesn't say you're deprecated; it just says the policy

I didn't totally follow on this point, but I'm willing to take your word for it. You mentioned publishing a preview release anyway, I expect that it might surface the most obvious oversights. And if some other constellation shows up later, we can fix it then.

For the time being, assuming the tests run through successfully this time, I think this can be merged.