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]

Is there a test for this? I'm not seeing it. It feel like there should definite should be one.

[commonsensesoftware]

Since I ISunsetPolicyManagerExtensions.cs was deleted and supplanted by IPolicyManagerExtensions.cs, shouldn't this file be renamed to IPolicyManagerExtensionsTest.cs?

[commonsensesoftware]

Correct me if I'm wrong, but this looks/feels like we lost the extensions for ISunsetPolicyManager, no? Sunset policies can still build links and effective dates, even if they have a slightly different meaning from deprecation.

Based on the other thread on Link and Effective, I'm wondering if we're missing a couple of interfaces so we can describe the behavior independently and compose them. This would retain the flexibility of separate interface evolution, keep the 2 distinct interfaces, but still allow for a unified set of extension methods.

I'm not settled on the names, but I could imagine this being refactored into something like:

public interface ILinkSource
{
	ILinkBuilder Link( Uri linkTarget );
}

public interface IPolicyEffectiveDate
{
	void Effective( DateTimeOffset date );
}

public interface IDeprecationPolicyBuilder : IPolicyBuilder<DeprecationPolicy>, ILinkSource, IPolicyEffectiveDate { }
public interface ISunsetPolicyBuilder : IPolicyBuilder<SunsetPolicy>, ILinkSource, IPolicyEffectiveDate { }

public static class IPolicyBuilderExtensions
{
    public static ILinkBuilder Link( this ILinkSource builder, string linkTarget )
    {
        ArgumentNullException.ThrowIfNull( builder );
        return builder.Link( new Uri( linkTarget, UriKind.RelativeOrAbsolute ) );
    }

    public static TBuilder Effective<TBuilder>( this TBuilder builder, int year, int month, int day )
        where TBuilder : notnull, IPolicyEffectiveDate
    {
        ArgumentNullException.ThrowIfNull( builder );
        builder.Effective( new DateTimeOffset( new DateTime( year, month, day ) ) );
        return builder;
    }
}

Thoughts?

[commonsensesoftware]

Shouldn't these log messages now include the date the API was deprecated if we know it from the deprecation policy?

[commonsensesoftware]

I don't see a change to the file, but shouldn't IsDeprecatedApi in ApiVersionHandler.cs now change also, at least, check and honor the deprecation policy, when present? In the absence of the policy, the current logic continues to make sense as a fallback.

[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.