fix: Fail on infinite recursion in `encode.js`

[ajmeese7]

Issue

Closes: #2098

Approach

Implemented the changes suggested by @mtrezza. Will give some clarity to the issues being encountered, and may illuminate further test cases that cause issues that can be handled in a better way in the future.

Tasks

[parse-github-assistant[bot]]

I will reformat the title to use the proper commit message syntax.

[parse-github-assistant[bot]]

Thanks for opening this pull request!

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 96.29630% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 99.96%. Comparing base (72bc9ac) to head (fe3a1bc). Report is 15 commits behind head on alpha.

:exclamation: Current head fe3a1bc differs from pull request most recent head a7333c4. Consider uploading reports for the commit a7333c4 to get more accurate results

Files	Patch %	Lines
src/encode.js	93.33%	1 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##            alpha    #2099      +/-   ##
==========================================
- Coverage   99.98%   99.96%   -0.02%     
==========================================
  Files          61       61              
  Lines        6185     6206      +21     
  Branches     1499     1503       +4     
==========================================
+ Hits         6184     6204      +20     
- Misses          1        2       +1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[mtrezza]

After looking at the encode method again I wonder whether we really need to introduce a max limit of recursive calls, or whether encoding can be "fixed" to properly handle recursive properties. The method already has a seen argument which seems to be used for a similar purpose. If an object has a circular reference, then maybe it could just stay circular in its encoded form. That is, if we consider a circular reference to make sense in the first place. In fact, we even have a existing test for circular pointers, does not stack-overflow when encoding recursive pointers.

[ajmeese7]

@mtrezza found a method that seems to be working on my end with complex objects containing cyclical references, I will be able to fully test by installing locally from my branch later tonight. Will update once I have done so.

Let me know if there's anything else you want changed and I can take a stab at it.

[ajmeese7]

A fix might still be worth implementing here, but I don't see myself having the cycles to work on this PR in the near future.

HOWEVER, I did figure out a cause of all these errors in the legacy code I inherited, if it helps anyone else. The cloud functions were returning the response of an Axios GET request, and the response object has infinite recursion in its properties with sockets. What I had to do was just parse out the data.result properties from the response instead of sending them across the wire with Parse Server, then I was good to go :)

Feel free to close this out if you want, so someone else can come in with a different implementation that you would prefer.

[mtrezza]

It would be too bad, since you've already put so much work into it. Thinking about it, adding the recursion limit that you implemented initially may be the least intrusive one and I'd think poses the least risk to performance. It's the simplest fix. But only if we set the limit ridiculously high (10k?), we can treat it as a non-breaking change, if we assume that nested objects with 10k levels deep are rather unlikely.

On the other hand the solution you implemented would not require the limit and properly handle recursions, as we already do in other parts of the code. But with hash generation, etc we run into perf issues, so we may need a perf comparison test. In fact we have https://github.com/parse-community/parse-server/issues/7610 for that.

[mtrezza]

Picking this up, I think there are 2 ways to move forward:

1. Merge the simplified version where the recursion is simply counted and limited to a fixed amount. This is unlikely to have any significant perf impact, as it's just a var counter. If we set the recursion count limit high enough we also don't have to consider it a breaking change. Probably up to https://github.com/parse-community/Parse-SDK-JS/pull/2099/commits/d33a54a9d6097863aa042dd382250e346b0acf50.

2. Properly handle recursion similar to how recursion is currently handled in that part of the code. That would be similar to what the current state of the PR is, but requires addressing open issues such as hash algo.

I would be hesitant to merge (2) without having a perf comparison in our CI, because of https://github.com/parse-community/Parse-SDK-JS/pull/2099#pullrequestreview-1999317003. So for now we could go ahead and merge (1), so we at least fix the issue of infinite recursion.