Skip to content

Refactor exception handling #2745

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.

Sign up for GitHub

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

Open
shaidar wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Refactor exception handling #2745

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
29d172c
Refactor exception handling
shaidar Oct 1, 2025
030da95
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Oct 1, 2025
aa9c700
Fix tests
shaidar Oct 1, 2025
0a8c94b
Fix conflicts
shaidar Oct 1, 2025
e4dccd0
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Oct 1, 2025
4d8e05d
Fix tests
shaidar Oct 1, 2025
868f208
Fix tests
shaidar Oct 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 32 additions & 9 deletions websites/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,9 +235,17 @@ def publish_version(self, name, version, request):
return Response(status=200)
except ValidationError as ve:
return Response(data=ve.detail, status=status.HTTP_400_BAD_REQUEST)
except Exception as exc: # pylint: disable=broad-except
log.exception("Error publishing %s version for %s", version, name)
return Response(status=500, data={"details": str(exc)})
except Exception: # pylint: disable=broad-except
log.exception(
"Error publishing %s version for %s (user: %s)",
version,
name,
request.user.username if request.user else "anonymous",
)
return Response(
status=status.HTTP_500_INTERNAL_SERVER_ERROR,
data={"error": "Failed to publish website"},
)
Comment on lines +262 to +265
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Oct 1, 2025

Choose a reason for hiding this comment

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

high

This change correctly prevents leaking exception details in the response. However, the related tests in websites/views_test.py are now broken and need to be updated. Specifically, test_websites_endpoint_preview_error and test_websites_endpoint_publish_error check for the old response body. They should be updated to assert the new response: {'error': 'Failed to publish website'}.


@action(
detail=True, methods=["post"], permission_classes=[HasWebsitePreviewPermission]
Expand Down Expand Up @@ -311,9 +319,16 @@ def unpublish(self, request, name=None):
status=200,
data="The site has been submitted for unpublishing.",
)
except Exception as exc: # pylint: disable=broad-except
log.exception("Error unpublishing %s", name)
return Response(status=500, data={"details": str(exc)})
except Exception: # pylint: disable=broad-except
log.exception(
"Error unpublishing %s (user: %s)",
name,
request.user.username if request.user else "anonymous",
)
return Response(
status=status.HTTP_500_INTERNAL_SERVER_ERROR,
data={"error": "Failed to unpublish website"},
)
Comment on lines +361 to +364
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Oct 1, 2025

Choose a reason for hiding this comment

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

high

Good job on securing this endpoint. As with the other changes, the corresponding test needs to be updated. test_websites_endpoint_unpublish_error in websites/views_test.py will fail because it expects the old response format. Please update it to check for {'error': 'Failed to unpublish website'}.


@action(detail=True, methods=["post"], permission_classes=[BearerTokenPermission])
def pipeline_status(self, request, name=None):
Expand Down Expand Up @@ -446,9 +461,17 @@ def site_configs(self, request):
sync_github_website_starters(
data["repository"]["html_url"], files, commit=data.get("after")
)
except Exception as exc: # pylint: disable=broad-except
log.exception("Error syncing config files")
return Response(status=500, data={"details": str(exc)})
except Exception: # pylint: disable=broad-except
log.exception(
"Error syncing config files from repo %s, files: %s, commit: %s",
data.get("repository", {}).get("html_url"),
files if "files" in locals() else [],
data.get("after"),
)
return Response(
status=status.HTTP_500_INTERNAL_SERVER_ERROR,
data={"error": "Failed to sync site configuration files"},
)
return Response(status=status.HTTP_202_ACCEPTED)
else:
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Oct 1, 2025

Choose a reason for hiding this comment

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

high

This is a great improvement for both security and logging. One last thing: the test test_website_starters_site_configs_exception in websites/views_test.py needs to be updated to assert the new generic error response {'error': 'Failed to sync site configuration files'}.

# Only github webhooks are currently supported
Expand Down
Loading