feat(api): Handle project redirect in API client #8799
Conversation
billyvg
force-pushed
the
feat/project-redirect/handle-in-api-client
branch
from
87cc08e to
907dcbb
Compare
src/sentry/api/exceptions.py
Outdated
| class ResourceMoved(SentryAPIException): | ||
| status_code = status.HTTP_302_FOUND | ||
| code = 'project-moved' | ||
| message = 'Project slug was renamed' |
There was a problem hiding this comment.
Don't know if we should couple this with Projects, specifically. Can we make it more generic to cover other redirects in the future?
src/sentry/api/bases/project.py
Outdated
| @@ -120,8 +122,8 @@ def convert_args(self, request, organization_slug, project_slug, *args, **kwargs | |||
| organization__slug=organization_slug, | |||
| redirect_slug=project_slug | |||
| ) | |||
|
|
|||
| raise ResourceMoved(detail={'slug': redirect.project.slug}) | |||
| new_url = request.path.replace('projects/%s/%s/' % (organization_slug, project_slug), 'projects/%s/%s/' % (organization_slug, redirect.project.slug)) | |||
There was a problem hiding this comment.
This is going to drop any query strings.
You can leverage request.get_full_path() https://docs.djangoproject.com/en/1.7/ref/request-response/#django.http.HttpRequest.get_full_path
Which also includes the querystring.
I'd also introduce a check for like, if new_url != old_url before triggering a redirect, otherwise, you'll end in a redirect loop. This would happen if say, a different url that didn't follow this pattern of projects/%s/%s/ came through. I'd say we'd want to raise a 404 in that case instead of an endless loop of redirection.
billyvg
changed the title
|
|
|
Couldn't we just retain the same JSON response body as before to not break sentry-cli? I'd argue that this change should only be adding an additional Location header. The body should remain the same. |
src/sentry/api/exceptions.py
Outdated
| @@ -28,6 +24,19 @@ def __init__(self, code=None, message=None, detail=None, **kwargs): | |||
| super(SentryAPIException, self).__init__(detail=detail) | |||
|
|
|||
|
|
|||
| class ResourceMoved(SentryAPIException): | |||
There was a problem hiding this comment.
Can we just rename this ProjectMoved since the slug is in the init
Previously we would only be handling API responses with a 302 in `ProjectContext`, which is only a subset of requests that can return a 302. Move the handling up to the API client. Also changes the 302 response body to be an object more consistent with our error response objects. Fixes JAVASCRIPT-3JM
…url before raising a ResourceMoved
billyvg
force-pushed
the
feat/project-redirect/handle-in-api-client
branch
from
e3ed002 to
6809c66
Compare
Previously we would only be handling API responses with a 302 in
ProjectContext, which is only a subset of requests that can return a 302.Move the handling up to the API client.
Also changes the 302 response body to be an object more consistent with ourerror response objects.
Previously the API would return an object with a new slug, this changes it to return a proper redirect response using the
Locationresponse header.Currently undecided how to handle this on the frontend because the user can be at an old URL in the browser while the API requests will transparently query the updated project (since the XHR requests will follow the redirects).
There doesn't seem to be an easy way to intercept 302s using our API client.
Fixes JAVASCRIPT-3JM