Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(api-idorslug): Rename Path paramaters to organization_id_or_slug #70081

Closed
Closed
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
f5ea30f
organization_slug to organization_id_or_slug
iamrajjoshi May 1, 2024
230208d
:hammer_and_wrench: apply eslint style fixes
getsantry[bot] May 1, 2024
57cc4ba
test fixes
iamrajjoshi May 1, 2024
1a32ac1
more bug fixes
iamrajjoshi May 1, 2024
cc0dda3
bug fixes
iamrajjoshi May 1, 2024
e93be66
fixes
iamrajjoshi May 1, 2024
1acb6d9
more fixes
iamrajjoshi May 1, 2024
c645f51
fix getsentry tests
iamrajjoshi May 2, 2024
497d58a
test fix
iamrajjoshi May 2, 2024
7b01160
:knife: regenerate mypy module blocklist
getsantry[bot] May 2, 2024
66eca20
bug fix
iamrajjoshi May 2, 2024
a39344d
fix tests
iamrajjoshi May 6, 2024
b3f9626
added todo
iamrajjoshi May 6, 2024
0b299fb
reset pyproject.toml
iamrajjoshi May 6, 2024
7793107
:knife: regenerate mypy module blocklist
getsantry[bot] May 6, 2024
12ff01b
Merge branch 'master' into raj/api-idorslug/rename-path-params/organi…
iamrajjoshi May 8, 2024
b92e84b
Merge branch 'master' into raj/api-idorslug/rename-path-params/organi…
iamrajjoshi May 8, 2024
049c7e0
Merge branch 'master' into raj/api-idorslug/rename-path-params/organi…
iamrajjoshi May 9, 2024
1069c76
Merge branch 'master' into raj/api-idorslug/rename-path-params/organi…
iamrajjoshi May 9, 2024
9a3fd90
Merge branch 'master' into raj/api-idorslug/rename-path-params/organi…
iamrajjoshi May 9, 2024
158fd40
Merge branch 'master' into raj/api-idorslug/rename-path-params/organi…
iamrajjoshi May 10, 2024
09ece06
Merge branch 'master' into raj/api-idorslug/rename-path-params/organi…
iamrajjoshi May 10, 2024
f7c0391
Merge branch 'master' into raj/api-idorslug/rename-path-params/organi…
iamrajjoshi May 10, 2024
d4e4786
Merge branch 'master' into raj/api-idorslug/rename-path-params/organi…
iamrajjoshi May 11, 2024
6447350
Merge branch 'master' into raj/api-idorslug/rename-path-params/organi…
iamrajjoshi May 13, 2024
6dbff1a
Merge branch 'master' into raj/api-idorslug/rename-path-params/organi…
iamrajjoshi May 13, 2024
fd524ce
Merge branch 'master' into raj/api-idorslug/rename-path-params/organi…
iamrajjoshi May 13, 2024
621246e
Merge branch 'master' into raj/api-idorslug/rename-path-params/organi…
iamrajjoshi May 14, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
5 changes: 0 additions & 5 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -120,14 +120,12 @@ ignore_missing_imports = true
module = [
"sentry.api.base",
"sentry.api.bases.external_actor",
"sentry.api.bases.incident",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getsantry made these changes so I am assuming they are correct

"sentry.api.bases.integration",
"sentry.api.bases.organization_events",
"sentry.api.bases.organization_request_change",
"sentry.api.bases.organizationmember",
"sentry.api.bases.project",
"sentry.api.bases.project_request_change",
"sentry.api.bases.rule",
"sentry.api.bases.sentryapps",
"sentry.api.bases.team",
"sentry.api.endpoints.accept_organization_invite",
Expand All @@ -152,11 +150,8 @@ module = [
"sentry.api.endpoints.integrations.sentry_apps.requests",
"sentry.api.endpoints.integrations.sentry_apps.stats.details",
"sentry.api.endpoints.internal.mail",
"sentry.api.endpoints.notifications.notification_actions_details",
"sentry.api.endpoints.organization_code_mapping_codeowners",
"sentry.api.endpoints.organization_code_mapping_details",
"sentry.api.endpoints.organization_code_mappings",
"sentry.api.endpoints.organization_dashboard_details",
"sentry.api.endpoints.organization_details",
"sentry.api.endpoints.organization_events",
"sentry.api.endpoints.organization_events_facets",
Expand Down
14 changes: 8 additions & 6 deletions src/sentry/api/bases/group.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,9 @@ class GroupEndpoint(Endpoint):
owner = ApiOwner.ISSUES
permission_classes = (GroupPermission,)

def convert_args(self, request: Request, issue_id, organization_slug=None, *args, **kwargs):
def convert_args(
self, request: Request, issue_id, organization_id_or_slug=None, *args, **kwargs
):
# TODO(tkaemming): Ideally, this would return a 302 response, rather
# than just returning the data that is bound to the new group. (It
# technically shouldn't be a 301, since the response could change again
Expand All @@ -51,17 +53,17 @@ def convert_args(self, request: Request, issue_id, organization_slug=None, *args
# string replacement, or making the endpoint aware of the URL pattern
# that caused it to be dispatched, and reversing it with the correct
# `issue_id` keyword argument.
if organization_slug:
if organization_id_or_slug:
try:
if (
id_or_slug_path_params_enabled(
self.convert_args.__qualname__, str(organization_slug)
self.convert_args.__qualname__, str(organization_id_or_slug)
)
and str(organization_slug).isdecimal()
and str(organization_id_or_slug).isdecimal()
):
organization = Organization.objects.get_from_cache(id=organization_slug)
organization = Organization.objects.get_from_cache(id=organization_id_or_slug)
else:
organization = Organization.objects.get_from_cache(slug=organization_slug)
organization = Organization.objects.get_from_cache(slug=organization_id_or_slug)
except Organization.DoesNotExist:
raise ResourceDoesNotExist

Expand Down
52 changes: 37 additions & 15 deletions src/sentry/api/bases/organization.py
Original file line number Diff line number Diff line change
Expand Up @@ -242,30 +242,42 @@ class ControlSiloOrganizationEndpoint(Endpoint):
def convert_args(
self,
request: Request,
organization_slug: str | int | None = None,
*args: Any,
**kwargs: Any,
) -> tuple[tuple[Any, ...], dict[str, Any]]:
organization_id_or_slug: int | str | None = None
if args and args[0] is not None:
organization_id_or_slug = args[0]
# Required so it behaves like the original convert_args, where organization_id_or_slug was another parameter
# TODO: Remove this once we remove the old `organization_slug` parameter from getsentry
args = args[1:]
else:
organization_id_or_slug = kwargs.pop("organization_id_or_slug", None) or kwargs.pop(
"organization_slug", None
)

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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Subdomain is never ID. Does this mean it would fail anytime we use the API with ID?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

raise ResourceDoesNotExist

if not organization_slug:
if not organization_id_or_slug:
raise ResourceDoesNotExist

if (
id_or_slug_path_params_enabled(self.convert_args.__qualname__, str(organization_slug))
and str(organization_slug).isdecimal()
id_or_slug_path_params_enabled(
self.convert_args.__qualname__, str(organization_id_or_slug)
)
and str(organization_id_or_slug).isdecimal()
):
# It is ok that `get_organization_by_id` doesn't check for visibility as we
# don't check the visibility in `get_organization_by_slug` either (only_active=False).
organization_context = organization_service.get_organization_by_id(
id=int(organization_slug), user_id=request.user.id
id=int(organization_id_or_slug), user_id=request.user.id
)
else:
organization_context = organization_service.get_organization_by_slug(
slug=str(organization_slug), only_visible=False, user_id=request.user.id
slug=str(organization_id_or_slug), only_visible=False, user_id=request.user.id
)
if organization_context is None:
raise ResourceDoesNotExist
Expand Down Expand Up @@ -536,32 +548,42 @@ def get_filter_params(
def convert_args(
self,
request: Request,
organization_slug: str | int | None = None,
*args: Any,
**kwargs: Any,
) -> tuple[tuple[Any, ...], dict[str, Any]]:
"""
We temporarily allow the organization_slug to be an integer as it actually can be both slug or id
We temporarily allow the organization_id_or_slug to be an integer as it actually can be both slug or id
Eventually, we will rename this method to organization_id_or_slug
"""
organization_id_or_slug: int | str | None = None
if args and args[0] is not None:
organization_id_or_slug = args[0]
# Required so it behaves like the original convert_args, where organization_id_or_slug was another parameter
# TODO: Remove this once we remove the old `organization_slug` parameter from getsentry
args = args[1:]
else:
organization_id_or_slug = kwargs.pop("organization_id_or_slug", None) or kwargs.pop(
"organization_slug", None
)

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:
raise ResourceDoesNotExist

if not organization_slug:
if not organization_id_or_slug:
raise ResourceDoesNotExist

try:
if (
id_or_slug_path_params_enabled(
self.convert_args.__qualname__, str(organization_slug)
self.convert_args.__qualname__, str(organization_id_or_slug)
)
and str(organization_slug).isdecimal()
and str(organization_id_or_slug).isdecimal()
):
organization = Organization.objects.get_from_cache(id=organization_slug)
organization = Organization.objects.get_from_cache(id=organization_id_or_slug)
else:
organization = Organization.objects.get_from_cache(slug=organization_slug)
organization = Organization.objects.get_from_cache(slug=organization_id_or_slug)
except Organization.DoesNotExist:
raise ResourceDoesNotExist

Expand Down
4 changes: 2 additions & 2 deletions src/sentry/api/bases/organization_integrations.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,12 +75,12 @@ class RegionOrganizationIntegrationBaseEndpoint(RegionIntegrationEndpoint):
def convert_args(
self,
request: Request,
organization_slug: str | int | None = None,
organization_id_or_slug: int | str | None = None,
integration_id: str | None = None,
*args: Any,
**kwargs: Any,
) -> tuple[tuple[Any, ...], dict[str, Any]]:
args, kwargs = super().convert_args(request, organization_slug, *args, **kwargs)
args, kwargs = super().convert_args(request, organization_id_or_slug, *args, **kwargs)

kwargs["integration_id"] = self.validate_integration_id(integration_id or "")
return args, kwargs
Expand Down
4 changes: 2 additions & 2 deletions src/sentry/api/bases/organizationmember.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,12 @@ class OrganizationMemberEndpoint(OrganizationEndpoint):
def convert_args(
self,
request: Request,
organization_slug: int | str | None = None,
organization_id_or_slug: str | int | None = None,
member_id: str = "me",
*args: Any,
**kwargs: Any,
) -> tuple[tuple[Any, ...], dict[str, Any]]:
args, kwargs = super().convert_args(request, organization_slug, *args, **kwargs)
args, kwargs = super().convert_args(request, organization_id_or_slug, *args, **kwargs)

serializer = MemberSerializer(data={"id": member_id})
if serializer.is_valid():
Expand Down
27 changes: 18 additions & 9 deletions src/sentry/api/bases/project.py
Original file line number Diff line number Diff line change
Expand Up @@ -111,10 +111,19 @@ class ProjectEndpoint(Endpoint):
def convert_args(
self,
request: Request,
organization_slug: str | int,
*args,
**kwargs,
):
if args and args[0] is not None:
organization_id_or_slug: int | str = args[0]
# Required so it behaves like the original convert_args, where organization_id_or_slug was another parameter
# TODO: Remove this once we remove the old `organization_slug` parameter from getsentry
args = args[1:]
else:
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]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a TODO comment and make sure they will be done right after. I see your point.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does org ever get passed as a kwarg?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope it isn't, Org is above Project in the hierarchy

# Required so it behaves like the original convert_args, where project_id_or_slug was another parameter
Expand All @@ -125,11 +134,11 @@ def convert_args(
)
try:
if id_or_slug_path_params_enabled(
self.convert_args.__qualname__, str(organization_slug)
self.convert_args.__qualname__, str(organization_id_or_slug)
):
project = (
Project.objects.filter(
organization__slug__id_or_slug=organization_slug,
organization__slug__id_or_slug=organization_id_or_slug,
slug__id_or_slug=project_id_or_slug,
)
.select_related("organization")
Expand All @@ -139,7 +148,7 @@ def convert_args(
else:
project = (
Project.objects.filter(
organization__slug=organization_slug, slug=project_id_or_slug
organization__slug=organization_id_or_slug, slug=project_id_or_slug
)
.select_related("organization")
.prefetch_related("teams")
Expand All @@ -151,24 +160,24 @@ def convert_args(
# This will only happen if the passed in project_id_or_slug is a slug and not an id
redirect = ProjectRedirect.objects.select_related("project")
if id_or_slug_path_params_enabled(
self.convert_args.__qualname__, str(organization_slug)
self.convert_args.__qualname__, str(organization_id_or_slug)
):
redirect = redirect.get(
organization__slug__id_or_slug=organization_slug,
organization__slug__id_or_slug=organization_id_or_slug,
redirect_slug=project_id_or_slug,
)
else:
redirect = redirect.get(
organization__slug=organization_slug, redirect_slug=project_id_or_slug
organization__slug=organization_id_or_slug, redirect_slug=project_id_or_slug
)
# Without object permissions don't reveal the rename
self.check_object_permissions(request, redirect.project)

# get full path so that we keep query strings
requested_url = request.get_full_path()
new_url = requested_url.replace(
f"projects/{organization_slug}/{project_id_or_slug}/",
f"projects/{organization_slug}/{redirect.project.slug}/",
f"projects/{organization_id_or_slug}/{project_id_or_slug}/",
f"projects/{organization_id_or_slug}/{redirect.project.slug}/",
)

# Resource was moved/renamed if the requested url is different than the new url
Expand Down
12 changes: 7 additions & 5 deletions src/sentry/api/bases/sentryapps.py
Original file line number Diff line number Diff line change
Expand Up @@ -325,22 +325,24 @@ def has_object_permission(self, request: Request, view, organization):
class SentryAppInstallationsBaseEndpoint(IntegrationPlatformEndpoint):
permission_classes = (SentryAppInstallationsPermission,)

def convert_args(self, request: Request, organization_slug, *args, **kwargs):
def convert_args(self, request: Request, organization_id_or_slug, *args, **kwargs):
extra_args = {}
# We need to pass user_id if the user is not a superuser
if not is_active_superuser(request):
extra_args["user_id"] = request.user.id

if (
id_or_slug_path_params_enabled(self.convert_args.__qualname__, str(organization_slug))
and str(organization_slug).isdecimal()
id_or_slug_path_params_enabled(
self.convert_args.__qualname__, str(organization_id_or_slug)
)
and str(organization_id_or_slug).isdecimal()
):
organization = organization_service.get_org_by_id(
id=int(organization_slug), **extra_args
id=int(organization_id_or_slug), **extra_args
)
else:
organization = organization_service.get_org_by_slug(
slug=organization_slug, **extra_args
slug=organization_id_or_slug, **extra_args
)

if organization is None:
Expand Down
14 changes: 9 additions & 5 deletions src/sentry/api/endpoints/accept_organization_invite.py
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ def get_invite_state(
if (
id_or_slug_path_params_enabled(
convert_args_class=AcceptOrganizationInvite,
organization_slug=str(organization_id_or_slug),
organization_id_or_slug=str(organization_id_or_slug),
)
and str(organization_id_or_slug).isdecimal()
):
Expand Down Expand Up @@ -127,12 +127,12 @@ def get(
request: Request,
member_id: int,
token: str,
organization_slug: int | str | None = None,
organization_id_or_slug: int | str | None = None,
) -> Response:

invite_context = get_invite_state(
member_id=int(member_id),
organization_id_or_slug=organization_slug,
organization_id_or_slug=organization_id_or_slug,
user_id=request.user.id,
request=request,
)
Expand Down Expand Up @@ -222,11 +222,15 @@ def get(
return response

def post(
self, request: Request, member_id: int, token: str, organization_slug: str | None = None
self,
request: Request,
member_id: int,
token: str,
organization_id_or_slug: int | str | None = None,
) -> Response:
invite_context = get_invite_state(
member_id=int(member_id),
organization_id_or_slug=organization_slug,
organization_id_or_slug=organization_id_or_slug,
user_id=request.user.id,
request=request,
)
Expand Down
4 changes: 2 additions & 2 deletions src/sentry/api/endpoints/artifact_bundles.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ def get(self, request: Request, project) -> Response:

Retrieve a list of artifact bundles for a given project.

:pparam string organization_slug: the slug of the organization the
:pparam string organization_id_or_slug: the id or slug of the organization the
artifact bundle belongs to.
:pparam string project_id_or_slug: the id or slug of the project to list the
artifact bundles of.
Expand Down Expand Up @@ -121,7 +121,7 @@ def delete(self, request: Request, project) -> Response:

Delete all artifacts inside given archive.

:pparam string organization_slug: the slug of the organization the
:pparam string organization_id_or_slug: the id or slug of the organization the
archive belongs to.
:pparam string project_id_or_slug: the id or slug of the project to delete the
archive of.
Expand Down
2 changes: 1 addition & 1 deletion src/sentry/api/endpoints/artifact_lookup.py
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ def get(self, request: Request, project: Project) -> Response:

Retrieve a list of individual artifacts or artifact bundles for a given project.

:pparam string organization_slug: the slug of the organization to query.
:pparam string organization_id_or_slug: the id or slug of the organization to query.
:pparam string project_id_or_slug: the id or slug of the project to query.
:qparam string debug_id: if set, will query and return the artifact
bundle that matches the given `debug_id`.
Expand Down
16 changes: 13 additions & 3 deletions src/sentry/api/endpoints/broadcast_index.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,19 @@ def _secondary_filtering(self, request: Request, organization_slug, queryset):
# used in the SAAS product
return list(queryset)

def convert_args(self, request: Request, organization_slug=None, *args, **kwargs):
if organization_slug:
args, kwargs = super().convert_args(request, organization_slug)
def convert_args(self, request: Request, *args, **kwargs):
organization_id_or_slug: int | str | None = None
if args and args[0] is not None:
organization_id_or_slug = args[0]
# Required so it behaves like the original convert_args, where organization_id_or_slug was another parameter
iamrajjoshi marked this conversation as resolved.
Show resolved Hide resolved
# TODO: Remove this once we remove the old `organization_slug` parameter from getsentry
args = args[1:]
else:
organization_id_or_slug = kwargs.pop("organization_id_or_slug", None) or kwargs.pop(
"organization_slug", None
)
if organization_id_or_slug:
args, kwargs = super().convert_args(request, organization_id_or_slug)

return (args, kwargs)

Expand Down
Loading
Loading