fix(builders): add proper snowflake validation #11290
Conversation
rie03p
requested review from
Jiralite,
didinele,
iCrawl,
kyranet and
vladfrangu
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Jiralite
left a comment
Jiralite
left a comment
There was a problem hiding this comment.
I believe this should accept a snowflake too:
|
@rie03p looks like something is not quite right because tests are failing, may need to update them? |
|
@Jiralite You're right! The validation is now working correctly and rejecting invalid IDs like 'hi!' and '0'. I've updated the existing tests to use valid IDs (numbers or proper snowflakes) so they pass with the new validation. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11290 +/- ##
==========================================
+ Coverage 44.97% 44.99% +0.01%
==========================================
Files 317 317
Lines 18222 18224 +2
Branches 1817 1817
==========================================
+ Hits 8196 8199 +3
+ Misses 10013 10012 -1
Partials 13 13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: Almeida <[email protected]>
|
I’ve fixed the tests that were failing. |
|
|
||
| export const idPredicate = z.int().min(0).max(2_147_483_647).optional(); | ||
| export const customIdPredicate = z.string().min(1).max(100); | ||
| export const snowflakePredicate = z.string().regex(/^[1-9]\d{16,18}$/); |
There was a problem hiding this comment.
| export const snowflakePredicate = z.string().regex(/^[1-9]\d{16,18}$/); | |
| export const snowflakePredicate = z.string().regex(/^[0-9]\d{16,19}$/); |
There was a problem hiding this comment.
0 is perfectly valid at the start?
Also, disagree on increasing the limit. The API doesn't even accept snowflakes that large...
There was a problem hiding this comment.
0 is perfectly valid at the start?
I do not like that we have that at the start, I completely missed that its two parts... But also why not? technically even 0 is a valid snowflake (and can be used in pagination to use as a starting point)
There was a problem hiding this comment.
This is builders though. Snowflakes here are not used for pagination. They're used for emoji ids, select menus (role ids, channel ids, and user ids)... I'd argue someone is building something not at the start of Discord's time, so realistically, 0 is invalid.
If in the future, we ever make builders for methods that use pagination, we can revisit this?
There was a problem hiding this comment.
🤷, my two cents is that this should've been \d{17,20} and not overly complex but w/e (plus for attachments it can literally be just \d+ when creating)...
And before you argue for attachments that it cannot be \d+ bc the spec says so -> API cares about the limit, not the ID last I checked
There was a problem hiding this comment.
This PR alters that too, soo maybe that needs to be undone? Also you miss the point, the string "0" will work too
There was a problem hiding this comment.
It alters it because we're being strict on input: snowflakes for editing and numbers for uploading, which is the documented way.
Also you miss the point, the string "0" will work too
0 actually gets set to undefined:
Is... is that an issue?
Someone shouldn't be passing "0" though, right? That seems like a mistake...
There was a problem hiding this comment.
Someone shouldn't be passing "0" though, right? That seems like a mistake...
yup. it should be 0 (number)
IMO the meaning of the ID is either snowflake (as in, literal, actual snowflake when editing, already uploaded attachment) or index, as in "im uploading this now, its the nth file"
re the code, yes. that's a bad falsy check
There was a problem hiding this comment.
I'm down to change this regex to the one from the spec.
5 participants
close #11289
Please describe the changes this PR makes and why it should be merged:
This PR adds proper Snowflake validation to the builders package. Previously, multiple instances were using plain
z.string()for Snowflake IDs, which didn't validate that the values are numeric strings as required by Discord's Snowflake format.Changes:
snowflakePredicate(z.string().regex(/^\d{17,20}$/)) to validate Snowflake formatz.string()withsnowflakePredicatein:emojiPredicate.idbuttonPremiumPredicate.sku_iddefault_values.idfor channel, mentionable, role, and user select menu predicatesStatus and versioning classification: