-
Notifications
You must be signed in to change notification settings - Fork 65
Map extension routes to permission-able action names #622
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.
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
Changes from 54 commits
daf3b2c
9c49d95
97d2a22
b5703ae
36b67cc
9238527
a9325d2
0a1425b
0b634be
38baa81
d504ed8
6826fef
0825b74
5afe8c8
ef085ce
4966514
b287fe5
ff158f6
a80cae7
636bcdc
b08d018
1c23d9c
e45ea0e
10127c4
1eeafe6
f4ab756
66654ba
df30efd
03025e4
09a84a3
9830335
f7697b1
6d7ec26
8576654
8a93770
bdea4ea
ee09450
a989151
af084b4
0b37c06
c04bda1
fb4f0bc
15c82db
54d057c
5fb6497
81c25ce
0c5ce25
b880d93
ecf51e4
59f316c
93b6daf
326422c
a926392
f42db71
06f8e98
024aace
9658efe
7aeef67
786f01b
70d9088
80ba90c
1e06d01
e0df300
058f12a
433a42a
20885c0
ba60cb7
f460699
6d2ee04
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| /* | ||
| * Copyright OpenSearch Contributors | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| * | ||
| * The OpenSearch Contributors require contributions made to | ||
| * this file be licensed under the Apache-2.0 license or a | ||
| * compatible open source license. | ||
| */ | ||
|
|
||
| package org.opensearch.sdk; | ||
|
|
||
| import org.opensearch.rest.RestRequest; | ||
| import org.opensearch.extensions.rest.ExtensionRestResponse; | ||
| import org.opensearch.extensions.rest.RouteHandler; | ||
|
|
||
| import java.util.function.Function; | ||
|
|
||
| public class ExtensionRouteHandler extends RouteHandler { | ||
|
|
||
| public ExtensionRouteHandler( | ||
| String handlerName, | ||
| RestRequest.Method method, | ||
| String path, | ||
| Function<RestRequest, ExtensionRestResponse> handler | ||
| ) { | ||
| super(ExtensionRouteHandlerFactory.getInstance().generateRouteName(handlerName), method, path, handler); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm trying to understand why we need an entire (sub)class just to call a generator function for one parameter. I don't think this class is necessary; we should just stick in an appropriate (probably static) method that does this name generation whenever we call |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| package org.opensearch.sdk; | ||
|
|
||
| import org.opensearch.OpenSearchException; | ||
|
|
||
| public class ExtensionRouteHandlerFactory { | ||
| private static ExtensionRouteHandlerFactory INSTANCE; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you think we can use Guice here to achieve this?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would be great. I'll look into it. The main problem I was facing was that I needed to have the shortName of the extension available in the REST Handler (i.e. inside of RestHelloAction for the sample extension). The way I went about doing that was to create a singleton class that could be fetched throughout the SDK but if there is a better way to do it then I will switch to using that.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Extensions are always initialized with their settings. Any method on the main In the AD Extension we usually pass the |
||
|
|
||
| private String extensionShortName; | ||
|
|
||
| private ExtensionRouteHandlerFactory() { } | ||
|
|
||
| public static ExtensionRouteHandlerFactory getInstance() { | ||
| if(INSTANCE == null) { | ||
| INSTANCE = new ExtensionRouteHandlerFactory(); | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we trying to create SINGLETON object for INSTANCE here? if yes is it multithreaded safe?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, the problem I was facing is that I needed the extensions shortName (abbreviation) inside the RestHandler classes like Do you know how I can test if this is multi-threaded safe? |
||
|
|
||
| return INSTANCE; | ||
| } | ||
|
|
||
| public void init(String extensionShortName) { | ||
| if (this.extensionShortName != null) { | ||
| throw new OpenSearchException("ExtensionRouteHandlerFactory was previously initialized"); | ||
| } | ||
| this.extensionShortName = extensionShortName; | ||
| } | ||
|
|
||
| public String generateRouteName(String handlerName) { | ||
| return extensionShortName + ":" + handlerName; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method call doesn't check whether There's no documentation in this method that it requires initialization first. This seems like a whole lot of class overhead, initializations, and initialization checks to create a factory object whose only purpose seems to be to prepend one string to another, that seems to be possible with a simple static method somewhere. |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -53,6 +53,8 @@ public class ExtensionSettings { | |||||
| private String hostPort; | ||||||
| private String opensearchAddress; | ||||||
| private String opensearchPort; | ||||||
| private String shortName; | ||||||
| private Map<String, String> securitySettings; | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this also be |
||||||
|
|
||||||
| public static final Set<String> SECURITY_SETTINGS_KEYS = Set.of( | ||||||
| "path.home", // TODO Find the right place to put this setting | ||||||
|
|
@@ -79,8 +81,6 @@ public class ExtensionSettings { | |||||
| SSL_TRANSPORT_TRUSTSTORE_TYPE | ||||||
| ); | ||||||
|
|
||||||
| private Map<String, String> securitySettings; | ||||||
|
|
||||||
| /** | ||||||
| * Jackson requires a no-arg constructor. | ||||||
| */ | ||||||
|
|
@@ -93,14 +93,23 @@ private ExtensionSettings() { | |||||
| * Instantiate this class using the specified parameters. | ||||||
| * | ||||||
| * @param extensionName The extension name. Provided to OpenSearch as a response to initialization query. Must match the defined extension name in OpenSearch. | ||||||
| * @param shortName The shortened name for the extension | ||||||
| * @param hostAddress The IP Address to bind this extension to. | ||||||
| * @param hostPort The port to bind this extension to. | ||||||
| * @param opensearchAddress The IP Address on which OpenSearch is running. | ||||||
| * @param opensearchPort The port on which OpenSearch is running. | ||||||
| */ | ||||||
| public ExtensionSettings(String extensionName, String hostAddress, String hostPort, String opensearchAddress, String opensearchPort) { | ||||||
| public ExtensionSettings( | ||||||
| String extensionName, | ||||||
| String shortName, | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll update it, I was torn between different names for this variable. One of the naming ideas was
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated |
||||||
| String hostAddress, | ||||||
| String hostPort, | ||||||
| String opensearchAddress, | ||||||
| String opensearchPort | ||||||
| ) { | ||||||
| super(); | ||||||
| this.extensionName = extensionName; | ||||||
| this.shortName = shortName; | ||||||
| this.hostAddress = hostAddress; | ||||||
| this.hostPort = hostPort; | ||||||
| this.opensearchAddress = opensearchAddress; | ||||||
|
|
@@ -112,6 +121,7 @@ public ExtensionSettings(String extensionName, String hostAddress, String hostPo | |||||
| * Instantiate this class using the specified parameters. | ||||||
| * | ||||||
| * @param extensionName The extension name. Provided to OpenSearch as a response to initialization query. Must match the defined extension name in OpenSearch. | ||||||
| * @param shortName The shortened name for the extension | ||||||
| * @param hostAddress The IP Address to bind this extension to. | ||||||
| * @param hostPort The port to bind this extension to. | ||||||
| * @param opensearchAddress The IP Address on which OpenSearch is running. | ||||||
|
|
@@ -120,20 +130,25 @@ public ExtensionSettings(String extensionName, String hostAddress, String hostPo | |||||
| */ | ||||||
| public ExtensionSettings( | ||||||
| String extensionName, | ||||||
| String shortName, | ||||||
| String hostAddress, | ||||||
| String hostPort, | ||||||
| String opensearchAddress, | ||||||
| String opensearchPort, | ||||||
| Map<String, String> securitySettings | ||||||
| ) { | ||||||
| this(extensionName, hostAddress, hostPort, opensearchAddress, opensearchPort); | ||||||
| this(extensionName, shortName, hostAddress, hostPort, opensearchAddress, opensearchPort); | ||||||
| this.securitySettings = securitySettings; | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This constructor overloading seems off somehow. Setting something and also calling another constructor seems unusual. Is there a way we can make this the primary constructor (that sets All The Things) and have all the other constructors delegate to it with defaults for the missing parameters? |
||||||
| } | ||||||
|
|
||||||
| public String getExtensionName() { | ||||||
| return extensionName; | ||||||
| } | ||||||
|
|
||||||
| public String getShortName() { | ||||||
| return shortName; | ||||||
| } | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This getter should have a test in
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This has been added to a few different tests to verify its behavior |
||||||
|
|
||||||
| public String getHostAddress() { | ||||||
| return hostAddress; | ||||||
| } | ||||||
|
|
@@ -166,6 +181,8 @@ public Map<String, String> getSecuritySettings() { | |||||
| public String toString() { | ||||||
| return "ExtensionSettings{extensionName=" | ||||||
| + extensionName | ||||||
| + ", shortName=" | ||||||
| + shortName | ||||||
| + ", hostAddress=" | ||||||
| + hostAddress | ||||||
| + ", hostPort=" | ||||||
|
|
@@ -205,6 +222,7 @@ public static ExtensionSettings readSettingsFromYaml(String extensionSettingsPat | |||||
| } | ||||||
| return new ExtensionSettings( | ||||||
| extensionMap.get("extensionName").toString(), | ||||||
| extensionMap.get("shortName").toString(), | ||||||
| extensionMap.get("hostAddress").toString(), | ||||||
| extensionMap.get("hostPort").toString(), | ||||||
| extensionMap.get("opensearchAddress").toString(), | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -60,7 +60,7 @@ public void registerHandler(ExtensionRestHandler restHandler) { | |
| * @param handler The handler to actually execute | ||
| * @param method GET, POST, etc. | ||
| */ | ||
| private void registerHandler(Method method, String path, ExtensionRestHandler extensionRestHandler) { | ||
| public void registerHandler(Method method, String path, ExtensionRestHandler extensionRestHandler) { | ||
| pathTrie.insertOrUpdate( | ||
| path, | ||
| new SDKMethodHandlers(path, extensionRestHandler, method), | ||
|
|
@@ -133,6 +133,24 @@ private void registerWithDeprecatedHandler( | |
| registerAsDeprecatedHandler(deprecatedMethod, deprecatedPath, handler, deprecationMessage); | ||
| } | ||
|
|
||
| /** | ||
| * Register a REST handler to handle a method and route in this extension's path registry. | ||
| * | ||
| * @param method The method to register. | ||
| * @param path The path to register. May include named wildcards. | ||
| * @param name An optional name of the REST handler | ||
| * @param extensionRestHandler The RestHandler to handle this route | ||
| */ | ||
| public void registerHandler(Method method, String path, String name, ExtensionRestHandler extensionRestHandler) { | ||
| pathTrie.insertOrUpdate( | ||
| path, | ||
| new SDKMethodHandlers(path, extensionRestHandler, method), | ||
| (mHandlers, newMHandler) -> mHandlers.addMethods(extensionRestHandler, method) | ||
| ); | ||
| String restPathWithName = restPathToString(method, path, name); | ||
| registeredPaths.add(restPathWithName); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are missing adding paths to deprecated paths here. One way to handle this would be to merge both the |
||
| } | ||
|
|
||
| /** | ||
| * Get the registered REST handler for the specified method and path. | ||
| * | ||
|
|
@@ -176,4 +194,16 @@ public List<String> getRegisteredDeprecatedPaths() { | |
| public static String restPathToString(Method method, String path) { | ||
| return method.name() + " " + path; | ||
| } | ||
|
|
||
| /** | ||
| * Converts a REST method and path to a space delimited string to be used as a map lookup key. | ||
| * | ||
| * @param method the method. | ||
| * @param path the path. | ||
| * @param name the name. | ||
| * @return A string appending the method and path. | ||
| */ | ||
| public static String restPathToString(Method method, String path, String name) { | ||
| return method.name() + " " + path + " " + name; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,9 +15,12 @@ | |
| import org.opensearch.common.xcontent.XContentType; | ||
| import org.opensearch.common.xcontent.json.JsonXContent; | ||
| import org.opensearch.extensions.rest.ExtensionRestResponse; | ||
| import org.opensearch.extensions.rest.RouteHandler; | ||
| import org.opensearch.rest.RestRequest; | ||
| import org.opensearch.sdk.ExtensionRouteHandlerFactory; | ||
| import org.opensearch.sdk.rest.BaseExtensionRestHandler; | ||
| import org.opensearch.sdk.rest.ExtensionRestHandler; | ||
| import org.opensearch.sdk.ExtensionRouteHandler; | ||
| import java.io.IOException; | ||
| import java.net.URLDecoder; | ||
| import java.nio.ByteBuffer; | ||
|
|
@@ -50,13 +53,21 @@ public class RestHelloAction extends BaseExtensionRestHandler { | |
| private List<String> worldAdjectives = new ArrayList<>(); | ||
| private Random rand = new Random(); | ||
|
|
||
| /** | ||
| * Instantiate this action | ||
| * | ||
| * @param runner The ExtensionsRunner instance | ||
| */ | ||
| public RestHelloAction() { | ||
| } | ||
|
|
||
| @Override | ||
| public List<RouteHandler> routeHandlers() { | ||
| return List.of( | ||
| new RouteHandler(GET, "/hello", handleGetRequest), | ||
| new RouteHandler(POST, "/hello", handlePostRequest), | ||
| new RouteHandler(PUT, "/hello/{name}", handlePutRequest), | ||
| new RouteHandler(DELETE, "/goodbye", handleDeleteRequest) | ||
| new ExtensionRouteHandler("greet", GET, "/hello", handleGetRequest), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This appears to be the only case where we are actually using the new This method signature is an override that only exists if we are extending |
||
| new ExtensionRouteHandler("greet_with_adjective", POST, "/hello", handlePostRequest), | ||
| new ExtensionRouteHandler("greet_with_name", PUT, "/hello/{name}", handlePutRequest), | ||
| new ExtensionRouteHandler("goodbye", DELETE, "/goodbye", handleDeleteRequest) | ||
| ); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| extensionName: hello-world | ||
| shortName: hw | ||
| hostAddress: 127.0.0.1 | ||
| hostPort: 4532 | ||
| opensearchAddress: 127.0.0.1 | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this class stays, it should not be in the base package. Probably would fit in the
.restsubpackage.