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

fix: application crash by clipping the recursion with a limit of 10 #1101

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

catosaurusrex2003
Copy link
Contributor

Description:
This fixes the issue of website crashing due to too many recursions.

BUT

Even if we put a recursion limit of 10 to the function like this -> https://github.com/catosaurusrex2003/asyncapi-react/blob/9b73e41b7a644992851068d6cf327a5775eb7884/library/src/helpers/schema.ts#L521-L569

We will still have the recursive part rendering 10 times like this
image

The best approach would be to create an ErrorBoundary ( using this ) on the whole application in library/src/containers/AsyncApi/Layout.tsx.

So we can throw error indiscriminately instead of a empty object in jsonFieldToSchema function.
We can render the error and make the app recoverable.

Let me know if my current approach is fine or should i modify my PR with the approach with an ErrorBoundary.

Related issue(s)
#1100

@reachaadrika
Copy link

@AceTheCreator can I pick this for Level 1 Review ?

@AceTheCreator
Copy link
Member

@AceTheCreator can I pick this for Level 1 Review ?

Please go ahead @reachaadrika

@reachaadrika
Copy link

@catosaurusrex2003 Thanks for picking this issue .

Also here I would suggest to go ahead with ErrorBoundary , since implement error boundary will be an asset for entire application and won't be limited to this issue itself .

Also to fix the multiple recursions , rather than specifying the MAX DEPTH to 10 , where we might encounter issue you mentioned above , we can maybe explore WeakSet ,
A WeakSet can help keep track of objects that have already been processed in the current path of recursion. This avoids re-processing objects and naturally handles circular references without limiting the depth. This approach works well for complex, deeply nested structures, as it relies on detecting cycles rather than counting recursion levels.

FYR : https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakSet

PS : Open to discussion / suggestions on the approach , from both you and @AceTheCreator .

cc : @AceTheCreator

@catosaurusrex2003
Copy link
Contributor Author

Also to fix the multiple recursions , rather than specifying the MAX DEPTH to 10 , where we might encounter issue you mentioned above , we can maybe explore WeakSet ,

This is the perfect solution for recursion, thanks @reachaadrika

Now since using WeakSet solves this specific issue, we wont require an Error Boundary for this.

But having an Error boundary in our application will be beneficial in future.

Should I include the Error boundary in this fix PR or create another one as feat.

@reachaadrika
Copy link

reachaadrika commented Nov 4, 2024

@catosaurusrex2003 Will review this change , once .

Add yes adding Error Boundary , Would be beneficial in the long run . Let's keep this isolated you can create a new issue for the same as Improvement .

I tested this for the same ,
Msg above :
Following is the tested screenshot .
Screenshot 2024-11-04 at 10 52 36 PM

@catosaurusrex2003 Let's also find more msgs like this and test 1-2 more samples

Copy link

@reachaadrika reachaadrika left a comment

Choose a reason for hiding this comment

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

Let's test with 1-2 samples , Rest LGTM ✌🏻
@AceTheCreator Level -1 Review and Testing is done , Please review once too .
Thanks

@catosaurusrex2003
Copy link
Contributor Author

catosaurusrex2003 commented Nov 4, 2024

Add yes adding Error Boundary , Would be beneficial in the long run . Let's keep this isolated you can create a new issue for the same as Improvement .

Perfect 👍

Let's test with 1-2 samples

while testing with the following yaml

asyncapi: 3.0.0
info:
  title: Account Service
  version: 1.0.0
  description: This service is in charge of processing user signups
channels:
  userSignedup:
    address: user/signedup
    messages:
      UserSignedUp:
        $ref: '#/components/messages/UserSignedUp'
    x-recursion:  #this RECURSION clips correctly
      $ref: '#'
operations:
  sendUserSignedup:
    action: send
    channel:
      $ref: '#/channels/userSignedup'
    messages:
      - $ref: '#/channels/userSignedup/messages/UserSignedUp'
    x-recursion: # this RECURSION clips correctly
      $ref: '#'
components:
  messages:
    UserSignedUp:
      x-recursion: #this RECURSION clips correctly
        $ref: '#'
      payload:
        x-recursion: # HERE  <------------------------
          $ref: '#'
        type: object
        properties:
          x-recursion: #this RECURSION clips correctly
            $ref: '#'
          displayName:
            type: string
            description: Name of the user
          email:
            type: string

I found all the recursions being handled correctly.

except for the corner case recursion at HERE <------------------------

this recursion is making the browser tab hang and go on in an infinite loop. weird .

will look into its root cause and update the PR with a fix accordingly

Copy link

sonarcloud bot commented Nov 4, 2024

@reachaadrika
Copy link

Add yes adding Error Boundary , Would be beneficial in the long run . Let's keep this isolated you can create a new issue for the same as Improvement .

Perfect 👍

Let's test with 1-2 samples

while testing with the following yaml

asyncapi: 3.0.0
info:
  title: Account Service
  version: 1.0.0
  description: This service is in charge of processing user signups
channels:
  userSignedup:
    address: user/signedup
    messages:
      UserSignedUp:
        $ref: '#/components/messages/UserSignedUp'
    x-recursion:  #this RECURSION clips correctly
      $ref: '#'
operations:
  sendUserSignedup:
    action: send
    channel:
      $ref: '#/channels/userSignedup'
    messages:
      - $ref: '#/channels/userSignedup/messages/UserSignedUp'
    x-recursion: # this RECURSION clips correctly
      $ref: '#'
components:
  messages:
    UserSignedUp:
      x-recursion: #this RECURSION clips correctly
        $ref: '#'
      payload:
        x-recursion: # HERE  <------------------------
          $ref: '#'
        type: object
        properties:
          x-recursion: #this RECURSION clips correctly
            $ref: '#'
          displayName:
            type: string
            description: Name of the user
          email:
            type: string

I found all the recursions being handled correctly.

except for the corner case recursion at HERE <------------------------

this recursion is making the browser tab hang and go on in an infinite loop. weird .

will look into its root cause and update the PR with a fix accordingly

we can check this case once , Let me know if you require any help here

@catosaurusrex2003
Copy link
Contributor Author

The issue arises due to a cyclic dependency between the Schema and Extensions components, as observed in the following code:
The code which is causing this issue is

<Extensions item={schema} />

and

<div className="mt-2">
<Schema schemaName={name} schema={schema} onlyTitle />
</div>

But this cyclic dependency is required for the components to render correctly.

However, it's a rare edge case and might not need a fix. Instead we can choose to leave it as-is for now to avoid overcomplicating the solution.

Cases like these reinforce the need of implementing an Error Boundary. This would allow us to fallback to a previous, correct version of the AsyncAPI document (similar to how undo works in the editor with Ctrl + Z).

@reachaadrika
Copy link

Agreed , Let's implement ErrorBoundary here .
@AceTheCreator LGTM , your reviews ?

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.

4 participants