From 1ceecc85f26d15a20bf68c872c62d109bc8dae90 Mon Sep 17 00:00:00 2001 From: Patrick Uiterwijk Date: May 16 2017 12:18:36 +0000 Subject: [PATCH 1/5] Add project-level locking mechanism Signed-off-by: Patrick Uiterwijk --- diff --git a/pagure/__init__.py b/pagure/__init__.py index 7f14cb9..86db99f 100644 --- a/pagure/__init__.py +++ b/pagure/__init__.py @@ -351,7 +351,8 @@ def is_repo_user(repo_obj): ) or (user in usergrps) -def get_authorized_project(session, project_name, user=None, namespace=None): +def get_authorized_project(session, project_name, user=None, namespace=None, + with_lock=False): ''' Retrieving the project with user permission constraint :arg session: The SQLAlchemy session to use @@ -367,7 +368,8 @@ def get_authorized_project(session, project_name, user=None, namespace=None): :rtype: Project ''' - repo = pagure.lib._get_project(session, project_name, user, namespace) + repo = pagure.lib._get_project(session, project_name, user, namespace, + with_lock) if repo and repo.private and not is_repo_admin(repo): return None @@ -387,6 +389,29 @@ def generate_user_key_files(): pagure.lib.git.generate_gitolite_acls() +def acquire_lock(function): + """ Flask decorator to indicate the repo needs to be locked. + + This function reretrieves the flask.g.repo object, but this time requests + that the repo object gets locked. + This lock is retrieved in a way that actively waits until the lock is + acquired. + """ + @wraps(function) + def decorated_function(*args, **kwargs): + set_variables(with_lock=True) + return function(*args, **kwargs) + return decorated_function + + +def ensure_lock(repo): + """ Function to make sure that `repo` was retrieved locked. """ + if not flask.g.repo_locked: + raise Exception('Repo was not locked') + if repo is not flask.g.repo: + raise Exception('Incorrect repo was locked') + + def login_required(function): """ Flask decorator to retrict access to logged in user. If the auth system is ``fas`` it will also require that the user sign @@ -447,7 +472,7 @@ def set_session(): @APP.before_request -def set_variables(): +def set_variables(with_lock=False): """ This method retrieves the repo and username set in the URLs and provides some of the variables that are most often used. """ @@ -468,7 +493,9 @@ def set_variables(): # endpoint called is part of the API, just don't do anything if repo: flask.g.repo = pagure.get_authorized_project( - SESSION, repo, user=username, namespace=namespace) + SESSION, repo, user=username, namespace=namespace, + with_lock=with_lock) + flask.g.repo_locked = with_lock if authenticated(): flask.g.repo_forked = pagure.get_authorized_project( SESSION, repo, user=flask.g.fas_user.username, diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index 65fbd4d..353b296 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -2073,7 +2073,7 @@ def search_projects( return query.all() -def _get_project(session, name, user=None, namespace=None): +def _get_project(session, name, user=None, namespace=None, with_lock=False): '''Get a project from the database ''' query = session.query( @@ -2089,6 +2089,10 @@ def _get_project(session, name, user=None, namespace=None): else: query = query.filter(model.Project.namespace == namespace) + if with_lock: + query = query.with_for_update(nowait=False, + read=False) + if user is not None: query = query.filter( model.User.user == user From 32c1ea91237ca1f19ddbc611d5b53c34f34050f6 Mon Sep 17 00:00:00 2001 From: Patrick Uiterwijk Date: May 16 2017 12:18:36 +0000 Subject: [PATCH 2/5] Acquire lock in operations changing the git repos Signed-off-by: Patrick Uiterwijk --- diff --git a/pagure/ui/fork.py b/pagure/ui/fork.py index d2559d8..848f05c 100644 --- a/pagure/ui/fork.py +++ b/pagure/ui/fork.py @@ -31,7 +31,8 @@ import pagure.exceptions import pagure.lib import pagure.lib.git import pagure.forms -from pagure import APP, SESSION, login_required, __get_file_in_tree +from pagure import (APP, SESSION, login_required, __get_file_in_tree, + acquire_lock) _log = logging.getLogger(__name__) @@ -171,6 +172,7 @@ def request_pulls(repo, username=None, namespace=None): '/fork////pull-request//') @APP.route( '/fork////pull-request/') +@acquire_lock def request_pull(repo, requestid, username=None, namespace=None): """ Create a pull request with the changes from the fork into the project. """ @@ -258,6 +260,7 @@ def request_pull(repo, requestid, username=None, namespace=None): @APP.route('/fork///pull-request/.patch') @APP.route( '/fork////pull-request/.patch') +@acquire_lock def request_pull_patch(repo, requestid, username=None, namespace=None): """ Returns the commits from the specified pull-request as patches. """ @@ -347,6 +350,7 @@ def request_pull_patch(repo, requestid, username=None, namespace=None): '/fork////pull-request//edit', methods=('GET', 'POST')) @login_required +@acquire_lock def request_pull_edit(repo, requestid, username=None, namespace=None): """ Edit the title of a pull-request. """ @@ -420,6 +424,7 @@ def request_pull_edit(repo, requestid, username=None, namespace=None): '/fork////pull-request//' 'comment///', methods=('GET', 'POST')) @login_required +@acquire_lock def pull_request_add_comment( repo, requestid, commit=None, filename=None, row=None, username=None, namespace=None): @@ -516,6 +521,7 @@ def pull_request_add_comment( '/fork////pull-request//' 'comment/drop', methods=['POST']) @login_required +@acquire_lock def pull_request_drop_comment( repo, requestid, username=None, namespace=None): """ Delete a comment of a pull-request. @@ -589,6 +595,7 @@ def pull_request_drop_comment( '/comment//edit', methods=('GET', 'POST')) @login_required +@acquire_lock def pull_request_edit_comment( repo, requestid, commentid, username=None, namespace=None): """Edit comment of a pull request @@ -684,6 +691,7 @@ def pull_request_edit_comment( '/fork////pull-request//merge', methods=['POST']) @login_required +@acquire_lock def merge_request_pull(repo, requestid, username=None, namespace=None): """ Create a pull request with the changes from the fork into the project. """ @@ -773,6 +781,7 @@ def merge_request_pull(repo, requestid, username=None, namespace=None): '/fork////pull-request/cancel/', methods=['POST']) @login_required +@acquire_lock def cancel_request_pull(repo, requestid, username=None, namespace=None): """ Cancel a pull request. """ @@ -834,6 +843,7 @@ def cancel_request_pull(repo, requestid, username=None, namespace=None): '/fork////pull-request//assign', methods=['POST']) @login_required +@acquire_lock def set_assignee_requests(repo, requestid, username=None, namespace=None): ''' Assign a pull-request. ''' repo = flask.g.repo @@ -960,6 +970,7 @@ def fork_project(repo, username=None, namespace=None): @APP.route( '/fork////diff/' '..', methods=('GET', 'POST')) +@acquire_lock def new_request_pull( repo, branch_to, branch_from, username=None, namespace=None): """ Create a pull request with the changes from the fork into the project. @@ -1112,6 +1123,7 @@ def new_request_pull( '/fork////diff/remote', methods=('GET', 'POST')) @login_required +@acquire_lock def new_remote_request_pull(repo, username=None, namespace=None): """ Create a pull request with the changes from a remote fork into the project. @@ -1257,6 +1269,7 @@ def new_remote_request_pull(repo, username=None, namespace=None): '/fork_edit/fork////edit//' 'f/', methods=['POST']) @login_required +@acquire_lock def fork_edit_file( repo, branchname, filename, username=None, namespace=None): """ Fork the project specified and open the specific file to edit diff --git a/pagure/ui/issues.py b/pagure/ui/issues.py index 32c44fe..868ff38 100644 --- a/pagure/ui/issues.py +++ b/pagure/ui/issues.py @@ -37,7 +37,8 @@ import pagure.lib import pagure.lib.encoding_utils import pagure.forms from pagure import (APP, SESSION, __get_file_in_tree, - login_required, authenticated, urlpattern) + login_required, authenticated, urlpattern, + acquire_lock) _log = logging.getLogger(__name__) @@ -70,6 +71,7 @@ _log = logging.getLogger(__name__) '/fork////issue//update', methods=['GET', 'POST']) @login_required +@acquire_lock def update_issue(repo, issueid, username=None, namespace=None): ''' Add a comment to an issue. ''' is_js = flask.request.args.get('js', False) @@ -385,6 +387,7 @@ def update_issue(repo, issueid, username=None, namespace=None): '/fork////tag//edit', methods=('GET', 'POST')) @login_required +@acquire_lock def edit_tag(repo, tag, username=None, namespace=None): """ Edit the specified tag associated with the issues of a project. """ @@ -454,6 +457,7 @@ def edit_tag(repo, tag, username=None, namespace=None): @APP.route('//update/tags', methods=['POST']) @APP.route('///update/tags', methods=['POST']) @login_required +@acquire_lock def update_tags(repo, username=None, namespace=None): """ Update the tags of a project. """ @@ -547,6 +551,7 @@ def update_tags(repo, username=None, namespace=None): @APP.route('/fork///droptag/', methods=['POST']) @APP.route('/fork////droptag/', methods=['POST']) @login_required +@acquire_lock def remove_tag(repo, username=None, namespace=None): """ Remove the specified tag, associated with the issues, from the project. """ @@ -875,6 +880,7 @@ def view_roadmap(repo, username=None, namespace=None): '/fork////new_issue', methods=('GET', 'POST')) @login_required +@acquire_lock def new_issue(repo, username=None, namespace=None): """ Create a new issue """ @@ -1061,6 +1067,7 @@ def view_issue(repo, issueid, username=None, namespace=None): methods=['POST']) @APP.route('/fork////issue//drop', methods=['POST']) +@acquire_lock def delete_issue(repo, issueid, username=None, namespace=None): """ Delete the specified issue """ @@ -1120,6 +1127,7 @@ def delete_issue(repo, issueid, username=None, namespace=None): @APP.route('/fork////issue//edit', methods=('GET', 'POST')) @login_required +@acquire_lock def edit_issue(repo, issueid, username=None, namespace=None): """ Edit the specified issue """ @@ -1245,6 +1253,7 @@ def edit_issue(repo, issueid, username=None, namespace=None): @APP.route('/fork////issue//upload', methods=['POST']) @login_required +@acquire_lock def upload_issue(repo, issueid, username=None, namespace=None): ''' Upload a file to a ticket. ''' @@ -1394,6 +1403,7 @@ def view_issue_raw_file( @APP.route('/fork////issue//comment' '//edit', methods=('GET', 'POST')) @login_required +@acquire_lock def edit_comment_issue( repo, issueid, commentid, username=None, namespace=None): """Edit comment of an issue @@ -1480,6 +1490,7 @@ def edit_comment_issue( @APP.route( '/fork////issues/reports', methods=['POST']) @login_required +@acquire_lock def save_reports(repo, username=None, namespace=None): """ Marked for watching or Unwatching """ diff --git a/pagure/ui/repo.py b/pagure/ui/repo.py index 5e4acd5..1915037 100644 --- a/pagure/ui/repo.py +++ b/pagure/ui/repo.py @@ -51,7 +51,7 @@ import pagure.forms import pagure import pagure.ui.plugins from pagure import (APP, SESSION, __get_file_in_tree, login_required, - admin_session_timedout) + admin_session_timedout, acquire_lock) from pagure.lib import encoding_utils @@ -989,6 +989,7 @@ def new_release(repo, username=None, namespace=None): @APP.route( '/fork////settings', methods=('GET', 'POST')) @login_required +@acquire_lock def view_settings(repo, username=None, namespace=None): """ Presents the settings of the project. """ @@ -1352,6 +1353,7 @@ def update_milestones(repo, username=None, namespace=None): @APP.route( '/fork////default/branch/', methods=['POST']) @login_required +@acquire_lock def change_ref_head(repo, username=None, namespace=None): """ Change HEAD reference """ @@ -1395,6 +1397,7 @@ def change_ref_head(repo, username=None, namespace=None): @APP.route('/fork///delete', methods=['POST']) @APP.route('/fork////delete', methods=['POST']) @login_required +@acquire_lock def delete_repo(repo, username=None, namespace=None): """ Delete the present project. """ @@ -1903,6 +1906,7 @@ def add_group_project(repo, username=None, namespace=None): @APP.route('/fork///regenerate', methods=['POST']) @APP.route('/fork////regenerate', methods=['POST']) @login_required +@acquire_lock def regenerate_git(repo, username=None, namespace=None): """ Regenerate the specified git repo with the content in the project. """ @@ -2095,6 +2099,7 @@ def revoke_api_token(repo, token_id, username=None, namespace=None): '/fork////edit//f/' '', methods=('GET', 'POST')) @login_required +@acquire_lock def edit_file(repo, branchname, filename, username=None, namespace=None): """ Edit a file online. """ @@ -2179,6 +2184,7 @@ def edit_file(repo, branchname, filename, username=None, namespace=None): @APP.route('/fork////b//delete', methods=['POST']) @login_required +@acquire_lock def delete_branch(repo, branchname, username=None, namespace=None): """ Delete the branch of a project. """ From 0a03346aba1ee4c6506222db083d9f6548f73d1a Mon Sep 17 00:00:00 2001 From: Patrick Uiterwijk Date: May 16 2017 12:18:36 +0000 Subject: [PATCH 3/5] Use ensure_lock rather than filelock Signed-off-by: Patrick Uiterwijk --- diff --git a/files/pagure.spec b/files/pagure.spec index fdb6898..e697662 100644 --- a/files/pagure.spec +++ b/files/pagure.spec @@ -25,7 +25,6 @@ BuildRequires: python-blinker BuildRequires: python-chardet BuildRequires: python-cryptography BuildRequires: python-docutils -BuildRequires: python-filelock BuildRequires: python-flask BuildRequires: python-flask-wtf BuildRequires: python-flask-multistatic @@ -63,7 +62,6 @@ Requires: python-chardet Requires: python-cryptography Requires: python-docutils Requires: python-enum34 -Requires: python-filelock Requires: python-flask Requires: python-flask-wtf Requires: python-flask-multistatic diff --git a/pagure/lib/git.py b/pagure/lib/git.py index ccb1f25..0009237 100644 --- a/pagure/lib/git.py +++ b/pagure/lib/git.py @@ -24,7 +24,6 @@ import subprocess import tempfile import arrow -import filelock import pygit2 import werkzeug @@ -214,93 +213,84 @@ def update_git(obj, repo, repofolder): if not repofolder: return + pagure.ensure_lock(repo) + # Get the fork repopath = os.path.join(repofolder, repo.path) - lockfile = '%s.lock' % repopath - - lock = filelock.FileLock(lockfile) - with lock: - - # Clone the repo into a temp folder - newpath = tempfile.mkdtemp(prefix='pagure-') - new_repo = pygit2.clone_repository(repopath, newpath) - - file_path = os.path.join(newpath, obj.uid) - - # Get the current index - index = new_repo.index - - # Are we adding files - added = False - if not os.path.exists(file_path): - added = True - - # Write down what changed - with open(file_path, 'w') as stream: - stream.write(json.dumps( - obj.to_json(), sort_keys=True, indent=4, - separators=(',', ': '))) - - # Retrieve the list of files that changed - diff = new_repo.diff() - files = [] - for patch in diff: - if hasattr(patch, 'new_file_path'): - files.append(patch.new_file_path) - elif hasattr(patch, 'delta'): - files.append(patch.delta.new_file.path) - - # Add the changes to the index - if added: - index.add(obj.uid) - for filename in files: - index.add(filename) - - # If not change, return - if not files and not added: - shutil.rmtree(newpath) - if os.path.exists(lockfile): - # Remove the lock file - os.unlink(lockfile) - return - # See if there is a parent to this commit - parent = None - try: - parent = new_repo.head.get_object().oid - except pygit2.GitError: - pass - - parents = [] - if parent: - parents.append(parent) - - # Author/commiter will always be this one - author = pygit2.Signature(name='pagure', email='pagure') - - # Actually commit - new_repo.create_commit( - 'refs/heads/master', - author, - author, - 'Updated %s %s: %s' % (obj.isa, obj.uid, obj.title), - new_repo.index.write_tree(), - parents) - index.write() - - # Push to origin - ori_remote = new_repo.remotes[0] - master_ref = new_repo.lookup_reference('HEAD').resolve() - refname = '%s:%s' % (master_ref.name, master_ref.name) + # Clone the repo into a temp folder + newpath = tempfile.mkdtemp(prefix='pagure-') + new_repo = pygit2.clone_repository(repopath, newpath) + + file_path = os.path.join(newpath, obj.uid) + + # Get the current index + index = new_repo.index + + # Are we adding files + added = False + if not os.path.exists(file_path): + added = True + + # Write down what changed + with open(file_path, 'w') as stream: + stream.write(json.dumps( + obj.to_json(), sort_keys=True, indent=4, + separators=(',', ': '))) + + # Retrieve the list of files that changed + diff = new_repo.diff() + files = [] + for patch in diff: + if hasattr(patch, 'new_file_path'): + files.append(patch.new_file_path) + elif hasattr(patch, 'delta'): + files.append(patch.delta.new_file.path) + + # Add the changes to the index + if added: + index.add(obj.uid) + for filename in files: + index.add(filename) + + # If not change, return + if not files and not added: + shutil.rmtree(newpath) + return - PagureRepo.push(ori_remote, refname) + # See if there is a parent to this commit + parent = None + try: + parent = new_repo.head.get_object().oid + except pygit2.GitError: + pass + + parents = [] + if parent: + parents.append(parent) + + # Author/commiter will always be this one + author = pygit2.Signature(name='pagure', email='pagure') + + # Actually commit + new_repo.create_commit( + 'refs/heads/master', + author, + author, + 'Updated %s %s: %s' % (obj.isa, obj.uid, obj.title), + new_repo.index.write_tree(), + parents) + index.write() + + # Push to origin + ori_remote = new_repo.remotes[0] + master_ref = new_repo.lookup_reference('HEAD').resolve() + refname = '%s:%s' % (master_ref.name, master_ref.name) - # Remove the clone - shutil.rmtree(newpath) + PagureRepo.push(ori_remote, refname) - if os.path.exists(lockfile): - # Remove the lock file - os.unlink(lockfile) + # Remove the clone + shutil.rmtree(newpath) def clean_git(obj, repo, repofolder): @@ -311,70 +301,64 @@ def clean_git(obj, repo, repofolder): if not repofolder: return + pagure.ensure_lock(repo) + _log.info('Update the git repo: %s to remove: %s', repo.path, obj) # Get the fork repopath = os.path.join(repofolder, repo.path) - lockfile = '%s.lock' % repopath - lock = filelock.FileLock(lockfile) - with lock: + # Clone the repo into a temp folder + newpath = tempfile.mkdtemp(prefix='pagure-') + new_repo = pygit2.clone_repository(repopath, newpath) - # Clone the repo into a temp folder - newpath = tempfile.mkdtemp(prefix='pagure-') - new_repo = pygit2.clone_repository(repopath, newpath) + file_path = os.path.join(newpath, obj.uid) - file_path = os.path.join(newpath, obj.uid) + # Get the current index + index = new_repo.index - # Get the current index - index = new_repo.index + # Are we adding files + if not os.path.exists(file_path): + shutil.rmtree(newpath) + return - # Are we adding files - if not os.path.exists(file_path): - shutil.rmtree(newpath) - return + # Remove the file + os.unlink(file_path) - # Remove the file - os.unlink(file_path) + # Add the changes to the index + index.remove(obj.uid) - # Add the changes to the index - index.remove(obj.uid) + # See if there is a parent to this commit + parent = None + if not new_repo.is_empty: + parent = new_repo.head.get_object().oid - # See if there is a parent to this commit - parent = None - if not new_repo.is_empty: - parent = new_repo.head.get_object().oid - - parents = [] - if parent: - parents.append(parent) - - # Author/commiter will always be this one - author = pygit2.Signature(name='pagure', email='pagure') - - # Actually commit - new_repo.create_commit( - 'refs/heads/master', - author, - author, - 'Removed %s %s: %s' % (obj.isa, obj.uid, obj.title), - new_repo.index.write_tree(), - parents) - index.write() - - # Push to origin - ori_remote = new_repo.remotes[0] - master_ref = new_repo.lookup_reference('HEAD').resolve() - refname = '%s:%s' % (master_ref.name, master_ref.name) + parents = [] + if parent: + parents.append(parent) - PagureRepo.push(ori_remote, refname) + # Author/commiter will always be this one + author = pygit2.Signature(name='pagure', email='pagure') - # Remove the clone - shutil.rmtree(newpath) + # Actually commit + new_repo.create_commit( + 'refs/heads/master', + author, + author, + 'Removed %s %s: %s' % (obj.isa, obj.uid, obj.title), + new_repo.index.write_tree(), + parents) + index.write() + + # Push to origin + ori_remote = new_repo.remotes[0] + master_ref = new_repo.lookup_reference('HEAD').resolve() + refname = '%s:%s' % (master_ref.name, master_ref.name) - if os.path.exists(lockfile): - # Remove the lock file - os.unlink(lockfile) + PagureRepo.push(ori_remote, refname) + + # Remove the clone + shutil.rmtree(newpath) def get_user_from_json(session, jsondata, key='user'): @@ -830,6 +814,8 @@ def add_file_to_git(repo, issue, ticketfolder, user, filename, filestream): if not ticketfolder: return + pagure.ensure_lock(repo) + # Prefix the filename with a timestamp: filename = '%s-%s' % ( hashlib.sha256(filestream.read()).hexdigest(), @@ -839,94 +825,85 @@ def add_file_to_git(repo, issue, ticketfolder, user, filename, filestream): # Get the fork repopath = os.path.join(ticketfolder, repo.path) - lockfile = '%s.lock' % repopath - - lock = filelock.FileLock(lockfile) - with lock: + # Clone the repo into a temp folder + newpath = tempfile.mkdtemp(prefix='pagure-') + new_repo = pygit2.clone_repository(repopath, newpath) - # Clone the repo into a temp folder - newpath = tempfile.mkdtemp(prefix='pagure-') - new_repo = pygit2.clone_repository(repopath, newpath) + folder_path = os.path.join(newpath, 'files') + file_path = os.path.join(folder_path, filename) - folder_path = os.path.join(newpath, 'files') - file_path = os.path.join(folder_path, filename) + # Get the current index + index = new_repo.index - # Get the current index - index = new_repo.index - - # Are we adding files - added = False - if not os.path.exists(file_path): - added = True - else: - # File exists, remove the clone and return - shutil.rmtree(newpath) - return os.path.join('files', filename) - - if not os.path.exists(folder_path): - os.mkdir(folder_path) + # Are we adding files + added = False + if not os.path.exists(file_path): + added = True + else: + # File exists, remove the clone and return + shutil.rmtree(newpath) + return os.path.join('files', filename) - # Write down what changed - filestream.seek(0) - with open(file_path, 'w') as stream: - stream.write(filestream.read()) + if not os.path.exists(folder_path): + os.mkdir(folder_path) - # Retrieve the list of files that changed - diff = new_repo.diff() - files = [patch.new_file_path for patch in diff] + # Write down what changed + filestream.seek(0) + with open(file_path, 'w') as stream: + stream.write(filestream.read()) - # Add the changes to the index - if added: - index.add(os.path.join('files', filename)) - for filename in files: - index.add(filename) + # Retrieve the list of files that changed + diff = new_repo.diff() + files = [patch.new_file_path for patch in diff] - # If not change, return - if not files and not added: - shutil.rmtree(newpath) - return + # Add the changes to the index + if added: + index.add(os.path.join('files', filename)) + for filename in files: + index.add(filename) - # See if there is a parent to this commit - parent = None - try: - parent = new_repo.head.get_object().oid - except pygit2.GitError: - pass - - parents = [] - if parent: - parents.append(parent) - - # Author/commiter will always be this one - author = pygit2.Signature( - name=user.username.encode('utf-8'), - email=user.default_email.encode('utf-8') - ) + # If not change, return + if not files and not added: + shutil.rmtree(newpath) + return - # Actually commit - new_repo.create_commit( - 'refs/heads/master', - author, - author, - 'Add file %s to ticket %s: %s' % ( - filename, issue.uid, issue.title), - new_repo.index.write_tree(), - parents) - index.write() - - # Push to origin - ori_remote = new_repo.remotes[0] - master_ref = new_repo.lookup_reference('HEAD').resolve() - refname = '%s:%s' % (master_ref.name, master_ref.name) + # See if there is a parent to this commit + parent = None + try: + parent = new_repo.head.get_object().oid + except pygit2.GitError: + pass + + parents = [] + if parent: + parents.append(parent) + + # Author/commiter will always be this one + author = pygit2.Signature( + name=user.username.encode('utf-8'), + email=user.default_email.encode('utf-8') + ) - PagureRepo.push(ori_remote, refname) + # Actually commit + new_repo.create_commit( + 'refs/heads/master', + author, + author, + 'Add file %s to ticket %s: %s' % ( + filename, issue.uid, issue.title), + new_repo.index.write_tree(), + parents) + index.write() + + # Push to origin + ori_remote = new_repo.remotes[0] + master_ref = new_repo.lookup_reference('HEAD').resolve() + refname = '%s:%s' % (master_ref.name, master_ref.name) - # Remove the clone - shutil.rmtree(newpath) + PagureRepo.push(ori_remote, refname) - if os.path.exists(lockfile): - # Remove the lock file - os.unlink(lockfile) + # Remove the clone + shutil.rmtree(newpath) return os.path.join('files', filename) @@ -945,102 +922,89 @@ def update_file_in_git( ''' _log.info('Updating file: %s in the repo: %s', filename, repo.path) + pagure.ensure_lock(repo) + # Get the fork repopath = pagure.get_repo_path(repo) - lockfile = '%s.lock' % repopath - - lock = filelock.FileLock(lockfile) - with lock: - - # Clone the repo into a temp folder - newpath = tempfile.mkdtemp(prefix='pagure-') - new_repo = pygit2.clone_repository( - repopath, newpath, checkout_branch=branch) - - file_path = os.path.join(newpath, filename) - - # Get the current index - index = new_repo.index - - # Write down what changed - with open(file_path, 'w') as stream: - stream.write(content.replace('\r', '').encode('utf-8')) + # Clone the repo into a temp folder + newpath = tempfile.mkdtemp(prefix='pagure-') + new_repo = pygit2.clone_repository( + repopath, newpath, checkout_branch=branch) + + file_path = os.path.join(newpath, filename) + + # Get the current index + index = new_repo.index + + # Write down what changed + with open(file_path, 'w') as stream: + stream.write(content.replace('\r', '').encode('utf-8')) + + # Retrieve the list of files that changed + diff = new_repo.diff() + files = [] + for patch in diff: + if hasattr(patch, 'new_file_path'): + files.append(patch.new_file_path) + elif hasattr(patch, 'delta'): + files.append(patch.delta.new_file.path) + + # Add the changes to the index + added = False + for filename in files: + added = True + index.add(filename) + + # If not change, return + if not files and not added: + shutil.rmtree(newpath) + return - # Retrieve the list of files that changed - diff = new_repo.diff() - files = [] - for patch in diff: - if hasattr(patch, 'new_file_path'): - files.append(patch.new_file_path) - elif hasattr(patch, 'delta'): - files.append(patch.delta.new_file.path) + # See if there is a parent to this commit + branch_ref = get_branch_ref(new_repo, branch) + parent = branch_ref.get_object() - # Add the changes to the index - added = False - for filename in files: - added = True - index.add(filename) + # See if we need to create the branch + nbranch_ref = None + if branchto not in new_repo.listall_branches(): + nbranch_ref = new_repo.create_branch(branchto, parent) - # If not change, return - if not files and not added: - shutil.rmtree(newpath) - if os.path.exists(lockfile): - # Remove the lock file - os.unlink(lockfile) - return - - # See if there is a parent to this commit - branch_ref = get_branch_ref(new_repo, branch) - parent = branch_ref.get_object() - - # See if we need to create the branch - nbranch_ref = None - if branchto not in new_repo.listall_branches(): - nbranch_ref = new_repo.create_branch(branchto, parent) - - parents = [] - if parent: - parents.append(parent.hex) - - # Author/commiter will always be this one - author = pygit2.Signature( - name=user.username.encode('utf-8'), - email=email.encode('utf-8') - ) + parents = [] + if parent: + parents.append(parent.hex) - # Actually commit - new_repo.create_commit( - nbranch_ref.name if nbranch_ref else branch_ref.name, - author, - author, - message.strip(), - new_repo.index.write_tree(), - parents) - index.write() - - # Push to origin - ori_remote = new_repo.remotes[0] - refname = '%s:refs/heads/%s' % ( - nbranch_ref.name if nbranch_ref else branch_ref.name, - branchto) + # Author/commiter will always be this one + author = pygit2.Signature( + name=user.username.encode('utf-8'), + email=email.encode('utf-8') + ) - try: - PagureRepo.push(ori_remote, refname) - except pygit2.GitError as err: # pragma: no cover - if os.path.exists(lockfile): - # Remove the lock file - os.unlink(lockfile) - shutil.rmtree(newpath) - raise pagure.exceptions.PagureException( - 'Commit could not be done: %s' % err) + # Actually commit + new_repo.create_commit( + nbranch_ref.name if nbranch_ref else branch_ref.name, + author, + author, + message.strip(), + new_repo.index.write_tree(), + parents) + index.write() + + # Push to origin + ori_remote = new_repo.remotes[0] + refname = '%s:refs/heads/%s' % ( + nbranch_ref.name if nbranch_ref else branch_ref.name, + branchto) - # Remove the clone + try: + PagureRepo.push(ori_remote, refname) + except pygit2.GitError as err: # pragma: no cover shutil.rmtree(newpath) + raise pagure.exceptions.PagureException( + 'Commit could not be done: %s' % err) - if os.path.exists(lockfile): - # Remove the lock file - os.unlink(lockfile) + # Remove the clone + shutil.rmtree(newpath) return os.path.join('files', filename) diff --git a/pagure/ui/fork.py b/pagure/ui/fork.py index 848f05c..6c28aab 100644 --- a/pagure/ui/fork.py +++ b/pagure/ui/fork.py @@ -21,7 +21,6 @@ import os from math import ceil import flask -import filelock import pygit2 from sqlalchemy.exc import SQLAlchemyError @@ -477,15 +476,6 @@ def pull_request_add_comment( return 'error' else: flask.flash(str(err), 'error') - except filelock.Timeout as err: # pragma: no cover - SESSION.rollback() - _log.exception(err) - if is_js: - return 'error' - else: - flask.flash( - 'We could not save all the info, please try again', - 'error') if is_js: return 'ok' @@ -649,15 +639,6 @@ def pull_request_edit_comment( else: flask.flash( 'Could not edit the comment: %s' % commentid, 'error') - except filelock.Timeout as err: # pragma: no cover - SESSION.rollback() - _log.exception(err) - if is_js: - return 'error' - else: - flask.flash( - 'We could not save all the info, please try again', - 'error') if is_js: return 'ok' @@ -817,12 +798,6 @@ def cancel_request_pull(repo, requestid, username=None, namespace=None): flask.flash( 'Could not update this pull-request in the database', 'error') - except filelock.Timeout as err: # pragma: no cover - SESSION.rollback() - _log.exception(err) - flask.flash( - 'We could not save all the info, please try again', - 'error') else: flask.flash('Invalid input submitted', 'error') @@ -883,12 +858,6 @@ def set_assignee_requests(repo, requestid, username=None, namespace=None): SESSION.rollback() _log.exception(err) flask.flash(str(err), 'error') - except filelock.Timeout as err: # pragma: no cover - SESSION.rollback() - _log.exception(err) - flask.flash( - 'We could not save all the info, please try again', - 'error') return flask.redirect(flask.url_for( 'request_pull', username=username, namespace=namespace, @@ -1060,12 +1029,6 @@ def new_request_pull( except SQLAlchemyError as err: # pragma: no cover SESSION.rollback() flask.flash(str(err), 'error') - except filelock.Timeout as err: # pragma: no cover - SESSION.rollback() - _log.exception(err) - flask.flash( - 'We could not save all the info, please try again', - 'error') if not flask.g.repo_committer: form = None diff --git a/pagure/ui/issues.py b/pagure/ui/issues.py index 868ff38..9cb2290 100644 --- a/pagure/ui/issues.py +++ b/pagure/ui/issues.py @@ -22,7 +22,6 @@ from collections import defaultdict from math import ceil import flask -import filelock import pygit2 import werkzeug.datastructures from sqlalchemy.exc import SQLAlchemyError @@ -347,13 +346,6 @@ def update_issue(repo, issueid, username=None, namespace=None): SESSION.rollback() _log.exception(err) flask.flash(str(err), 'error') - except filelock.Timeout as err: # pragma: no cover - is_js = False - SESSION.rollback() - _log.exception(err) - flask.flash( - 'We could not save all the info, please try again', - 'error') else: if is_js: return 'notok: %s' % form.errors @@ -950,12 +942,6 @@ def new_issue(repo, username=None, namespace=None): except SQLAlchemyError as err: # pragma: no cover SESSION.rollback() flask.flash(str(err), 'error') - except filelock.Timeout as err: # pragma: no cover - SESSION.rollback() - _log.exception(err) - flask.flash( - 'We could not save all the info, please try again', - 'error') types = None default = None @@ -1221,12 +1207,6 @@ def edit_issue(repo, issueid, username=None, namespace=None): except SQLAlchemyError as err: # pragma: no cover SESSION.rollback() flask.flash(str(err), 'error') - except filelock.Timeout as err: # pragma: no cover - SESSION.rollback() - _log.exception(err) - flask.flash( - 'We could not save all the info, please try again', - 'error') elif flask.request.method == 'GET': form.title.data = issue.title @@ -1279,21 +1259,14 @@ def upload_issue(repo, issueid, username=None, namespace=None): if form.validate_on_submit(): filestream = flask.request.files['filestream'] - try: - new_filename = pagure.lib.git.add_file_to_git( - repo=repo, - issue=issue, - ticketfolder=APP.config['TICKETS_FOLDER'], - user=user_obj, - filename=filestream.filename, - filestream=filestream.stream, - ) - except filelock.Timeout as err: # pragma: no cover - SESSION.rollback() - _log.exception(err) - flask.flash( - 'We could not save all the info, please try again', - 'error') + new_filename = pagure.lib.git.add_file_to_git( + repo=repo, + issue=issue, + ticketfolder=APP.config['TICKETS_FOLDER'], + user=user_obj, + filename=filestream.filename, + filestream=filestream.stream, + ) return flask.jsonify({ 'output': 'ok', @@ -1455,12 +1428,6 @@ def edit_comment_issue( return 'error' flask.flash( 'Could not edit the comment: %s' % commentid, 'error') - except filelock.Timeout as err: # pragma: no cover - SESSION.rollback() - _log.exception(err) - flask.flash( - 'We could not save all the info, please try again', - 'error') if is_js: return 'ok' diff --git a/requirements-fedora.txt b/requirements-fedora.txt index 58012b8..7dd2001 100644 --- a/requirements-fedora.txt +++ b/requirements-fedora.txt @@ -11,7 +11,6 @@ python-fedora-flask python-flask python-flask-wtf python-flask-multistatic -python-filelock python-bcrypt python-jinja2 python-markdown diff --git a/requirements.txt b/requirements.txt index 5d7eb28..30902b3 100644 --- a/requirements.txt +++ b/requirements.txt @@ -8,7 +8,6 @@ blinker chardet < 3.0.0 docutils enum34 -filelock flask flask-wtf flask-multistatic diff --git a/tests/test_pagure_lib_git.py b/tests/test_pagure_lib_git.py index 8a05b25..604aff3 100644 --- a/tests/test_pagure_lib_git.py +++ b/tests/test_pagure_lib_git.py @@ -1517,13 +1517,11 @@ index 458821a..77674a8 #print patch self.assertEqual(patch, exp) - @patch('filelock.FileLock') - def test_clean_git(self, mock_fl): + def test_clean_git(self): """ Test the clean_git method of pagure.lib.git. """ pagure.lib.git.clean_git(None, None, None) self.test_update_git() - self.assertEqual(mock_fl.call_count, 3) gitpath = os.path.join(self.path, 'test_ticket_repo.git') gitrepo = pygit2.init_repository(gitpath, bare=True) @@ -1545,9 +1543,6 @@ index 458821a..77674a8 issue = pagure.lib.search_issues(self.session, repo, issueid=1) pagure.lib.git.clean_git(issue, repo, self.path) - # 4 times: 3 in test_update_git + 1 here - self.assertEqual(mock_fl.call_count, 4) - # No more files in the git repo commit = gitrepo.revparse_single('HEAD') files = [entry.name for entry in commit.tree] From 00afe2ba9bd817147c02e6eee6a86d94eaadfa8f Mon Sep 17 00:00:00 2001 From: Patrick Uiterwijk Date: May 16 2017 12:18:36 +0000 Subject: [PATCH 4/5] Patch ensure_lock out for lib.git tests Signed-off-by: Patrick Uiterwijk --- diff --git a/tests/test_pagure_flask_dump_load_ticket.py b/tests/test_pagure_flask_dump_load_ticket.py index 48ff558..f2da049 100644 --- a/tests/test_pagure_flask_dump_load_ticket.py +++ b/tests/test_pagure_flask_dump_load_ticket.py @@ -55,8 +55,9 @@ class PagureFlaskDumpLoadTicketTests(tests.Modeltests): self.path, 'requests') self.app = pagure.APP.test_client() + @patch('pagure.ensure_lock') @patch('pagure.lib.notify.send_email') - def test_dumping_reloading_ticket(self, send_email): + def test_dumping_reloading_ticket(self, elock, send_email): """ Test dumping a ticket into a JSON blob. """ send_email.return_value = True diff --git a/tests/test_pagure_flask_ui_repo.py b/tests/test_pagure_flask_ui_repo.py index 34d14ee..5511c1e 100644 --- a/tests/test_pagure_flask_ui_repo.py +++ b/tests/test_pagure_flask_ui_repo.py @@ -2598,87 +2598,88 @@ index 0000000..fb7093d 'Forks 0', output.data) - # add issues - repo = pagure.get_authorized_project(self.session, 'test') - msg = pagure.lib.new_issue( - session=self.session, - repo=repo, - title='Test issue', - content='We should work on this', - user='pingou', - ticketfolder=os.path.join(self.path, 'tickets') - ) - self.session.commit() - self.assertEqual(msg.title, 'Test issue') - - msg = pagure.lib.new_issue( - session=self.session, - repo=repo, - title='Test issue #2', - content='We should work on this, really', - user='pingou', - ticketfolder=os.path.join(self.path, 'tickets') - ) - self.session.commit() - self.assertEqual(msg.title, 'Test issue #2') - - # Add a comment to an issue - issue = pagure.lib.search_issues(self.session, repo, issueid=1) - msg = pagure.lib.add_issue_comment( - session=self.session, - issue=issue, - comment='Hey look a comment!', - user='foo', - ticketfolder=None - ) - self.session.commit() - self.assertEqual(msg, 'Comment added') - - # add pull-requests - req = pagure.lib.new_pull_request( - session=self.session, - repo_from=repo, - branch_from='feature', - repo_to=repo, - branch_to='master', - title='test pull-request', - user='pingou', - requestfolder=os.path.join(self.path, 'requests'), - ) - self.session.commit() - self.assertEqual(req.id, 3) - self.assertEqual(req.title, 'test pull-request') - - req = pagure.lib.new_pull_request( - session=self.session, - repo_from=repo, - branch_from='feature2', - repo_to=repo, - branch_to='master', - title='test pull-request', - user='pingou', - requestfolder=os.path.join(self.path, 'requests'), - ) - self.session.commit() - self.assertEqual(req.id, 4) - self.assertEqual(req.title, 'test pull-request') - - # Add comment on a pull-request - request = pagure.lib.search_pull_requests( - self.session, requestid=3) - - msg = pagure.lib.add_pull_request_comment( - session=self.session, - request=request, - commit='commithash', - tree_id=None, - filename='file', - row=None, - comment='This is awesome, I got to remember it!', - user='foo', - requestfolder=None, - ) - self.assertEqual(msg, 'Comment added') + with patch('pagure.ensure_lock'): + # add issues + repo = pagure.get_authorized_project(self.session, 'test') + msg = pagure.lib.new_issue( + session=self.session, + repo=repo, + title='Test issue', + content='We should work on this', + user='pingou', + ticketfolder=os.path.join(self.path, 'tickets') + ) + self.session.commit() + self.assertEqual(msg.title, 'Test issue') + + msg = pagure.lib.new_issue( + session=self.session, + repo=repo, + title='Test issue #2', + content='We should work on this, really', + user='pingou', + ticketfolder=os.path.join(self.path, 'tickets') + ) + self.session.commit() + self.assertEqual(msg.title, 'Test issue #2') + + # Add a comment to an issue + issue = pagure.lib.search_issues(self.session, repo, issueid=1) + msg = pagure.lib.add_issue_comment( + session=self.session, + issue=issue, + comment='Hey look a comment!', + user='foo', + ticketfolder=None + ) + self.session.commit() + self.assertEqual(msg, 'Comment added') + + # add pull-requests + req = pagure.lib.new_pull_request( + session=self.session, + repo_from=repo, + branch_from='feature', + repo_to=repo, + branch_to='master', + title='test pull-request', + user='pingou', + requestfolder=os.path.join(self.path, 'requests'), + ) + self.session.commit() + self.assertEqual(req.id, 3) + self.assertEqual(req.title, 'test pull-request') + + req = pagure.lib.new_pull_request( + session=self.session, + repo_from=repo, + branch_from='feature2', + repo_to=repo, + branch_to='master', + title='test pull-request', + user='pingou', + requestfolder=os.path.join(self.path, 'requests'), + ) + self.session.commit() + self.assertEqual(req.id, 4) + self.assertEqual(req.title, 'test pull-request') + + # Add comment on a pull-request + request = pagure.lib.search_pull_requests( + self.session, requestid=3) + + msg = pagure.lib.add_pull_request_comment( + session=self.session, + request=request, + commit='commithash', + tree_id=None, + filename='file', + row=None, + comment='This is awesome, I got to remember it!', + user='foo', + requestfolder=None, + ) + self.assertEqual(msg, 'Comment added') # Check before deleting the project output = self.app.get('/') @@ -2849,87 +2850,89 @@ index 0000000..fb7093d 'Forks 0', output.data) - # add issues - repo = pagure.get_authorized_project(self.session, 'test') - msg = pagure.lib.new_issue( - session=self.session, - repo=repo, - title='Test issue', - content='We should work on this', - user='pingou', - ticketfolder=os.path.join(self.path, 'tickets') - ) - self.session.commit() - self.assertEqual(msg.title, 'Test issue') - - msg = pagure.lib.new_issue( - session=self.session, - repo=repo, - title='Test issue #2', - content='We should work on this, really', - user='pingou', - ticketfolder=os.path.join(self.path, 'tickets') - ) - self.session.commit() - self.assertEqual(msg.title, 'Test issue #2') - - # Add a comment to an issue - issue = pagure.lib.search_issues(self.session, repo, issueid=1) - msg = pagure.lib.add_issue_comment( - session=self.session, - issue=issue, - comment='Hey look a comment!', - user='foo', - ticketfolder=None - ) - self.session.commit() - self.assertEqual(msg, 'Comment added') - - # add pull-requests - req = pagure.lib.new_pull_request( - session=self.session, - repo_from=repo, - branch_from='feature', - repo_to=repo, - branch_to='master', - title='test pull-request', - user='pingou', - requestfolder=os.path.join(self.path, 'requests'), - ) - self.session.commit() - self.assertEqual(req.id, 3) - self.assertEqual(req.title, 'test pull-request') - - req = pagure.lib.new_pull_request( - session=self.session, - repo_from=repo, - branch_from='feature2', - repo_to=repo, - branch_to='master', - title='test pull-request', - user='pingou', - requestfolder=os.path.join(self.path, 'requests'), - ) - self.session.commit() - self.assertEqual(req.id, 4) - self.assertEqual(req.title, 'test pull-request') - - # Add comment on a pull-request - request = pagure.lib.search_pull_requests( - self.session, requestid=3) - - msg = pagure.lib.add_pull_request_comment( - session=self.session, - request=request, - commit='commithash', - tree_id=None, - filename='file', - row=None, - comment='This is awesome, I got to remember it!', - user='foo', - requestfolder=None, - ) - self.assertEqual(msg, 'Comment added') + # This part of the code calls lib stuff directly + with patch('pagure.ensure_lock'): + # add issues + repo = pagure.get_authorized_project(self.session, 'test') + msg = pagure.lib.new_issue( + session=self.session, + repo=repo, + title='Test issue', + content='We should work on this', + user='pingou', + ticketfolder=os.path.join(self.path, 'tickets') + ) + self.session.commit() + self.assertEqual(msg.title, 'Test issue') + + msg = pagure.lib.new_issue( + session=self.session, + repo=repo, + title='Test issue #2', + content='We should work on this, really', + user='pingou', + ticketfolder=os.path.join(self.path, 'tickets') + ) + self.session.commit() + self.assertEqual(msg.title, 'Test issue #2') + + # Add a comment to an issue + issue = pagure.lib.search_issues(self.session, repo, issueid=1) + msg = pagure.lib.add_issue_comment( + session=self.session, + issue=issue, + comment='Hey look a comment!', + user='foo', + ticketfolder=None + ) + self.session.commit() + self.assertEqual(msg, 'Comment added') + + # add pull-requests + req = pagure.lib.new_pull_request( + session=self.session, + repo_from=repo, + branch_from='feature', + repo_to=repo, + branch_to='master', + title='test pull-request', + user='pingou', + requestfolder=os.path.join(self.path, 'requests'), + ) + self.session.commit() + self.assertEqual(req.id, 3) + self.assertEqual(req.title, 'test pull-request') + + req = pagure.lib.new_pull_request( + session=self.session, + repo_from=repo, + branch_from='feature2', + repo_to=repo, + branch_to='master', + title='test pull-request', + user='pingou', + requestfolder=os.path.join(self.path, 'requests'), + ) + self.session.commit() + self.assertEqual(req.id, 4) + self.assertEqual(req.title, 'test pull-request') + + # Add comment on a pull-request + request = pagure.lib.search_pull_requests( + self.session, requestid=3) + + msg = pagure.lib.add_pull_request_comment( + session=self.session, + request=request, + commit='commithash', + tree_id=None, + filename='file', + row=None, + comment='This is awesome, I got to remember it!', + user='foo', + requestfolder=None, + ) + self.assertEqual(msg, 'Comment added') # Check before deleting the project output = self.app.get('/') @@ -3032,16 +3035,17 @@ index 0000000..fb7093d 'Forks 0', output.data) - # add user - repo = pagure.get_authorized_project(self.session, 'test') - msg = pagure.lib.add_user_to_project( - session=self.session, - project=repo, - new_user='foo', - user='pingou', - ) - self.session.commit() - self.assertEqual(msg, 'User added') + with patch('pagure.ensure_lock'): + # add user + repo = pagure.get_authorized_project(self.session, 'test') + msg = pagure.lib.add_user_to_project( + session=self.session, + project=repo, + new_user='foo', + user='pingou', + ) + self.session.commit() + self.assertEqual(msg, 'User added') # Check before deleting the project output = self.app.get('/') @@ -3112,34 +3116,35 @@ index 0000000..fb7093d 'Forks 0', output.data) - # Create group - msg = pagure.lib.add_group( - self.session, - group_name='foo', - display_name='foo group', - description=None, - group_type='bar', - user='pingou', - is_admin=False, - blacklist=[], - ) - self.session.commit() - self.assertEqual(msg, 'User `pingou` added to the group `foo`.') - - # Add group to the project - repo = pagure.get_authorized_project(self.session, 'test') - msg = pagure.lib.add_group_to_project( - session=self.session, - project=repo, - new_group='foo', - user='pingou', - ) - self.session.commit() - self.assertEqual(msg, 'Group added') - - # check if group where we expect it - repo = pagure.get_authorized_project(self.session, 'test') - self.assertEqual(len(repo.projects_groups), 1) + with patch('pagure.ensure_lock'): + # Create group + msg = pagure.lib.add_group( + self.session, + group_name='foo', + display_name='foo group', + description=None, + group_type='bar', + user='pingou', + is_admin=False, + blacklist=[], + ) + self.session.commit() + self.assertEqual(msg, 'User `pingou` added to the group `foo`.') + + # Add group to the project + repo = pagure.get_authorized_project(self.session, 'test') + msg = pagure.lib.add_group_to_project( + session=self.session, + project=repo, + new_group='foo', + user='pingou', + ) + self.session.commit() + self.assertEqual(msg, 'Group added') + + # check if group where we expect it + repo = pagure.get_authorized_project(self.session, 'test') + self.assertEqual(len(repo.projects_groups), 1) # Check before deleting the project output = self.app.get('/') @@ -3206,30 +3211,31 @@ index 0000000..fb7093d 'Forks 0', output.data) - # Create the issue - repo = pagure.get_authorized_project(self.session, 'test') - msg = pagure.lib.new_issue( - session=self.session, - repo=repo, - title='Test issue', - content='We should work on this', - user='pingou', - ticketfolder=os.path.join(self.path, 'tickets') - ) - self.session.commit() - self.assertEqual(msg.title, 'Test issue') - - # Add a tag to the issue - repo = pagure.get_authorized_project(self.session, 'test') - issue = pagure.lib.search_issues(self.session, repo, issueid=1) - msg = pagure.lib.add_tag_obj( - session=self.session, - obj=issue, - tags='tag1', - user='pingou', - ticketfolder=None) - self.session.commit() - self.assertEqual(msg, 'Issue tagged with: tag1') + with patch('pagure.ensure_lock'): + # Create the issue + repo = pagure.get_authorized_project(self.session, 'test') + msg = pagure.lib.new_issue( + session=self.session, + repo=repo, + title='Test issue', + content='We should work on this', + user='pingou', + ticketfolder=os.path.join(self.path, 'tickets') + ) + self.session.commit() + self.assertEqual(msg.title, 'Test issue') + + # Add a tag to the issue + repo = pagure.get_authorized_project(self.session, 'test') + issue = pagure.lib.search_issues(self.session, repo, issueid=1) + msg = pagure.lib.add_tag_obj( + session=self.session, + obj=issue, + tags='tag1', + user='pingou', + ticketfolder=None) + self.session.commit() + self.assertEqual(msg, 'Issue tagged with: tag1') # Check before deleting the project output = self.app.get('/') diff --git a/tests/test_pagure_lib_git.py b/tests/test_pagure_lib_git.py index 604aff3..f87659c 100644 --- a/tests/test_pagure_lib_git.py +++ b/tests/test_pagure_lib_git.py @@ -1298,8 +1298,9 @@ index 9f44358..2a552bb 100644 patch = '\n'.join(npatch) self.assertEqual(patch, exp) + @patch('pagure.ensure_lock') @patch('pagure.lib.notify.send_email') - def test_update_git(self, email_f): + def test_update_git(self, elock, email_f): """ Test the update_git of pagure.lib.git. """ email_f.return_value = True @@ -1517,7 +1518,8 @@ index 458821a..77674a8 #print patch self.assertEqual(patch, exp) - def test_clean_git(self): + @patch('pagure.ensure_lock') + def test_clean_git(self, elock): """ Test the clean_git method of pagure.lib.git. """ pagure.lib.git.clean_git(None, None, None) @@ -1548,8 +1550,9 @@ index 458821a..77674a8 files = [entry.name for entry in commit.tree] self.assertEqual(files, []) + @patch('pagure.ensure_lock') @patch('pagure.lib.notify.send_email') - def test_update_git_requests(self, email_f): + def test_update_git_requests(self, elock, email_f): """ Test the update_git of pagure.lib.git for pull-requests. """ email_f.return_value = True @@ -2390,7 +2393,8 @@ index 0000000..60f7480 'test request to namespaced repo' ) - def test_read_git_lines(self): + @patch('pagure.ensure_lock') + def test_read_git_lines(self, elock): """ Test the read_git_lines method of pagure.lib.git. """ self.test_update_git() @@ -2413,7 +2417,8 @@ index 0000000..60f7480 output[0].endswith(": Test issue'\n") ) - def test_get_revs_between(self): + @patch('pagure.ensure_lock') + def test_get_revs_between(self, elock): """ Test the get_revs_between method of pagure.lib.git. """ self.test_update_git() @@ -2480,7 +2485,8 @@ index 0000000..60f7480 '0', branch_commit.oid.hex, gitrepo, 'refs/heads/feature') self.assertEqual(output4, [branch_commit.oid.hex]) - def test_get_author(self): + @patch('pagure.ensure_lock') + def test_get_author(self, elock): """ Test the get_author method of pagure.lib.git. """ self.test_update_git() From eabc8e87fb89e490474aafc854ea3e00f54f0efa Mon Sep 17 00:00:00 2001 From: Patrick Uiterwijk Date: May 16 2017 12:18:36 +0000 Subject: [PATCH 5/5] Add get_authorized_api_project Signed-off-by: Patrick Uiterwijk --- diff --git a/pagure/api/__init__.py b/pagure/api/__init__.py index cd83a24..f94ef66 100644 --- a/pagure/api/__init__.py +++ b/pagure/api/__init__.py @@ -87,6 +87,16 @@ class APIERROR(enum.Enum): ENOGROUP = 'Group not found' +def get_authorized_api_project(SESSION, repo, user=None, namespace=None, + with_lock=False): + ''' Helper function to get an authorized_project with optional lock. ''' + repo = pagure.get_authorized_project( + SESSION, repo, user=user, namespace=namespace, with_lock=with_lock) + flask.g.repo_locked = with_lock + flask.g.repo = repo + return repo + + def check_api_acls(acls, optional=False): ''' Checks if the user provided an API token with its request and if this token allows the user to access the endpoint desired. @@ -347,7 +357,7 @@ def api_project_tags(repo, username=None): if pattern is not None and not pattern.endswith('*'): pattern += '*' - project_obj = pagure.get_authorized_project(SESSION, repo, username) + project_obj = get_authorized_api_project(SESSION, repo, username) if not project_obj: output = {'output': 'notok', 'error': 'Project not found'} jsonout = flask.jsonify(output) diff --git a/pagure/api/ci/jenkins.py b/pagure/api/ci/jenkins.py index 1309bf6..88f4210 100644 --- a/pagure/api/ci/jenkins.py +++ b/pagure/api/ci/jenkins.py @@ -45,7 +45,9 @@ def jenkins_ci_notification( """ project = pagure.lib._get_project( - SESSION, repo, user=username, namespace=namespace) + SESSION, repo, user=username, namespace=namespace, with_lock=True) + flask.g.repo_locked = True + flask.g.repo = project if not project: raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOPROJECT) diff --git a/pagure/api/fork.py b/pagure/api/fork.py index 1a2efd0..7457fcb 100644 --- a/pagure/api/fork.py +++ b/pagure/api/fork.py @@ -16,7 +16,8 @@ import pagure import pagure.exceptions import pagure.lib from pagure import APP, SESSION, is_repo_committer -from pagure.api import API, api_method, api_login_required, APIERROR +from pagure.api import (API, api_method, api_login_required, APIERROR, + get_authorized_api_project) @API.route('//pull-requests') @@ -123,7 +124,7 @@ def api_pull_request_views(repo, username=None, namespace=None): """ - repo = pagure.get_authorized_project( + repo = get_authorized_api_project( SESSION, repo, user=username, namespace=namespace) if repo is None: @@ -249,7 +250,7 @@ def api_pull_request_view(repo, requestid, username=None, namespace=None): """ - repo = pagure.get_authorized_project( + repo = get_authorized_api_project( SESSION, repo, user=username, namespace=namespace) if repo is None: @@ -308,7 +309,7 @@ def api_pull_request_merge(repo, requestid, username=None, namespace=None): """ # noqa output = {} - repo = pagure.get_authorized_project( + repo = get_authorized_api_project( SESSION, repo, user=username, namespace=namespace) if repo is None: @@ -395,7 +396,7 @@ def api_pull_request_close(repo, requestid, username=None, namespace=None): """ # noqa output = {} - repo = pagure.get_authorized_project( + repo = get_authorized_api_project( SESSION, repo, user=username, namespace=namespace) if repo is None: @@ -498,7 +499,7 @@ def api_pull_request_add_comment( } """ # noqa - repo = pagure.get_authorized_project( + repo = get_authorized_api_project( SESSION, repo, user=username, namespace=namespace) output = {} @@ -642,7 +643,7 @@ def api_pull_request_add_flag(repo, requestid, username=None, namespace=None): } """ # noqa - repo = pagure.get_authorized_project( + repo = get_authorized_api_project( SESSION, repo, user=username, namespace=namespace) output = {} diff --git a/pagure/api/issue.py b/pagure/api/issue.py index 5598e23..923525c 100644 --- a/pagure/api/issue.py +++ b/pagure/api/issue.py @@ -22,7 +22,8 @@ from pagure import ( urlpattern, is_repo_user ) from pagure.api import ( - API, api_method, api_login_required, api_login_optional, APIERROR + API, api_method, api_login_required, api_login_optional, APIERROR, + get_authorized_api_project ) @@ -35,8 +36,8 @@ def _get_repo(repo_name, username=None, namespace=None): is disabled :return: repository name """ - repo = pagure.get_authorized_project( - SESSION, repo_name, user=username, namespace=namespace) + repo = get_authorized_api_project( + SESSION, repo_name, user=username, namespace=namespace, with_lock=True) if repo is None: raise pagure.exceptions.APIError( diff --git a/pagure/api/project.py b/pagure/api/project.py index 1839828..c8e7fd5 100644 --- a/pagure/api/project.py +++ b/pagure/api/project.py @@ -17,7 +17,8 @@ import pagure.exceptions import pagure.lib import pagure.lib.git from pagure import SESSION, APP, authenticated -from pagure.api import API, api_method, APIERROR, api_login_required +from pagure.api import (API, api_method, APIERROR, api_login_required, + get_authorized_api_project) @API.route('//git/tags') @@ -52,7 +53,7 @@ def api_git_tags(repo, username=None, namespace=None): } """ - repo = pagure.get_authorized_project( + repo = get_authorized_api_project( SESSION, repo, user=username, namespace=namespace) if repo is None: raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOPROJECT) @@ -102,7 +103,7 @@ def api_project_watchers(repo, username=None, namespace=None): } } ''' - repo = pagure.get_authorized_project( + repo = get_authorized_api_project( SESSION, repo, user=username, namespace=namespace) if repo is None: raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOPROJECT) @@ -175,7 +176,7 @@ def api_git_branches(repo, username=None, namespace=None): } ''' - repo = pagure.get_authorized_project( + repo = get_authorized_api_project( SESSION, repo, user=username, namespace=namespace) if repo is None: raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOPROJECT) @@ -387,7 +388,7 @@ def api_project(repo, username=None, namespace=None): } """ - repo = pagure.get_authorized_project( + repo = get_authorized_api_project( SESSION, repo, user=username, namespace=namespace) if repo is None: @@ -576,7 +577,7 @@ def api_fork_project(): username = form.username.data or None namespace = form.namespace.data.strip() or None - repo = pagure.get_authorized_project( + repo = get_authorized_api_project( SESSION, repo, user=username, namespace=namespace) if repo is None: raise pagure.exceptions.APIError(