asyncapi / asyncapi-react

React component for rendering documentation from your specification in real-time in the browser. It also provides a WebComponent and bundle for Angular and Vue
https://asyncapi.github.io/asyncapi-react/
Apache License 2.0
188 stars 127 forks source link

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

Open catosaurusrex2003 opened 1 month ago

catosaurusrex2003 commented 1 month ago

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 commented 3 weeks ago

@AceTheCreator can I pick this for Level 1 Review ?

AceTheCreator commented 3 weeks ago

@AceTheCreator can I pick this for Level 1 Review ?

Please go ahead @reachaadrika

reachaadrika commented 3 weeks ago

@catosaurusrex2003 Thanks for picking this issue .

Also here I would suggest to go ahead withErrorBoundary , 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 commented 3 weeks ago

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 commented 3 weeks ago

@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

catosaurusrex2003 commented 3 weeks ago

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

reachaadrika commented 3 weeks ago

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 commented 3 weeks ago

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 https://github.com/asyncapi/asyncapi-react/blob/076f36bd98cb867c26e3c8c2a5426b5551c1af4d/library/src/components/Schema.tsx#L361-L363 and

https://github.com/asyncapi/asyncapi-react/blob/076f36bd98cb867c26e3c8c2a5426b5551c1af4d/library/src/components/Extensions.tsx#L29-L31

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 commented 2 weeks ago

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

catosaurusrex2003 commented 1 week ago

I think this should be merged after https://github.com/asyncapi/asyncapi-react/pull/1108 is merged

sonarcloud[bot] commented 1 week ago

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud