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

[FEATURE] Improve robustness of WorkflowNode parsing of User Inputs #650

Open
dbwiddis opened this issue Apr 5, 2024 · 0 comments
Open
Labels
backlog Good to have functionality not critical for next release enhancement New feature or request

Comments

@dbwiddis
Copy link
Member

dbwiddis commented Apr 5, 2024

Is your feature request related to a problem?

Parsing of Workflow Nodes (steps) assumes that JSON objects are key-value maps unless specifically coded otherwise:

case START_OBJECT:
if (GUARDRAILS_FIELD.equals(inputFieldName)) {
userInputs.put(inputFieldName, Guardrails.parse(parser));
break;
} else if (CONFIGURATIONS.equals(inputFieldName)) {
Map<String, Object> configurationsMap = parser.map();
try {
String configurationsString = ParseUtils.parseArbitraryStringToObjectMapToString(configurationsMap);
userInputs.put(inputFieldName, configurationsString);
} catch (Exception ex) {
String errorMessage = "Failed to parse" + inputFieldName + "map";
logger.error(errorMessage, ex);
throw new FlowFrameworkException(errorMessage, RestStatus.BAD_REQUEST);
}
break;
} else {
userInputs.put(inputFieldName, parseStringToStringMap(parser));
}

This is brittle and can lead to failure if a new feature/API is added which includes a more complex nested object. As more workflow steps are created, the chances of more custom objects increases.

What solution would you like?

Add a processing layer in between encountering the start of an object and reading the map:

  • Start building a new object with a new XContent Builder
  • Process tokens, copying them over into the new builder until the corresponding END_OBJECT is reached
  • Build the object
  • Send the entire (new) XContent object to the existing map parsing method
  • Catch a parsing exception creating the map; on exception, simply place a String value (containing the JSON form of the object) into the map for that key. Alternately, just ignore that value (or place null in the map) since in any case, if the step doesn't include that as a required or optional input, it's not even going to be evaluated. In either case, log the parsing exception.

What alternatives have you considered?

We can also try to parse multiple layers of maps, but would need to protect against arbitrary levels of recursion. This is essentially what's done by other JSON parsers such as GSON, but we decided early on to use XContent parsing.

Do you have any additional context?

In addition to robustness in parsing, we need to set up a way to test against all implemented APIs (steps) using their specifications, to catch API changes early.

@dbwiddis dbwiddis added enhancement New feature or request untriaged v2.14.0 and removed untriaged labels Apr 5, 2024
@joshpalis joshpalis added v2.15.0 and removed v2.14.0 labels Apr 8, 2024
@dbwiddis dbwiddis added backlog Good to have functionality not critical for next release and removed v2.15.0 labels May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Good to have functionality not critical for next release enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants