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

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

Merged
Merged
Show file tree
Hide file tree
Changes from all 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 @@ -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
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
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
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
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