Skip to content

Commit

Permalink
Better support for RouteBuilder for deprecation removal vs. keep (ela…
Browse files Browse the repository at this point in the history
…stic#113712)

This commit is a follow up to elastic#113151 to better clarify how to deprecate HTTP routes. That change
 introduced RouteBuilder.deprecateAndKeep to enable the ability to deprecate an HTTP API, but 
not require REST compatibilty headers in the next major version. This commit deprecates the java 
methnod RouteBuilder.deprecated and introduces RouteBuilder.deprecatedForRemoval. The valid 
options are now RouteBuilder.deprecateAndKeep vs. RouteBuilder.deprecatedForRemoval where 
the later will require compatiblity headers to access the route in any newer REST API versions 
than the lastFullySupportedVersion declared. 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 deprecations
Route.builder(GET, "/foo/bar").deprecateAndKeep("my message").build() -> deprecated, but removal is not influenced by REST API compatibilty
Route.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 available
Route.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") and deprecatedForRemoval the path being replaced.
```

The functionality remains unchanged and this refactoring only provides better contracts 
and cleans up some of the implementation.
  • Loading branch information
jakelandis authored Oct 4, 2024
1 parent 663ab04 commit 3c5b74b
Show file tree
Hide file tree
Showing 7 changed files with 318 additions and 124 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -29,7 +28,6 @@ public class DeprecationRestHandler extends FilterRestHandler implements RestHan
private final DeprecationLogger deprecationLogger;
private final boolean compatibleVersionWarning;
private final String deprecationKey;
@Nullable
private final Level deprecationLevel;

/**
Expand All @@ -39,6 +37,8 @@ public class DeprecationRestHandler extends FilterRestHandler implements RestHan
* @param handler The rest handler to deprecate (it's possible that the handler is reused with a different name!)
* @param method a method of a deprecated endpoint
* @param path a path of a deprecated endpoint
* @param deprecationLevel The level of the deprecation warning, must be non-null
* and either {@link Level#WARN} or {@link DeprecationLogger#CRITICAL}
* @param deprecationMessage The message to warn users with when they use the {@code handler}
* @param deprecationLogger The deprecation logger
* @param compatibleVersionWarning set to false so that a deprecation warning will be issued for the handled request,
Expand All @@ -51,7 +51,7 @@ public DeprecationRestHandler(
RestHandler handler,
RestRequest.Method method,
String path,
@Nullable Level deprecationLevel,
Level deprecationLevel,
String deprecationMessage,
DeprecationLogger deprecationLogger,
boolean compatibleVersionWarning
Expand All @@ -61,7 +61,7 @@ public DeprecationRestHandler(
this.deprecationLogger = Objects.requireNonNull(deprecationLogger);
this.compatibleVersionWarning = compatibleVersionWarning;
this.deprecationKey = DEPRECATED_ROUTE_KEY + "_" + method + "_" + path;
if (deprecationLevel != null && (deprecationLevel != Level.WARN && deprecationLevel != DeprecationLogger.CRITICAL)) {
if (deprecationLevel != Level.WARN && deprecationLevel != DeprecationLogger.CRITICAL) {
throw new IllegalArgumentException(
"unexpected deprecation logger level: " + deprecationLevel + ", expected either 'CRITICAL' or 'WARN'"
);
Expand All @@ -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) {
// emit a standard deprecation warning
if (Level.WARN == deprecationLevel) {
deprecationLogger.warn(DeprecationCategory.API, deprecationKey, deprecationMessage);
} else {
} else if (DeprecationLogger.CRITICAL == deprecationLevel) {
deprecationLogger.critical(DeprecationCategory.API, deprecationKey, deprecationMessage);
}
} else {
// The default value for deprecated requests with a version warning is CRITICAL,
// because they have a specific version where the endpoint is removed
if (deprecationLevel == null || deprecationLevel == DeprecationLogger.CRITICAL) {
deprecationLogger.compatibleCritical(deprecationKey, deprecationMessage);
} else {
// emit a compatibility warning
if (Level.WARN == deprecationLevel) {
deprecationLogger.compatible(Level.WARN, deprecationKey, deprecationMessage);
} else if (DeprecationLogger.CRITICAL == deprecationLevel) {
deprecationLogger.compatibleCritical(deprecationKey, deprecationMessage);
}
}

Expand Down Expand Up @@ -139,4 +138,9 @@ public static String requireValidHeader(String value) {

return value;
}

// test only
Level getDeprecationLevel() {
return deprecationLevel;
}
}
91 changes: 28 additions & 63 deletions server/src/main/java/org/elasticsearch/rest/RestController.java
Original file line number Diff line number Diff line change
Expand Up @@ -144,25 +144,6 @@ public ServerlessApiProtections getApiProtections() {
return apiProtections;
}

/**
* Registers a REST handler to be executed when the provided {@code method} and {@code path} match the request.
*
* @param method GET, POST, etc.
* @param path Path to handle (e.g. "/{index}/{type}/_bulk")
* @param version API version to handle (e.g. RestApiVersion.V_8)
* @param handler The handler to actually execute
* @param deprecationMessage The message to log and send as a header in the response
*/
protected void registerAsDeprecatedHandler(
RestRequest.Method method,
String path,
RestApiVersion version,
RestHandler handler,
String deprecationMessage
) {
registerAsDeprecatedHandler(method, path, version, handler, deprecationMessage, null);
}

/**
* Registers a REST handler to be executed when the provided {@code method} and {@code path} match the request.
*
Expand All @@ -179,40 +160,23 @@ protected void registerAsDeprecatedHandler(
RestApiVersion version,
RestHandler handler,
String deprecationMessage,
@Nullable Level deprecationLevel
Level deprecationLevel
) {
assert (handler instanceof DeprecationRestHandler) == false;
if (version == RestApiVersion.current()) {
// e.g. it was marked as deprecated in 8.x, and we're currently running 8.x
registerHandler(
method,
path,
version,
new DeprecationRestHandler(handler, method, path, deprecationLevel, deprecationMessage, deprecationLogger, false)
);
} else if (version == RestApiVersion.minimumSupported()) {
// e.g. it was marked as deprecated in 7.x, and we're currently running 8.x
if (RestApiVersion.onOrAfter(RestApiVersion.minimumSupported()).test(version)) {
registerHandler(
method,
path,
version,
new DeprecationRestHandler(handler, method, path, deprecationLevel, deprecationMessage, deprecationLogger, true)
);
} else {
// e.g. it was marked as deprecated in 7.x, and we're currently running *9.x*
logger.debug(
"Deprecated route ["
+ method
+ " "
+ path
+ "] for handler ["
+ handler.getClass()
+ "] "
+ "with version ["
+ version
+ "], which is less than the minimum supported version ["
+ RestApiVersion.minimumSupported()
+ "]"
new DeprecationRestHandler(
handler,
method,
path,
deprecationLevel,
deprecationMessage,
deprecationLogger,
version != RestApiVersion.current()
)
);
}
}
Expand Down Expand Up @@ -250,21 +214,12 @@ 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);
registerAsDeprecatedHandler(replacedMethod, replacedPath, replacedVersion, handler, replacedMessage);
registerAsDeprecatedHandler(replacedMethod, replacedPath, replacedVersion, handler, replacedMessage, deprecationLevel);
}

/**
Expand All @@ -284,7 +239,15 @@ protected void registerHandler(RestRequest.Method method, String path, RestApiVe

private void registerHandlerNoWrap(RestRequest.Method method, String path, RestApiVersion version, RestHandler handler) {
assert RestApiVersion.minimumSupported() == version || RestApiVersion.current() == version
: "REST API compatibility is only supported for version " + RestApiVersion.minimumSupported().major;
: "REST API compatibility is only supported for version "
+ RestApiVersion.minimumSupported().major
+ " [method="
+ method
+ ", path="
+ path
+ ", handler="
+ handler.getClass().getCanonicalName()
+ "]";

if (RESERVED_PATHS.contains(path)) {
throw new IllegalArgumentException("path [" + path + "] is a reserved path and may not be registered");
Expand All @@ -299,7 +262,7 @@ private void registerHandlerNoWrap(RestRequest.Method method, String path, RestA
}

public void registerHandler(final Route route, final RestHandler handler) {
if (route.isReplacement()) {
if (route.hasReplacement()) {
Route replaced = route.getReplacedRoute();
registerAsReplacedHandler(
route.getMethod(),
Expand All @@ -308,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(
Expand Down
Loading

0 comments on commit 3c5b74b

Please sign in to comment.