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

Jump to bottom

[Backport 2.x] Remove useCase and defaultParams field in WorkflowRequest (#952) #954

Merged
merged 3 commits into from
Nov 11, 2024
+12 −194
Merged

[Backport 2.x] Remove useCase and defaultParams field in WorkflowRequest (#952) #954

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5af12f6
Remove useCase and defaultParams field in WorkflowRequest (#952)
junweid62 Nov 11, 2024
404f47f
Remove useCase and defaultParams field in WorkflowRequest (#952)
junweid62 Nov 11, 2024
42f6b39
Merge branch '2.x' into backport/backport-952-to-2.x
junweid62 Nov 11, 2024
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.1.0/)
- Incrementally remove resources from workflow state during deprovisioning ([#898](https://github.com/opensearch-project/flow-framework/pull/898))

### Bug Fixes
- Remove useCase and defaultParams field in WorkflowRequest ([#758](https://github.com/opensearch-project/flow-framework/pull/758))
- Fixed Template Update Location and Improved Logger Statements in ReprovisionWorkflowTransportAction ([#918](https://github.com/opensearch-project/flow-framework/pull/918))

### Infrastructure
Expand Down
2 changes: 0 additions & 2 deletions src/main/java/org/opensearch/flowframework/rest/RestCreateWorkflowAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,8 +226,6 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli
validation,
provision || updateFields,
params,
useCase,
useCaseDefaultsMap,
reprovision
);

Expand Down
57 changes: 2 additions & 55 deletions src/main/java/org/opensearch/flowframework/transport/WorkflowRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,23 +62,13 @@ public class WorkflowRequest extends ActionRequest {
*/
private Map<String, String> params;

/**
* use case flag
*/
private String useCase;

/**
* Deafult params map from use case
*/
private Map<String, String> defaultParams;

/**
* Instantiates a new WorkflowRequest, set validation to all, no provisioning
* @param workflowId the documentId of the workflow
* @param template the use case template which describes the workflow
*/
public WorkflowRequest(@Nullable String workflowId, @Nullable Template template) {
this(workflowId, template, new String[] { "all" }, false, Collections.emptyMap(), null, Collections.emptyMap(), false);
this(workflowId, template, new String[] { "all" }, false, Collections.emptyMap(), false);
}

/**
Expand All @@ -88,28 +78,7 @@ public WorkflowRequest(@Nullable String workflowId, @Nullable Template template)
* @param params The parameters from the REST path
*/
public WorkflowRequest(@Nullable String workflowId, @Nullable Template template, Map<String, String> params) {
this(workflowId, template, new String[] { "all" }, true, params, null, Collections.emptyMap(), false);
}

/**
* Instantiates a new WorkflowRequest with params map, set validation to all, provisioning to true
* @param workflowId the documentId of the workflow
* @param template the use case template which describes the workflow
* @param useCase the default use case give by user
* @param defaultParams The parameters from the REST body when a use case is given
*/
public WorkflowRequest(@Nullable String workflowId, @Nullable Template template, String useCase, Map<String, String> defaultParams) {
this(workflowId, template, new String[] { "all" }, false, Collections.emptyMap(), useCase, defaultParams, false);
}

/**
* Instantiates a new WorkflowRequest, set validation to all, sets reprovision flag
* @param workflowId the documentId of the workflow
* @param template the updated template
* @param reprovision the reprovision flag
*/
public WorkflowRequest(String workflowId, Template template, boolean reprovision) {
this(workflowId, template, new String[] { "all" }, false, Collections.emptyMap(), null, Collections.emptyMap(), reprovision);
this(workflowId, template, new String[] { "all" }, true, params, false);
}

/**
Expand All @@ -119,8 +88,6 @@ public WorkflowRequest(String workflowId, Template template, boolean reprovision
* @param validation flag to indicate if validation is necessary
* @param provisionOrUpdate provision or updateFields flag. Only one may be true, the presence of update_fields key in map indicates if updating fields, otherwise true means it's provisioning.
* @param params map of REST path params. If provisionOrUpdate is false, must be an empty map. If update_fields key is present, must be only key.
* @param useCase default use case given
* @param defaultParams the params to be used in the substitution based on the default use case.
* @param reprovision flag to indicate if request is to reprovision
*/
public WorkflowRequest(
Expand All @@ -129,8 +96,6 @@ public WorkflowRequest(
String[] validation,
boolean provisionOrUpdate,
Map<String, String> params,
String useCase,
Map<String, String> defaultParams,
boolean reprovision
) {
this.workflowId = workflowId;
Expand All @@ -142,8 +107,6 @@ public WorkflowRequest(
throw new IllegalArgumentException("Params may only be included when provisioning.");
}
this.params = this.updateFields ? Collections.emptyMap() : params;
this.useCase = useCase;
this.defaultParams = defaultParams;
this.reprovision = reprovision;
}

Expand Down Expand Up @@ -222,22 +185,6 @@ public Map<String, String> getParams() {
return Map.copyOf(this.params);
}

/**
* Gets the use case
* @return the use case
*/
public String getUseCase() {
return this.useCase;
}

/**
* Gets the params map
* @return the params map
*/
public Map<String, String> getDefaultParams() {
return Map.copyOf(this.defaultParams);
}

/**
* Gets the reprovision flag
* @return the reprovision boolean
Expand Down
83 changes: 8 additions & 75 deletions src/test/java/org/opensearch/flowframework/transport/CreateWorkflowTransportActionTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ public void testMaxWorkflow() {

@SuppressWarnings("unchecked")
ActionListener<WorkflowResponse> listener = mock(ActionListener.class);
WorkflowRequest workflowRequest = new WorkflowRequest(null, template, new String[] { "off" }, false, Collections.emptyMap(), null, Collections.emptyMap(), false);
WorkflowRequest workflowRequest = new WorkflowRequest(null, template, new String[] { "off" }, false, Collections.emptyMap(), false);

doAnswer(invocation -> {
ActionListener<SearchResponse> searchListener = invocation.getArgument(1);
Expand Down Expand Up @@ -289,16 +289,7 @@ public void onFailure(Exception e) {
public void testFailedToCreateNewWorkflow() {
@SuppressWarnings("unchecked")
ActionListener<WorkflowResponse> listener = mock(ActionListener.class);
WorkflowRequest workflowRequest = new WorkflowRequest(
null,
template,
new String[] { "off" },
false,
Collections.emptyMap(),
null,
Collections.emptyMap(),
false
);
WorkflowRequest workflowRequest = new WorkflowRequest(null, template, new String[] { "off" }, false, Collections.emptyMap(), false);

// Bypass checkMaxWorkflows and force onResponse
doAnswer(invocation -> {
Expand Down Expand Up @@ -329,16 +320,7 @@ public void testFailedToCreateNewWorkflow() {
public void testCreateNewWorkflow() {
@SuppressWarnings("unchecked")
ActionListener<WorkflowResponse> listener = mock(ActionListener.class);
WorkflowRequest workflowRequest = new WorkflowRequest(
null,
template,
new String[] { "off" },
false,
Collections.emptyMap(),
null,
Collections.emptyMap(),
false
);
WorkflowRequest workflowRequest = new WorkflowRequest(null, template, new String[] { "off" }, false, Collections.emptyMap(), false);

// Bypass checkMaxWorkflows and force onResponse
doAnswer(invocation -> {
Expand Down Expand Up @@ -402,16 +384,7 @@ public void testCreateWithUserAndFilterOn() {
);

ActionListener<WorkflowResponse> listener = mock(ActionListener.class);
WorkflowRequest workflowRequest = new WorkflowRequest(
null,
template,
new String[] { "off" },
false,
Collections.emptyMap(),
null,
Collections.emptyMap(),
false
);
WorkflowRequest workflowRequest = new WorkflowRequest(null, template, new String[] { "off" }, false, Collections.emptyMap(), false);

// Bypass checkMaxWorkflows and force onResponse
doAnswer(invocation -> {
Expand Down Expand Up @@ -475,16 +448,7 @@ public void testFailedToCreateNewWorkflowWithNullUser() {

ActionListener<WorkflowResponse> listener = mock(ActionListener.class);

WorkflowRequest workflowRequest = new WorkflowRequest(
null,
template,
new String[] { "off" },
false,
Collections.emptyMap(),
null,
Collections.emptyMap(),
false
);
WorkflowRequest workflowRequest = new WorkflowRequest(null, template, new String[] { "off" }, false, Collections.emptyMap(), false);

createWorkflowTransportAction1.doExecute(mock(Task.class), workflowRequest, listener);
ArgumentCaptor<Exception> exceptionCaptor = ArgumentCaptor.forClass(Exception.class);
Expand Down Expand Up @@ -519,16 +483,7 @@ public void testFailedToCreateNewWorkflowWithNoBackendRoleUser() {

ActionListener<WorkflowResponse> listener = mock(ActionListener.class);

WorkflowRequest workflowRequest = new WorkflowRequest(
null,
template,
new String[] { "off" },
false,
Collections.emptyMap(),
null,
Collections.emptyMap(),
false
);
WorkflowRequest workflowRequest = new WorkflowRequest(null, template, new String[] { "off" }, false, Collections.emptyMap(), false);

createWorkflowTransportAction1.doExecute(mock(Task.class), workflowRequest, listener);
ArgumentCaptor<Exception> exceptionCaptor = ArgumentCaptor.forClass(Exception.class);
Expand All @@ -542,16 +497,7 @@ public void testFailedToCreateNewWorkflowWithNoBackendRoleUser() {
public void testUpdateWorkflowWithReprovision() throws IOException {
@SuppressWarnings("unchecked")
ActionListener<WorkflowResponse> listener = mock(ActionListener.class);
WorkflowRequest workflowRequest = new WorkflowRequest(
"1",
template,
new String[] { "off" },
false,
Collections.emptyMap(),
null,
Collections.emptyMap(),
true
);
WorkflowRequest workflowRequest = new WorkflowRequest("1", template, new String[] { "off" }, false, Collections.emptyMap(), true);

doAnswer(invocation -> {
ActionListener<GetResponse> getListener = invocation.getArgument(1);
Expand Down Expand Up @@ -595,16 +541,7 @@ public void testUpdateWorkflowWithReprovision() throws IOException {
public void testFailedToUpdateWorkflowWithReprovision() throws IOException {
@SuppressWarnings("unchecked")
ActionListener<WorkflowResponse> listener = mock(ActionListener.class);
WorkflowRequest workflowRequest = new WorkflowRequest(
"1",
template,
new String[] { "off" },
false,
Collections.emptyMap(),
null,
Collections.emptyMap(),
true
);
WorkflowRequest workflowRequest = new WorkflowRequest("1", template, new String[] { "off" }, false, Collections.emptyMap(), true);

doAnswer(invocation -> {
ActionListener<GetResponse> getListener = invocation.getArgument(1);
Expand Down Expand Up @@ -904,8 +841,6 @@ public void testCreateWorkflow_withValidation_withProvision_Success() throws Exc
new String[] { "all" },
true,
Collections.emptyMap(),
null,
Collections.emptyMap(),
false
);

Expand Down Expand Up @@ -966,8 +901,6 @@ public void testCreateWorkflow_withValidation_withProvision_FailedProvisioning()
new String[] { "all" },
true,
Collections.emptyMap(),
null,
Collections.emptyMap(),
false
);

Expand Down
63 changes: 1 addition & 62 deletions src/test/java/org/opensearch/flowframework/transport/WorkflowRequestResponseTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,69 +153,10 @@ public void testWorkflowRequestWithParams() throws IOException {
assertEquals("bar", streamInputRequest.getParams().get("foo"));
}

public void testWorkflowRequestWithUseCase() throws IOException {
WorkflowRequest workflowRequest = new WorkflowRequest("123", template, "cohere-embedding_model_deploy", Collections.emptyMap());
assertNotNull(workflowRequest.getWorkflowId());
assertEquals(template, workflowRequest.getTemplate());
assertNull(workflowRequest.validate());
assertFalse(workflowRequest.isProvision());
assertFalse(workflowRequest.isUpdateFields());
assertTrue(workflowRequest.getDefaultParams().isEmpty());
assertEquals(workflowRequest.getUseCase(), "cohere-embedding_model_deploy");

BytesStreamOutput out = new BytesStreamOutput();
workflowRequest.writeTo(out);
BytesStreamInput in = new BytesStreamInput(BytesReference.toBytes(out.bytes()));

WorkflowRequest streamInputRequest = new WorkflowRequest(in);

assertEquals(workflowRequest.getWorkflowId(), streamInputRequest.getWorkflowId());
assertEquals(workflowRequest.getTemplate().toString(), streamInputRequest.getTemplate().toString());
assertNull(streamInputRequest.validate());
assertFalse(streamInputRequest.isProvision());
assertFalse(streamInputRequest.isUpdateFields());
// THESE TESTS FAIL
// assertTrue(streamInputRequest.getDefaultParams().isEmpty());
// assertEquals(streamInputRequest.getUseCase(), "cohere-embedding_model_deploy");
}

public void testWorkflowRequestWithUseCaseAndParamsInBody() throws IOException {
WorkflowRequest workflowRequest = new WorkflowRequest("123", template, "cohere-embedding_model_deploy", Map.of("step", "model"));
assertNotNull(workflowRequest.getWorkflowId());
assertEquals(template, workflowRequest.getTemplate());
assertNull(workflowRequest.validate());
assertFalse(workflowRequest.isProvision());
assertFalse(workflowRequest.isUpdateFields());
assertEquals(workflowRequest.getDefaultParams().get("step"), "model");

BytesStreamOutput out = new BytesStreamOutput();
workflowRequest.writeTo(out);
BytesStreamInput in = new BytesStreamInput(BytesReference.toBytes(out.bytes()));

WorkflowRequest streamInputRequest = new WorkflowRequest(in);

assertEquals(workflowRequest.getWorkflowId(), streamInputRequest.getWorkflowId());
assertEquals(workflowRequest.getTemplate().toString(), streamInputRequest.getTemplate().toString());
assertNull(streamInputRequest.validate());
assertFalse(streamInputRequest.isProvision());
assertFalse(streamInputRequest.isUpdateFields());
// THIS TEST FAILS
// assertEquals(streamInputRequest.getDefaultParams().get("step"), "model");
}

public void testWorkflowRequestWithParamsNoProvision() throws IOException {
IllegalArgumentException ex = assertThrows(
IllegalArgumentException.class,
() -> new WorkflowRequest(
"123",
template,
new String[] { "all" },
false,
Map.of("foo", "bar"),
null,
Collections.emptyMap(),
false
)
() -> new WorkflowRequest("123", template, new String[] { "all" }, false, Map.of("foo", "bar"), false)
);
assertEquals("Params may only be included when provisioning.", ex.getMessage());
}
Expand All @@ -227,8 +168,6 @@ public void testWorkflowRequestWithOnlyUpdateParamNoProvision() throws IOExcepti
new String[] { "all" },
true,
Map.of(UPDATE_WORKFLOW_FIELDS, "true"),
null,
Collections.emptyMap(),
false
);
assertNotNull(workflowRequest.getWorkflowId());
Expand Down
Loading