fix(core): Remove useless entries from Stage.context
#3359
Merged
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
Fixes spinnaker/spinnaker#5257 The `Stage` constructor was keeping `refId`, `startTimeExpiry` and `requisiteStageRefIds` from the context map argument in its internal context map. This caused issues when a parent stage's context was used to create a child stage, as these entries aren't expected to be inherited. This wasn't a problem for a long time because these values were accidentally removed by the side effect of `StartStageHandler#shouldSkip`. `shouldSkip()` uses `objectMapper.convertValue()` on a stage context expecting to have a cloned map returned. A long standing design/documentation issue with Jackson made `convertValue()` return the input parameter as is instead of converting/cloning it. This made every `Stage` passing through `StartStageHandler.handle()` having their context map indirectly 'corrected' and persisted. An upcoming upgrade to Spring Boot 2.2 comes with Jackson 2.10 where `objectMapper.convertValue()` has a new behaviour and would make `shouldSkip()` fail to silently 'correct' the stages context map. FasterXML/jackson-databind#2220 `BakeStageSpec` was validating `context['refId']` on the generated contexts but the contexts aren't Stages yet; once a context is made into a Stage and added to the graph (`graph.add`) it will have its `refId` generated based on the parent refId; which isn't under test in that spec.
| @@ -114,6 +113,8 @@ public Stage(Execution execution, String type, String name, Map<String, Object> | |||
| this.requisiteStageRefIds = | |||
| Optional.ofNullable((Collection<String>) context.remove("requisiteStageRefIds")) | |||
| .orElse(emptySet()); | |||
|
|
|||
There was a problem hiding this comment.
this whole function seems suspect to me (i've looked into this before but ran into some issues trying to fix it - don't recall what they were)
the context this function receives as the param is passed by ref so we actually modify the context from the caller which is a bit messed up.
but AFAIR there were no calls to this that mattered so i sort of put it aside...
anyway, the change LGTM
There was a problem hiding this comment.
Since it is passed by reference, shouldn't a stage copy the context? Would we need to do a deep copy or would a copy of the map only be sufficient?
robzienert
approved these changes
Jan 21, 2020
KathrynLewis
pushed a commit
to KathrynLewis/orca
that referenced
this pull request
Jan 31, 2021
Fixes spinnaker/spinnaker#5257 The `Stage` constructor was keeping `refId`, `startTimeExpiry` and `requisiteStageRefIds` from the context map argument in its internal context map. This caused issues when a parent stage's context was used to create a child stage, as these entries aren't expected to be inherited. This wasn't a problem for a long time because these values were accidentally removed by the side effect of `StartStageHandler#shouldSkip`. `shouldSkip()` uses `objectMapper.convertValue()` on a stage context expecting to have a cloned map returned. A long standing design/documentation issue with Jackson made `convertValue()` return the input parameter as is instead of converting/cloning it. This made every `Stage` passing through `StartStageHandler.handle()` having their context map indirectly 'corrected' and persisted. An upcoming upgrade to Spring Boot 2.2 comes with Jackson 2.10 where `objectMapper.convertValue()` has a new behaviour and would make `shouldSkip()` fail to silently 'correct' the stages context map. FasterXML/jackson-databind#2220 `BakeStageSpec` was validating `context['refId']` on the generated contexts but the contexts aren't Stages yet; once a context is made into a Stage and added to the graph (`graph.add`) it will have its `refId` generated based on the parent refId; which isn't under test in that spec. Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
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.
Fixes spinnaker/spinnaker#5257
The
Stageconstructor was keepingrefId,startTimeExpiryandrequisiteStageRefIdsfrom the context map argument in its internal context map. This caused issues when a parent stage's context was used to create a child stage, as these entries aren't expected to be inherited. This wasn't a problem for a long time because these values were accidentally removed by the side effect ofStartStageHandler#shouldSkip.shouldSkip()usesobjectMapper.convertValue()on a stage context expecting to have a cloned map returned. A long standing design/documentation issue with Jackson madeconvertValue()return the input parameter as is instead of converting/cloning it. This made everyStagepassing throughStartStageHandler.handle()having their context map indirectly 'corrected' and persisted.An upcoming upgrade to Spring Boot 2.2 comes with Jackson 2.10 where
objectMapper.convertValue()has a new behaviour and would makeshouldSkip()fail to silently 'correct' the stages context map.FasterXML/jackson-databind#2220
BakeStageSpecwas validatingcontext['refId']on the generated contexts but the contexts aren't Stages yet; once a context is made into a Stage and added to the graph (graph.add) it will have itsrefIdgenerated based on the parent refId; which isn't under test in that spec.