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.

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

[Extensions] Adding versioning support for registering Rest actions #7302

Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
## [Unreleased 2.x]
### Added
- [Extensions] Moving Extensions APIs to protobuf serialization. ([#6960](https://github.com/opensearch-project/OpenSearch/pull/6960))
- [Extensions] Moving RestActions APIs to protobuf serialization. ([#7302](https://github.com/opensearch-project/OpenSearch/pull/7302))

### Dependencies

Expand Down
4 changes: 4 additions & 0 deletions server/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,10 @@ tasks.named("missingJavadoc").configure {
*/
dependsOn("generateProto")
javadocMissingIgnore = [
"org.opensearch.extensions.proto.ExtensionIdentityProto",
"org.opensearch.extensions.proto.ExtensionIdentityProto.ExtensionIdentityOrBuilder",
"org.opensearch.extensions.proto.RegisterRestActionsProto",
"org.opensearch.extensions.proto.RegisterRestActionsProto.RegisterRestActionsOrBuilder",
"org.opensearch.extensions.proto.ExtensionRequestProto",
"org.opensearch.extensions.proto.ExtensionRequestProto.ExtensionRequestOrBuilder",
"org.opensearch.extensions.proto"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import org.opensearch.common.Nullable;
import org.opensearch.common.io.stream.StreamInput;
import org.opensearch.common.io.stream.StreamOutput;
import org.opensearch.extensions.proto.ExtensionIdentityProto;
import org.opensearch.extensions.proto.ExtensionRequestProto;
import org.opensearch.transport.TransportRequest;

Expand All @@ -35,7 +36,7 @@ public ExtensionRequest(ExtensionRequestProto.RequestType requestType) {
public ExtensionRequest(ExtensionRequestProto.RequestType requestType, @Nullable String uniqueId) {
ExtensionRequestProto.ExtensionRequest.Builder builder = ExtensionRequestProto.ExtensionRequest.newBuilder();
if (uniqueId != null) {
builder.setUniqueId(uniqueId);
builder.setIdentity(ExtensionIdentityProto.ExtensionIdentity.newBuilder().setUniqueId(uniqueId).build());
}
this.request = builder.setRequestType(requestType).build();
}
Expand All @@ -56,24 +57,28 @@ public ExtensionRequestProto.RequestType getRequestType() {
}

public String getUniqueId() {
return request.getUniqueId();
return request.getIdentity().getUniqueId();
}

public String toString() {
return "ExtensionRequest{" + request.toString() + '}';
}

public ExtensionIdentityProto.ExtensionIdentity getExtensionIdentity() {
return request.getIdentity();
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
ExtensionRequest that = (ExtensionRequest) o;
return Objects.equals(request.getRequestType(), that.request.getRequestType())
&& Objects.equals(request.getUniqueId(), that.request.getUniqueId());
&& Objects.equals(request.getIdentity().getUniqueId(), that.request.getIdentity().getUniqueId());
}

@Override
public int hashCode() {
return Objects.hash(request.getRequestType(), request.getUniqueId());
return Objects.hash(request.getRequestType(), request.getIdentity().getUniqueId());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,11 @@

import org.opensearch.common.io.stream.StreamInput;
import org.opensearch.common.io.stream.StreamOutput;
import org.opensearch.extensions.proto.ExtensionIdentityProto.ExtensionIdentity;
import org.opensearch.extensions.proto.RegisterRestActionsProto.RegisterRestActions;
import org.opensearch.transport.TransportRequest;

import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import java.util.Objects;

Expand All @@ -23,51 +24,48 @@
* @opensearch.internal
*/
public class RegisterRestActionsRequest extends TransportRequest {
private String uniqueId;
private List<String> restActions;
private List<String> deprecatedRestActions;
private final RegisterRestActions request;

public RegisterRestActionsRequest(String uniqueId, List<String> restActions, List<String> deprecatedRestActions) {
this.uniqueId = uniqueId;
this.restActions = new ArrayList<>(restActions);
this.deprecatedRestActions = new ArrayList<>(deprecatedRestActions);
ExtensionIdentity identity = ExtensionIdentity.newBuilder().setUniqueId(uniqueId).build();
this.request = RegisterRestActions.newBuilder()
.setIdentity(identity)
.addAllRestActions(restActions)
.addAllDeprecatedRestActions(deprecatedRestActions)
.build();
Copy link
Member

Choose a reason for hiding this comment

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

I haven't had time to follow up with a thorough design, but I think there may need to be one more bit of information passed from the extension here, and it would be nice to do it with this PR rather than a separate one.

It relates to SDK #355 and replaced/deprecated routes. Basically:

  • we want the deprecated routes coming from the extensions to be verbatim (with /_plugin prefix) without making any changes on OpenSearch side (but with possible exception handling if the plugin is installed and the prefix is already taken, per the linked issue)
  • we want active/new routes coming from the extension to use the uniqueId and /_extension prefix but also possibly register with a /_plugin prefix, per the linked issue.

So we may want to actually include some sort of "prefix" string.

This may be exactly what you are doing with ExtensionIdentity but I'm thinking perhaps that may need a second string argument.

Sorry this isn't fully thought out, but perhaps something we should all agree on (and possibly implement SDK #355 shortly after this).

Copy link
Member

Choose a reason for hiding this comment

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

(To be clear, my suggestion is to add a second string field to Identity, so it would have a uniqueID and a prefix.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Had a chat with @dbwiddis, we'll add a new bool in a different PR to solve it.
Will try to get this PR merged as the framework.

}

public RegisterRestActionsRequest(StreamInput in) throws IOException {
super(in);
uniqueId = in.readString();
restActions = in.readStringList();
deprecatedRestActions = in.readStringList();
request = RegisterRestActions.parseFrom(in.readByteArray());
}

@Override
public void writeTo(StreamOutput out) throws IOException {
super.writeTo(out);
out.writeString(uniqueId);
out.writeStringCollection(restActions);
out.writeStringCollection(deprecatedRestActions);
out.writeByteArray(request.toByteArray());
}

public String getUniqueId() {
return uniqueId;
return request.getIdentity().getUniqueId();
}

public List<String> getRestActions() {
return List.copyOf(restActions);
return List.copyOf(request.getRestActionsList());
}

public List<String> getDeprecatedRestActions() {
return List.copyOf(deprecatedRestActions);
return List.copyOf(request.getDeprecatedRestActionsList());
}

@Override
public String toString() {
return "RestActionsRequest{uniqueId="
+ uniqueId
return "RestActionsRequest{Identity="
+ request.getIdentity()
+ ", restActions="
+ restActions
+ request.getRestActionsList()
+ ", deprecatedRestActions="
+ deprecatedRestActions
+ request.getDeprecatedRestActionsList()
+ "}";
}

Expand All @@ -76,13 +74,13 @@ public boolean equals(Object obj) {
if (this == obj) return true;
if (obj == null || getClass() != obj.getClass()) return false;
RegisterRestActionsRequest that = (RegisterRestActionsRequest) obj;
return Objects.equals(uniqueId, that.uniqueId)
&& Objects.equals(restActions, that.restActions)
&& Objects.equals(deprecatedRestActions, that.deprecatedRestActions);
return Objects.equals(request.getIdentity().getUniqueId(), that.request.getIdentity().getUniqueId())
&& Objects.equals(request.getRestActionsList(), that.request.getRestActionsList())
&& Objects.equals(request.getDeprecatedRestActionsList(), that.request.getDeprecatedRestActionsList());
}

@Override
public int hashCode() {
return Objects.hash(uniqueId, restActions, deprecatedRestActions);
return Objects.hash(request.getIdentity(), request.getRestActionsList(), request.getDeprecatedRestActionsList());
}
}
19 changes: 19 additions & 0 deletions server/src/main/proto/extensions/ExtensionIdentityProto.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/*
* 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.
*
* Modifications Copyright OpenSearch Contributors. See
* GitHub history for details.
*/

syntax = "proto3";
package org.opensearch.extensions.proto;

option java_outer_classname = "ExtensionIdentityProto";

message ExtensionIdentity {
string uniqueId = 1;
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
syntax = "proto3";
package org.opensearch.extensions.proto;

import "extensions/ExtensionIdentityProto.proto";
option java_outer_classname = "ExtensionRequestProto";

enum RequestType {
Expand All @@ -27,6 +28,6 @@ enum RequestType {
}

message ExtensionRequest {
RequestType requestType = 1;
string uniqueId = 2;
ExtensionIdentity identity = 1;
RequestType requestType = 2;
}
22 changes: 22 additions & 0 deletions server/src/main/proto/extensions/RegisterRestActionsProto.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/*
* 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.
*
* Modifications Copyright OpenSearch Contributors. See
* GitHub history for details.
*/

syntax = "proto3";
package org.opensearch.extensions.proto;

import "extensions/ExtensionIdentityProto.proto";
option java_outer_classname = "RegisterRestActionsProto";

message RegisterRestActions {
ExtensionIdentity identity = 1;
repeated string restActions = 2;
repeated string deprecatedRestActions = 3;
}