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.

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

frontend: 404 when build_chroot is not found in database from task_id #3649

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+14 −9
Open

frontend: 404 when build_chroot is not found in database from task_id #3649

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4290ad8
frontend: 404 when build_chroot is not found in database from task_id
nikromen Mar 1, 2025
96e791f
frontend: 400 instead of 500 for invalid arguments provided by user
nikromen Mar 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
2 changes: 1 addition & 1 deletion frontend/coprs_frontend/coprs/exceptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ class InsufficientStorage(CoprHttpException):
_code = 500


class MalformedArgumentException(CoprHttpException, ValueError):
class MalformedArgumentException(BadRequest, ValueError):
pass


Expand Down
21 changes: 13 additions & 8 deletions frontend/coprs_frontend/coprs/logic/builds_logic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
from sqlalchemy.sql import false,true
from werkzeug.utils import secure_filename
from sqlalchemy import bindparam, Integer, String
from sqlalchemy.exc import IntegrityError, NoResultFound
from sqlalchemy.exc import DataError, IntegrityError, NoResultFound

from copr_common.enums import FailTypeEnum, StatusEnum
from coprs import app
Expand Down Expand Up @@ -460,8 +460,8 @@ def get_build_task(cls, task_id):
except ValueError:
raise MalformedArgumentException("Invalid task_id {}".format(task_id))

build_chroot = BuildChrootsLogic.get_by_build_id_and_name(build_id, chroot_name)
try:
build_chroot = BuildChrootsLogic.get_by_build_id_and_name(build_id, chroot_name)
Copy link
Member

Choose a reason for hiding this comment

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

This is a valid code movement.

Copy link
Member Author

Choose a reason for hiding this comment

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

true, that's maybe the only thing I would want to accept right now from this PR

return build_chroot.join(models.Build).one()
except NoResultFound as ex:
raise ObjectNotFound("Specified task ID not found") from ex
Expand Down Expand Up @@ -1498,13 +1498,18 @@ def new(cls, build, mock_chroot, **kwargs):

@classmethod
def get_by_build_id_and_name(cls, build_id, name):
mc = MockChrootsLogic.get_from_name(name).one()
try:
mc = MockChrootsLogic.get_from_name(name).one()

return (
BuildChroot.query
.filter(BuildChroot.build_id == build_id)
.filter(BuildChroot.mock_chroot_id == mc.id)
)
return (
BuildChroot.query
.filter(BuildChroot.build_id == build_id)
.filter(BuildChroot.mock_chroot_id == mc.id)
)
except DataError as ex:
raise MalformedArgumentException(
f"Invalid build_id: {build_id} or name: {name}"
) from ex
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure where the DataError comes from. Do we really need this part at all?

Copy link
Member Author

@nikromen nikromen Mar 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

reproducer: https://copr.fedorainfracloud.org/backend/get-build-task/6742s605-fedora-39-aarch64

but looking at usage of this method... it's only used for backend related part so it really isn't a big deal tbh and it can be dropped


@classmethod
def get_multiply(cls):
Expand Down