Closed liaokunhua closed 2 years ago
Can you provide a regression test that this fixes?
Sorry bro, this is a bit out of my league, I haven't thought of a special object that would prove i didn't fix this bug.
So, let's talk about how I found this bug. I started learning front-end and github a few days ago, starting with a simple project. I noticed that the stringify.js file in the test report had a line of code that was not covered.
=> throw new RangeError('Cyclic object value');
I did some tests, the result is that non-circular duplicated references can work, but circular dependencies result in "RangeError: Maximum call stack size exceeded", not "RangeError: Cyclic object value".
I think the exception thrown in the use case test of the circular dependency happens to be RangeError, but the exception message is not checked.
Surely you provided some input that caused that error?
Surely you provided some input that caused that error?
Right, an example of a circular reference is constructed, which is inside the use case test. It is easy to reproduce.
Based on the original code for testing.
Step1:
modify line 79 in the stringify.js file, throw new RangeError('Cyclic object value');
to throw new Error('Cyclic object value');
.
Step2: Create a new test and select an example of a circular reference in the stringify.js file inside the test folder and make the changes.
t.test("does not crash when parsing circular references", function(st) {
var circular = { a: 'value' };
circular.a = circular;
try {
qs.stringify(circular);
} catch(e) {
console.dir(e)
}
st['throws'](
function () { qs.stringify(circular); },
RangeError,
'Cyclic values throw'
);
st.end();
})
Step3: Run test and get output:
TAP version 13
# stringify()
# does not crash when parsing circular references
RangeError: Maximum call stack size exceeded
// ....omitted stack info
ok 1 Cyclic values throw
1..1
# tests 1
# pass 1
# ok
I was dumbfounded!! not the expected "Error:Cyclic object value".
Environment information: system: ubuntu 20.04 node: v14.16.1 npm: 6.14.12
ah, i see - so changing that test to check the error message - so we know it's not a stack overflow - would reproduce the error that this PR fixes?
if so, could you make that change?
ah, i see - so changing that test to check the error message - so we know it's not a stack overflow - would reproduce the error that this PR fixes?
if so, could you make that change?
emmm...kinda wish it was me, but i don't think i have it in me yet.😅
I rebased this PR, and updated the test cases in the default branch that were asserting a RangeError, but not the right one, and added another test case that confirms a fix for #403.
Merging #426 (9aee773) into master (24c19cc) will increase coverage by
0.07%
. The diff coverage is100.00%
.
@@ Coverage Diff @@
## master #426 +/- ##
==========================================
+ Coverage 99.78% 99.85% +0.07%
==========================================
Files 8 8
Lines 1393 1408 +15
Branches 169 172 +3
==========================================
+ Hits 1390 1406 +16
+ Misses 3 2 -1
Impacted Files | Coverage Δ | |
---|---|---|
lib/stringify.js | 100.00% <100.00%> (+0.70%) |
:arrow_up: |
test/stringify.js | 99.75% <100.00%> (+<0.01%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 24c19cc...9aee773. Read the comment docs.
With all the testing, I think this is the ultimate solution for circular references.
Fixes #403.