Force serialization always for convertValue(); avoid short-cuts
#2220
Milestone
Comments
cowtowncoder
added
3.x
cowtowncoder
added
2.10
and removed
3.x
|
Actually. No, I think it will make sense to do this in 2.10. |
pdelagrave
pushed a commit
to pdelagrave/orca
that referenced
this issue
Jan 10, 2020
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.
cowtowncoder
added a commit
that referenced
this issue
Jan 10, 2020
mergify bot
added a commit
to spinnaker/orca
that referenced
this issue
Jan 21, 2020
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>
KathrynLewis
pushed a commit
to KathrynLewis/orca
that referenced
this issue
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
Various "optimizations" for
ObjectMapper.convertValue()-- that is, short-circuit code that tries to figure out if it is possible to omit write+read handling altogether and simply return input value -- have caused much confusion, and even breakage for legit use cases (even if most cases have been resolved by more heuristics).But it seems that best course would be to simplify things and simply always do write+read handling and let caller handle optimizations if ones are desired.
For backwards compatibility reasons let's do this for 3.0 however.
The text was updated successfully, but these errors were encountered: