Fix mapping #6531
Merged
Fix mapping #6531
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
|
whoops, got confused and accidentally pushed to the wrong fork, reopening |
davidporter-id-au
requested review from
Shaddoll,
neil-xie,
Groxx,
shijiesheng,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
| if input == nil { | ||
| panic("version history item is null") | ||
| return nil |
There was a problem hiding this comment.
are callers assuming this function will always return non-nil or panic? If so we should go over them and handle nil case gracefully (potentially by returning a proper error) to avoid nil pointer exceptions downstream
There was a problem hiding this comment.
I tried to do that in the previous PR, though it's certainly possible I missed a few locations. The value I expect to be nil on the first request and subsequently used to keep track of the workflow state.
this case (CDNC-11702) I missed
taylanisikdemir
approved these changes
Dec 3, 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.
What changed?
Fixes (hopefully) some mapping bugs I introduced with #6523 due to the version history being passed in being nil sometimes, therefore causing panics.
The mapping code as it's currently written panics (and is changed by this PR) which I think is the correct architectural choice. The mappers, by panicing as they presently are, are confusing validation with mapping. This corrects the mapper to gracefully handle nil input, and a quick survey appears to show that there are no expected areas which should never be nil that don't already have nil-check validation in place.
Specifically, in the case of
cadence/service/frontend/api/handler.go
Line 1954 in 5f579dd
Why?
How did you test it?
Potential risks
Release notes
Documentation Changes