diff --git a/server/src/main/java/org/elasticsearch/rest/DeprecationRestHandler.java b/server/src/main/java/org/elasticsearch/rest/DeprecationRestHandler.java index f3cf4038426b2..8d363f6e63511 100644 --- a/server/src/main/java/org/elasticsearch/rest/DeprecationRestHandler.java +++ b/server/src/main/java/org/elasticsearch/rest/DeprecationRestHandler.java @@ -13,7 +13,6 @@ import org.elasticsearch.common.Strings; import org.elasticsearch.common.logging.DeprecationCategory; import org.elasticsearch.common.logging.DeprecationLogger; -import org.elasticsearch.core.Nullable; import java.util.Objects; diff --git a/server/src/main/java/org/elasticsearch/rest/RestController.java b/server/src/main/java/org/elasticsearch/rest/RestController.java index 5bb78e46004a2..b8d064c6dec51 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestController.java +++ b/server/src/main/java/org/elasticsearch/rest/RestController.java @@ -145,7 +145,6 @@ public ServerlessApiProtections getApiProtections() { return apiProtections; } - /** * Registers a REST handler to be executed when the provided {@code method} and {@code path} match the request. * @@ -185,8 +184,8 @@ protected void registerAsDeprecatedHandler( } else { // e.g. it was marked as deprecated in 7.x, and we're currently running 9.x // should never happen as we only support the current and current -1 versions - //TODO: comment this back in when V_7 is no longer supported - //assert false : "Unsupported REST API version " + version; + // TODO: comment this back in when V_7 is no longer supported + // assert false : "Unsupported REST API version " + version; } } @@ -223,22 +222,11 @@ protected void registerAsReplacedHandler( RestHandler handler, RestRequest.Method replacedMethod, String replacedPath, - RestApiVersion replacedVersion + RestApiVersion replacedVersion, + String replacedMessage, + Level deprecationLevel ) { - // e.g. [POST /_optimize] is deprecated! Use [POST /_forcemerge] instead. - final String replacedMessage = "[" - + replacedMethod.name() - + " " - + replacedPath - + "] is deprecated! Use [" - + method.name() - + " " - + path - + "] instead."; - registerHandler(method, path, version, handler); - //TODO: test this variant - Level deprecationLevel = replacedVersion == RestApiVersion.current() ? Level.WARN : DeprecationLogger.CRITICAL; registerAsDeprecatedHandler(replacedMethod, replacedPath, replacedVersion, handler, replacedMessage, deprecationLevel); } @@ -283,7 +271,9 @@ public void registerHandler(final Route route, final RestHandler handler) { handler, replaced.getMethod(), replaced.getPath(), - replaced.getRestApiVersion() + replaced.getRestApiVersion(), + replaced.getDeprecationMessage(), + replaced.getDeprecationLevel() ); } else if (route.isDeprecated()) { registerAsDeprecatedHandler( diff --git a/server/src/main/java/org/elasticsearch/rest/RestHandler.java b/server/src/main/java/org/elasticsearch/rest/RestHandler.java index 2d8f7c0e15171..99584171c7667 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestHandler.java +++ b/server/src/main/java/org/elasticsearch/rest/RestHandler.java @@ -153,12 +153,16 @@ private Route( ) { this.method = Objects.requireNonNull(method); this.path = Objects.requireNonNull(path); - // the last version in which this route was fully supported, defaults to the current version + // the last version in which this route was fully supported this.restApiVersion = Objects.requireNonNull(restApiVersion); // a route marked as deprecated to keep or remove will have a deprecation message and level (warn for keep, critical for remove) this.deprecationMessage = deprecationMessage; - this.deprecationLevel = deprecationLevel; + this.deprecationLevel = Objects.requireNonNull(deprecationLevel); + + if (deprecationMessage == null && deprecationLevel != Level.OFF) { + throw new IllegalArgumentException("deprecationMessage must be set if deprecationLevel is not OFF"); + } // a route that replaces another route will have a reference to the route that was replaced this.replacedRoute = replacedRoute; @@ -237,17 +241,38 @@ public RouteBuilder deprecatedForRemoval(String deprecationMessage, RestApiVersi * Route.builder(GET, "/_security/user/") * .replaces(GET, "/_xpack/security/user/", RestApiVersion.V_7).build()} * - * @param method the method being replaced - * @param path the path being replaced + * @param replacedMethod the method being replaced + * @param replacedPath the path being replaced * @param lastFullySupportedVersion the last {@link RestApiVersion} (i.e. 7) for which this route is fully supported. * The next major version (i.e. 8) will require compatibility header(s). (;compatible-with=7) * The next major version (i.e. 9) will have no support whatsoever for this route. * @return a reference to this object. */ - public RouteBuilder replaces(Method method, String path, RestApiVersion lastFullySupportedVersion) { + public RouteBuilder replaces(Method replacedMethod, String replacedPath, RestApiVersion lastFullySupportedVersion) { assert this.deprecationMessage == null; - // the deprecation message and level will be built later - this.replacedRoute = new Route(method, path, lastFullySupportedVersion, null, null, null); + + // if being replaced in the current version, then it's a warning, otherwise it's critical + Level deprecationLevel = lastFullySupportedVersion == RestApiVersion.current() ? Level.WARN : DeprecationLogger.CRITICAL; + + // e.g. [POST /_optimize] is deprecated! Use [POST /_forcemerge] instead. + final String replacedMessage = "[" + + replacedMethod.name() + + " " + + replacedPath + + "] is deprecated! Use [" + + this.method.name() + + " " + + this.path + + "] instead."; + + this.replacedRoute = new Route( + replacedMethod, + replacedPath, + lastFullySupportedVersion, + replacedMessage, + deprecationLevel, + null + ); return this; } @@ -281,7 +306,6 @@ public Route build() { deprecationLevel == null ? Level.OFF : deprecationLevel, replacedRoute ); - } } diff --git a/server/src/test/java/org/elasticsearch/rest/DeprecationRestHandlerTests.java b/server/src/test/java/org/elasticsearch/rest/DeprecationRestHandlerTests.java index 16551c3197899..b534a6be0dc5f 100644 --- a/server/src/test/java/org/elasticsearch/rest/DeprecationRestHandlerTests.java +++ b/server/src/test/java/org/elasticsearch/rest/DeprecationRestHandlerTests.java @@ -172,7 +172,7 @@ public void testSupportsBulkContentFalse() { ); } - public void testDeprecationLevel(){ + public void testDeprecationLevel() { DeprecationRestHandler handler = new DeprecationRestHandler( this.handler, METHOD, @@ -195,26 +195,16 @@ public void testDeprecationLevel(){ ); assertEquals(DeprecationLogger.CRITICAL, handler.getDeprecationLevel()); - IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () -> new DeprecationRestHandler( - this.handler, - METHOD, - PATH, - null, - deprecationMessage, - deprecationLogger, - false - )); + IllegalArgumentException exception = expectThrows( + IllegalArgumentException.class, + () -> new DeprecationRestHandler(this.handler, METHOD, PATH, null, deprecationMessage, deprecationLogger, false) + ); assertEquals(exception.getMessage(), "unexpected deprecation logger level: null, expected either 'CRITICAL' or 'WARN'"); - exception = expectThrows(IllegalArgumentException.class, () -> new DeprecationRestHandler( - this.handler, - METHOD, - PATH, - Level.OFF, - deprecationMessage, - deprecationLogger, - false - )); + exception = expectThrows( + IllegalArgumentException.class, + () -> new DeprecationRestHandler(this.handler, METHOD, PATH, Level.OFF, deprecationMessage, deprecationLogger, false) + ); assertEquals(exception.getMessage(), "unexpected deprecation logger level: OFF, expected either 'CRITICAL' or 'WARN'"); } diff --git a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java index 4442b77a4b5ff..4cdd89bcd320c 100644 --- a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java +++ b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java @@ -390,36 +390,35 @@ public void testRegisterAsReplacedHandler() { for (RestApiVersion replacedInVersion : replacedInVersions) { clearInvocations(controller); Route route = Route.builder(method, path).replaces(replacedMethod, replacedPath, replacedInVersion).build(); - // don't want to test everything -- just that it actually wraps the handler doCallRealMethod().when(controller).registerHandler(route, handler); + Level level = replacedInVersion == current ? Level.WARN : DeprecationLogger.CRITICAL; doCallRealMethod().when(controller) - .registerAsReplacedHandler(method, path, current, handler, replacedMethod, replacedPath, replacedInVersion); - - controller.registerHandler(route, handler); - - verify(controller).registerHandler(method, path, current, handler); - if (replacedInVersion.equals(current)) { - // is replaced in current version, which results in a critical deprecation warning - verify(controller).registerAsDeprecatedHandler( - replacedMethod, - replacedPath, - replacedInVersion, + .registerAsReplacedHandler( + method, + path, + current, handler, - deprecationMessage, - Level.WARN - ); - } else { - // is replaced in previous version, which results in a critical deprecation warning - verify(controller).registerAsDeprecatedHandler( replacedMethod, replacedPath, replacedInVersion, - handler, deprecationMessage, - DeprecationLogger.CRITICAL + level ); - } + + controller.registerHandler(route, handler); + + // verify we registered the primary handler + verify(controller).registerHandler(method, path, current, handler); + // verify we register the replaced handler with the correct deprecation message and level + verify(controller).registerAsDeprecatedHandler( + replacedMethod, + replacedPath, + replacedInVersion, + handler, + deprecationMessage, + level + ); } }