sql: audit json.AsText for nil return value#81927
sql: audit json.AsText for nil return value#81927craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
|
We ended up reverting the changes from the first half of the collab session - some more context is in here. |
mgartner
left a comment
There was a problem hiding this comment.
Bummer that you had to undo all the work! Thanks for auditing this.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @linaqiu22, @wenyihu6, and @yuzefovich)
pkg/util/json/json.go line 2063 at r1 (raw file):
return nil, false, err } if t == nil || *t != s {
t should never be nil here because we check that it's a StringJSONType above. Maybe an assertion failure is less confusing?
This commit audits all calls of `json.AsText` to make sure that we always do nil-pointer check. I think all of the callsites where this commit adds the check are impossible to hit the nil-pointer panic, but it's still good to have the checks explicitly. Release note: None
yuzefovich
left a comment
There was a problem hiding this comment.
TFTR!
bors r+
Reviewable status:
pkg/util/json/json.go line 2063 at r1 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
t should never be
nilhere because we check that it's a StringJSONType above. Maybe an assertion failure is less confusing?
Good point, done.
|
Build succeeded: |
This commit audits all calls of
json.AsTextto make sure that wealways do nil-pointer check. I think all of the callsites where this
commit adds the check are impossible to hit the nil-pointer panic, but
it's still good to have the checks explicitly.
Informs: #81097.
Release note: None