Limit maximum output of 'inspect'

[IvanGoncharov]

It's a partial fix for #1753
I basically copied how Node.js inspect limits output but with different limits:

• 10 array items instead of 100
• 2 levels of recursion instead of 3

Note: Node.js inspect has no limits on the max number of properties inside the object so I didn't add it either.

@mjmahone Can you please take a look?

@Cito AFAIK Python version also has this problem. So it would be great if you also review it.

Note: It doesn't fully fix #1753 since you can still produce a huge number of errors but at least those error messages become shorter in most of the cases.

[Cito]

@IvanGoncharov Right, this needs to be fixed in Python as well. I had first used repr() which at least took care of recursion, but then switched to inspect(). Will have a look at your fix and maybe add a few more tests later this week.

[slicejunk]

in the current stable release, inspect causes RangeError in this line from "values.js"

       error.message =
                `Variable "$${varName}" got invalid value ${inspect(value)}; ` +
                error.message;

when value contains an object such as a file to upload, maybe due to its circular structure

please merge this fix!

[Cito]

@IvanGoncharov Looks good to me. Maybe for the sake of safety and consistency we should still restrict the number of object properties even if node,js doesn't do this? This could be implemented very similar to how you do it with arrays (i.e. also giving a hint how many entries were left out). Also, I wonder whether we should truncate long strings as well (those returned by JSON.stringify(), String(), and custom inspect functions)? Again, there should be an ellipsis with a hint how many chars were left out when this happens.

[Cito]

Probably you should also increment recurseTimes when the customInspectFn is not a string. Otherwise objects like const a = { inspect: () => a } can cause an infinite recursion.

[IvanGoncharov]

Probably you should also increment recurseTimes when the customInspectFn is not a string. Otherwise objects like const a = { inspect: () => a } can cause an infinite recursion.

@Cito Addressed in 8f2c383
I also added detection of Circular objects similar to how it's done in Node.js inspect.
Can you please take a look?

[Cito]

@IvanGoncharov with that check you can only avoid direct recursions, but not larger circles like this:

a = {}
b = {}
a.inspect = () => b
b.inspect = () => a
inspect(a) // -> Maximum call stack size exceeded

[IvanGoncharov]

with that check you can only avoid direct recursions, but not larger circles like this:

Thanks for the great test case 👍
I added it in fadbc78 and it's correctly detected as [Circular].
The big benefit in copying Node.js inspect is that it's already battle-tested.

[Cito]

Nice. You only mentioned 8f2c383 which was not sufficient, but the actual fix is 2700388 which does the job and I agree is the best solution - both keeping track of seen objects and limiting the track record (recursion depth).

Btw, the inspect property seems to be deprecated in favor of the symbol in node.js. Should we also print a deprecation message?

[IvanGoncharov]

Btw, the inspect property seems to be deprecated in favor of the symbol in node.js. Should we also print a deprecation message?

@Cito In theory entire graphql-js should be usable in the browser (in practice people mostly use parse, visit, buildClientSchema, etc.) printing debug/deprecation messages on the client side is usually a bad idea at least in production builds.

In the future, we can add detection of dev/prod builds and do something similar to React warnings but it's topic for the separate PR.

[mjmahone]

Sorry for the late review: I probably would have picked the same limits as node, but this seems like a good enough improvement that I'm happy with it as-is!

[mjmahone]

Should these be customizeable values? I realize these match, for instance, the current console.log max depth. For now it is probably better to hard-code, as we can always open up a customizeable option in the future, whereas if we supply that option now we can't easily take it back.

[IvanGoncharov]

@mjmahone Thanks for the review 👍

I probably would have picked the same limits as node, but this seems like a good enough improvement that I'm happy with it as-is!

Since DoS scenario described in #1753 based on MEMORY = NUM_ERRORS * ERROR_SIZE.
We should either limit the number of errors or potential size of the error.
I think if limit number of errors it will be more negatively for DX.