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: circular reference issue resolved #2538

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

Conversation

SB-rohitdesai
Copy link

@SB-rohitdesai SB-rohitdesai commented Jun 3, 2024

Mock server not working In case of circular references

Summary

Getting 500 Error or Sometimes might need to wait a long time before getting this error when spec having circular references
Checklist

  • The basics
    • I tested these changes manually in my local or dev environment
  • Tests
    • Added or updated
    • N/A
  • Event Tracking
    • I added event tracking and followed the event tracking guidelines
    • N/A
  • Error Reporting
    • I reported errors and followed the error reporting guidelines
    • N/A

@SB-rohitdesai SB-rohitdesai requested a review from a team as a code owner June 3, 2024 14:51
@SB-rohitdesai SB-rohitdesai requested review from kaylachun, rainum and mnaumanali94 and removed request for a team June 3, 2024 14:51
@SB-rohitdesai SB-rohitdesai changed the title circular reference issue resolved fix: circular reference issue resolved Jun 3, 2024
Copy link

@bhaskarsontakke bhaskarsontakke left a comment

Choose a reason for hiding this comment

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

looks good to me.

Copy link
Contributor

@brendarearden brendarearden left a comment

Choose a reason for hiding this comment

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

could you add a test to cover the 500 scenario?

@brendarearden
Copy link
Contributor

is this supposed to include a version of json-schema-ref-parser that has your optimization changes in it? stoplightio/json-schema-ref-parser#62 in it?

@SB-rohitdesai
Copy link
Author

could you add a test to cover the 500 scenario?

Test case added

@SB-rohitdesai
Copy link
Author

is this supposed to include a version of json-schema-ref-parser that has your optimization changes in it? stoplightio/json-schema-ref-parser#62 in it?

Not required. currently we just merging this PR only

@@ -676,5 +689,31 @@ describe('mocker', () => {
});
});
});

describe('should return 500 error', () => {
Copy link
Contributor

@brendarearden brendarearden Jun 24, 2024

Choose a reason for hiding this comment

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

This test does not seem relevant to this PR. Could you please add a test that covers the original circular reference problem and show that it is no longer throwing a 500?

});
});

describe('With __bundled__ should get response', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is passing with and without your changes so it still doesn't seem to be testing the initial bug you were seeing / the fix hasn't changed anything. It should be removed or changed to properly capture the issue you are seeing and fixing.

We have test harness tests for circular references and those have been passing in the pipeline so it seems like circular references are getting resolved in most cases. Maybe a test harness test for the specific scenario that is causing you not to see circular references get resolved would help demonstrate the issue a bit better.

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.

None yet

3 participants