PR#565: Add a db-session-scope and fix #563 - copr/copr

copr / copr

#565 Add a db-session-scope and fix #563

Merged 5 years ago by frostyx. Opened 5 years ago by praiskup.

Unknown source db-session-scope into master

[frontend] don't forget to commit in 'manage.py alter_chroot'

Pavel Raiskup • 5 years ago

1375d2a

[frontend] new 'db_session_scope' idiom

Pavel Raiskup • 5 years ago

bc9fdb6

frontend/coprs_frontend/commands/alter_chroot.py

file modified

+12 -12

		`@@ -1,6 +1,6 @@`
		`import datetime`
		`from flask_script import Option`
		`- from coprs import db`
		`+ from coprs import db_session_scope`
		`from coprs import app`
		`from coprs import exceptions`
		`from coprs.logic import coprs_logic`
		`@@ -15,19 +15,19 @@`
		`activate = (action == "activate")`
		`for chroot_name in chroot_names:`
		`try:`
		`- mock_chroot = coprs_logic.MockChrootsLogic.edit_by_name(`
		`- chroot_name, activate)`
		`+ with db_session_scope():`
		`+ mock_chroot = coprs_logic.MockChrootsLogic.edit_by_name(`
		`+ chroot_name, activate)`

		`- if action != "eol":`
		`- continue`
		`+ if action != "eol":`
		`+ continue`

		`- for copr_chroot in mock_chroot.copr_chroots:`
		`- delete_after_days = app.config["DELETE_EOL_CHROOTS_AFTER"] + 1`
		`- delete_after_timestamp = datetime.datetime.now() + datetime.timedelta(delete_after_days)`
		`- # Workarounding an auth here`
		`- coprs_logic.CoprChrootsLogic.update_chroot(copr_chroot.copr.user, copr_chroot,`
		`- delete_after=delete_after_timestamp)`
		`- db.session.commit()`
		`+ for copr_chroot in mock_chroot.copr_chroots:`
		`+ delete_after_days = app.config["DELETE_EOL_CHROOTS_AFTER"] + 1`
		`+ delete_after_timestamp = datetime.datetime.now() + datetime.timedelta(delete_after_days)`
		`+ # Workarounding an auth here`
		`+ coprs_logic.CoprChrootsLogic.update_chroot(copr_chroot.copr.user, copr_chroot,`
		`+ delete_after=delete_after_timestamp)`
		`except exceptions.MalformedArgumentException:`
		`self.print_invalid_format(chroot_name)`
		`except exceptions.NotFoundException:`

frontend/coprs_frontend/coprs/__init__.py

file modified

+13

		`@@ -4,6 +4,7 @@`
		`import flask`

		`from flask_sqlalchemy import SQLAlchemy`
		`+ from contextlib import contextmanager`
		`from flask_openid import OpenID`
		`from flask_whooshee import Whooshee`
		`from openid_teams.teams import TeamsResponse`
		`@@ -31,6 +32,18 @@`
		`)`

		`db = SQLAlchemy(app)`
		`+`
		`+ @contextmanager`
		`+ def db_session_scope():`
		`+ """Provide a transactional scope around a series of operations."""`
		`+ session = db.session`
		`+ try:`
		`+ yield session`
		`+ session.commit()`
		`+ except Exception as err:`
		`+ session.rollback()`
		`+ raise`
		`+`
		`whooshee = Whooshee(app)`

praiskup commented 5 years ago

Please have a look at the db_session_scope idiom, whether it makes sense.

2 new commits added

[frontend] don't forget to commit in 'manage.py alter_chroot'
[frontend] new 'db_session_scope' idiom

5 years ago

frostyx commented 5 years ago

I don't think that it is necessary to do this. Looking at #563, I am the one who introduced the bug. When running "deactivate", the change is not committed to the database because of

if action != "eol":
    continue

The condition itself is fine because after it follows only code, that is specific for EOL-ing the chroot. And also committing - which should be elsewhere. I would like to suggest this change

--- a/frontend/coprs_frontend/commands/alter_chroot.py
+++ b/frontend/coprs_frontend/commands/alter_chroot.py
@@ -27,11 +27,11 @@ class AlterChrootCommand(ChrootCommand):
                     # Workarounding an auth here
                     coprs_logic.CoprChrootsLogic.update_chroot(copr_chroot.copr.user, copr_chroot,
                                                                delete_after=delete_after_timestamp)
-                db.session.commit()
             except exceptions.MalformedArgumentException:
                 self.print_invalid_format(chroot_name)
             except exceptions.NotFoundException:
                 self.print_doesnt_exist(chroot_name)
+        db.session.commit()

     option_list = ChrootCommand.option_list + (
         Option("--action",

frostyx commented 5 years ago

Taking my comment back. I viewed this PR as a most straightforward attempt to fix #653, which it is not, therefore I suggested the solution in my first comment. However, I now realized, that the db_session_scope() is an intentional enhancement and the #653 is fixed with it just by the way.

LGTM, +1

praiskup commented 5 years ago

Yup, I'm playing with the context-manager idea for some time (I like the with keyword, and the "barrier" all-or-nothing feeling for SQL transactions)... so I've misused the bug-fix for adding that. Not a problem to propose it separately though.

praiskup commented 5 years ago

Hmpfs, it's a pity that we can not see the git show b2efcd9269f7322ef9c3610ee552ddc1f47c82e1 --ignore-space-change type of diffs in Pagure. edit: reported here: https://pagure.io/pagure/issue/4338

Edited 5 years ago by praiskup