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] Simplify WorkflowData to a concrete class #53

Closed
dbwiddis opened this issue Sep 25, 2023 · 0 comments · Fixed by #54
Closed

[FEATURE] Simplify WorkflowData to a concrete class #53

dbwiddis opened this issue Sep 25, 2023 · 0 comments · Fixed by #54
Assignees
Labels
enhancement New feature or request

Comments

@dbwiddis
Copy link
Member

Is your feature request related to a problem?

WorkflowData was created as an interface, originally envisioning multiple types of data. However, during development we realized it was far easier to just pass around the same single class. Because it's an interface, this requires anonymous subclassing.

Creating an anonymous subclass returning values generated at runtime can be dangerous. One can spend several hours trying to debug why the following sequence hangs the JVM:

final String s = inputIndex;
WorkflowData wfd = new WorkflowData() {
    @Override
    public Map<String, Object> getContent() {
        return Map.of("index", s);
    }
};
wfd.getContent(); // hangs creating Map.of if s was null

Using Map.of() with a null key or value causes a NullPointerException which stopped processing without any information on why.

What solution would you like?

Just create a regular class and pass the params and content in as arguments to the constructors. We can even overload the constructors to allow both, or just params, to default to an empty map.

What alternatives have you considered?

Some sort of wrapper class around actual Request and Response objects in OpenSearch. This is overly complicated for our purpose.

Do you have any additional context?

I'd add this to my #47 but the changes also need to be made in #38, #44, and #52. I suggest the PRs that are ready to merge get merged, we fix this, and future development can conform to the new signature before we do any more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant