Cap buildroot overrides notes to a maximum of 2k characters and conve…

…rt the database field to UnicodeText

Signed-off-by: Mattia Verga <[email protected]>

bodhi/server/migrations/versions/1c97477e38ee_convert_br_override_notes_to_unicodetext.py

@@ -0,0 +1,41 @@
+# Copyright (c) 2020 Mattia Verga
+#
+# This file is part of Bodhi.
+#
+# 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 2
+# 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, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+"""
+Convert buildroot override notes to unicodetext.
+
+Revision ID: 1c97477e38ee
+Revises: 325954bac9f7
+Create Date: 2020-06-09 13:03:03.489398
+"""
+from alembic import op
+import sqlalchemy as sa
+
+
+# revision identifiers, used by Alembic.
+revision = '1c97477e38ee'
+down_revision = 'f50dc199039c'
+
+
+def upgrade():
+    """Convert buildrootoverrides notes column to UnicodeText."""
+    op.alter_column("buildroot_overrides", "notes", type_=sa.UnicodeText)
+
+
+def downgrade():
+    """Convert back buildrootoverrides notes column to Unicode."""
+    op.alter_column("buildroot_overrides", "notes", type_=sa.Unicode)

bodhi/server/models.py

@@ -4718,7 +4718,7 @@ class BuildrootOverride(Base):
     __include_extras__ = ('nvr',)
     __get_by__ = ('build_id',)
 
-    notes = Column(Unicode, nullable=False)
+    notes = Column(UnicodeText, nullable=False)
 
     submission_date = Column(DateTime, default=datetime.utcnow, nullable=False)
     expiration_date = Column(DateTime, nullable=False)

bodhi/server/services/overrides.py

@@ -33,6 +33,7 @@
 from bodhi.server.validators import (
     validate_override_builds,
     validate_expiration_date,
+    validate_override_notes,
     validate_packages,
     validate_releases,
     validate_username,
@@ -222,6 +223,7 @@ def query_overrides(request):
                     colander_body_validator,
                     validate_override_builds,
                     validate_expiration_date,
+                    validate_override_notes,
                 ))
 @overrides.post(schema=bodhi.server.schemas.SaveOverrideSchema,
                 permission='edit',
@@ -231,6 +233,7 @@ def query_overrides(request):
                     colander_body_validator,
                     validate_override_builds,
                     validate_expiration_date,
+                    validate_override_notes,
                 ))
 def save_override(request):
     """
@@ -269,20 +272,20 @@ def save_override(request):
                         data['expiration_date'] = max(existing_override.expiration_date,
                                                       data['expiration_date'])
 
-                    data['notes'] = """{0}
-
-_@{1} ({2})_
-
+                    new_notes = f"""{data['notes']}
 _____________
-{3}""".format(existing_override.notes, existing_override.submitter.name,
-                        existing_override.submission_date.strftime("%b %d, %Y"), data['notes'])
+_@{existing_override.submitter.name} ({existing_override.submission_date.strftime('%b %d, %Y')})_
+{existing_override.notes}"""
+                    # Truncate notes at 2000 chars
+                    if len(new_notes) > 2000:
+                        new_notes = new_notes[:1972] + '(...)\n___Notes truncated___'
 
                     overrides.append(BuildrootOverride.edit(
                         request,
                         edited=build,
                         submitter=submitter,
                         submission_date=datetime.now(),
-                        notes=data['notes'],
+                        notes=new_notes,
                         expiration_date=data['expiration_date'],
                         expired=None,
                     ))

bodhi/server/templates/override.html

@@ -137,7 +137,7 @@ <h5 class="font-weight-bold d-flex align-items-center">
                 Notes
             </h5>
           <div class="form-group">
-            <textarea class="form-control" id="notes" name="notes" rows="6" placeholder="Buildroot override notes go here.  They're really *really* useful, so feel free to write as you please." required="required">${override.notes if override is not UNDEFINED else ''}</textarea>
+            <textarea class="form-control" id="notes" name="notes" rows="6" placeholder="Buildroot override notes go here.  They're really *really* useful, so feel free to write as you please." required="required" maxlength=2000>${override.notes if override is not UNDEFINED else ''}</textarea>
           </div>
         </div>
 

bodhi/server/validators.py

@@ -1206,6 +1206,28 @@ def validate_expiration_date(request, **kwargs):
     request.validated['expiration_date'] = expiration_date
 
 
+@postschema_validator
+def validate_override_notes(request, **kwargs):
+    """
+    Ensure the override notes is less than 500 chars.
+
+    Args:
+        request (pyramid.request.Request): The current request.
+        kwargs (dict): The kwargs of the related service definition. Unused.
+    """
+    notes = request.validated.get('notes')
+
+    if notes is None:
+        return
+
+    if len(notes) > 2000:
+        request.errors.add('body', 'notes',
+                           'Notes may not contain more than 2000 chars')
+        return
+
+    request.validated['notes'] = notes
+
+
 def _get_valid_requirements(request, requirements):
     """
     Return a list of valid testcases from taskotron.

bodhi/tests/server/services/test_overrides.py

@@ -340,6 +340,7 @@ def test_create_override_for_build_with_test_gating_status_failed(self):
             "Cannot create a buildroot override if build's test gating status is failed."
 
     def test_create_duplicate_override(self):
+        """When creating a duplicate override, old notes are appended."""
         release = Release.get('F17')
         package = RpmPackage(name='not-bodhi')
         self.db.add(package)
@@ -366,14 +367,43 @@ def test_create_duplicate_override(self):
         data['notes'] = 'new blah blah'
         res = self.app.post('/overrides/', data)
         o = res.json_body
-        new_notes = """blah blah blah
-
-_@guest ({})_
-
+        new_notes = f"""new blah blah
 _____________
-new blah blah""".format(datetime.utcnow().strftime("%b %d, %Y"))
+_@guest ({datetime.utcnow().strftime('%b %d, %Y')})_
+blah blah blah"""
         assert o['notes'] == new_notes
 
+    def test_create_duplicate_override_notes_too_long(self):
+        """When notes are too long, truncate the older."""
+        release = Release.get('F17')
+        package = RpmPackage(name='not-bodhi')
+        self.db.add(package)
+        build = RpmBuild(nvr='not-bodhi-2.0-2.fc17', package=package, release=release)
+        self.db.add(build)
+        self.db.flush()
+
+        expiration_date = datetime.utcnow() + timedelta(days=1)
+
+        data = {'nvr': build.nvr, 'notes': 'blah' * 500,
+                'expiration_date': expiration_date,
+                'csrf_token': self.get_csrf_token()}
+
+        with fml_testing.mock_sends(override_schemas.BuildrootOverrideTagV1):
+            res = self.app.post('/overrides/', data)
+
+        o = res.json_body
+        assert o['build_id'] == build.id
+        assert o['notes'] == 'blah' * 500
+        assert o['expiration_date'] == expiration_date.strftime("%Y-%m-%d %H:%M:%S")
+        assert o['expired_date'] is None
+
+        # Submit it again
+        data['notes'] = 'new blah blah'
+        res = self.app.post('/overrides/', data)
+        o = res.json_body
+        assert o['notes'].endswith('(...)\n___Notes truncated___')
+        assert len(o['notes']) <= 2000
+
     def test_create_override_multiple_nvr(self):
         release = Release.get('F17')
         package = RpmPackage(name='not-bodhi')

bodhi/tests/server/test_validators.py

@@ -382,6 +382,35 @@ def test_past(self):
         assert request.errors.status == exceptions.HTTPBadRequest.code
 
 
+class TestValidateOverrideNotes(BasePyTestCase):
+    """Test the validate_override_notes() function."""
+
+    def test_none(self):
+        """Empty notes should be OK, since we will populate with a default text."""
+        request = mock.Mock()
+        request.errors = Errors()
+        request.validated = {'notes': None}
+
+        validators.validate_override_notes(request)
+
+        assert not len(request.errors)
+
+    def test_length_above_range(self):
+        """We don't allow too verbose notes."""
+        request = mock.Mock()
+        request.errors = Errors()
+        request.validated = {
+            'notes': 'n' * 2001}
+
+        validators.validate_override_notes(request)
+
+        assert request.errors == [
+            {'location': 'body', 'name': 'notes',
+             'description': 'Notes may not contain more than 2000 chars'}
+        ]
+        assert request.errors.status == exceptions.HTTPBadRequest.code
+
+
 class TestValidateOverrideBuild(BasePyTestCase):
     """Test the validate_override_build() function."""
 

news/4044.bug

@@ -0,0 +1 @@
+Cap buildroot overrides notes to a maximum of 2k characters and convert the database field to UnicodeText

news/summary.migration

@@ -1,3 +1,3 @@
-Migrate relationship between TestCase and Package to TestCase and Build. The migration script will take care of migrate existing data to the new relation.
-The user_id column in comments table has been set to be not nullable.
-
+- Migrate relationship between TestCase and Package to TestCase and Build. The migration script will take care of migrate existing data to the new relation.
+- The user_id column in comments table has been set to be not nullable.
+- The notes column in buildroot_overrides table has been converted to UnicodeText (from Unicode).