Skip to content
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.

Sign up for GitHub

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

Pass REST params and content to extensions #163

Merged
merged 8 commits into from
Sep 30, 2022
Merged

Pass REST params and content to extensions #163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
70a8c32
Rename/merge duplicate ExtensionRestRequest implementations
dbwiddis Sep 29, 2022
16854bd
Track consumed params inside request object
dbwiddis Sep 29, 2022
61efa08
Put back test for the "this will never happen" case
dbwiddis Sep 29, 2022
f29c893
Add content and expand param getters
dbwiddis Sep 30, 2022
89eee75
Rebase to main and update after conficts
dbwiddis Sep 30, 2022
ffa6b38
More merge conflict cleanup
dbwiddis Sep 30, 2022
a9033cb
Whitespace diff reduction
dbwiddis Sep 30, 2022
ca71302
Change uri to path in RestRequest handling
dbwiddis Sep 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/main/java/org/opensearch/sdk/ExtensionRestHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

import java.util.List;

import org.opensearch.extensions.rest.ExtensionRestRequest;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking out loud. Why are we keeping ExtensionRestRequest in OpenSearch and not in SDK?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the previous RestSendToExtensionRequest entirely duplicated it. Literally all the lines were the same except for a few naming differences.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also it makes sense (see #165) to move the ExtensionRestResponse there as well since it's really the same thing as the BytesRestResponse with some tweaks.

Copy link
Member

@owaiskazi19 owaiskazi19 Sep 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This answers my questions. Just one thing to add can we keep the name as RestSendToExtensionRequest instead of ExtensionRestRequest for better understanding. Can be address in the next PR

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let’s discuss on #165, because both classes are part of the API we expect extension authors to use. The ExtensionRestRequest is the argument type for the handleRequest method and it closely parallels the same method on plugins and I’d really like similar names.

import org.opensearch.rest.BaseRestHandler;
import org.opensearch.rest.RestHandler;
import org.opensearch.rest.RestHandler.Route;
Expand All @@ -29,12 +30,11 @@ public interface ExtensionRestHandler {

/**
* Handles REST Requests forwarded from OpenSearch for a configured route on an extension.
* Parameters are components of the {@link RestRequest} received from a user.
* Parameter contains components of the {@link RestRequest} received from a user.
* This method corresponds to the {@link BaseRestHandler#prepareRequest} method.
* As in that method, consumed parameters must be tracked and returned in the response.
*
* @param restRequest a REST request object for a request to be forwarded to extensions
* @param request a REST request object for a request to be forwarded to extensions
* @return An {@link ExtensionRestResponse} to the request.
*/
ExtensionRestResponse handleRequest(ExtensionRestRequest restRequest);
ExtensionRestResponse handleRequest(ExtensionRestRequest request);
}
22 changes: 11 additions & 11 deletions src/main/java/org/opensearch/sdk/ExtensionRestPathRegistry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,11 @@ public class ExtensionRestPathRegistry {
* Register a REST handler to handle a method and route in this extension's path registry.
*
* @param method The method to register.
* @param uri The URI to register. May include named wildcards.
* @param path The path to register. May include named wildcards.
* @param extensionRestHandler The RestHandler to handle this route
*/
public void registerHandler(Method method, String uri, ExtensionRestHandler extensionRestHandler) {
String restPath = restPathToString(method, uri);
public void registerHandler(Method method, String path, ExtensionRestHandler extensionRestHandler) {
owaiskazi19 marked this conversation as resolved.
Show resolved Hide resolved
String restPath = restPathToString(method, path);
pathTrie.insert(restPath, extensionRestHandler);
registeredPaths.add(restPath);
}
Expand All @@ -41,11 +41,11 @@ public void registerHandler(Method method, String uri, ExtensionRestHandler exte
* Get the registered REST handler for the specified method and URI.
*
* @param method the registered method.
* @param uri the registered URI.
* @param path the registered path.
* @return The REST handler registered to handle this method and URI combination if found, null otherwise.
*/
public ExtensionRestHandler getHandler(Method method, String uri) {
return pathTrie.retrieve(restPathToString(method, uri));
public ExtensionRestHandler getHandler(Method method, String path) {
return pathTrie.retrieve(restPathToString(method, path));
}

/**
Expand All @@ -58,13 +58,13 @@ public List<String> getRegisteredPaths() {
}

/**
* Converts a REST method and URI to a string.
* Converts a REST method and path to a space delimited string to be used as a map lookup key.
*
* @param method the method.
* @param uri the URI.
* @return A string appending the method and URI.
* @param path the path.
* @return A string appending the method and path.
*/
public static String restPathToString(Method method, String uri) {
return method.name() + " " + uri;
public static String restPathToString(Method method, String path) {
return method.name() + " " + path;
}
}
121 changes: 0 additions & 121 deletions src/main/java/org/opensearch/sdk/ExtensionRestRequest.java
View file
Open in desktop

This file was deleted.

38 changes: 22 additions & 16 deletions src/main/java/org/opensearch/sdk/ExtensionRestResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

import org.opensearch.common.bytes.BytesReference;
import org.opensearch.common.xcontent.XContentBuilder;
import org.opensearch.extensions.rest.ExtensionRestRequest;
import org.opensearch.rest.BytesRestResponse;
import org.opensearch.rest.RestStatus;

Expand All @@ -23,71 +24,76 @@ public class ExtensionRestResponse extends BytesRestResponse {
* Key passed in {@link BytesRestResponse} headers to identify parameters consumed by the handler. For internal use.
*/
static final String CONSUMED_PARAMS_KEY = "extension.consumed.parameters";
/**
* Key passed in {@link BytesRestResponse} headers to identify content consumed by the handler. For internal use.
*/
static final String CONSUMED_CONTENT_KEY = "extension.consumed.content";

/**
* Creates a new response based on {@link XContentBuilder}.
*
* @param request the REST request being responded to.
* @param status The REST status.
* @param builder The builder for the response.
* @param consumedParams Parameters consumed by the handler.
*/
public ExtensionRestResponse(RestStatus status, XContentBuilder builder, List<String> consumedParams) {
public ExtensionRestResponse(ExtensionRestRequest request, RestStatus status, XContentBuilder builder) {
super(status, builder);
addConsumedParamHeader(consumedParams);
addConsumedHeaders(request.consumedParams(), request.isContentConsumed());
}

/**
* Creates a new plain text response.
*
* @param request the REST request being responded to.
* @param status The REST status.
* @param content A plain text response string.
* @param consumedParams Parameters consumed by the handler.
*/
public ExtensionRestResponse(RestStatus status, String content, List<String> consumedParams) {
public ExtensionRestResponse(ExtensionRestRequest request, RestStatus status, String content) {
super(status, content);
addConsumedParamHeader(consumedParams);
addConsumedHeaders(request.consumedParams(), request.isContentConsumed());
}

/**
* Creates a new plain text response.
*
* @param request the REST request being responded to.
* @param status The REST status.
* @param contentType The content type of the response string.
* @param content A response string.
* @param consumedParams Parameters consumed by the handler.
*/
public ExtensionRestResponse(RestStatus status, String contentType, String content, List<String> consumedParams) {
public ExtensionRestResponse(ExtensionRestRequest request, RestStatus status, String contentType, String content) {
super(status, contentType, content);
addConsumedParamHeader(consumedParams);
addConsumedHeaders(request.consumedParams(), request.isContentConsumed());
}

/**
* Creates a binary response.
*
* @param request the REST request being responded to.
* @param status The REST status.
* @param contentType The content type of the response bytes.
* @param content Response bytes.
* @param consumedParams Parameters consumed by the handler.
*/
public ExtensionRestResponse(RestStatus status, String contentType, byte[] content, List<String> consumedParams) {
public ExtensionRestResponse(ExtensionRestRequest request, RestStatus status, String contentType, byte[] content) {
super(status, contentType, content);
addConsumedParamHeader(consumedParams);
addConsumedHeaders(request.consumedParams(), request.isContentConsumed());
}

/**
* Creates a binary response.
*
* @param request the REST request being responded to.
* @param status The REST status.
* @param contentType The content type of the response bytes.
* @param content Response bytes.
* @param consumedParams Parameters consumed by the handler.
*/
public ExtensionRestResponse(RestStatus status, String contentType, BytesReference content, List<String> consumedParams) {
public ExtensionRestResponse(ExtensionRestRequest request, RestStatus status, String contentType, BytesReference content) {
super(status, contentType, content);
addConsumedParamHeader(consumedParams);
addConsumedHeaders(request.consumedParams(), request.isContentConsumed());
}

private void addConsumedParamHeader(List<String> consumedParams) {
private void addConsumedHeaders(List<String> consumedParams, boolean contentConusmed) {
peternied marked this conversation as resolved.
Show resolved Hide resolved
consumedParams.stream().forEach(p -> addHeader(CONSUMED_PARAMS_KEY, p));
addHeader(CONSUMED_CONTENT_KEY, Boolean.toString(contentConusmed));
}
}
6 changes: 3 additions & 3 deletions src/main/java/org/opensearch/sdk/ExtensionsRunner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@
import org.opensearch.common.io.stream.NamedWriteableRegistry;
import org.opensearch.common.io.stream.NamedWriteableRegistryParseRequest;
import org.opensearch.extensions.OpenSearchRequest;
import org.opensearch.extensions.rest.ExtensionRestRequest;
import org.opensearch.extensions.rest.RegisterRestActionsRequest;
import org.opensearch.extensions.rest.RestExecuteOnExtensionRequest;
import org.opensearch.extensions.settings.RegisterCustomSettingsRequest;
import org.opensearch.common.network.NetworkModule;
import org.opensearch.common.network.NetworkService;
Expand Down Expand Up @@ -357,7 +357,7 @@ public void startTransportService(TransportService transportService) {
ThreadPool.Names.GENERIC,
false,
false,
RestExecuteOnExtensionRequest::new,
ExtensionRestRequest::new,
((request, channel, task) -> channel.sendResponse(extensionsRestRequestHandler.handleRestExecuteOnExtensionRequest(request)))
);

Expand Down Expand Up @@ -477,7 +477,7 @@ public void sendLocalNodeRequest(TransportService transportService) {
* Requests the ActionListener onFailure method to be run by OpenSearch. The result will be handled by a {@link ActionListenerOnFailureResponseHandler}.
*
* @param transportService The TransportService defining the connection to OpenSearch.
* @param failureException The exception to be sent to OpenSearch
* @param failureException The exception to be sent to OpenSearch
*/
public void sendActionListenerOnFailureRequest(TransportService transportService, Exception failureException) {
logger.info("Sending ActionListener onFailure request to OpenSearch");
Expand Down
17 changes: 5 additions & 12 deletions src/main/java/org/opensearch/sdk/handlers/ExtensionsRestRequestHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,12 @@
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.opensearch.common.bytes.BytesReference;
import org.opensearch.extensions.rest.RestExecuteOnExtensionRequest;
import org.opensearch.extensions.rest.ExtensionRestRequest;
import org.opensearch.extensions.rest.RestExecuteOnExtensionResponse;
import org.opensearch.rest.RestStatus;
import org.opensearch.sdk.ExtensionRestHandler;
import org.opensearch.sdk.ExtensionsRunner;
import org.opensearch.sdk.ExtensionRestPathRegistry;
import org.opensearch.sdk.ExtensionRestRequest;
import org.opensearch.sdk.ExtensionRestResponse;

/**
Expand All @@ -33,24 +32,18 @@ public class ExtensionsRestRequestHandler {
* @param request The REST request to execute.
* @return A response acknowledging the request.
*/
public RestExecuteOnExtensionResponse handleRestExecuteOnExtensionRequest(RestExecuteOnExtensionRequest request) {
public RestExecuteOnExtensionResponse handleRestExecuteOnExtensionRequest(ExtensionRestRequest request) {

ExtensionRestHandler restHandler = extensionRestPathRegistry.getHandler(request.getMethod(), request.getUri());
ExtensionRestHandler restHandler = extensionRestPathRegistry.getHandler(request.method(), request.path());
if (restHandler == null) {
return new RestExecuteOnExtensionResponse(
RestStatus.NOT_FOUND,
"No handler for " + ExtensionRestPathRegistry.restPathToString(request.getMethod(), request.getUri())
"No handler for " + ExtensionRestPathRegistry.restPathToString(request.method(), request.path())
);
}
// ExtensionRestRequest restRequest = new ExtensionRestRequest(request);
ExtensionRestRequest restRequest = new ExtensionRestRequest(
request.getMethod(),
request.getUri(),
request.getRequestIssuerIdentity()
);

// Get response from extension
ExtensionRestResponse response = restHandler.handleRequest(restRequest);
ExtensionRestResponse response = restHandler.handleRequest(request);
logger.info("Sending extension response to OpenSearch: " + response.status());
return new RestExecuteOnExtensionResponse(
response.status(),
Expand Down
Loading