Remove panics in enum mappers #7258
Open
+1,800
−137
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?
Removes
panic()
from enum mappers in types/mapper/...Why?
It is idiomatic in go to return an error value when something expected goes wrong.
In general panics should be reserved for situations which are unrecoverable for the entire process, and killing the process and restarting it is the expected behavior.
Even in these scenarios, libraries or portions of the call stack that are not at the request level should favor returning errors to the caller and allowing the caller to decide whether or not the runtime should panic - deferring the decision to the last moment (e.g a panic/recover handler at the request level.
How did you test it?
There are three scenarios for each of our mapping functions:
nil
orPROTO_ENUM_INVALID
)With the existing mapping round trip tests scenarios 1 and 2 were tested, but scenario 3 was not.
This PR adds tests for the ToXXX and FromXXX mappers as well to ensure that there aren't strange behaviors that are covered up by the full round trip.
Potential risks
This could be a breaking change to our API for some users. If, for example, a consumer is passing an invalid enum value to a specific RPC and then relying on an Internal_Server_Error with a specific panic message to do some business logic, they will unfortunately not be happy.
Release notes
N/A
Documentation Changes
N/A