From 657ca4949d893f6d6a38625aca7304d168ef8324 Mon Sep 17 00:00:00 2001 From: Pierre Delagrave Date: Tue, 21 Jan 2020 14:04:55 -0500 Subject: [PATCH] fix(core): Remove useless entries from `Stage.context` (#3359) 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. https://github.com/FasterXML/jackson-databind/issues/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> --- .../spinnaker/orca/bakery/pipeline/BakeStageSpec.groovy | 2 +- .../java/com/netflix/spinnaker/orca/pipeline/model/Stage.java | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/orca-bakery/src/test/groovy/com/netflix/spinnaker/orca/bakery/pipeline/BakeStageSpec.groovy b/orca-bakery/src/test/groovy/com/netflix/spinnaker/orca/bakery/pipeline/BakeStageSpec.groovy index 6f2e0f03068..a15ca968fd8 100644 --- a/orca-bakery/src/test/groovy/com/netflix/spinnaker/orca/bakery/pipeline/BakeStageSpec.groovy +++ b/orca-bakery/src/test/groovy/com/netflix/spinnaker/orca/bakery/pipeline/BakeStageSpec.groovy @@ -218,7 +218,7 @@ class BakeStageSpec extends Specification { private static List expectedContexts(String cloudProvider, String amiSuffix, String... regions) { return regions.collect { - [cloudProviderType: cloudProvider, amiSuffix: amiSuffix, type: BakeStage.PIPELINE_CONFIG_TYPE, "region": it, name: "Bake in ${it}", refId: "1"] + [cloudProviderType: cloudProvider, amiSuffix: amiSuffix, type: BakeStage.PIPELINE_CONFIG_TYPE, "region": it, name: "Bake in ${it}"] } } } diff --git a/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/model/Stage.java b/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/model/Stage.java index 6b6dbab1074..1dfc23e67db 100644 --- a/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/model/Stage.java +++ b/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/model/Stage.java @@ -104,7 +104,6 @@ public Stage(Execution execution, String type, String name, Map this.execution = execution; this.type = type; this.name = name; - this.context.putAll(context); this.refId = (String) context.remove("refId"); this.startTimeExpiry = @@ -114,6 +113,8 @@ public Stage(Execution execution, String type, String name, Map this.requisiteStageRefIds = Optional.ofNullable((Collection) context.remove("requisiteStageRefIds")) .orElse(emptySet()); + + this.context.putAll(context); } public Stage(Execution execution, String type, Map context) {