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

Processing a self-referencing subResource leads to a series of recursive scans #1424

Closed
elakito opened this issue Sep 3, 2015 · 8 comments
Closed
Milestone

Comments

@elakito
Copy link
Contributor

elakito commented Sep 3, 2015

The problem is described here.
https://groups.google.com/forum/#!topic/swagger-swaggersocket/v_VTJgMm3PM

I'll attach a PR with a proposed solution which caches classes at the Reader instance.

Alternatively, a slightly more complex solution would be to create a private read method that looks like the current recursively invoked read method but with an extra argument of a map that can be used to detect the self-referencing loop during its recursive calls. The original read method can call this private read method with an empty map object.

Which one to take depends on how the reader is used and how its expected behavior should be.
And I would like to ask for your feedback.

regards, aki

elakito added a commit to elakito/swagger-core that referenced this issue Sep 3, 2015
elakito added a commit to elakito/swagger-core that referenced this issue Sep 3, 2015
@webron
Copy link
Contributor

webron commented Sep 3, 2015

Thanks for this, but setting aside the solution, what would be the expected output? Wouldn't you end up with something like /resource/{*subresource}?

@elakito
Copy link
Contributor Author

elakito commented Sep 4, 2015

Hi Ron,
that is a good question. I noticed this scanner issue when using some test resources included in cxf's system tests.

In the case of its wadl generation, the wadl initially has an resource entry for this subresource unexpanded. And every time when I make a call a step deeper, this entry gets expanded.
http://pastebin.com/5sCbj24y

I don't think this expanding of the path on demand in this way is nice.

So I don't know how swagger should show recursive sub-resources.

I'll revert the initial PR for now because I don't think it is right to bookkeep sub-resources per reader anyway.

regards, aki

@webron
Copy link
Contributor

webron commented Sep 4, 2015

Yeah, that's a recursive definition, which is generally ok, but is not supported in Swagger (for now), so I don't think we can add support for in the reader. That said, perhaps we should find a nicer way to deal with it than go into an infinite loop.

@elakito
Copy link
Contributor Author

elakito commented Sep 4, 2015

Yes. I agree with you.
We should separate the two things and get the infinite loop thing fixed for now.

So for fixing the infinite loop, we need to cache the resources seen during the recursive traversal and just stop going deeper. I think we should make a private recursive read method with a resource map. With this approach, we won't stop and come back too early, which could happen when we cache the resources at the reader instance.

I'll think about it. Let me know how you think.
thanks.
regards, aki
p.s. for the displaying part, if swagger can expand the node interactively over its UI, that would be cool. ;-)

@webron
Copy link
Contributor

webron commented Sep 4, 2015

well, we don't support the recrusiveness at all, so no need to expand anything ;)

elakito added a commit to elakito/swagger-core that referenced this issue Sep 7, 2015
@elakito
Copy link
Contributor Author

elakito commented Sep 7, 2015

Hi Ron,
I attached the new PR for the infinite loop issue, as mentioned earlier.
regards, aki

@webron
Copy link
Contributor

webron commented Sep 7, 2015

Thanks for all the effort, @elakito, it's really great. I'll do my best to review it tomorrow.

fehguy added a commit that referenced this issue Sep 20, 2015
 #1424 avoid an endless scan by not scanning resources over a recursive reference
@elakito
Copy link
Contributor Author

elakito commented Sep 21, 2015

Thanks Ron and Tony,
I have verified the merged master.

@elakito elakito closed this as completed Sep 21, 2015
@webron webron added this to the v1.5.4 milestone Sep 21, 2015
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

No branches or pull requests

2 participants