Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Invalid URL for paged requests #88

Closed
ebusto opened this issue Aug 7, 2018 · 8 comments
Closed

Invalid URL for paged requests #88

ebusto opened this issue Aug 7, 2018 · 8 comments
Labels
type: bug A confirmed report of unexpected behavior in the application
Milestone

Comments

@ebusto
Copy link

ebusto commented Aug 7, 2018

pynetbox is building a URL for subsequent requests when paging that is invalid, i.e. /api/virtualization/virtual-machines&limit=112&offset=50.

Should pynetbox just use the next field?

@ebusto
Copy link
Author

ebusto commented Aug 7, 2018

As a quick test, this seems to work, but I'm probably overlooking something.

227,233c227
<                     next_url = "{}{}limit={}&offset={}".format(
<                         self.url,
<                         '&' if self.url[-1] != '/' else '?',
<                         req['count'],
<                         len(req['results'])
<                     ) if first_run else req['next']
<                     req = make_request(next_url)
---
>                     req = make_request(req['next'])

@zachmoody
Copy link
Contributor

next_url = "{}{}limit={}&offset={}".format(
    self.url,
    '&' if self.url[-1] != '/' else '?',
    req['count'],
    len(req['results'])
) if first_run else req['next']

^ I had to go way back for this this one, but iirc this is an optimization. Just dropping req['next'] in there on the first go around could mean a lot more calls than letting this figure out the max and making the fewest calls possible. I think the ternary is for paginated results when filters are already applied. Not sure how this is getting mangled, but are you able to paste an entire example?

@ebusto
Copy link
Author

ebusto commented Aug 7, 2018

Sorry, it looks like I left out a critical detail, which I didn't initially realize was relevant. The paging works fine when calling all, but fails when it is an empty filter, which I would expect to be functionally equivalent.

My use case is to provide a CLI for NetBox, and commands like vm list can optionally take filters, but default to all VMs.

import pynetbox

nb = pynetbox.api("https://netbox", ssl_verify=False, token="<token>")

# Success:
# "GET /api/virtualization/virtual-machines/ HTTP/1.1" 200 31997 "-" "python-requests/2.19.1"
# "GET /api/virtualization/virtual-machines/?limit=111&offset=50 HTTP/1.1" 200 38972 "-" "python-requests/2.19.1"
nb.virtualization.virtual_machines.all()

# Failure:
# "GET /api/virtualization/virtual-machines HTTP/1.1" 301 0 "-" "python-requests/2.19.1"
# "GET /api/virtualization/virtual-machines/ HTTP/1.1" 200 31997 "-" "python-requests/2.19.1"
# "GET /api/virtualization/virtual-machines&limit=111&offset=50 HTTP/1.1" 404 1522 "-" "python-requests/2.19.1"
nb.virtualization.virtual_machines.filter()

@zachmoody
Copy link
Contributor

Ohhh, I see. Wondering if empty kwargs there should raise an Exception or if we should add a trailing slash to self.url in Endpoint and fix up Request.url. 🤔

@ebusto
Copy link
Author

ebusto commented Aug 7, 2018

Well, selfishly, my vote is for the latter, as I wouldn't have to change my code. :-D

@zachmoody
Copy link
Contributor

haha, does .filter() work anywhere w/o kwargs though?

@ebusto
Copy link
Author

ebusto commented Aug 7, 2018

If I hit an endpoint with few enough results that paging isn't required, filter works like a charm sans kwargs.

@zachmoody
Copy link
Contributor

ah, right. Let me mull it over. You bring up a good point - it'd probably be a major version bump if we raised an exception, and there's a few other changes queued up for that.

@zachmoody zachmoody added this to the v4.0.0 milestone Aug 22, 2018
zachmoody pushed a commit that referenced this issue Nov 20, 2018
Raise a ValueError when kwargs for filter() is empty to keep a uniform API and prevent the issues seen in issue #88.
@zachmoody zachmoody added the type: bug A confirmed report of unexpected behavior in the application label Nov 23, 2018
zachmoody pushed a commit that referenced this issue Nov 26, 2018
Raise a ValueError when kwargs for filter() is empty to keep a uniform API and prevent the issues seen in issue #88.
zachmoody pushed a commit that referenced this issue Dec 4, 2018
Raise a ValueError when kwargs for filter() is empty to keep a uniform API and prevent the issues seen in issue #88.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
type: bug A confirmed report of unexpected behavior in the application
Projects
None yet
Development

No branches or pull requests

2 participants