fix: Check inputs type before doing truncation #1299
Conversation
| @@ -492,7 +496,7 @@ describe('utils', function() { | |||
| b: 'asd', | |||
| c: true, | |||
| d: undefined, | |||
| e: 'very long string that is definitely ove\u2026', | |||
| e: 'very long string that is definitely over\u2026', | |||
There was a problem hiding this comment.
do you care about this off by one?
There was a problem hiding this comment.
No, I didn't realize implementation of truncate that we already had didnt count ellipsis as the character so I just unified it.
| if (typeof value === 'string') { | ||
| return value.length <= maxLength ? value : value.substr(0, maxLength - 1) + '\u2026'; | ||
| var maxLength = 40; |
There was a problem hiding this comment.
was this change just tangential, or is it part of the fix?
There was a problem hiding this comment.
(thinking it was just a tangential fix)
src/utils.js
Outdated
| @@ -160,7 +160,8 @@ function objectFrozen(obj) { | |||
| } | |||
|
|
|||
| function truncate(str, max) { | |||
| return !max || str.length <= max ? str : str.substr(0, max) + '\u2026'; | |||
| if (typeof str !== 'string' || typeof max !== 'number' || max === 0) return str; | |||
There was a problem hiding this comment.
max should never be dynamically resolved - it's taken off of global options. Maybe it could be better to just error here like you were instead of silently not truncating.
Also - the places that truncate gets used, we probably don't want to return anything besides a string - right? If we were to pass through exception.value as an object, does the rest of the code even accept that? maybe it's better to return undefined or an empty string?
we could also coerce str to a string if it's not.
There was a problem hiding this comment.
Agree, I'll error out if max is not a number.
As for the second question, I think leaving it as is should be fine. Reason being:
a) we won't blow up when we send something else as instead of a string value
b) it'll be captured in the breadcrumb as [object Object] which may indicate that there's something funky going on and that could help developers find the culprit
c) in raw json event that we have available in Sentry, it should contain all the object attributes, which is basically everything someone might need to find out what was the error's value (in case it's some strange object like, in this case, jQuery Error - #1275)
wdyt?
There was a problem hiding this comment.
ah, that sounds good to me then! thanks for explaining
|
approved pending the ‘max’ thing 👍 |
kamilogorek
force-pushed
the
truncate-guard
branch
from
8eb213b to
cee77ce
Compare
kamilogorek
force-pushed
the
truncate-guard
branch
from
cee77ce to
23bdd24
Compare
Fixes #1275