billdeitrick / pypco

A Python client for the Planning Center Online API.
MIT License
39 stars 13 forks source link

Need to catch KeyError when there are no relationships #30

Closed pastorhudson closed 4 years ago

pastorhudson commented 4 years ago

I started playing with the dev 1.0 code.

I found a bug :bug:

This code raises a KeyError: 'relationships'

for report in pco.iterate('/services/v2/report_templates'):
    print(f'{report["data"]["attributes"]["body"]}')

This is an endpoint Planning Center recently added so I could build import export for pcochef and services reports.

Traceback:

Traceback (most recent call last):
  File "/home/rhudson/PycharmProjects/newpco/getreport", line 10, in <module>
    for report in pco.iterate('/services/v2/report_templates'):
  File "/home/rhudson/PycharmProjects/newpco/venv/lib/python3.8/site-packages/pypco/pco.py", line 390, in iterate
    for key in cur['relationships']:
KeyError: 'relationships'

If this is known, and I'm just poking your baby before it's ready feel free to close it up.

pastorhudson commented 4 years ago

This fixes it:

                try:
                    for key in cur['relationships']:
                        relationships = cur['relationships'][key]['data']

                        if relationships is not None:
                            if isinstance(relationships, dict):
                                for include in response['included']:
                                    if include['type'] == relationships['type'] and \
                                        include['id'] == relationships['id']:

                                        record['included'].append(include)

                            elif isinstance(relationships, list):
                                for relationship in relationships:
                                    for include in response['included']:
                                        if include['type'] == relationship['type'] and \
                                            include['id'] == relationship['id']:

                                            record['included'].append(include)
                except KeyError:
                    pass

I'd do a pull request, but I can't figure out how to test this with your json mock-ups.

pastorhudson commented 4 years ago

32 has a fix. I’m trying to refactor pcochef to use the new branch. Feel free to merge, or if you want to go another way that’s fine too.

Thanks for all your work on this!

billdeitrick commented 4 years ago

🎉Woohoo--nice work! Glad you found this before I published to PyPi. I will make sure this is fixed before 1.0 is published.

The mocks that aren't done manually are all automatically generated by vcr.py.

As long as you've got creds set up in your dev environment as described here, the mocks will auto generate (and NOT store your API keys 🙂).

Thank you!

pastorhudson commented 4 years ago

I'll leave it open so you have a bookmark.

billdeitrick commented 4 years ago

This is fixed in 7a1c1a3b2e7b524bdc7265c7f7ad1167df5fff55.

I decided to fix this by checking for the existence of the relationships key rather than looking for a KeyError on the whole block. I'm thinking that catching KeyError on the whole block could cause silent failures in situations where we wouldn't want things to fail silently. For instance, if a dict key is missing from one of the other places in this block, falsely make the caller think there are no included objects, when there actually are (but they've been hidden because of catching KeyError).