Skip to content

Conversation

@infused-kim
Copy link

Hi,

This PR solves an issue with the ncm library's API GET loop.

The problem is in this code block:

            url = get_url
            while url and (len(results) < limit):
                ncm = self.session.get(url, params=params)
                if not (200 <= ncm.status_code < 300):
                    break
                self._return_handler(ncm.status_code, ncm.json()['data'],
                                      call_type)
                url = ncm.json()['meta']['next']
                for d in ncm.json()['data']:
                    results.append(d)

The next url returned by the API already has all the query parameters.

On the next loop, this line...

ncm = self.session.get(url, params=params)

...adds the parameters again. But the requests lib doesn't merge the parameters. Instead it appends duplicate parameters.

This continuous until the URL gets too long and fails with the error:

HTTPError: 414 Client Error: Request-URI Too Large for url: https://www.eu4.cradlepointecm.com/api/v2/net_device_usage_samples/?net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&net_device__in=XXX,YYY,ZZZ&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&limit=1000000&created_at_timeuuid__lt=eb3f1d18-6f2b-11ef-92fc-1aec5073ca63&net_device__in=XXX,YYY,ZZZ&limit=1000000

For most requests this error never triggers, because the URL doesn't get long enough, but with requests that use an __in parameter and 100 IDs, it does happen.

Regular non-in requests also suffer from a bigger request then necessary and the associated overhead.

No exception is raised...

Another issue is that in those cases the API doesn't raise any errors. Instead it returns an empty results list.

It seems that currently it's not possible to distinguish between the API returning 0 rows or an error occuring.

I have solved this in my fork by adding...

            url = get_url
            if params is not None:
                query_string = urlencode(params)
                url = f'{url}?{query_string}'
            while url and (len(results) < limit):
                ncm = self.session.get(url)
+               ncm.raise_for_status()
                if not (200 <= ncm.status_code < 300):
                    break
                self._return_handler(ncm.status_code, ncm.json()['data'],
                                      call_type)
                url = ncm.json()['meta']['next']
                for d in ncm.json()['data']:
                    results.append(d)

But this would break backwards compatibility and perhaps requires another discussion on how to best handle. So, it's not included in this PR.

# Conflicts:
#	ncm/ncm/ncm.py
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.

1 participant