Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

lib/helpers: fixed is_valid_packet() #382

Merged
merged 1 commit into from Feb 11, 2014

Conversation

Projects
None yet
3 participants
Contributor

SaveTheRbtz commented Dec 31, 2013

This commit introduces following changes:

  • Be liberal in what you recieve - do not rely on regexps for packet
    parsing but instead accept anything that can be parsed by JavaScript
    as a number e.g: +1e-17, -0511, 0xDEADbeef, etc
  • Accept both positive and negative counters with explicit / implicit
    sign
  • Provides more strict error checking then regexp, for example
    following strings match previously used '([-+\d.]+' regexp:
    .
    .123.
    .-+\0+-.1-

Also while here added more tests.

Closes: #350 #357 #363

Signed-off-by: Alexey Ivanov SaveTheRbtz@GMail.com

lib/helpers: fixed is_valid_packet()
This commit introduces following changes:
 * Be liberal in what you recieve - do not rely on regexps for packet
   parsing but instead accept anything that can be parsed by JavaScript
   as a number e.g: +1e-17, -0511, 0xDEADbeef, etc
 * Accept both positive and negative counters with explicit / implicit
   sign
 * Provides more strict error checking then regexp, for example
   following strings match previously used '([\-\+\d\.]+' regexp:
   .
   .123.
   .\-+\0\+-.1-

Also while here added more tests.

Closes: #350 #357

Signed-off-by: Alexey Ivanov <SaveTheRbtz@GMail.com>
Owner

mrtazz commented Feb 11, 2014

Looks good, thanks for submitting this!

mrtazz added a commit that referenced this pull request Feb 11, 2014

Merge pull request #382 from SaveTheRbtz/helpers
lib/helpers: fixed is_valid_packet()

@mrtazz mrtazz merged commit 5b90071 into etsy:master Feb 11, 2014

1 check passed

default The Travis CI build passed
Details

@jeffyip jeffyip commented on the diff Feb 19, 2014

lib/helpers.js
- // filter out malformed sample rates
- else if (fields[2] && !fields[2].match(/^@([\d\.]+$)/)) {
- return false;
- }
- // looks like we're good
- else {
- return true;
+
+ // filter out invalid metrics values
+ switch(fields[1]) {
+ case 's':
+ return true;
+ case 'g':
+ return isNumber(fields[0]);
+ case 'ms':
+ return isNumber(fields[0]) && Number(fields[0]) > 0;
@jeffyip

jeffyip Feb 19, 2014

Should this be Number(fields[0]) >= 0?

@SaveTheRbtz

SaveTheRbtz Feb 20, 2014

Contributor

Indeed it should, if you have time you can patch it and add test for that particular case. If not, I'll probably do it myself within a week or so.

SaveTheRbtz pushed a commit to SaveTheRbtz/statsd that referenced this pull request Feb 23, 2014

SaveTheRbtz pushed a commit to SaveTheRbtz/statsd that referenced this pull request Feb 23, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment