From f2fb5f3071113ecafa5879ab265373658a7567c8 Mon Sep 17 00:00:00 2001 From: Jiri Kyjovsky Date: Tue, 19 Sep 2023 15:42:38 +0200 Subject: [PATCH] frontend: unify naming convention for safe methods 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 --- backend/run/copr_print_results_to_delete.py | 4 ++-- common/copr_common/worker_manager.py | 4 ++-- dist-git/copr_dist_git/importer.py | 6 +++--- dist-git/tests/test_importer.py | 8 +++---- .../coprs/logic/builds_logic.py | 4 ++-- .../coprs/logic/complex_logic.py | 21 +++++++++++++++---- .../coprs_frontend/coprs/logic/coprs_logic.py | 14 ++++++------- .../views/apiv3_ns/apiv3_build_chroots.py | 8 +++---- .../coprs/views/apiv3_ns/apiv3_builds.py | 2 +- .../coprs/views/apiv3_ns/apiv3_monitor.py | 2 +- .../coprs/views/backend_ns/backend_general.py | 6 +++--- 11 files changed, 46 insertions(+), 33 deletions(-) diff --git a/backend/run/copr_print_results_to_delete.py b/backend/run/copr_print_results_to_delete.py index 52e1996a6..ed7b57672 100755 --- a/backend/run/copr_print_results_to_delete.py +++ b/backend/run/copr_print_results_to_delete.py @@ -42,14 +42,14 @@ def main(): for chroot in os.listdir(projectpath): if chroot in ["srpm-builds", "modules", "repodata", "pubkey.gpg"]: continue - if not is_outdated_to_be_deleted(get_chroot_safe(client, ownername, projectname, chroot)): + if not is_outdated_to_be_deleted(get_chroot_none_safe(client, ownername, projectname, chroot)): continue print(os.path.join(projectpath, chroot)) except NotADirectoryError as ex: print(str(ex)) -def get_chroot_safe(client, ownername, projectname, chrootname): +def get_chroot_none_safe(client, ownername, projectname, chrootname): try: return client.project_chroot_proxy.get(ownername=ownername, projectname=projectname, chrootname=chrootname) except CoprNoResultException: diff --git a/common/copr_common/worker_manager.py b/common/copr_common/worker_manager.py index 8b5b8433b..cf6ad332e 100644 --- a/common/copr_common/worker_manager.py +++ b/common/copr_common/worker_manager.py @@ -386,7 +386,7 @@ def add_task(self, task): task.priority) self.tasks.add_task(task, task.priority) - def _drop_task_id_safe(self, task_id): + def _drop_task_id_noop_safe(self, task_id): try: self.tasks.remove_task_by_id(task_id) except KeyError: @@ -399,7 +399,7 @@ def cancel_task_id(self, task_id): :return: True if worker is running on background, False otherwise """ - self._drop_task_id_safe(task_id) + self._drop_task_id_noop_safe(task_id) worker_id = self.get_worker_id(task_id) if worker_id not in self.worker_ids(): self.log.info("Cancel request, worker %s is not running", worker_id) diff --git a/dist-git/copr_dist_git/importer.py b/dist-git/copr_dist_git/importer.py index 701e2a767..5a20bf6c1 100644 --- a/dist-git/copr_dist_git/importer.py +++ b/dist-git/copr_dist_git/importer.py @@ -57,7 +57,7 @@ def post_back(self, data_dict): log.debug("Sending back: \n{}".format(json.dumps(data_dict))) return post(self.post_back_url, auth=self.auth, data=json.dumps(data_dict), headers=self.headers) - def post_back_safe(self, data_dict): + def post_back_noop_safe(self, data_dict): """ Ignores any error. """ @@ -98,7 +98,7 @@ def do_import(self, task): shutil.rmtree(workdir) log.info("sending a response for task {}".format(result)) - self.post_back_safe(result) + self.post_back_noop_safe(result) self.teardown_per_task_logging(per_task_log_handler) def setup_per_task_logging(self, task): @@ -120,7 +120,7 @@ def run(self): worker_cls = Worker if self.opts.multiple_threads else SingleThreadWorker self.is_running = True while self.is_running: - pool.terminate_timeouted(callback=self.post_back_safe) + pool.terminate_timeouted(callback=self.post_back_noop_safe) pool.remove_dead() if pool.busy: diff --git a/dist-git/tests/test_importer.py b/dist-git/tests/test_importer.py index 5671282b1..69510d65d 100644 --- a/dist-git/tests/test_importer.py +++ b/dist-git/tests/test_importer.py @@ -94,13 +94,13 @@ def test_post_back(self, mc_post): def test_post_back_safe(self, mc_post): dd = {"foo": "bar"} - self.importer.post_back_safe(dd) + self.importer.post_back_noop_safe(dd) assert mc_post.called mc_post.reset_mock() assert not mc_post.called mc_post.side_effect = IOError - self.importer.post_back_safe(dd) + self.importer.post_back_noop_safe(dd) assert mc_post.called def test_do_import(self, mc_import_package, mc_helpers): @@ -111,7 +111,7 @@ def test_do_import(self, mc_import_package, mc_helpers): reponame='foo', branch_commits={self.BRANCH: '123', self.BRANCH2: '124'} ) - self.importer.post_back_safe = MagicMock() + self.importer.post_back_noop_safe = MagicMock() self.importer.do_import(self.url_task) assert mc_import_package.call_args[0][0] == self.opts @@ -119,7 +119,7 @@ def test_do_import(self, mc_import_package, mc_helpers): assert mc_import_package.call_args[0][2] == self.url_task.branches assert mc_import_package.call_args[0][3] == 'somepath.src.rpm' - self.importer.post_back_safe.assert_has_calls([ + self.importer.post_back_noop_safe.assert_has_calls([ mock.call({ 'build_id': 123, 'pkg_name': 'foo', diff --git a/frontend/coprs_frontend/coprs/logic/builds_logic.py b/frontend/coprs_frontend/coprs/logic/builds_logic.py index a051ade7d..b30ac66a6 100644 --- a/frontend/coprs_frontend/coprs/logic/builds_logic.py +++ b/frontend/coprs_frontend/coprs/logic/builds_logic.py @@ -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: @@ -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 diff --git a/frontend/coprs_frontend/coprs/logic/complex_logic.py b/frontend/coprs_frontend/coprs/logic/complex_logic.py index 4c1007298..c7501b30b 100644 --- a/frontend/coprs_frontend/coprs/logic/complex_logic.py +++ b/frontend/coprs_frontend/coprs/logic/complex_logic.py @@ -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() @@ -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 @@ -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: @@ -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. @@ -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)) diff --git a/frontend/coprs_frontend/coprs/logic/coprs_logic.py b/frontend/coprs_frontend/coprs/logic/coprs_logic.py index d3a41746c..c6c2e468d 100644 --- a/frontend/coprs_frontend/coprs/logic/coprs_logic.py +++ b/frontend/coprs_frontend/coprs/logic/coprs_logic.py @@ -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. @@ -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): @@ -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 """ @@ -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( @@ -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 @@ -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 """ diff --git a/frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_build_chroots.py b/frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_build_chroots.py index dc5ec446c..4444d254f 100644 --- a/frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_build_chroots.py +++ b/frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_build_chroots.py @@ -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), @@ -40,7 +40,7 @@ def build_config(build_chroot): @apiv3_ns.route("/build-chroot//", 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)) @@ -62,7 +62,7 @@ def get_build_chroot_list(build_id, **kwargs): @apiv3_ns.route("/build-chroot/build-config//", 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)) @@ -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) diff --git a/frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_builds.py b/frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_builds.py index 83a352ece..d8766db83 100644 --- a/frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_builds.py +++ b/frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_builds.py @@ -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): diff --git a/frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_monitor.py b/frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_monitor.py index ed848a692..8a18a5f5f 100644 --- a/frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_monitor.py +++ b/frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_monitor.py @@ -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 diff --git a/frontend/coprs_frontend/coprs/views/backend_ns/backend_general.py b/frontend/coprs_frontend/coprs/views/backend_ns/backend_general.py index 8a7b4c34a..b3f522486 100755 --- a/frontend/coprs_frontend/coprs/views/backend_ns/backend_general.py +++ b/frontend/coprs_frontend/coprs/views/backend_ns/backend_general.py @@ -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} @@ -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 @@ -369,7 +369,7 @@ def _stream(): @backend_ns.route("/get-build-task/") 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: