Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Force serialization always for convertValue(); avoid short-cuts #2220

Closed
cowtowncoder opened this issue Jan 11, 2019 · 1 comment
Closed
Milestone

Comments

@cowtowncoder
Copy link
Member

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.

@cowtowncoder cowtowncoder added 3.x Issues to be only tackled for Jackson 3.x, not 2.x ACTIVE labels Jan 11, 2019
@cowtowncoder cowtowncoder added 2.10 and removed 3.x Issues to be only tackled for Jackson 3.x, not 2.x ACTIVE labels Jan 26, 2019
@cowtowncoder
Copy link
Member Author

Actually. No, I think it will make sense to do this in 2.10.

@cowtowncoder cowtowncoder added this to the 2.10.0 milestone Jan 26, 2019
cowtowncoder added a commit that referenced this issue Jan 26, 2019
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.
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
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant