Better support for RouteBuilder for deprecate for removal vs. deprecate and keep #113712
+318
−124
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jakelandis
changed the title
|
@elasticmachine merge upstream |
jakelandis
commented
Sep 30, 2024
| ) { | ||
| // e.g. [POST /_optimize] is deprecated! Use [POST /_forcemerge] instead. | ||
| final String replacedMessage = "[" |
There was a problem hiding this comment.
this was moved in to the builder since that is the only place we can express "replaced"
jakelandis
commented
Sep 30, 2024
| @@ -173,7 +177,7 @@ private Route( | |||
| * @param path the path, e.g. "/" | |||
| */ | |||
| public Route(Method method, String path) { | |||
| this(method, path, RestApiVersion.current(), null, null, null); | |||
| this(method, path, RestApiVersion.current(), null, Level.OFF, null); | |||
There was a problem hiding this comment.
we effectively ignore Level.OFF, but it more expressive than passing around null (which used to have meaning based on the other context .. here even though it is not used, it should be expressive)
jakelandis
commented
Sep 30, 2024
| @@ -77,19 +77,18 @@ public DeprecationRestHandler( | |||
| @Override | |||
| public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception { | |||
| if (compatibleVersionWarning == false) { | |||
| // The default value for deprecated requests without a version warning is WARN | |||
| if (deprecationLevel == null || deprecationLevel == Level.WARN) { | |||
There was a problem hiding this comment.
this is where defaulted to null to mean WARN (and below we defaulted null to mean CRITICAL). This PR moves that be more explicit and closer to where it is declared.
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit is a follow up to #113151 to better clarify how to deprecate HTTP routes. That change introduced
RouteBuilder.deprecateAndKeepto enable the ability to deprecate an HTTP API, but not require REST compatibilty headers in the next major version. This commit deprecates the java methnodRouteBuilder.deprecatedand introducesRouteBuilder.deprecatedForRemoval. The valid options are nowRouteBuilder.deprecateAndKeepvs.RouteBuilder.deprecatedForRemovalwhere the later will require compatiblity headers to access the route in any newer REST API versions than thelastFullySupportedVersiondeclared. The javadoc should help to provide some clarification.Additionally, the deprecation level should not be configurable. The deprecation level of WARN, when applied to routes, is informational only (and may require compatibility headers in the next version). The deprecation level of CRITICAL means means no access what so ever in the next major version. Generally, CRTICIAL is used for any cases where the compatibility is required (which means it is the last version for any access), or no compatibility is planned.
Some examples:
Route.builder(GET, "/foo/bar").build()-> no deprecationsRoute.builder(GET, "/foo/bar").deprecateAndKeep("my message").build()-> deprecated, but removal is not influenced by REST API compatibiltyRoute.builder(GET, "/foo/bar").deprecatedForRemoval("my message", V_8).build()-> will be available in V_8, but emit a warn level deprecation, V_9 will require compatiblity headers and emit a CRITICAL deprecation, and V_10 this will no longer be availableRoute.builder(GET, "/foo/bar").replaces(GET, "/foo/baz", V_8).build()-> /foo/bar will be available in all versions. /foo/baz will be available in V_8 but emit a warn level deprecation, V_9 will require compatibility headers and emit a CRITICAL deprecation, and V_10 this will no longer be available. This is effectively a short cut to register a new route ("/foo/bar") anddeprecatedForRemovalthe path being replaced.The functionality remains unchanged and this refactoring only provides better contracts and cleans up some of the implementation.