feat(api-idorslug): Rename Path paramaters to organization_id_or_slug
#70081
Conversation
github-actions
bot
added
Scope: Frontend
|
🚨 Warning: This pull request contains Frontend and Backend changes! It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently. Have questions? Please ask in the |
Bundle ReportBundle size has no change ✅ |
api-docs/openapi.json
Outdated
| @@ -87,115 +87,115 @@ | |||
| } | |||
| ], | |||
There was a problem hiding this comment.
I hate this json file 😭 looking forward to a day when we delete it #wishful-thinking
src/sentry/api/bases/organization.py
Outdated
| *args: Any, | ||
| **kwargs: Any, | ||
| ) -> tuple[tuple[Any, ...], dict[str, Any]]: | ||
| if not subdomain_is_region(request): | ||
| subdomain = getattr(request, "subdomain", None) | ||
| if subdomain is not None and subdomain != organization_slug: | ||
| if subdomain is not None and subdomain != organization_id_or_slug: |
There was a problem hiding this comment.
Subdomain is never ID. Does this mean it would fail anytime we use the API with ID?
There was a problem hiding this comment.
I think this is related to my question here: https://sentry.slack.com/archives/C032E82D338/p1714605291155389, the git blame is related to customer domain stuff.
There was a problem hiding this comment.
tldr from the thread: this would fail if we get a request like acme.sentry.io/organizations/12345/issues because we wouldn't redirect the user to http://12345.sentry.io/issues. However, most of our customers will hit our endpoints with ids programmatically, so we should be good!
src/sentry/api/bases/project.py
Outdated
| organization_id_or_slug = kwargs.pop("organization_id_or_slug", None) or kwargs.pop( | ||
| "organization_slug" | ||
| ) | ||
|
|
||
| if args and args[0] is not None: | ||
| project_id_or_slug: int | str = args[0] |
There was a problem hiding this comment.
I don't get this. So args[0] is both project slug and org slug?!
Edit: Oh I see. You're removing args one by one. Why not keeping organization_slug as a named arg organization_id_or_slug? Like what you do with the rest of convert_args
There was a problem hiding this comment.
This is a temporary jerry rig so that we maintain backwards compatibility with getsentry endpoints until I update them. They expect convert_args to accept organization_slug, so I can't keep it named, I have to keep it unnamed and extract it. I don't like it, but couldn't think of a better way.
There was a problem hiding this comment.
Please add a TODO comment and make sure they will be done right after. I see your point.
There was a problem hiding this comment.
Does org ever get passed as a kwarg?
There was a problem hiding this comment.
Nope it isn't, Org is above Project in the hierarchy
I am working on making our APIs work with slug and id in our path parameters. Over the path month, we have updated the endpoints so they can support both parameters. As a final step, we are renaming the parameters from {resource}.slug to {resource}.id_or_slug.
A Breakdown of the changes:
#70378
#70384
#70389
For more information, refer to: https://www.notion.so/sentry/Make-APIs-work-with-Slug-ID-cbaf68b883b843d499191e9cc4c116d3 and #69716 (comment)