flow-framework icon indicating copy to clipboard operation
flow-framework copied to clipboard

[BUG] WorkflowRequest doesn't serialize the use case and default parameters over transport

Open dbwiddis opened this issue 1 year ago • 1 comments

What is the bug?

The WorkflowRequest class doesn't serialize the use case and default parameters over transport.

The writeTo() only writes the provision parameter and params map: https://github.com/opensearch-project/flow-framework/blob/cf1016fd72e0f9196db498cbb52f4a57bafadade/src/main/java/org/opensearch/flowframework/transport/WorkflowRequest.java#L197-L206

Similarly, the constructor using StreamInput doesn't read them https://github.com/opensearch-project/flow-framework/blob/cf1016fd72e0f9196db498cbb52f4a57bafadade/src/main/java/org/opensearch/flowframework/transport/WorkflowRequest.java#L128-L136

This was missed in unit testing because the tests incorrectly checked the original workflowRequest and not the streamInputRequest after passing through transport. (Note the last 4 lines.) https://github.com/opensearch-project/flow-framework/blob/cf1016fd72e0f9196db498cbb52f4a57bafadade/src/test/java/org/opensearch/flowframework/transport/WorkflowRequestResponseTests.java#L155-L166

How can one reproduce the bug?

Fix the unit test to properly test the workflow created from transport. Change lines 163-166 above to:

        assertNull(streamInputRequest.validate());
        assertFalse(streamInputRequest.isProvision());
        // FAILS, `getDefaultParams()` is null
        assertTrue(streamInputRequest.getDefaultParams().isEmpty()); 
        // FAILS, `getUseCase()` is null
        assertEquals(streamInputRequest.getUseCase(), "cohere-embedding_model_deploy");

What is the expected behavior?

If these fields are required to be part of the WorkflowRequest they should be properly serialized. Note that adding these fields would break compatibility across versions and would require version-checking for the additional fields.

However, these fields are never accessed in the code, other than in the (failing) tests. If they are not needed, they should be removed.

Do you have any additional context?

The fields were added in #583.

The tests have been fixed in #757, with the failing tests commented out until this issue is resolved.

dbwiddis avatar Jul 04 '24 17:07 dbwiddis

This can be addressed as part of #823

dbwiddis avatar Aug 07 '24 05:08 dbwiddis