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

Option to support recursion #10

Closed
Amnesthesia opened this issue Jan 25, 2018 · 17 comments
Closed

Option to support recursion #10

Amnesthesia opened this issue Jan 25, 2018 · 17 comments
Assignees
Milestone

Comments

@Amnesthesia
Copy link

Swagger spec supports recursion, and schemas that have recursive references are perfectly valid — no python library seems to support this though. RefResolver has a built-in __recursion_protection, but it protects against all recursion.

Would it be possible to allow it to add an option for recursion depth limit instead, and allow recursion but only until a certain depth limit is reached, and fail silently with a warning?

@jfinkhaeuser
Copy link
Collaborator

Makes sense to me. Sorry for the slow reply, this got lost in my email flood.
I'll look into it!

@Amnesthesia
Copy link
Author

Awesome!

@jfinkhaeuser
Copy link
Collaborator

So... just to be clear.
Object O1 referencing O2 which references O1 again simply makes no sense. You'll never reach the end for resolving them, so the resolution protection can be as strict as it is.
What I'm looking into is allow recursive file references. So object O1 in file F1 references object O2 in file F2, which references object O3 back in file F1.
We're both talking about the same thing, aren't we?
Prance more or less supports this - I say more or less, because there is a bug here. But that's supposed to work at any rate, yes!
Working on it, either way.

@Amnesthesia
Copy link
Author

In my case we actually do have the first thing (which makes no sense), because we have models that reference other models that reference back to the first one; an example would be models like this:

User {
    friend_requests: [ {...} ]
},

FriendRequest {
    from_user: User,
    to_user: User
}

We also have recursive models where we accept JSON search criteria that are nested, like:

SearchBody {
    nestedSearchBody: SearchBody,
    name: [...],
    flags: [...]
}

It's particularly in this case that I'd want to be able to allow recursion but only, say, once or twice — eg show the first user.friend_requests.user but then break out of that otherwise infinite recursion loop

@jfinkhaeuser
Copy link
Collaborator

jfinkhaeuser commented Mar 8, 2018

The first case works with prance, or should. The second won't. I'm not sure how small a change it would be to support the second.

But you can model things differently right now, e.g.

SearchBody {
  name: [...],
  flags: [...]
}

Search {
   searches: [SearchBody, SearchBody, ...]
}

Effectively turning your linked list into an array.

@Amnesthesia
Copy link
Author

I wish I could, but the project is massive and I'm not on the back-end team — I've currently written my own parsing since no Python library supports this, but it's really not pretty :(

@jfinkhaeuser
Copy link
Collaborator

https://github.com/jfinkhaeuser/prance/blob/c27eed8f959092524dec95ea49b66a12f92b3e33/tests/test_util_resolver.py#L82 is the test case that solves the file recursion issue I mentioned earlier. It probably doesn't solve your issue.

@jfinkhaeuser
Copy link
Collaborator

Alright. Well, I understand what your issue is better now. What I am not sure is what to do with it.

I can make a change that allows, say, N recursions fairly easily. What I don't know is what to do when the limit is reached.

Because what you'd typically do is raise an exception when the maximum recursion depth is reached, but that'll not help you at all. It'll have the same behaviour, but be slower.

The other option is to ignore recursive definitions after a limit is reached, but the resolution result would not actually match what's specified. I'm not exactly in love with that.

Hmm.

@Amnesthesia
Copy link
Author

Personally, the last solution is exactly what I was hoping for — or to just leave the field with a "null" / None when recursion limit is reached

@jfinkhaeuser
Copy link
Collaborator

Right, so I've got a solution that I'm not too unhappy about - it complicates the code a little, but not too much, and should work for what you want.
The only restriction is that the best I can do here is effectively this:

foo:
  schema:
     $ref: '#foo'

Will get resolved to:

{
  'foo': {
    'schema': None
  }
}

(I simplified this example - yours had the self-reference in a field.) The point is, with how the code is structured, I can't really remove the schema key, or set the outer property to None - I have to set schema to None.

I hope that'd work for you - because that's what I can do relatively easily.

@Amnesthesia
Copy link
Author

Sounds great! Thank you!

@Amnesthesia
Copy link
Author

Amnesthesia commented Mar 12, 2018

Is this in the latest version? 👍

@jfinkhaeuser
Copy link
Collaborator

No, it's not - I thought I was close, but it turns out I've got a few issues to work through in order to get there, and I'm not sure about the cost of the feature.

So one thing is super easy to implement at practically no cost: keeping true, that is unlimited recursion, by simply assigning the parent element to the child field.

Python deals with this just fine - except when you start iterating down the list. At some point it'll raise an exception about its own, internal recursion limits or circular references.

The YAML output works better: YAML has its own references, so you can get e.g. this here:

definitions:
  Pet: &id001
    properties:
      id:
        format: int64
        type: integer
      name:
        type: string
      next:
        schema: *id001
      tag:
        type: string
    required:
    - id
    - name

You can't output JSON, though, due to the circular reference. The serializer doesn't like it.

The TL;DR is, if you're happy with circular references in the specs and you deal with whatever recursion depth you want all by yourself, I can push something fairly quickly. It doesn't feel like a particularly production quality feature to me, but if it's off by default, I can live with it.

If you want something that produces something with limited recursion, I'll need a bit longer to implement it somewhat sanely.

@Amnesthesia
Copy link
Author

Awesome — that sounds great; can definitely live with setting recursion limits. Thank you so much for your time and commitment!

@jfinkhaeuser jfinkhaeuser self-assigned this Apr 10, 2018
@jfinkhaeuser jfinkhaeuser added this to the v0.10.0 milestone Apr 10, 2018
jfinkhaeuser added a commit that referenced this issue Apr 12, 2018
doesn't change much about the resolver logic. Now for test cases that
make some kind of sense...
@jfinkhaeuser
Copy link
Collaborator

Took a while, but it's now in master.

@hdramzi
Copy link

hdramzi commented Dec 20, 2019

Any chance to support a config option to make None when recursion limit is reached ?

@jfinkhaeuser
Copy link
Collaborator

Yes, that's possible. I'm not sure I'll find the time over Christmas, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants