search

cityofaustin / knackpy

A Python client for interacting with Knack applications
https://cityofaustin.github.io/knackpy/docs/user-guide/
Other
39 stars 18 forks source link

api._continue has a infinite loop bug #91

Closed aidanlister closed 3 years ago

aidanlister commented 3 years ago

I added some debugging into the loop print(f"Getting {len(records)} / {total_records} records from page {page} from {url}"):

Getting 0 / None records from page 1 from https://api.knack.com/v1/objects/object_7/records/
Getting 954 / 6693 records from page 2 from https://api.knack.com/v1/objects/object_7/records/
Getting 1954 / 6693 records from page 3 from https://api.knack.com/v1/objects/object_7/records/
Getting 2954 / 6693 records from page 4 from https://api.knack.com/v1/objects/object_7/records/
Getting 3954 / 6693 records from page 5 from https://api.knack.com/v1/objects/object_7/records/
Getting 4954 / 6693 records from page 6 from https://api.knack.com/v1/objects/object_7/records/
Getting 5954 / 6693 records from page 7 from https://api.knack.com/v1/objects/object_7/records/
Getting 6647 / 6693 records from page 8 from https://api.knack.com/v1/objects/object_7/records/
Getting 6647 / 6693 records from page 9 from https://api.knack.com/v1/objects/object_7/records/
Getting 6647 / 6693 records from page 10 from https://api.knack.com/v1/objects/object_7/records/
Getting 6647 / 6693 records from page 11 from https://api.knack.com/v1/objects/object_7/records/
Getting 6647 / 6693 records from page 12 from https://api.knack.com/v1/objects/object_7/records/
Getting 6647 / 6693 records from page 13 from https://api.knack.com/v1/objects/object_7/records/
Getting 6647 / 6693 records from page 14 from https://api.knack.com/v1/objects/object_7/records/

It looks like even though we requested 1000 rows we only got 954. Should we add a failsafe like below:

while knackpy.api._continue(total_records, len(records), record_limit):
            ...
            fetched_records = res.json()["records"]
            records += fetched_records
            page += 1
            if len(fetched_records) == 0:  # If we're no longer fetching rows, break out of the loop
                break
johnclary commented 3 years ago

@aidanlister thanks for flagging. i remember once bumping into this and then forgetting the exact situation it happens. feel free to open a PR or i'll get around to it in the next week or so.

johnclary commented 3 years ago

@aidanlister if you're able to reproduce this API behavior you can test the patch by installing our dev package (v1.0.19).

aidanlister commented 3 years ago

Great, thanks!

aidanlister commented 3 years ago

Yep, that works. Thanks!

It'd be super handy if you could follow the python logging convention in that file, something like:

import logging

logger = logging.getLogger(__name__)
logger.debug('foo')
johnclary commented 3 years ago

Thanks for the TIL. Done. You'll need to test it by pulling this branch. The CI on the dev package can't handle another build. It's passing tests and will be included in v1.0.19 patch.

aidanlister commented 3 years ago

Unreal, thanks!

johnclary commented 3 years ago

Really appreciate you catching bugs. I'm actually going to bump this to v1.0.20 so we can build the dev package and run some tests on our own ETLs. This should definitely be backwards compatible but I don't want to break our own stuff. Will run it through the ringer.