Skip to content

Commit

Permalink
frontend: unify naming convention for safe methods
Browse files Browse the repository at this point in the history
Originally, safe methods were considered as methods raising
ObjectNotFound for API (mainly with session.one() method). Now
methods which returns None or ignores the error completely
are also considered as "safe".

For the sake of consistency:
- *_noop_safe -> methods that ignores error completely (they basically
  logs)
- *_none_safe -> returns None if object was not found in the database
- *_safe -> raises an exception (ObjectNotFound) if no object found
  • Loading branch information
nikromen committed Sep 19, 2023
1 parent 251d76f commit 1d8b232
Show file tree
Hide file tree
Showing 8 changed files with 36 additions and 22 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import time

import sqlalchemy as sa
from sqlalchemy.ext import declarative
from alembic import op

revision = '4d06318043d3'
Expand Down
4 changes: 2 additions & 2 deletions frontend/coprs_frontend/coprs/logic/builds_logic.py
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,7 @@ def get_pending_build_tasks(cls, background=None, data_type=None):
return query

@classmethod
def get_build_task(cls, task_id):
def get_build_task_safe(cls, task_id):
try:
build_id, chroot_name = task_id.split("-", 1)
except ValueError:
Expand Down Expand Up @@ -411,7 +411,7 @@ def get_multiple_by_copr(cls, copr):
def get_copr_builds_list(cls, copr, dirname=None):
query = models.Build.query.filter(models.Build.copr_id==copr.id)
if dirname:
copr_dir = coprs_logic.CoprDirsLogic.get_by_copr(copr, dirname)
copr_dir = coprs_logic.CoprDirsLogic.get_by_copr_safe(copr, dirname)
query = query.filter(models.Build.copr_dir_id==copr_dir.id)
query = query.options(selectinload('build_chroots'), selectinload('package'))
return query
Expand Down
21 changes: 17 additions & 4 deletions frontend/coprs_frontend/coprs/logic/complex_logic.py
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ def get_group_copr_safe(group_name, copr_name, **kwargs):
def get_copr_safe(user_name, copr_name, **kwargs):
""" Get one project.
This always return personal project. For group projects see get_group_copr_safe().
This always return personal project. For group projects see get_group_copr_raise_safe().
"""
try:
return CoprsLogic.get(user_name, copr_name, **kwargs).filter(Copr.group_id.is_(None)).one()
Expand All @@ -201,6 +201,19 @@ def get_copr_by_owner_safe(owner_name, copr_name, **kwargs):

@staticmethod
def get_copr_by_repo_safe(repo_url):
"""
Safely get copr repo by repo url.
Args:
repo_url: str
Returns:
Copr repo or None in case of invalid url format or different url
scheme than copr.
Raises:
ObjectNotFound if no such Copr (group) repo was found.
"""
copr_repo = helpers.copr_repo_fullname(repo_url)
if not copr_repo:
return None
Expand All @@ -212,7 +225,7 @@ def get_copr_by_repo_safe(repo_url):
return ComplexLogic.get_copr_by_owner_safe(owner, copr)

@staticmethod
def get_copr_dir_safe(ownername, copr_dirname, **kwargs):
def get_copr_dir_safe(ownername, copr_dirname):
try:
return CoprDirsLogic.get_by_ownername(ownername, copr_dirname).one()
except sqlalchemy.orm.exc.NoResultFound:
Expand All @@ -238,7 +251,7 @@ def get_build_safe(build_id):
message="Build {} does not exist.".format(build_id))

@staticmethod
def get_build_chroot(build_id, chrootname):
def get_build_chroot_safe(build_id, chrootname):
"""
Return a `models.BuildChroot` instance based on build ID and name of the chroot.
If there is no such chroot, `ObjectNotFound` execption is raised.
Expand Down Expand Up @@ -281,7 +294,7 @@ def get_group_by_name_safe(group_name):
@staticmethod
def get_copr_chroot_safe(copr, chroot_name):
try:
chroot = CoprChrootsLogic.get_by_name_safe(copr, chroot_name)
chroot = CoprChrootsLogic.get_by_name_none_safe(copr, chroot_name)
except (ValueError, KeyError, RuntimeError) as e:
raise ObjectNotFound(message=str(e))

Expand Down
14 changes: 7 additions & 7 deletions frontend/coprs_frontend/coprs/logic/coprs_logic.py
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ def get_by_ownername(cls, ownername):
return cls.filter_by_ownername(query, ownername)

@classmethod
def get_by_ownername_coprname(cls, ownername, coprname):
def get_by_ownername_coprname_safe(cls, ownername, coprname):
"""
Return a Copr object owned by OWNERNAME (either '@groupname' or
'username') with a name COPRNAME.
Expand All @@ -206,7 +206,7 @@ def get_by_ownername_and_dirname(cls, ownername, dirname):
# Some of the calling methods (routes) strictly require us not to check
# if the dirname exists.
coprname = CoprDirsLogic.copr_name_from_dirname(dirname)
return cls.get_by_ownername_coprname(ownername, coprname)
return cls.get_by_ownername_coprname_safe(ownername, coprname)

@classmethod
def filter_without_ids(cls, query, ids):
Expand Down Expand Up @@ -622,7 +622,7 @@ def request_permission(cls, copr, user, permission, req_bool):

class CoprDirsLogic(object):
@classmethod
def get_by_copr_safe(cls, copr, dirname):
def get_by_copr_none_safe(cls, copr, dirname):
"""
Return _query_ for getting CoprDir by Copr and dirname
"""
Expand All @@ -632,12 +632,12 @@ def get_by_copr_safe(cls, copr, dirname):
.filter(models.CoprDir.name==dirname)).first()

@classmethod
def get_by_copr(cls, copr, dirname):
def get_by_copr_safe(cls, copr, dirname):
"""
Return CoprDir instance per given Copr instance and dirname. Raise
ObjectNotFound if it doesn't exist.
"""
coprdir = cls.get_by_copr_safe(copr, dirname)
coprdir = cls.get_by_copr_none_safe(copr, dirname)
if not coprdir:
raise exceptions.ObjectNotFound(
"Dirname '{}' doesn't exist in '{}' copr".format(
Expand Down Expand Up @@ -669,7 +669,7 @@ def get_or_create(cls, copr, dirname, trusted_caller=False):
submitted. We don't create the "main" CoprDirs here (those are created
when a new project is created.
"""
copr_dir = cls.get_by_copr_safe(copr, dirname)
copr_dir = cls.get_by_copr_none_safe(copr, dirname)
if copr_dir:
return copr_dir

Expand Down Expand Up @@ -948,7 +948,7 @@ def get_by_name(cls, copr, chroot_name, active_only=True):
return cls.get_by_mock_chroot_id(copr, mc.id)

@classmethod
def get_by_name_safe(cls, copr, chroot_name):
def get_by_name_none_safe(cls, copr, chroot_name):
"""
:rtype: models.CoprChroot
"""
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ def to_dict(build_chroot):

def build_config(build_chroot):
config = BuildConfigLogic.generate_build_config(build_chroot.build.copr, build_chroot.name)
copr_chroot = CoprChrootsLogic.get_by_name_safe(build_chroot.build.copr, build_chroot.name)
copr_chroot = CoprChrootsLogic.get_by_name_none_safe(build_chroot.build.copr, build_chroot.name)
dict_data = {
"repos": config.get("repos"),
"additional_repos": BuildConfigLogic.generate_additional_repos(copr_chroot),
Expand All @@ -40,7 +40,7 @@ def build_config(build_chroot):
@apiv3_ns.route("/build-chroot/<int:build_id>/<chrootname>", methods=GET) # deprecated
@query_params()
def get_build_chroot(build_id, chrootname):
chroot = ComplexLogic.get_build_chroot(build_id, chrootname)
chroot = ComplexLogic.get_build_chroot_safe(build_id, chrootname)
return flask.jsonify(to_dict(chroot))


Expand All @@ -62,7 +62,7 @@ def get_build_chroot_list(build_id, **kwargs):
@apiv3_ns.route("/build-chroot/build-config/<int:build_id>/<chrootname>", methods=GET) # deprecated
@query_params()
def get_build_chroot_config(build_id, chrootname):
chroot = ComplexLogic.get_build_chroot(build_id, chrootname)
chroot = ComplexLogic.get_build_chroot_safe(build_id, chrootname)
return flask.jsonify(build_config(chroot))


Expand All @@ -72,5 +72,5 @@ def get_build_chroot_built_packages(build_id, chrootname):
"""
Return built packages (NEVRA dicts) for a given build chroot
"""
chroot = ComplexLogic.get_build_chroot(build_id, chrootname)
chroot = ComplexLogic.get_build_chroot_safe(build_id, chrootname)
return flask.jsonify(chroot.results_dict)
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ def check_before_build():

# Raises an exception if CoprDir doesn't exist
if data["project_dirname"]:
CoprDirsLogic.get_by_copr(copr, data["project_dirname"])
CoprDirsLogic.get_by_copr_safe(copr, data["project_dirname"])

# Permissions check
if not flask.g.user.can_build_in(copr):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ def package_monitor(ownername, projectname, project_dirname=None):
additional_fields = set()

if project_dirname:
copr_dir = CoprDirsLogic.get_by_copr(copr, project_dirname)
copr_dir = CoprDirsLogic.get_by_copr_safe(copr, project_dirname)
else:
copr_dir = copr.main_dir

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ def get_build_record(task, for_backend=False):
"repo_priority": task.build.copr.repo_priority
})

copr_chroot = CoprChrootsLogic.get_by_name_safe(task.build.copr, task.mock_chroot.name)
copr_chroot = CoprChrootsLogic.get_by_name_none_safe(task.build.copr, task.mock_chroot.name)
modules = copr_chroot.module_setup_commands
if modules:
build_record["modules"] = {'toggle': modules}
Expand Down Expand Up @@ -275,7 +275,7 @@ def build_task_canceled(task_id):
if not was_running:
if '-' in task_id:
try:
build_chroot = BuildsLogic.get_build_task(task_id)
build_chroot = BuildsLogic.get_build_task_safe(task_id)
build_chroot.status = StatusEnum("canceled")
except ObjectNotFound:
pass
Expand Down Expand Up @@ -369,7 +369,7 @@ def _stream():
@backend_ns.route("/get-build-task/<task_id>")
def get_build_task(task_id):
try:
task = BuildsLogic.get_build_task(task_id)
task = BuildsLogic.get_build_task_safe(task_id)
build_record = get_build_record(task)
return flask.jsonify(build_record)
except CoprHttpException as ex:
Expand Down

0 comments on commit 1d8b232

Please sign in to comment.