[BUG] WorkflowStepFactory sets static fields in constructor

[dbwiddis]

What is the bug?

The WorkflowStepFactory class defines static fields needed by some of the steps:

flow-framework/src/main/java/org/opensearch/flowframework/workflow/WorkflowStepFactory.java

Lines 64 to 67 in be0df19

	private static ThreadPool threadPool;
	private static MachineLearningNodeClient mlClient;
	private static FlowFrameworkIndicesHandler flowFrameworkIndicesHandler;
	private static FlowFrameworkSettings flowFrameworkSettings;

These are initialized in the (non-static) constructor:

flow-framework/src/main/java/org/opensearch/flowframework/workflow/WorkflowStepFactory.java

Lines 87 to 90 in be0df19

	this.threadPool = threadPool;
	this.mlClient = mlClient;
	this.flowFrameworkIndicesHandler = flowFrameworkIndicesHandler;
	this.flowFrameworkSettings = flowFrameworkSettings;

This is considered a code smell

How can one reproduce the bug?

1. Initialize the WorkflowStepFactory with values for the parameters.
2. Initialize it a second time with different parameters
3. Observe that the fields used in the second initialization are used in steps generated by the first initialization.

While we only initialize this class once in the main source tree, we iniitalize it separately in two different unit tests, using different thread pools (which are terminated at the end of the test):
https://github.com/search?q=repo%3Aopensearch-project%2Fflow-framework+%22new+workflowstepfactory%22&type=code

This could lead to flaky tests if the second test executed overwrites the first test's thread pool, terminates, and the first test still tries to access the thread pool to construct a workflow step, or if the defined mock have differing behavior.

What is the expected behavior?

We should not use user-settable static fields.

Do you have any additional context?

The issue was introduced in #523 as a result of trying to include the workflow step constructors inside the enum as I suggested here:

If you want to keep it in this class, we can add aSupplier<WorkflowStep> field to each enum value (which would be a good use of the workflowStep name you've presently given to a String...) and get rid of the map.

Originally posted by @dbwiddis in #523 (comment)

I clearly didn't foresee the complexity of the suppliers requiring these parameters.

This is not trivial to fix:

• Removing the static modifier makes the instance variable inaccessible from the enum
• Moving the static fields into the enum is not directly possible because enums must declare their fields first, and the fields are thus defined prior to the variables being initialized
  • It's possible to wrap these inside a statically defined inner class but this just adds complexity and doesn't solve the issue of initializing twice in possibly different threads/classes.
• We could just make the whole class static and initialize the fields to obfuscate the code smell, but this doesn't solve the issue of initializing twice in possibly different threads/classes.

Proposed solutions:

1. Pull the step supplier initialization back out of the enum.
2. Replace the Supplier factory method with a FunctionalInterface that takes in the 4 arguments. This would require every workflow step to take these arguments in its constructor even if unused.
3. Change the WorkflowStep interface to require a factory method and do not initialize normally. The interface method would take the args listed in option 2.
4. Use injection to provide the values of the parameters to the workflow steps. This requires binding all the workflow steps to the injector in createComponents(). Note that the injection we're used to uses singletons so we'd need to inject a FactoryProvider.