kallithea Changeset - af7b367f6b5a

Changeset - af7b367f6b5a

Parent rev.

Child rev.

[Not reviewed]

default

0 1 1

Thomas De Schampheleire - 5 years ago 2020-06-15 12:37:55
thomas.de_schampheleire@nokia.com

db: introduce constraint ensuring no duplicate (reviewer, pullrequest) combinations

A reviewer should only be added once to a review.

Previously, this was not ensured by the database itself, although that the
controller would try to not add duplicate reviewers. But there was no hard
guarantee: e.g. simultaneous adding of the same reviewer to the same review
by a review owner and admin, a framework bug that sends the same request
twice, ... could still trigger duplicate addition. Additionally, code
changes (e.g. a new API) could introduce bugs at the controller level.

Existing production databases were found to contain such duplicate entries.
Nevertheless, as the code displaying reviewers in a pull request filtered
out duplicates, this never showed in the UI, and never was a 'real' problem.

Add a UniqueConstraint in the database to prevent such entries, with a
database migration step that will first find and remove existing duplicates.

2 files changed with 74 insertions and 0 deletions:

kallithea/alembic/versions/f62826179f39_add_unique_constraint_on_.py

kallithea/model/db.py

0 comments (0 inline, 0 general)

kallithea/alembic/versions/f62826179f39_add_unique_constraint_on_.py

➞

Show inline comments

@@ new file 100644 @@
 # This program is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
 # the Free Software Foundation, either version 3 of the License, or
 # (at your option) any later version.
+#
 # This program is distributed in the hope that it will be useful,
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 # GNU General Public License for more details.
+#
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 """add unique constraint on PullRequestReviewer
 Revision ID: f62826179f39
 Revises: a0a1bf09c143
 Create Date: 2020-06-15 12:30:37.420321
 """
 # The following opaque hexadecimal identifiers ("revisions") are used
 # by Alembic to track this migration script and its relations to others.
 revision = 'f62826179f39'
 down_revision = 'a0a1bf09c143'
 branch_labels = None
 depends_on = None
 import sqlalchemy as sa
 from alembic import op
 from kallithea.model.db import PullRequestReviewer
 def upgrade():
     session = sa.orm.session.Session(bind=op.get_bind())
     # there may be existing duplicates in the database, remove them first
     seen = set()
     # duplicate_values contains one copy of each duplicated pair
     duplicate_values = (
         session
         .query(PullRequestReviewer.pull_request_id, PullRequestReviewer.user_id)
         .group_by(PullRequestReviewer.pull_request_id, PullRequestReviewer.user_id)
         .having(sa.func.count(PullRequestReviewer.pull_request_reviewers_id) > 1)
+    )
     for pull_request_id, user_id in duplicate_values:
         # duplicate_occurrences contains all db records of the duplicate_value
         # currently being processed
         duplicate_occurrences = (
             session
             .query(PullRequestReviewer)
             .filter(PullRequestReviewer.pull_request_id == pull_request_id)
             .filter(PullRequestReviewer.user_id == user_id)
+        )
         for prr in duplicate_occurrences:
             if (pull_request_id, user_id) in seen:
                 session.delete(prr)
             else:
                 seen.add((pull_request_id, user_id))
     session.commit()
     # after deleting all duplicates, add the unique constraint
     with op.batch_alter_table('pull_request_reviewers', schema=None) as batch_op:
         batch_op.create_unique_constraint(batch_op.f('uq_pull_request_reviewers_pull_request_id'), ['pull_request_id', 'user_id'])
 def downgrade():
     with op.batch_alter_table('pull_request_reviewers', schema=None) as batch_op:
         batch_op.drop_constraint(batch_op.f('uq_pull_request_reviewers_pull_request_id'), type_='unique')

kallithea/model/db.py

➞

Show inline comments

@@ @@ -2118,96 +2118,97 @@ class PullRequest(Base, BaseDbModel): @@
     def nice_id(self):
         '''Return the id of this pull request, nicely formatted for displaying'''
         return self.make_nice_id(self.pull_request_id)
     def get_api_data(self):
         return self.__json__()
     def __json__(self):
         clone_uri_tmpl = kallithea.CONFIG.get('clone_uri_tmpl') or Repository.DEFAULT_CLONE_URI
         return dict(
             pull_request_id=self.pull_request_id,
             url=self.url(),
             reviewers=self.reviewers,
             revisions=self.revisions,
             owner=self.owner.username,
             title=self.title,
             description=self.description,
             org_repo_url=self.org_repo.clone_url(clone_uri_tmpl=clone_uri_tmpl),
             org_ref_parts=self.org_ref_parts,
             other_ref_parts=self.other_ref_parts,
             status=self.status,
             comments=self.comments,
             statuses=self.statuses,
             created_on=self.created_on.replace(microsecond=0),
             updated_on=self.updated_on.replace(microsecond=0),
+        )
     def url(self, **kwargs):
         canonical = kwargs.pop('canonical', None)
         import kallithea.lib.helpers as h
         b = self.org_ref_parts[1]
         if b != self.other_ref_parts[1]:
             s = '/_/' + b
         else:
             s = '/_/' + self.title
         kwargs['extra'] = urlreadable(s)
         if canonical:
             return h.canonical_url('pullrequest_show', repo_name=self.other_repo.repo_name,
                                    pull_request_id=self.pull_request_id, **kwargs)
         return h.url('pullrequest_show', repo_name=self.other_repo.repo_name,
                      pull_request_id=self.pull_request_id, **kwargs)
 class PullRequestReviewer(Base, BaseDbModel):
     __tablename__ = 'pull_request_reviewers'
     __table_args__ = (
         Index('pull_request_reviewers_user_id_idx', 'user_id'),
         UniqueConstraint('pull_request_id', 'user_id'),
         _table_args_default_dict,
+    )
     def __init__(self, user=None, pull_request=None):
         self.user = user
         self.pull_request = pull_request
     pull_request_reviewers_id = Column('pull_requests_reviewers_id', Integer(), primary_key=True)
     pull_request_id = Column(Integer(), ForeignKey('pull_requests.pull_request_id'), nullable=False)
     user_id = Column(Integer(), ForeignKey('users.user_id'), nullable=False)
     user = relationship('User')
     pull_request = relationship('PullRequest')
     def __json__(self):
         return dict(
             username=self.user.username if self.user else None,
+        )
 class Notification(object):
     __tablename__ = 'notifications'
 class UserNotification(object):
     __tablename__ = 'user_to_notification'
 class Gist(Base, BaseDbModel):
     __tablename__ = 'gists'
     __table_args__ = (
         Index('g_gist_access_id_idx', 'gist_access_id'),
         Index('g_created_on_idx', 'created_on'),
         _table_args_default_dict,
+    )
     GIST_PUBLIC = 'public'
     GIST_PRIVATE = 'private'
     DEFAULT_FILENAME = 'gistfile1.txt'
     gist_id = Column(Integer(), primary_key=True)
     gist_access_id = Column(Unicode(250), nullable=False)
     gist_description = Column(UnicodeText(), nullable=False)
     owner_id = Column('user_id', Integer(), ForeignKey('users.user_id'), nullable=False)
     gist_expires = Column(Float(53), nullable=False)
     gist_type = Column(Unicode(128), nullable=False)
     created_on = Column(DateTime(timezone=False), nullable=False, default=datetime.datetime.now)
     modified_at = Column(DateTime(timezone=False), nullable=False, default=datetime.datetime.now)

0 comments (0 inline, 0 general)