RestHandlers declare handled routes #51950

jaymode · 2020-02-05T15:57:30Z

This commit changes how RestHandlers are registered with the
RestController so that a RestHandler no longer needs to register itself
with the RestController. Instead the RestHandler interface has new
methods which when called provide information about the method and path
combinations that are handled by the handler including any deprecated
and/or replaced combinations.

This change also makes the publication of RestHandlers safe since they
no longer publish a reference to themselves within their constructors.

Closes #51622

This commit changes how RestHandlers are registered with the RestController so that a RestHandler no longer needs to register itself with the RestController. Instead the RestHandler interface has new methods which when called provide information about the method and path combinations that are handled by the handler including any deprecated and/or replaced combinations. This change also makes the publication of RestHandlers safe since they no longer publish a reference to themselves within their constructors. Closes elastic#51622

elasticmachine · 2020-02-05T15:57:33Z

Pinging @elastic/es-core-infra (:Core/Infra/REST API)

jasontedor

I left a comment.

jasontedor · 2020-02-05T18:50:24Z

...p-api-plugin/src/main/java/org/elasticsearch/plugin/noop/action/bulk/RestNoopBulkAction.java

+        return Collections.unmodifiableMap(MapBuilder.<String, List<Method>>newMapBuilder()
+            .put("/_noop_bulk", unmodifiableList(asList(POST, PUT)))
+            .put("/{index}/_noop_bulk", unmodifiableList(asList(POST, PUT)))
+            .map());


Or:

diff --git a/client/client-benchmark-noop-api-plugin/src/main/java/org/elasticsearch/plugin/noop/action/bulk/RestNoopBulkAction.java b/client/client-benchmark-noop-api-plugin/src/main/java/org/elasticsearch/plugin/noop/action/bulk/RestNoopBulkAction.java index d6c0ef736e3..b0d17af1846 100644 --- a/client/client-benchmark-noop-api-plugin/src/main/java/org/elasticsearch/plugin/noop/action/bulk/RestNoopBulkAction.java +++ b/client/client-benchmark-noop-api-plugin/src/main/java/org/elasticsearch/plugin/noop/action/bulk/RestNoopBulkAction.java @@ -27,7 +27,6 @@ import org.elasticsearch.action.support.ActiveShardCount; import org.elasticsearch.action.update.UpdateResponse; import org.elasticsearch.client.Requests; import org.elasticsearch.client.node.NodeClient; -import org.elasticsearch.common.collect.MapBuilder; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.index.shard.ShardId; import org.elasticsearch.rest.BaseRestHandler; @@ -39,12 +38,9 @@ import org.elasticsearch.rest.RestResponse; import org.elasticsearch.rest.action.RestBuilderListener; import java.io.IOException; -import java.util.Collections; import java.util.List; import java.util.Map; -import static java.util.Arrays.asList; -import static java.util.Collections.unmodifiableList; import static org.elasticsearch.rest.RestRequest.Method.POST; import static org.elasticsearch.rest.RestRequest.Method.PUT; import static org.elasticsearch.rest.RestStatus.OK; @@ -53,10 +49,9 @@ public class RestNoopBulkAction extends BaseRestHandler { @Override public Map<String, List<Method>> handledMethodsAndPaths() { - return Collections.unmodifiableMap(MapBuilder.<String, List<Method>>newMapBuilder() - .put("/_noop_bulk", unmodifiableList(asList(POST, PUT))) - .put("/{index}/_noop_bulk", unmodifiableList(asList(POST, PUT))) - .map()); + return Map.of( + "/_noop_bulk", List.of(POST, PUT), + "/{index}/_noop_bulk", List.of(POST, PUT)); } @Override

I had been working to remove uses of MapBuilder in master, in light of the new JDK APIs for map construction.

And I wonder if we should take this farther, and instead of using a simple datatype we formalize the notion of a route and make that part of the API:

diff --git a/client/client-benchmark-noop-api-plugin/src/main/java/org/elasticsearch/plugin/noop/action/bulk/RestNoopBulkAction.java b/client/client-benchmark-noop-api-plugin/src/main/java/org/elasticsearch/plugin/noop/action/bulk/RestNoopBulkAction.java index d6c0ef736e3..1e70bc9f15b 100644 --- a/client/client-benchmark-noop-api-plugin/src/main/java/org/elasticsearch/plugin/noop/action/bulk/RestNoopBulkAction.java +++ b/client/client-benchmark-noop-api-plugin/src/main/java/org/elasticsearch/plugin/noop/action/bulk/RestNoopBulkAction.java @@ -27,7 +27,6 @@ import org.elasticsearch.action.support.ActiveShardCount; import org.elasticsearch.action.update.UpdateResponse; import org.elasticsearch.client.Requests; import org.elasticsearch.client.node.NodeClient; -import org.elasticsearch.common.collect.MapBuilder; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.index.shard.ShardId; import org.elasticsearch.rest.BaseRestHandler; @@ -37,14 +36,12 @@ import org.elasticsearch.rest.RestRequest; import org.elasticsearch.rest.RestRequest.Method; import org.elasticsearch.rest.RestResponse; import org.elasticsearch.rest.action.RestBuilderListener; +import org.elasticsearch.rest.action.Route; import java.io.IOException; -import java.util.Collections; import java.util.List; import java.util.Map; -import static java.util.Arrays.asList; -import static java.util.Collections.unmodifiableList; import static org.elasticsearch.rest.RestRequest.Method.POST; import static org.elasticsearch.rest.RestRequest.Method.PUT; import static org.elasticsearch.rest.RestStatus.OK; @@ -52,11 +49,10 @@ import static org.elasticsearch.rest.RestStatus.OK; public class RestNoopBulkAction extends BaseRestHandler { @Override - public Map<String, List<Method>> handledMethodsAndPaths() { - return Collections.unmodifiableMap(MapBuilder.<String, List<Method>>newMapBuilder() - .put("/_noop_bulk", unmodifiableList(asList(POST, PUT))) - .put("/{index}/_noop_bulk", unmodifiableList(asList(POST, PUT))) - .map()); + public List<Route> handledMethodsAndPaths() { + return List.of( + new Route("/_noop_bulk", List.of(POST, PUT)), + new Route("/{index}/_noop_bulk", List.of(POST, PUT))); } @Override diff --git a/server/src/main/java/org/elasticsearch/rest/BaseRestHandler.java b/server/src/main/java/org/elasticsearch/rest/BaseRestHandler.java index 26ba8aa27fb..66d65b0b70e 100644 --- a/server/src/main/java/org/elasticsearch/rest/BaseRestHandler.java +++ b/server/src/main/java/org/elasticsearch/rest/BaseRestHandler.java @@ -28,6 +28,7 @@ import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Setting.Property; import org.elasticsearch.plugins.ActionPlugin; import org.elasticsearch.rest.RestRequest.Method; +import org.elasticsearch.rest.action.Route; import org.elasticsearch.rest.action.admin.cluster.RestNodesUsageAction; import java.io.IOException; @@ -74,7 +75,7 @@ public abstract class BaseRestHandler implements RestHandler { * {@inheritDoc} */ @Override - public abstract Map<String, List<Method>> handledMethodsAndPaths(); + public abstract List<Route> handledMethodsAndPaths(); @Override public final void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception { diff --git a/server/src/main/java/org/elasticsearch/rest/RestController.java b/server/src/main/java/org/elasticsearch/rest/RestController.java index 239f89c5c20..9bf52ced99d 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestController.java +++ b/server/src/main/java/org/elasticsearch/rest/RestController.java @@ -160,8 +160,8 @@ public class RestController implements HttpServerTransport.Dispatcher { * and {@code path} combinations. */ public void registerHandler(RestHandler restHandler) { - restHandler.handledMethodsAndPaths().forEach((path, methods) -> - methods.forEach(method -> registerHandler(method, path, restHandler))); + restHandler.handledMethodsAndPaths().forEach(route -> + route.methods().forEach(method -> registerHandler(method, route.path(), restHandler))); restHandler.deprecatedHandledMethodsAndPaths().forEach(api -> api.getMethods().forEach(method -> registerAsDeprecatedHandler(method, api.getPath(), restHandler, api.getDeprecationMessage(), api.getLogger()))); restHandler.replacedMethodsAndPaths().forEach(api -> registerWithDeprecatedHandler(api.getMethod(), api.getPath(), restHandler, diff --git a/server/src/main/java/org/elasticsearch/rest/RestHandler.java b/server/src/main/java/org/elasticsearch/rest/RestHandler.java index 5f0862ed80e..6504656ee3c 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestHandler.java +++ b/server/src/main/java/org/elasticsearch/rest/RestHandler.java @@ -23,6 +23,7 @@ import org.elasticsearch.client.node.NodeClient; import org.elasticsearch.common.logging.DeprecationLogger; import org.elasticsearch.common.xcontent.XContent; import org.elasticsearch.rest.RestRequest.Method; +import org.elasticsearch.rest.action.Route; import java.util.Collections; import java.util.List; @@ -69,8 +70,8 @@ public interface RestHandler { /** * The map of {@code path} to {@code methods} that this RestHandler is responsible for handling. */ - default Map<String, List<Method>> handledMethodsAndPaths() { - return Collections.emptyMap(); + default List<Route> handledMethodsAndPaths() { + return List.of(); } /** diff --git a/server/src/main/java/org/elasticsearch/rest/action/Route.java b/server/src/main/java/org/elasticsearch/rest/action/Route.java index d42b4fb851a..05e38fdce74 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/Route.java +++ b/server/src/main/java/org/elasticsearch/rest/action/Route.java @@ -1,4 +1,26 @@ package org.elasticsearch.rest.action; +import org.elasticsearch.rest.RestRequest; + +import java.util.List; + public class Route { + + private final String path; + + public String path() { + return path; + } + + private List<RestRequest.Method> methods; + + public List<RestRequest.Method> methods() { + return methods; + } + + public Route(final String path, final List<RestRequest.Method> methods) { + this.path = path; + this.methods = methods; + } + }

This is close to what you have done with deprecated routes, but I think we should take it the whole way and cover all routes. Then a DeprecatedRoute is a Route with some additional fields, etc.

jasontedor

I left one other thought.

jasontedor · 2020-02-05T19:01:18Z

...p-api-plugin/src/main/java/org/elasticsearch/plugin/noop/action/bulk/RestNoopBulkAction.java

+        return Collections.unmodifiableMap(MapBuilder.<String, List<Method>>newMapBuilder()
+            .put("/_noop_bulk", unmodifiableList(asList(POST, PUT)))
+            .put("/{index}/_noop_bulk", unmodifiableList(asList(POST, PUT)))
+            .map());


And I wonder if we should take this farther, and instead of using a simple datatype we formalize the notion of a route and make that part of the API:

diff --git a/client/client-benchmark-noop-api-plugin/src/main/java/org/elasticsearch/plugin/noop/action/bulk/RestNoopBulkAction.java b/client/client-benchmark-noop-api-plugin/src/main/java/org/elasticsearch/plugin/noop/action/bulk/RestNoopBulkAction.java index d6c0ef736e3..1e70bc9f15b 100644 --- a/client/client-benchmark-noop-api-plugin/src/main/java/org/elasticsearch/plugin/noop/action/bulk/RestNoopBulkAction.java +++ b/client/client-benchmark-noop-api-plugin/src/main/java/org/elasticsearch/plugin/noop/action/bulk/RestNoopBulkAction.java @@ -27,7 +27,6 @@ import org.elasticsearch.action.support.ActiveShardCount; import org.elasticsearch.action.update.UpdateResponse; import org.elasticsearch.client.Requests; import org.elasticsearch.client.node.NodeClient; -import org.elasticsearch.common.collect.MapBuilder; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.index.shard.ShardId; import org.elasticsearch.rest.BaseRestHandler; @@ -37,14 +36,12 @@ import org.elasticsearch.rest.RestRequest; import org.elasticsearch.rest.RestRequest.Method; import org.elasticsearch.rest.RestResponse; import org.elasticsearch.rest.action.RestBuilderListener; +import org.elasticsearch.rest.action.Route; import java.io.IOException; -import java.util.Collections; import java.util.List; import java.util.Map; -import static java.util.Arrays.asList; -import static java.util.Collections.unmodifiableList; import static org.elasticsearch.rest.RestRequest.Method.POST; import static org.elasticsearch.rest.RestRequest.Method.PUT; import static org.elasticsearch.rest.RestStatus.OK; @@ -52,11 +49,10 @@ import static org.elasticsearch.rest.RestStatus.OK; public class RestNoopBulkAction extends BaseRestHandler { @Override - public Map<String, List<Method>> handledMethodsAndPaths() { - return Collections.unmodifiableMap(MapBuilder.<String, List<Method>>newMapBuilder() - .put("/_noop_bulk", unmodifiableList(asList(POST, PUT))) - .put("/{index}/_noop_bulk", unmodifiableList(asList(POST, PUT))) - .map()); + public List<Route> handledMethodsAndPaths() { + return List.of( + new Route("/_noop_bulk", List.of(POST, PUT)), + new Route("/{index}/_noop_bulk", List.of(POST, PUT))); } @Override diff --git a/server/src/main/java/org/elasticsearch/rest/BaseRestHandler.java b/server/src/main/java/org/elasticsearch/rest/BaseRestHandler.java index 26ba8aa27fb..66d65b0b70e 100644 --- a/server/src/main/java/org/elasticsearch/rest/BaseRestHandler.java +++ b/server/src/main/java/org/elasticsearch/rest/BaseRestHandler.java @@ -28,6 +28,7 @@ import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Setting.Property; import org.elasticsearch.plugins.ActionPlugin; import org.elasticsearch.rest.RestRequest.Method; +import org.elasticsearch.rest.action.Route; import org.elasticsearch.rest.action.admin.cluster.RestNodesUsageAction; import java.io.IOException; @@ -74,7 +75,7 @@ public abstract class BaseRestHandler implements RestHandler { * {@inheritDoc} */ @Override - public abstract Map<String, List<Method>> handledMethodsAndPaths(); + public abstract List<Route> handledMethodsAndPaths(); @Override public final void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception { diff --git a/server/src/main/java/org/elasticsearch/rest/RestController.java b/server/src/main/java/org/elasticsearch/rest/RestController.java index 239f89c5c20..9bf52ced99d 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestController.java +++ b/server/src/main/java/org/elasticsearch/rest/RestController.java @@ -160,8 +160,8 @@ public class RestController implements HttpServerTransport.Dispatcher { * and {@code path} combinations. */ public void registerHandler(RestHandler restHandler) { - restHandler.handledMethodsAndPaths().forEach((path, methods) -> - methods.forEach(method -> registerHandler(method, path, restHandler))); + restHandler.handledMethodsAndPaths().forEach(route -> + route.methods().forEach(method -> registerHandler(method, route.path(), restHandler))); restHandler.deprecatedHandledMethodsAndPaths().forEach(api -> api.getMethods().forEach(method -> registerAsDeprecatedHandler(method, api.getPath(), restHandler, api.getDeprecationMessage(), api.getLogger()))); restHandler.replacedMethodsAndPaths().forEach(api -> registerWithDeprecatedHandler(api.getMethod(), api.getPath(), restHandler, diff --git a/server/src/main/java/org/elasticsearch/rest/RestHandler.java b/server/src/main/java/org/elasticsearch/rest/RestHandler.java index 5f0862ed80e..6504656ee3c 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestHandler.java +++ b/server/src/main/java/org/elasticsearch/rest/RestHandler.java @@ -23,6 +23,7 @@ import org.elasticsearch.client.node.NodeClient; import org.elasticsearch.common.logging.DeprecationLogger; import org.elasticsearch.common.xcontent.XContent; import org.elasticsearch.rest.RestRequest.Method; +import org.elasticsearch.rest.action.Route; import java.util.Collections; import java.util.List; @@ -69,8 +70,8 @@ public interface RestHandler { /** * The map of {@code path} to {@code methods} that this RestHandler is responsible for handling. */ - default Map<String, List<Method>> handledMethodsAndPaths() { - return Collections.emptyMap(); + default List<Route> handledMethodsAndPaths() { + return List.of(); } /** diff --git a/server/src/main/java/org/elasticsearch/rest/action/Route.java b/server/src/main/java/org/elasticsearch/rest/action/Route.java index d42b4fb851a..05e38fdce74 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/Route.java +++ b/server/src/main/java/org/elasticsearch/rest/action/Route.java @@ -1,4 +1,26 @@ package org.elasticsearch.rest.action; +import org.elasticsearch.rest.RestRequest; + +import java.util.List; + public class Route { + + private final String path; + + public String path() { + return path; + } + + private List<RestRequest.Method> methods; + + public List<RestRequest.Method> methods() { + return methods; + } + + public Route(final String path, final List<RestRequest.Method> methods) { + this.path = path; + this.methods = methods; + } + }

This is close to what you have done with deprecated routes, but I think we should take it the whole way and cover all routes. Then a DeprecatedRoute is a Route with some additional fields, etc.

jaymode · 2020-02-05T19:35:02Z

in light of the new JDK APIs for map construction.

I was trying to make this easier to backport to 7.x by not using List.of and Map.of. I'm happy to do that in a follow-up though for master.

I think we should take it the whole way and cover all routes

I agree and thank you for the reminder of the name Route. I'll implement the change from Map<String, List<Method>> to List<Route> and also change the naming to use Route instead of RestApi and MethodsAndPaths.

jasontedor · 2020-02-05T19:47:52Z

I was trying to make this easier to backport to 7.x by not using List.of and Map.of. I'm happy to do that in a follow-up though for master.

I'm fine either way, for example using the collection convenience methods here, and changing them before backporting, whichever you prefer.

I agree and thank you for the reminder of the name Route. I'll implement the change from Map<String, List<Method>> to List<Route> and also change the naming to use Route instead of RestApi and MethodsAndPaths.

Perfect, thanks.

jaymode · 2020-02-05T21:13:39Z

Given the name Route, I do wonder whether having a list of methods still makes sense. My current inclination is that a Route is one path and one method. This will play better with the concept of a replaced route (ie PUT foo -> POST foo). What do you think?

jasontedor · 2020-02-05T22:09:05Z

That sounds better to me @jaymode.

nik9000

You are brave to do this all in one pr!

nik9000 · 2020-02-07T12:25:22Z

...p-api-plugin/src/main/java/org/elasticsearch/plugin/noop/action/bulk/RestNoopBulkAction.java

-        controller.registerHandler(POST, "/{index}/_noop_bulk", this);
-        controller.registerHandler(PUT, "/{index}/_noop_bulk", this);
+    @Override
+    public List<Route> handledRoutes() {


handlesRoutes?

Any chance you could reverse the order of the args on the routes ctor? In my mind the method comes first.

On further thought, I lean more towards routes rather than handledRoutes or handlesRoutes. What do you think?

I agree regarding the parameter ordering.

I pushed 086a295 to change the method name and 2653271 to change the parameter ordering.

nik9000 · 2020-02-07T12:28:32Z

qa/smoke-test-http/src/test/java/org/elasticsearch/http/TestDeprecationHeaderRestAction.java

-                                               DEPRECATED_ENDPOINT, deprecationLogger);
+    @Override
+    public List<DeprecatedRoute> deprecatedRoutes() {
+        return singletonList(


I haven't looked deeply but I wonder if we need both a separate method and a separate class for deprecated routes. Could you return Deprecated routes in the handledRoutes method?

I looked at that briefly and decided against doing so to avoid instanceOf checks

I'm not sure if this is better, it's merely a suggestion for how we could do something like this:

diff --git a/server/src/main/java/org/elasticsearch/rest/RestController.java b/server/src/main/java/org/elasticsearch/rest/RestController.java index 6a16007465c..90763fd1336 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestController.java +++ b/server/src/main/java/org/elasticsearch/rest/RestController.java @@ -48,6 +48,7 @@ import java.util.List; import java.util.Map; import java.util.Set; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.function.Predicate; import java.util.function.Supplier; import java.util.function.UnaryOperator; import java.util.stream.Collectors; @@ -62,6 +63,7 @@ import static org.elasticsearch.rest.RestStatus.OK; public class RestController implements HttpServerTransport.Dispatcher { private static final Logger logger = LogManager.getLogger(RestController.class); + private static final DeprecationLogger deprecationLogger = new DeprecationLogger(logger); private final PathTrie<MethodHandlers> handlers = new PathTrie<>(RestUtils.REST_DECODER); @@ -130,11 +132,7 @@ public class RestController implements HttpServerTransport.Dispatcher { */ protected void registerWithDeprecatedHandler(RestRequest.Method method, String path, RestHandler handler, RestRequest.Method deprecatedMethod, String deprecatedPath, - DeprecationLogger logger) { - // e.g., [POST /_optimize] is deprecated! Use [POST /_forcemerge] instead. - final String deprecationMessage = - "[" + deprecatedMethod.name() + " " + deprecatedPath + "] is deprecated! Use [" + method.name() + " " + path + "] instead."; - + String deprecationMessage, DeprecationLogger logger) { registerHandler(method, path, handler); registerAsDeprecatedHandler(deprecatedMethod, deprecatedPath, handler, deprecationMessage, logger); } @@ -160,11 +158,12 @@ public class RestController implements HttpServerTransport.Dispatcher { * and {@code path} combinations. */ public void registerHandler(final RestHandler restHandler) { - restHandler.routes().forEach(route -> registerHandler(route.getMethod(), route.getPath(), restHandler)); - restHandler.deprecatedRoutes().forEach(route -> - registerAsDeprecatedHandler(route.getMethod(), route.getPath(), restHandler, route.getDeprecationMessage(), route.getLogger())); - restHandler.replacedRoutes().forEach(route -> registerWithDeprecatedHandler(route.getMethod(), route.getPath(), - restHandler, route.getDeprecatedMethod(), route.getDeprecatedPath(), route.getLogger())); + final List<RestHandler.Route> routes = restHandler.routes(); + routes.stream().filter(Predicate.not(RestHandler.Route::isDeprecated)).forEach(route -> registerHandler(route.getMethod(), route.getPath(), restHandler)); + routes.stream().filter(RestHandler.Route::isDeprecated).forEach(route -> + registerAsDeprecatedHandler(route.getMethod(), route.getPath(), restHandler, route.deprecationMessage(), deprecationLogger)); + routes.stream().filter(RestHandler.Route::isReplacement).forEach(route -> registerWithDeprecatedHandler(route.getMethod(), route.getPath(), + restHandler, route.replacedRoute().getMethod(), route.replacedRoute().getPath(), route.replacedRoute().deprecationMessage(), deprecationLogger)); } @Override diff --git a/server/src/main/java/org/elasticsearch/rest/RestHandler.java b/server/src/main/java/org/elasticsearch/rest/RestHandler.java index ab7b468f757..9638933053d 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestHandler.java +++ b/server/src/main/java/org/elasticsearch/rest/RestHandler.java @@ -20,7 +20,6 @@ package org.elasticsearch.rest; import org.elasticsearch.client.node.NodeClient; -import org.elasticsearch.common.logging.DeprecationLogger; import org.elasticsearch.common.xcontent.XContent; import org.elasticsearch.rest.RestRequest.Method; @@ -72,32 +71,30 @@ public interface RestHandler { return Collections.emptyList(); } - /** - * A list of routes handled by this RestHandler that are deprecated and do not have a direct - * replacement. If changing the {@code path} or {@code method} of a route, - * use {@link #replacedRoutes()}. - */ - default List<DeprecatedRoute> deprecatedRoutes() { - return Collections.emptyList(); - } - - /** - * A list of routes handled by this RestHandler that have had their {@code path} and/or - * {@code method} changed. The pre-existing {@code route} will be registered - * as deprecated alongside the updated {@code route}. - */ - default List<ReplacedRoute> replacedRoutes() { - return Collections.emptyList(); - } - class Route { private final String path; private final Method method; + private final DeprecationInfo deprecationInfo; + private final ReplacementInfo replacementInfo; public Route(Method method, String path) { + this(method, path, DeprecationInfo.NON_DEPRECATED_ROUTE, ReplacementInfo.NONE); + } + + public Route(Method method, String path, Method deprecatedMethod, String deprecatedPath) { + this( + method, + path, + DeprecationInfo.NON_DEPRECATED_ROUTE, + ReplacementInfo.replacedRoute(new Route(deprecatedMethod, deprecatedPath, DeprecationInfo.deprecatedRoute("[" + deprecatedMethod.name() + " " + deprecatedPath + "] is deprecated! Use [" + method.name() + " " + path + "] instead."), ReplacementInfo.NONE))); + } + + private Route(Method method, String path, DeprecationInfo deprecationInfo, ReplacementInfo replacementInfo) { this.path = path; this.method = method; + this.deprecationInfo = deprecationInfo; + this.replacementInfo = replacementInfo; } public String getPath() { @@ -107,58 +104,88 @@ public interface RestHandler { public Method getMethod() { return method; } + + public boolean isDeprecated() { + return deprecationInfo.isDeprecated(); + } + + public String deprecationMessage() { + return deprecationInfo.deprecationMessage(); + } + + public boolean isReplacement() { + return replacementInfo.isReplacement(); + } + + public Route replacedRoute() { + return replacementInfo.replacedRoute(); + } + } - /** - * Represents an API that has been deprecated and is slated for removal. - */ - class DeprecatedRoute extends Route { + class DeprecationInfo { + private final boolean isDeprecated; private final String deprecationMessage; - private final DeprecationLogger logger; - public DeprecatedRoute(Method method, String path, String deprecationMessage, DeprecationLogger logger) { - super(method, path); + public static final DeprecationInfo NON_DEPRECATED_ROUTE = new DeprecationInfo(false, null); + + public static DeprecationInfo deprecatedRoute(String deprecationMessage) { + return new DeprecationInfo(true, deprecationMessage); + } + + private DeprecationInfo(boolean isDeprecated, String deprecationMessage) { + assert (isDeprecated && deprecationMessage != null) || (isDeprecated == false && deprecationMessage == null); + this.isDeprecated = isDeprecated; this.deprecationMessage = deprecationMessage; - this.logger = logger; } - public String getDeprecationMessage() { - return deprecationMessage; + public boolean isDeprecated() { + return isDeprecated; } - public DeprecationLogger getLogger() { - return logger; + public String deprecationMessage() { + if (isDeprecated) { + return deprecationMessage; + } else { + throw new IllegalStateException(); + } } + } - /** - * Represents an API that has had its {@code path} or {@code method} changed. Holds both the - * new and previous {@code path} and {@code method} combination. - */ - class ReplacedRoute extends Route { + class ReplacementInfo { + + private final boolean isReplacement; + private final Route replacedRoute; - private final String deprecatedPath; - private final Method deprecatedMethod; - private final DeprecationLogger logger; + public static final ReplacementInfo NONE = new ReplacementInfo(false, null); - public ReplacedRoute(Method method, String path, Method deprecatedMethod, String deprecatedPath, DeprecationLogger logger) { - super(method, path); - this.deprecatedMethod = deprecatedMethod; - this.deprecatedPath = deprecatedPath; - this.logger = logger; + public static ReplacementInfo replacedRoute(Route replacedRoute) { + return new ReplacementInfo(true, replacedRoute); } - public String getDeprecatedPath() { - return deprecatedPath; + private ReplacementInfo(boolean isReplacement, Route replacedRoute) { + assert (isReplacement && replacedRoute != null) || (isReplacement == false && replacedRoute == null); + if (replacedRoute.isDeprecated() == false) { + throw new IllegalArgumentException(); + } + this.isReplacement = isReplacement; + this.replacedRoute = replacedRoute; } - public Method getDeprecatedMethod() { - return deprecatedMethod; + public boolean isReplacement() { + return isReplacement; } - public DeprecationLogger getLogger() { - return logger; + public Route replacedRoute() { + if (isReplacement) { + return replacedRoute; + } else { + throw new IllegalStateException(); + } } + } + } diff --git a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/rest/RestDeleteExpiredDataAction.java b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/rest/RestDeleteExpiredDataAction.java index 11ca99d9d3a..faa96d23755 100644 --- a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/rest/RestDeleteExpiredDataAction.java +++ b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/rest/RestDeleteExpiredDataAction.java @@ -5,9 +5,7 @@ */ package org.elasticsearch.xpack.ml.rest; -import org.apache.logging.log4j.LogManager; import org.elasticsearch.client.node.NodeClient; -import org.elasticsearch.common.logging.DeprecationLogger; import org.elasticsearch.rest.BaseRestHandler; import org.elasticsearch.rest.RestRequest; import org.elasticsearch.rest.action.RestToXContentListener; @@ -22,21 +20,10 @@ import static org.elasticsearch.rest.RestRequest.Method.DELETE; public class RestDeleteExpiredDataAction extends BaseRestHandler { - private static final DeprecationLogger deprecationLogger = - new DeprecationLogger(LogManager.getLogger(RestDeleteExpiredDataAction.class)); - @Override public List<Route> routes() { - return Collections.emptyList(); - } - - @Override - public List<ReplacedRoute> replacedRoutes() { - // TODO: remove deprecated endpoint in 8.0.0 return Collections.singletonList( - new ReplacedRoute(DELETE, MachineLearning.BASE_PATH + "_delete_expired_data", - DELETE, MachineLearning.PRE_V7_BASE_PATH + "_delete_expired_data", - deprecationLogger) + new Route(DELETE, MachineLearning.BASE_PATH + "_delete_expired_data", DELETE, MachineLearning.PRE_V7_BASE_PATH + "_delete_expired_data") ); }

The more I think about it, the more I think we should keep it how you have it for now. If we find reasons to combine this into a single pattern in the future, the above gives us a way forward on how we do that. Let's leave it out of this change though, this is already a huge improvement over where we were, and no need to delay getting it in any further when it's not clear what the benefits are.

nik9000 · 2020-02-07T12:30:03Z

server/src/main/java/org/elasticsearch/action/ActionModule.java

        for (ActionPlugin plugin : actionPlugins) {
            for (RestHandler handler : plugin.getRestHandlers(settings, restController, clusterSettings, indexScopedSettings,
                    settingsFilter, indexNameExpressionResolver, nodesInCluster)) {
                registerHandler.accept(handler);
            }
        }
-        registerHandler.accept(new RestCatAction(restController, catActions));
+        registerHandler.accept(new RestCatAction(catActions));


jasontedor · 2020-02-09T01:00:52Z

server/src/main/java/org/elasticsearch/rest/RestHandler.java

+        private final String deprecationMessage;
+        private final DeprecationLogger logger;
+
+        public DeprecatedRoute(Method method, String path, String deprecationMessage, DeprecationLogger logger) {


In a follow-up, let's get these loggers out of here. It doesn't make sense they are put of the route, and reflecting on it, I don't think we need a dedicated deprecation logger for every REST handler that deprecates some method. I think we can create a top-level RestController deprecation logger for this purpose.

jasontedor · 2020-02-09T01:01:00Z

server/src/main/java/org/elasticsearch/rest/RestHandler.java

+        private final Method deprecatedMethod;
+        private final DeprecationLogger logger;
+
+        public ReplacedRoute(Method method, String path, Method deprecatedMethod, String deprecatedPath, DeprecationLogger logger) {


Same comment here about the deprecation logger.

jasontedor

LGTM.

nik9000 · 2020-02-09T01:24:55Z

Lgtm2

…

On Sat, Feb 8, 2020, 20:04 Jason Tedor ***@***.***> wrote: ***@***.**** approved this pull request. LGTM. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#51950?email_source=notifications&email_token=AABUXIXYSNXVVXZLO5TUG6LRB5JCNA5CNFSM4KQNSCTKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCUY24QI#pullrequestreview-355577409>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABUXITDHKUNR44NAIEFNYLRB5JCNANCNFSM4KQNSCTA> .

This commit changes how RestHandlers are registered with the RestController so that a RestHandler no longer needs to register itself with the RestController. Instead the RestHandler interface has new methods which when called provide information about the routes (method and path combinations) that are handled by the handler including any deprecated and/or replaced combinations. This change also makes the publication of RestHandlers safe since they no longer publish a reference to themselves within their constructors. Closes #51622 Co-authored-by: Jason Tedor <jason@tedor.me> Backport of #51950

This commit adapts searchable snapshots to the latest changes from master. The REST handlers declare their routes differently since #51950 and SearchableSnapshotIndexInput need to account for segment infos files that are stored in shard snapshot metadata hash and not uploaded to the blob store anymore since #51729.

This commit removes the need for DeprecatedRoute and ReplacedRoute to have an instance of a DeprecationLogger. Instead the RestController now has a DeprecationLogger that will be used for all deprecated and replaced route messages. Relates elastic#51950

This commit removes the need for DeprecatedRoute and ReplacedRoute to have an instance of a DeprecationLogger. Instead the RestController now has a DeprecationLogger that will be used for all deprecated and replaced route messages. Relates #51950

This commit removes the need for DeprecatedRoute and ReplacedRoute to have an instance of a DeprecationLogger. Instead the RestController now has a DeprecationLogger that will be used for all deprecated and replaced route messages. Relates elastic#51950

This commit replaces usages of older collection utility methods with the JDK 9 convenience method `List.of()`. Relates elastic#51950

This commit removes the need for DeprecatedRoute and ReplacedRoute to have an instance of a DeprecationLogger. Instead the RestController now has a DeprecationLogger that will be used for all deprecated and replaced route messages. Relates #51950 Backport of #52278

This commit replaces usages of older collection utility methods with the JDK 9 convenience method `List.of()`. Relates #51950

* upgrade gradle to 6.4 * Routes replace usage of RestController.register elastic/elasticsearch#51950 * add setting index.hidden=true to createIndex * solve IT allowed action list problem * try hard code jvmArgs * add release note * revert immutableList change, use listOf jvmArgs put into afterEvaluate phase

jaymode added :Core/Infra/REST API REST infrastructure and utilities >refactoring v8.0.0 v7.7.0 labels Feb 5, 2020

jaymode added 2 commits February 5, 2020 10:51

fixes

9167512

fix inference api

263e2d2

jasontedor reviewed Feb 5, 2020

View reviewed changes

jaymode changed the title ~~RestHandlers declare handled methods and paths~~ RestHandlers declare handled routes Feb 5, 2020

jaymode added 4 commits February 6, 2020 14:20

refactor to routes

171e368

Merge branch 'master' into rest_handler_safe_register

7c14750

update javadoc

94f1124

fix duplicated api

771b4e1

jaymode marked this pull request as ready for review February 7, 2020 02:54

jaymode requested a review from jasontedor February 7, 2020 02:55

jaymode added the >breaking-java label Feb 7, 2020

nik9000 reviewed Feb 7, 2020

View reviewed changes

jaymode added 5 commits February 7, 2020 09:36

Merge branch 'master' into rest_handler_safe_register

995f5f4

fix new autoscaling handler

2b69b84

method first then path

2653271

handledRoutes -> routes

086a295

checkstyle

e40372a

jasontedor reviewed Feb 9, 2020

View reviewed changes

Merge branch 'master' into rest_handler_safe_register

76ec125

jasontedor approved these changes Feb 9, 2020

View reviewed changes

jasontedor and others added 2 commits February 9, 2020 09:10

Fix imports

bda4cc5

Merge branch 'master' into rest_handler_safe_register

8ad163e

jaymode merged commit 627d853 into elastic:master Feb 10, 2020

jaymode added the backport pending label Feb 10, 2020

jaymode mentioned this pull request Feb 10, 2020

RestHandlers declare handled routes #52123

Merged

jaymode removed the backport pending label Feb 10, 2020

tlrx mentioned this pull request Feb 12, 2020

Adapt searchable snapshots code to latest master changes #52258

Merged

jaymode mentioned this pull request Feb 12, 2020

Remove DeprecationLogger from route objects #52278

Merged

jaymode mentioned this pull request Feb 12, 2020

Remove DeprecationLogger from route objects #52285

Merged

jaymode added a commit to jaymode/elasticsearch that referenced this pull request Feb 12, 2020

Use List convenience methods in rest actions

e2fe084

This commit replaces usages of older collection utility methods with the JDK 9 convenience method `List.of()`. Relates elastic#51950

jaymode mentioned this pull request Feb 12, 2020

Use List convenience methods in rest actions #52286

Merged

jaymode added a commit that referenced this pull request Feb 12, 2020

Use List convenience methods in rest actions (#52286)

a11ccb8

This commit replaces usages of older collection utility methods with the JDK 9 convenience method `List.of()`. Relates #51950

sujithvm mentioned this pull request May 16, 2020

Fix routes in master opensearch-project/security#461

Merged

This was referenced May 16, 2020

Adds support for ES 7.7.0 opendistro-for-elasticsearch/alerting#205

Merged

Support ES version 7.7.0 opendistro-for-elasticsearch/job-scheduler#58

Merged

bowenlan-amzn mentioned this pull request May 20, 2020

Adds support for ES 7.7.0 opendistro-for-elasticsearch/index-management#228

Merged

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RestHandlers declare handled routes #51950

RestHandlers declare handled routes #51950

jaymode commented Feb 5, 2020

elasticmachine commented Feb 5, 2020

jasontedor left a comment

jasontedor Feb 5, 2020

jasontedor Feb 5, 2020

jasontedor left a comment

jasontedor Feb 5, 2020

jaymode commented Feb 5, 2020

jasontedor commented Feb 5, 2020

jaymode commented Feb 5, 2020

jasontedor commented Feb 5, 2020

nik9000 left a comment

nik9000 Feb 7, 2020

jaymode Feb 7, 2020

jaymode Feb 7, 2020

nik9000 Feb 7, 2020

jaymode Feb 7, 2020

jasontedor Feb 7, 2020

jasontedor Feb 9, 2020

nik9000 Feb 7, 2020

jasontedor Feb 9, 2020

jasontedor Feb 9, 2020

jasontedor left a comment

nik9000 commented Feb 9, 2020 via email

RestHandlers declare handled routes #51950

RestHandlers declare handled routes #51950

Conversation

jaymode commented Feb 5, 2020

elasticmachine commented Feb 5, 2020

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaymode commented Feb 5, 2020

jasontedor commented Feb 5, 2020

jaymode commented Feb 5, 2020

jasontedor commented Feb 5, 2020

nik9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasontedor left a comment

Choose a reason for hiding this comment

nik9000 commented Feb 9, 2020 via email