Skip to content

Conversation

@sc-a-zhukov
Copy link
Contributor

Fix FORK_JOIN_DYNAMIC input wrapping when fed directly by JSON_JQ_TRANSFORM (#575)

———

Pull Request type

  • Bugfix
  • Feature
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • WHOSUSING.md
  • Other (please describe):

Changes in this PR

  • Normalized JSON_JQ_TRANSFORM outputs so both result and resultList are plain Java maps/lists. In reproduction workflow fork_join_dynamic now receives inputs like
{
	"commonProperty": "common value",
	"property": "value1",
	"__index": 0
}
image

instead of

{
	"input": {
		"commonProperty": "common value",
		"property": "value1"
	},
	"__index": 0
}

in the version before changes
image

Also this behaviour matching with Orkes Cloud behaviour.

  • tests:
    - JsonJqTransformTest checks resultList equals a list of standard maps.
    - ForkJoinDynamicTaskMapperTest asserts dynamic fork inputs remain unwrapped and keep their parameters.
    - Verified manually on the workflow from issue FORK_JOIN_DYNAMIC adds unexpected input nesting when using JSON_JQ_TRANSFORM output #575. Before fixing the bug, it was reproduced only when JSON_JQ_TRANSFORM went immediately before FORK_JOIN_DYNAMIC; inserting an asynchronous task (WAIT/SIMPLE/...) between them serialized the data and hid the error.

the screenshot from the version before fix with WAIT task
image

The fix removes this dependency and the need for such workarounds

@sc-a-zhukov
Copy link
Contributor Author

It seems that the tests are failing, which are not related to my changes.
https://github.com/conductor-oss/conductor/actions/runs/18373928280/job/52467688832?pr=599#step:7:828

59 tests completed, 1 failed
> Task :conductor-rest:test FAILED

Gradle Test Executor 26 > TaskResourceTest > testGetTaskLogs FAILED
    java.lang.AssertionError: expected:<[com.netflix.conductor.common.metadata.tasks.TaskExecLog@dea1c1ab]> but was:<<200 OK OK,[com.netflix.conductor.common.metadata.tasks.TaskExecLog@dea1c1ab],[]>>
        at org.junit.Assert.fail(Assert.java:89)
        at org.junit.Assert.failNotEquals(Assert.java:835)
        at org.junit.Assert.assertEquals(Assert.java:120)
        at org.junit.Assert.assertEquals(Assert.java:146)
        at com.netflix.conductor.rest.controllers.TaskResourceTest.testGetTaskLogs(TaskResourceTest.java:110)


Deprecated Gradle features were used in this build, making it incompatible with Gradle 9.0.
FAILURE: Build failed with an exception.

If you look at the test code, it is not very clear how it ever worked, because the assertEquals expected and actual results have at least different data types.

image

@nthmost-orkes
Copy link
Contributor

It seems that the tests are failing, which are not related to my changes.

Yep, @sc-a-zhukov i am working on smoothing out build failures as we speak. Thanks for being patient with us!

@nthmost-orkes
Copy link
Contributor

@sc-a-zhukov Can you resolve the merge conflict that's arisen since you initially committed this PR? It's just a need to sync up to main since a bugfix went in in this file.

@sc-a-zhukov
Copy link
Contributor Author

@nthmost-orkes Yes, of course.

@sc-a-zhukov
Copy link
Contributor Author

@nthmost-orkes Do I need to squash commits, rebase commits on top of the latest changes, or something like that?

@nthmost-orkes
Copy link
Contributor

@nthmost-orkes Do I need to squash commits, rebase commits on top of the latest changes, or something like that?

Great question, let's go with rebase for this one, thanks!

Copy link
Contributor

@nthmost-orkes nthmost-orkes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @sc-a-zhukov , good to go!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants