PagerDuty / pdpyras

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

Wrapped entity key error for Tags linked entity endpoint #130

Closed DianaOlympos closed 6 months ago

DianaOlympos commented 6 months ago

The tags/{id}/{entity_type} endpoint is officially supported by pdpyras (see https://github.com/PagerDuty/pdpyras/blob/main/pdpyras.py#L198), but it is a paginated wrapped entity endpoint that does not respect the rules, as it uses the entity_type as its wrapper key. We get errors if we try to use iter_all/list_all on it.

Is there a solution, or should I just use the "raw" queries and do my loop pagination myself?

I would be happy to make a PR if an idea of what the solution should be is offered; right now, I am not sure of the best way, if any.

Deconstrained commented 6 months ago

Hello, and pardon the late reply! For which values of {entity_type} do the errors happen, and what are the errors?

The documentation for GET /tags/{id}/{entity_type} indicates that the entity wrapper name matches the last path node. If the API is not always responding with the documented schema, it is within reason to consider this a bug in the API itself. In that case, I'd be glad to create an issue report for it.

Deconstrained commented 6 months ago

I think I identified the issue (pdpyras.URLError). Thanks for bringing it to my attention. I can think of a few ways to make the client recognize the endpoint as an exception to the otherwise-global rule (which I overlooked last spring when I worked on v5.0.0) and the best way is an allowlist of canonical paths where the last part of the path can be a variable parameter albeit one that refers to a value in a fixed list of well-recognized entity types (as opposed to a separate but similar API endpoint with its own documentation for each {entity_type}).

In short, the idea behind an adequate fix is to make the guardrail that says "this endpoint wasn't meant for pagination" able to properly evaluate the path as referring to a collection and not an individual resource.

Edit: I have decided on a solution.

DianaOlympos commented 6 months ago

Thank you so much! yeah, when I started diving, it definitely sounded like this was a "strange" endpoint from the data type pov.

If it helps, it was with users :)

Deconstrained commented 5 months ago

For the record: the solution decided upon was to expand the single canonical path into multiple paths as though they were separately documented for each explicit {entity_type}. For each of the resulting paths (which end in a constant string users, teams or escalation_policies vs. a variable parameter) the entity wrapping logic works as it should for any other path with no need for special new logic particular to this endpoint.

If new entity types are added to the tags API endpoint in question, support for them will need to be added by updating EXPAND_PATHS in the script that generates the global CANONICAL_PATHS list (and updating the declaration of said global accordingly).