-
Notifications
You must be signed in to change notification settings - Fork 716
Support Deprecation header #1151
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
base: main
Are you sure you want to change the base?
Conversation
|
@dotnet-policy-service agree |
|
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. 😉 |
|
Have you had any chance to look at this yet? |
commonsensesoftware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
examples/AspNetCore/OData/ODataOpenApiExample/ConfigureSwaggerOptions.cs
Show resolved
Hide resolved
src/Abstractions/src/Asp.Versioning.Abstractions/ISunsetPolicyManager.cs
Outdated
Show resolved
Hide resolved
src/Abstractions/src/Asp.Versioning.Abstractions/DeprecationPolicy.cs
Outdated
Show resolved
Hide resolved
src/Abstractions/src/Asp.Versioning.Abstractions/DeprecationPolicy.cs
Outdated
Show resolved
Hide resolved
src/Abstractions/src/Asp.Versioning.Abstractions/SunsetPolicy.cs
Outdated
Show resolved
Hide resolved
| /// <param name="sunsetDate">The <see cref="DateTimeOffset">date and time</see> when a | ||
| /// sunset policy is applied.</param> | ||
| /// <returns>The current <see cref="ISunsetPolicyBuilder">sunset policy builder</see>.</returns> | ||
| ISunsetPolicyBuilder Effective( DateTimeOffset sunsetDate ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this were to return IPolicyBuilder<TPolicy> with slightly more generic wording for the date parameter and comments, Effective could be rolled up into the IPolicyBuilder<TPolicy> interface and get rid of this interface, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, I can write something like policyBuilder.Effective( DateTimeOffset.Now ).Link(/*...*/). If I change the return type of Effective to be the base interface IPolicyBuilder, then this chained call only works if I pull Link into the base interface as well. To do that, I need to pull the implementation of Link from SunsetPolicyBuilder into the base PolicyBuilder, and to do that I need to get rid of SunsetLinkBuilder.
That seems like an awfully big refactoring to get rid of an interface, but maybe I'm missing an easy way out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thiing I could do is to pull Effective and Link into IPolicyBuilder but leave the implementation of Link in PolicyBuilder<TPolicy> as abstract. That would avoid most of the refactoring needed to make Link work while winning the benefit related to Effective.
But is it right to have Link in the base PolicyBuilder? For the policies that exist currently, yes. But will there be other policies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... I think that would be OK, but let me look at this some more.
src/AspNetCore/WebApi/src/Asp.Versioning.Mvc.ApiExplorer/ApiDescriptionExtensions.cs
Show resolved
Hide resolved
src/AspNetCore/WebApi/test/Asp.Versioning.Http.Tests/DefaultApiVersionReporterTest.cs
Outdated
Show resolved
Hide resolved
8d1a6b6 to
064c5c9
Compare
|
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. |
|
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. 😉 |
Add new policy to configure
Deprecationheader fieldAdd a new
DeprecationPolicywhich emits aDeprecationheader when a matching API version is used.Description
An API may emit a
Deprecationheader to inform clients that a resource is/will become deprecated (but may still be available past that point). The newDeprecationPolicyallows users to configure this header, as well as the (optional) associated links. This is analogous to the existingSunsetPolicy.The implementation follows the existing
SunsetPolicy(and associated classes likeSunsetPolicyBuilder,SunsetPolicyManageretc) 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:
deprecationas per the spec.Fixes #1128