Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correctly handle objects with circular reference(s) #51

Merged

Conversation

nick-penaranda-ck
Copy link
Contributor

Issue

When passed an object with a circular reference, Flatten would infinitely loop until memory exhaustion/stack overflow.

Fix

Flatten will detect circular references and bail out. We incur some memory overhead to maintain a stack of "seen" objects.

@barry-dutton-ck
Copy link
Contributor

Can you paste the results of running the tests?

@nick-penaranda-ck
Copy link
Contributor Author

npm run test output:

> @creditkarma/good-influx@4.0.1 test /src/good-influx
> lab -m 5000 -t 94 -v -La code

Formatters - Int
  ✔ 1) formats an integer (188 ms and 1 assertions)
  ✔ 2) formats a float (1 ms and 1 assertions)
  ✔ 3) formats a string (0 ms and 1 assertions)
  ✔ 4) does not format a non-number (0 ms and 1 assertions)
Formatters - String
  ✔ 5) formats a basic string (0 ms and 1 assertions)
  ✔ 6) formats a object (0 ms and 1 assertions)
  ✔ 7) formats an array (0 ms and 1 assertions)
  ✔ 8) formats a string with newlines (0 ms and 1 assertions)
Formatters - Measurement
  ✔ 9) formats a measurement (0 ms and 1 assertions)
Formatters - Flatten
  ✔ 10) flatten a nested object (1 ms and 6 assertions)
  ✔ 11) flatten a nested object no prefix (0 ms and 1 assertions)
  ✔ 12) flatten a string (0 ms and 1 assertions)
  ✔ 13) flatten an array (0 ms and 1 assertions)
  ✔ 14) flatten a number (0 ms and 1 assertions)
  ✔ 15) flatten a float (0 ms and 1 assertions)
  ✔ 16) flatten undefined (0 ms and 1 assertions)
  ✔ 17) flatten null (1 ms and 1 assertions)
  ✔ 18) flatten an object with circular reference (0 ms and 2 assertions)
  ✔ 19) flatten a complex object with circular reference (0 ms and 3 assertions)
measurementPrefix
  ✔ 20) should preserve raw strings (1 ms and 1 assertions)
  ✔ 21) should join string arrays with a / by default (0 ms and 1 assertions)
  ✔ 22) should join string arrays with a specified delimiter (0 ms and 1 assertions)
  ✔ 23) should return an empty string, given an empty config (0 ms and 1 assertions)
  ✔ 24) should return an empty string, given no config (0 ms and 1 assertions)
GoodInflux
  ✔ 25) Unsupported protocol => throw error (1 ms and 1 assertions)
  HTTP URL =>
    ✔ 26) Sends events in a stream to HTTP server (30 ms and 12 assertions)
  UDP URL =>
    ✔ 27) Threshold not set => Sends 5 events in a stream to UDP server (7 ms and 12 assertions)
    ✔ 28) Threshold of 3 => Sends 3 events in a stream to UDP server (3 ms and 8 assertions)
    ✔ 29) Threshold of 13 => Sends 5 events in a stream to UDP server (6 ms and 12 assertions)
log
  ✔ 30) Event log is formatted as expected (1 ms and 1 assertions)
request
  ✔ 31) Event request is formatted as expected (1 ms and 1 assertions)
response
  ✔ 32) Event response is formatted as expected (4 ms and 1 assertions)
error
  ✔ 33) Event error is formatted as expected (2 ms and 1 assertions)
Unrecognized event
  ✔ 34) LineProtocol simply returns (0 ms and 1 assertions)
Measurement prefixes
  ✔ 35) Are applied correctly (1 ms and 1 assertions)
  ✔ 36) Special characters are escaped (1 ms and 1 assertions)
configs
  ✔ 37) Event log is formatted as expected with default tags (0 ms and 1 assertions)
  ✔ 38) Event log is formatted as expected with default fields (1 ms and 1 assertions)
  ✔ 39) Event log is formatted as expected with event name redirection (1 ms and 1 assertions)
  ✔ 40) Event log is formatted as expected with event name function redirection (0 ms and 1 assertions)
ops all events
  ✔ 41) One port => five events created (6 ms and 1 assertions)
  ✔ 42) Two ports => nine events created (7 ms and 1 assertions)
  ✔ 43) No ports reported => Only format the ops event (3 ms and 1 assertions)
ops_responseTimes avg max
  ✔ 44) avg is null and max is string => avg and max shall be 0s (5 ms and 1 assertions)
  ✔ 45) avg and max are both numbers => avg and max shall be numbers (6 ms and 1 assertions)


45 tests complete
Test duration: 286 ms
Assertions count: 93 (verbosity: 2.07)
No global variable leaks detected
Coverage: 96.41% (18/501)
lib/line-protocol.js missing coverage on line(s): 22, 28, 36, 48, 55, 56, 93, 113, 122, 131, 141, 142, 144, 152, 157, 163, 195, 196
Linting results: No issues

Copy link
Contributor

@OuranosSkia OuranosSkia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@OuranosSkia OuranosSkia merged commit 4435555 into creditkarma:master Sep 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants