PagerDuty / pdpyras

Low-level PagerDuty REST/Events API client for Python
MIT License
129 stars 29 forks source link

KeyError when using rget/rput/rpost/list_all for EventOrchestrations API #96

Closed bellmatt closed 1 year ago

bellmatt commented 1 year ago

There is an assumption in resource_envelope used for rget/rpost/rput and list_all functions that the name of the resource provided in the url path is the same as the name of the resource that comes back in the API response: https://github.com/PagerDuty/pdpyras/blob/main/pdpyras.py#L127 https://github.com/PagerDuty/pdpyras/blob/main/pdpyras.py#L1138

However this doesn't always seem to be the case. For the event orchestrations APIs, the url path is /event_orchestrations and the response contains "orchestrations" or "orchestration_path" for the router/unrouted/service APIs https://developer.pagerduty.com/api-reference/7ba0fe7bdb26a-list-event-orchestrations

so the result of list_all is a KeyError at pdpyras.py:1187 KeyError: 'event_orchestrations'

Deconstrained commented 1 year ago

This is a known issue:

https://pagerduty.github.io/pdpyras/#list-of-non-conformal-endpoints

Deconstrained commented 1 year ago

Keeping this open; there may be a compromise that does not require an abstraction layer for resource types (which is not the intent of design) but enables the convenience methods to be used with the newer endpoints. With some of the newer APIs, the envelope name is the only major departure from the conventions that were once universal. Such APIs could be accommodated without cluttering the codebase with special exceptions and workarounds.

Deconstrained commented 1 year ago

This may take a while and in the end I might decide on leaving this as-is. There are a lot of endpoints I need to review in order to decide on a design for a whole new level of abstraction to handle non-conformal APIs, and even then, future APIs might break assumptions that are safe to make for all the current APIs.

Some endpoints like POST /schedules/{id}/overrides and POST /business_services/{id}/subscribers are practically beyond redemption, i.e. because they use inconsistent entity wrapping between the response and request bodies, and it will require extremely specialized logic to support them. It's just a matter of finding a way of doing this that doesn't require completely rewriting how entity wrapping is currently implemented and keeps the specialized logic in one place versus littered throughout the codebase.

Deconstrained commented 1 year ago

It's coming along nicely. It's going to be a new major release. All current endpoints will be supported, although entity wrapping won't always be enabled. I'm developing a rubric for determining when it is enabled based on the endpoint. This won't completely eliminate the need to check the documentation of a given API before using it, but it will do away with the extreme rigidity and usability landmine of having supported vs non-conformal endpoints with respect to entity wrapping.