Use lists instead of arrays for FlowNode actions to make deserialization more robust
#263
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dwnusbaum
commented
Apr 29, 2024
There was a problem hiding this comment.
Probably various files here could be deleted, and in general I prefer to use zip files to avoid cluttering GitHub search results, but for now at least having the bare files as test resources makes it easy to see what is being tested.
Comment on lines
86
to
90
| var b = p.getLastBuild(); | ||
| var stageBodyStartNode = (StepStartNode) b.getExecution().getNode("4"); | ||
| assertThat(stageBodyStartNode, not(nullValue())); | ||
| var label = stageBodyStartNode.getPersistentAction(LabelAction.class); | ||
| assertThat(label.getDisplayName(), equalTo("test")); |
There was a problem hiding this comment.
Without the patch, the whole Pipeline itself fails to load because the active head refers to a node with the action from the class that is no longer present.
dwnusbaum
commented
Apr 29, 2024
src/main/java/org/jenkinsci/plugins/workflow/support/storage/BulkFlowNodeStorage.java
Outdated
Show resolved
Hide resolved
jglick
approved these changes
Apr 29, 2024
Dohbedoh
approved these changes
Apr 30, 2024
dwnusbaum
commented
Apr 30, 2024
src/test/java/org/jenkinsci/plugins/workflow/test/steps/SemaphoreStep.java
Outdated
Show resolved
Hide resolved
jtnord
approved these changes
May 1, 2024
jglick
approved these changes
May 1, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I did not realize this, but if you try to deserialize a Pipeline that has a
FlowNodewith an action from a plugin that is disabled or no longer installed, theFlowNodeitself will fail to be deserialized, leaving the flow graph corrupted. It seems like it would be preferential to haveRobustCollectionConverterwork forFlowNodeactions, so that only the problematic action itself fails to be deserialized, but theFlowNodeitself and all other actions are preserved.The reason for the current behavior is that right now, the fields used to serialize
FlowNodeactions are arrays, and core'sRobustCollectionConverterhas no equivalent for arrays (fair enough I guess).I created some tests to demonstrate the original behavior, but then I realized that the serial format of lists and arrays as far as XStream is concerned actually seems to be interchangeable (maybe not true in general?), so I figured I might as well file a patch instead of just tests. Besides potential concerns with backwards compatibility, I am not sure if there is any specific reason that arrays were used originally. Maybe to reduce memory usage or something like that?
For now I'm just filing as a draft because I'm not sure if this is safe or if there were any particular reasons we were using arrays previously.
Testing done
Only tested briefly against this plugin using the new and existing automated tests.
Submitter checklist