Skip to content

Commit c4a5d5b

Browse files
authored
populate time fields for connectors on return (opensearch-project#2922) (opensearch-project#3035)
* populate time fields for connectors on return fixes opensearch-project#2890 Currently any class that extends the AbstractConnector class has the fields createdTime and lastUpdatedTime set to null. The solution was instantiating the fields in the constructor of the AbstractConnector class, as well updating it within the HTTPConnector class whenever an update happens. Many tests were modified to catch the time fields being populated as such there will be many differences on the string in order to get around the timing issue when doing tests. Signed-off-by: Brian Flores <iflorbri@amazon.com> * fixes backward compatability issues with old connectors fixes opensearch-project#2890 when applying a code change like this previous connectors would have a weird bug where upon calling GET on them would change the timestamp. In this commit, it remains the old connectors without time fields while new ones have time fields, newer connectors will have correct and updated Timestamp. Manual testing was done on a local cluster and two unit tests were done to inspect the time changes on creation and update Signed-off-by: Brian Flores <iflorbri@amazon.com> * fix failing MLRegisterModelInutTest.testToXContent tests Originally this commit was cherry picked from the 2.x branch and as such code changes affected the new build that werent caught on the previous commit 8c006de. Reformatted tests that were failing as the behavior implemented in previous commits was to not display time fields if a connector does not have them in the first place. gradlew build was done to assure the tests passed Signed-off-by: Brian Flores <iflorbri@amazon.com> * Reverts back model tests that were modified incorrectly by connector change When creating a code change to the connector it propagated the new change of the object that affected many UTs, but after changing the logic of indexing the new connector, change the old changes for the unit test involving models with connectors had to be reverted back. UTs specifically for the indexed connectors have been created in UpdateConnectorTransportActionTests were done to capture this Signed-off-by: Brian Flores <iflorbri@amazon.com> * Adds lastUpdateTime to Old Connectors Previoulsy we didnt consider the old connectors to have time fields at all, But given offline discussion if we add time fields to old connectors users could get more information moving forward without breaking any backward features. The solution to this was setting the last updated time in the update connector api; now moving forward any connector gets attached a last updated time field. I updated the testUpdateConnectorDoesNotUpdateHTTPCOnnectorTimeFields method to check that lastUpdateTime has a timestamp but that createdTime has no time field. Signed-off-by: Brian Flores <iflorbri@amazon.com> * Fixes wildcard import in UpdateConnectorTransportActionTests Signed-off-by: Brian Flores <iflorbri@amazon.com> --------- Signed-off-by: Brian Flores <iflorbri@amazon.com> (cherry picked from commit 88eaefd)
1 parent af54af9 commit c4a5d5b

File tree

9 files changed

+163
-34
lines changed

9 files changed

+163
-34
lines changed

common/src/main/java/org/opensearch/ml/common/connector/AbstractConnector.java

+2
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,9 @@ public abstract class AbstractConnector implements Connector {
6565
protected User owner;
6666
@Setter
6767
protected AccessMode access;
68+
@Setter
6869
protected Instant createdTime;
70+
@Setter
6971
protected Instant lastUpdateTime;
7072
@Setter
7173
protected ConnectorClientConfig connectorClientConfig;

common/src/main/java/org/opensearch/ml/common/connector/Connector.java

+5
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
import java.security.AccessController;
1313
import java.security.PrivilegedActionException;
1414
import java.security.PrivilegedExceptionAction;
15+
import java.time.Instant;
1516
import java.util.List;
1617
import java.util.Map;
1718
import java.util.Optional;
@@ -44,6 +45,10 @@ public interface Connector extends ToXContentObject, Writeable {
4445

4546
String getProtocol();
4647

48+
void setCreatedTime(Instant createdTime);
49+
50+
void setLastUpdateTime(Instant lastUpdateTime);
51+
4752
User getOwner();
4853

4954
void setOwner(User user);

common/src/test/java/org/opensearch/ml/common/RemoteModelTests.java

+16-16
Original file line numberDiff line numberDiff line change
@@ -61,22 +61,22 @@ public void toXContent_InternalConnector() throws IOException {
6161
XContentBuilder builder = XContentBuilder.builder(XContentType.JSON.xContent());
6262
mlModel.toXContent(builder, EMPTY_PARAMS);
6363
String mlModelContent = TestHelper.xContentBuilderToString(builder);
64-
assertEquals(
65-
"{\"name\":\"test_model_name\",\"model_group_id\":\"test_group_id\","
66-
+ "\"algorithm\":\"REMOTE\",\"model_version\":\"1.0.0\",\"description\":\"test model\","
67-
+ "\"connector\":{\"name\":\"test_connector_name\",\"version\":\"1\","
68-
+ "\"description\":\"this is a test connector\",\"protocol\":\"http\","
69-
+ "\"parameters\":{\"input\":\"test input value\"},\"credential\":{\"key\":\"test_key_value\"},"
70-
+ "\"actions\":[{\"action_type\":\"PREDICT\",\"method\":\"POST\",\"url\":\"https://test.com\","
71-
+ "\"headers\":{\"api_key\":\"${credential.key}\"},"
72-
+ "\"request_body\":\"{\\\"input\\\": \\\"${parameters.input}\\\"}\","
73-
+ "\"pre_process_function\":\"connector.pre_process.openai.embedding\","
74-
+ "\"post_process_function\":\"connector.post_process.openai.embedding\"}],"
75-
+ "\"backend_roles\":[\"role1\",\"role2\"],\"access\":\"public\","
76-
+ "\"client_config\":{\"max_connection\":30,\"connection_timeout\":30000,\"read_timeout\":30000,"
77-
+ "\"retry_backoff_millis\":10,\"retry_timeout_seconds\":10,\"max_retry_times\":-1,\"retry_backoff_policy\":\"constant\"}}}",
78-
mlModelContent
79-
);
64+
65+
String expectedConnectorResponse = "{\"name\":\"test_model_name\",\"model_group_id\":\"test_group_id\","
66+
+ "\"algorithm\":\"REMOTE\",\"model_version\":\"1.0.0\",\"description\":\"test model\","
67+
+ "\"connector\":{\"name\":\"test_connector_name\",\"version\":\"1\","
68+
+ "\"description\":\"this is a test connector\",\"protocol\":\"http\","
69+
+ "\"parameters\":{\"input\":\"test input value\"},\"credential\":{\"key\":\"test_key_value\"},"
70+
+ "\"actions\":[{\"action_type\":\"PREDICT\",\"method\":\"POST\",\"url\":\"https://test.com\","
71+
+ "\"headers\":{\"api_key\":\"${credential.key}\"},"
72+
+ "\"request_body\":\"{\\\"input\\\": \\\"${parameters.input}\\\"}\","
73+
+ "\"pre_process_function\":\"connector.pre_process.openai.embedding\","
74+
+ "\"post_process_function\":\"connector.post_process.openai.embedding\"}],"
75+
+ "\"backend_roles\":[\"role1\",\"role2\"],\"access\":\"public\","
76+
+ "\"client_config\":{\"max_connection\":30,\"connection_timeout\":30000,\"read_timeout\":30000,"
77+
+ "\"retry_backoff_millis\":10,\"retry_timeout_seconds\":10,\"max_retry_times\":-1,\"retry_backoff_policy\":\"constant\"}}}";
78+
79+
assertEquals(expectedConnectorResponse, mlModelContent);
8080
}
8181

8282
@Test

common/src/test/java/org/opensearch/ml/common/connector/HttpConnectorTest.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -85,12 +85,12 @@ public void toXContent() throws IOException {
8585
XContentBuilder builder = XContentBuilder.builder(XContentType.JSON.xContent());
8686
connector.toXContent(builder, ToXContent.EMPTY_PARAMS);
8787
String content = TestHelper.xContentBuilderToString(builder);
88+
8889
Assert.assertEquals(TEST_CONNECTOR_JSON_STRING, content);
8990
}
9091

9192
@Test
9293
public void constructor_Parser() throws IOException {
93-
9494
XContentParser parser = XContentType.JSON
9595
.xContent()
9696
.createParser(

common/src/test/java/org/opensearch/ml/common/transport/connector/MLConnectorGetResponseTests.java

+15-15
Original file line numberDiff line numberDiff line change
@@ -58,21 +58,21 @@ public void toXContentTest() throws IOException {
5858
mlConnectorGetResponse.toXContent(builder, ToXContent.EMPTY_PARAMS);
5959
assertNotNull(builder);
6060
String jsonStr = builder.toString();
61-
assertEquals(
62-
"{\"name\":\"test_connector_name\",\"version\":\"1\","
63-
+ "\"description\":\"this is a test connector\",\"protocol\":\"http\","
64-
+ "\"parameters\":{\"input\":\"test input value\"},\"credential\":{\"key\":\"test_key_value\"},"
65-
+ "\"actions\":[{\"action_type\":\"PREDICT\",\"method\":\"POST\",\"url\":\"https://test.com\","
66-
+ "\"headers\":{\"api_key\":\"${credential.key}\"},"
67-
+ "\"request_body\":\"{\\\"input\\\": \\\"${parameters.input}\\\"}\","
68-
+ "\"pre_process_function\":\"connector.pre_process.openai.embedding\","
69-
+ "\"post_process_function\":\"connector.post_process.openai.embedding\"}],"
70-
+ "\"backend_roles\":[\"role1\",\"role2\"],\"access\":\"public\","
71-
+ "\"client_config\":{\"max_connection\":30,"
72-
+ "\"connection_timeout\":30000,\"read_timeout\":30000,"
73-
+ "\"retry_backoff_millis\":10,\"retry_timeout_seconds\":10,\"max_retry_times\":-1,\"retry_backoff_policy\":\"constant\"}}",
74-
jsonStr
75-
);
61+
62+
String expectedControllerResponse = "{\"name\":\"test_connector_name\",\"version\":\"1\","
63+
+ "\"description\":\"this is a test connector\",\"protocol\":\"http\","
64+
+ "\"parameters\":{\"input\":\"test input value\"},\"credential\":{\"key\":\"test_key_value\"},"
65+
+ "\"actions\":[{\"action_type\":\"PREDICT\",\"method\":\"POST\",\"url\":\"https://test.com\","
66+
+ "\"headers\":{\"api_key\":\"${credential.key}\"},"
67+
+ "\"request_body\":\"{\\\"input\\\": \\\"${parameters.input}\\\"}\","
68+
+ "\"pre_process_function\":\"connector.pre_process.openai.embedding\","
69+
+ "\"post_process_function\":\"connector.post_process.openai.embedding\"}],"
70+
+ "\"backend_roles\":[\"role1\",\"role2\"],\"access\":\"public\","
71+
+ "\"client_config\":{\"max_connection\":30,"
72+
+ "\"connection_timeout\":30000,\"read_timeout\":30000,"
73+
+ "\"retry_backoff_millis\":10,\"retry_timeout_seconds\":10,\"max_retry_times\":-1,\"retry_backoff_policy\":\"constant\"}}";
74+
75+
assertEquals(expectedControllerResponse, jsonStr);
7676
}
7777

7878
@Test

common/src/test/java/org/opensearch/ml/common/transport/model/MLUpdateModelInputTest.java

+3-2
Original file line numberDiff line numberDiff line change
@@ -64,8 +64,8 @@ public class MLUpdateModelInputTest {
6464
+ "{\"name\":\"test\",\"version\":\"1\",\"protocol\":\"http\",\"parameters\":{\"param1\":\"value1\"},\"credential\":"
6565
+ "{\"api_key\":\"credential_value\"},\"actions\":[{\"action_type\":\"PREDICT\",\"method\":\"POST\",\"url\":"
6666
+ "\"https://api.openai.com/v1/chat/completions\",\"headers\":{\"Authorization\":\"Bearer ${credential.api_key}\"},\"request_body\":"
67-
+ "\"{ \\\"model\\\": \\\"${parameters.model}\\\", \\\"messages\\\": ${parameters.messages} }\"}]},\"connector_id\":"
68-
+ "\"test-connector_id\",\"last_updated_time\":1}";
67+
+ "\"{ \\\"model\\\": \\\"${parameters.model}\\\", \\\"messages\\\": ${parameters.messages} }\"}]},"
68+
+ "\"connector_id\":\"test-connector_id\",\"last_updated_time\":1}";
6969

7070
private final String expectedOutputStr = "{\"model_id\":null,\"name\":\"name\",\"description\":\"description\",\"model_group_id\":"
7171
+ "\"modelGroupId\",\"is_enabled\":false,\"rate_limiter\":"
@@ -152,6 +152,7 @@ public void readInputStreamSuccessWithNullFields() throws IOException {
152152
@Test
153153
public void testToXContent() throws Exception {
154154
String jsonStr = serializationWithToXContent(updateModelInput);
155+
155156
assertEquals(expectedOutputStrForUpdateRequestDoc, jsonStr);
156157
}
157158

plugin/src/main/java/org/opensearch/ml/action/connector/TransportCreateConnectorAction.java

+5
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
import static org.opensearch.ml.common.CommonValue.ML_CONNECTOR_INDEX;
99
import static org.opensearch.ml.settings.MLCommonsSettings.ML_COMMONS_TRUSTED_CONNECTOR_ENDPOINTS_REGEX;
1010

11+
import java.time.Instant;
1112
import java.util.HashSet;
1213
import java.util.List;
1314

@@ -134,6 +135,10 @@ private void indexConnector(Connector connector, ActionListener<MLCreateConnecto
134135
listener.onResponse(response);
135136
}, listener::onFailure);
136137

138+
Instant currentTime = Instant.now();
139+
connector.setCreatedTime(currentTime);
140+
connector.setLastUpdateTime(currentTime);
141+
137142
IndexRequest indexRequest = new IndexRequest(ML_CONNECTOR_INDEX);
138143
indexRequest.source(connector.toXContent(XContentBuilder.builder(XContentType.JSON.xContent()), ToXContent.EMPTY_PARAMS));
139144
indexRequest.setRefreshPolicy(WriteRequest.RefreshPolicy.IMMEDIATE);

plugin/src/main/java/org/opensearch/ml/action/connector/UpdateConnectorTransportAction.java

+4
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import static org.opensearch.ml.common.CommonValue.ML_MODEL_INDEX;
1010
import static org.opensearch.ml.settings.MLCommonsSettings.ML_COMMONS_TRUSTED_CONNECTOR_ENDPOINTS_REGEX;
1111

12+
import java.time.Instant;
1213
import java.util.ArrayList;
1314
import java.util.Arrays;
1415
import java.util.List;
@@ -93,6 +94,9 @@ protected void doExecute(Task task, ActionRequest request, ActionListener<Update
9394
if (Boolean.TRUE.equals(hasPermission)) {
9495
connector.update(mlUpdateConnectorAction.getUpdateContent(), mlEngine::encrypt);
9596
connector.validateConnectorURL(trustedConnectorEndpointsRegex);
97+
98+
connector.setLastUpdateTime(Instant.now());
99+
96100
UpdateRequest updateRequest = new UpdateRequest(ML_CONNECTOR_INDEX, connectorId);
97101
updateRequest.setRefreshPolicy(WriteRequest.RefreshPolicy.IMMEDIATE);
98102
updateRequest.doc(connector.toXContent(XContentBuilder.builder(XContentType.JSON.xContent()), ToXContent.EMPTY_PARAMS));

plugin/src/test/java/org/opensearch/ml/action/connector/UpdateConnectorTransportActionTests.java

+112
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414

1515
import java.io.IOException;
1616
import java.nio.file.Path;
17+
import java.time.Instant;
1718
import java.util.Arrays;
1819
import java.util.List;
1920
import java.util.Map;
@@ -202,6 +203,117 @@ public void setup() throws IOException {
202203
}).when(connectorAccessControlHelper).getConnector(any(Client.class), any(String.class), isA(ActionListener.class));
203204
}
204205

206+
@Test
207+
public void testUpdateConnectorDoesNotUpdateHttpConnectorTimeFields() {
208+
HttpConnector connector = HttpConnector
209+
.builder()
210+
.name("test")
211+
.protocol("http")
212+
.version("1")
213+
.credential(Map.of("api_key", "credential_value"))
214+
.parameters(Map.of("param1", "value1"))
215+
.actions(
216+
Arrays
217+
.asList(
218+
ConnectorAction
219+
.builder()
220+
.actionType(ConnectorAction.ActionType.PREDICT)
221+
.method("POST")
222+
.url("https://api.openai.com/v1/chat/completions")
223+
.headers(Map.of("Authorization", "Bearer ${credential.api_key}"))
224+
.requestBody("{ \"model\": \"${parameters.model}\", \"messages\": ${parameters.messages} }")
225+
.build()
226+
)
227+
)
228+
.build();
229+
230+
assertNull(connector.getCreatedTime());
231+
assertNull(connector.getLastUpdateTime());
232+
233+
doReturn(true).when(connectorAccessControlHelper).validateConnectorAccess(any(Client.class), any(Connector.class));
234+
235+
doAnswer(invocation -> {
236+
ActionListener<Connector> listener = invocation.getArgument(2);
237+
listener.onResponse(connector);
238+
return null;
239+
}).when(connectorAccessControlHelper).getConnector(any(Client.class), any(String.class), isA(ActionListener.class));
240+
241+
doAnswer(invocation -> {
242+
ActionListener<SearchResponse> actionListener = invocation.getArgument(1);
243+
actionListener.onResponse(searchResponse);
244+
return null;
245+
}).when(client).search(any(SearchRequest.class), isA(ActionListener.class));
246+
247+
doAnswer(invocation -> {
248+
ActionListener<UpdateResponse> listener = invocation.getArgument(1);
249+
listener.onResponse(updateResponse);
250+
return null;
251+
}).when(client).update(any(UpdateRequest.class), isA(ActionListener.class));
252+
253+
updateConnectorTransportAction.doExecute(task, updateRequest, actionListener);
254+
255+
assertNull(connector.getCreatedTime());
256+
assertNotNull(connector.getLastUpdateTime());
257+
}
258+
259+
@Test
260+
public void testUpdateConnectorUpdatesHttpConnectorTimeFields() {
261+
HttpConnector connector = HttpConnector
262+
.builder()
263+
.name("test")
264+
.protocol("http")
265+
.version("1")
266+
.credential(Map.of("api_key", "credential_value"))
267+
.parameters(Map.of("param1", "value1"))
268+
.actions(
269+
Arrays
270+
.asList(
271+
ConnectorAction
272+
.builder()
273+
.actionType(ConnectorAction.ActionType.PREDICT)
274+
.method("POST")
275+
.url("https://api.openai.com/v1/chat/completions")
276+
.headers(Map.of("Authorization", "Bearer ${credential.api_key}"))
277+
.requestBody("{ \"model\": \"${parameters.model}\", \"messages\": ${parameters.messages} }")
278+
.build()
279+
)
280+
)
281+
.build();
282+
283+
Instant testInitialTime = Instant.now();
284+
connector.setCreatedTime(testInitialTime);
285+
connector.setLastUpdateTime(testInitialTime);
286+
287+
assert (connector.getCreatedTime().toEpochMilli() == connector.getLastUpdateTime().toEpochMilli());
288+
289+
doReturn(true).when(connectorAccessControlHelper).validateConnectorAccess(any(Client.class), any(Connector.class));
290+
291+
doAnswer(invocation -> {
292+
ActionListener<Connector> listener = invocation.getArgument(2);
293+
listener.onResponse(connector);
294+
return null;
295+
}).when(connectorAccessControlHelper).getConnector(any(Client.class), any(String.class), isA(ActionListener.class));
296+
297+
doAnswer(invocation -> {
298+
ActionListener<SearchResponse> actionListener = invocation.getArgument(1);
299+
actionListener.onResponse(searchResponse);
300+
return null;
301+
}).when(client).search(any(SearchRequest.class), isA(ActionListener.class));
302+
303+
doAnswer(invocation -> {
304+
ActionListener<UpdateResponse> listener = invocation.getArgument(1);
305+
listener.onResponse(updateResponse);
306+
return null;
307+
}).when(client).update(any(UpdateRequest.class), isA(ActionListener.class));
308+
309+
updateConnectorTransportAction.doExecute(task, updateRequest, actionListener);
310+
311+
assertTrue(
312+
"Last update time must be bigger than the creation time",
313+
connector.getLastUpdateTime().toEpochMilli() >= connector.getCreatedTime().toEpochMilli()
314+
);
315+
}
316+
205317
@Test
206318
public void testExecuteConnectorAccessControlSuccess() {
207319
doReturn(true).when(connectorAccessControlHelper).validateConnectorAccess(any(Client.class), any(Connector.class));

0 commit comments

Comments
 (0)