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

Mark component.Ref as visited in resolveSchemRef #571

Closed
wants to merge 2 commits into from

Conversation

shlomi-dr
Copy link

First attempt at solving #570

@shlomi-dr
Copy link
Author

shlomi-dr commented Jul 21, 2022

This fix seemed to make the parser not halt on this file, but I am seeing CI failures that I don't understand. Hope this could be a step in the right direction for a more proper fix..


func TestIssue570(t *testing.T) {
loader := NewLoader()
_, err := loader.LoadFromURI(&url.URL{RawPath: "https://rubrikinc.github.io/api-doc-internal-6.0/openapi.json"})
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd suggest you first expand this link into its JSON then prune parts of that document until the infinite loop issue disappears. Then you'll have a minimized regression test and should be able to actually solve this from here :)

Copy link
Author

@shlomi-dr shlomi-dr Jul 29, 2022

Choose a reason for hiding this comment

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

Yeah, you're absolutely right. I spent over 3 hours trying to do just that prior to submitting this commit (pruning to a minimum) but unfortunately I was unable to succeed, and could not spend more time on this.. This is a gigantic file, and every time I thought it should happen, the problem didn't occur.. I left this in to at least show the problem.

I know that as it is, this pr cannot be merged, hoping we could locate a better solution from the other pr you marked this as duplicate of.

Thanks!!

Copy link
Collaborator

@fenollp fenollp Sep 1, 2022

Choose a reason for hiding this comment

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

WIP at #587

@fenollp
Copy link
Collaborator

fenollp commented Sep 23, 2022

Closing now that #607 is merged.

@fenollp fenollp closed this Sep 23, 2022
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.

2 participants