koji

Clone

#4405 Add check if int/str options are empty in listBuilds
Closed 10 days ago by mikem. Opened 2 months ago by jcupova.
jcupova/koji issue-4402  into  master

file modified
+40 -7 
@@ -12595,51 +12595,81 @@ 
 

                   ]
 

          clauses = []
 

          if packageID is not None:
 

+             if packageID == '':
 

+                 raise koji.ParameterError('packageID option is empty')
 

+             # packageID could be ID or name
 

              packageID = get_package_id(packageID)
 

              if not packageID:
 

                  return []
 

              clauses.append('package.id = %(packageID)i')
 

          if userID is not None:
 

+             if userID == '':
 

+                 raise koji.ParameterError('userID option is empty')
 

+             # userID could be ID or name
 

              userinfo = get_user(userID)
 

              if not userinfo:
 

                  return []
 

              userID = userinfo['id']
 

              clauses.append('users.id = %(userID)i')
 

          if volumeID is not None:
 

+             if volumeID == '':
 

+                 raise koji.ParameterError('volumeID option is empty')
 

              clauses.append('volume.id = %(volumeID)i')
 

          if taskID is not None:
 

+             if taskID == '':
 

+                 raise koji.ParameterError('taskID option is empty')
 

              if taskID == -1:
 

                  clauses.append('build.task_id IS NOT NULL')
 

              else:
 

                  clauses.append('build.task_id = %(taskID)i')
 

          if source is not None:
 

+             if source == '':
 

+                 raise koji.ParameterError('source option is empty')
 

              source = self._prepareSearchTerms(source, 'glob')
 

              clauses.append('build.source ilike %(source)s')
 

-         if prefix:
 

+         if prefix is not None:
 

+             if prefix == '':
 

+                 raise koji.ParameterError('prefix option is empty')
 

              clauses.append("package.name ilike %(prefix)s || '%%'")
 

-         if pattern:
 

+         if pattern is not None:
 

+             if pattern == '':
 

+                 raise koji.ParameterError('pattern option is empty')
 

              pattern = self._prepareSearchTerms(pattern, 'glob')
 

              clauses.append("package.name || '-' || build.version || '-' || build.release"
 

                             " ilike %(pattern)s")
 

          if state is not None:
 

+             if state == '':
 

+                 raise koji.ParameterError('state option is empty')
 

+             clauses.append('build.state = %(state)i')
 

+         if createdBefore is not None:
 

+             if createdBefore == '':
 

+                 raise koji.ParameterError('createdBefore option is empty')
 

              clauses.append('build.state = %(state)i')
 

-         if createdBefore:
 

              if not isinstance(createdBefore, str):
 

                  createdBefore = convert_timestamp(createdBefore)
 

              clauses.append('events.time < %(createdBefore)s')
 

-         if createdAfter:
 

+         if createdAfter is not None:
 

+             if createdAfter == '':
 

+                 raise koji.ParameterError('createdAfter option is empty')
 

              if not isinstance(createdAfter, str):
 

                  createdAfter = convert_timestamp(createdAfter)
 

              clauses.append('events.time > %(createdAfter)s')
 

-         if completeBefore:
 

+         if completeBefore is not None:
 

+             if completeBefore == '':
 

+                 raise koji.ParameterError('completeBefore option is empty')
 

              if not isinstance(completeBefore, str):
 

                  completeBefore = convert_timestamp(completeBefore)
 

              clauses.append('build.completion_time < %(completeBefore)s')
 

-         if completeAfter:
 

+         if completeAfter is not None:
 

+             if completeAfter == '':
 

+                 raise koji.ParameterError('completeAfter option is empty')
 

              if not isinstance(completeAfter, str):
 

                  completeAfter = convert_timestamp(completeAfter)
 

              clauses.append('build.completion_time > %(completeAfter)s')
 

-         if cgID:
 

+         if cgID is not None:
 

+             if cgID == '':
 

+                 raise koji.ParameterError('cgID option is empty')
 

+             # cgID could be ID or name
 

              cgID = lookup_name('content_generator', cgID, strict=False)
 

              if not cgID:
 

                  return []
 
@@ -12672,6 +12702,9 @@ 
 

              joins.append('image_builds ON build.id = image_builds.build_id')
 

              fields.append(('image_builds.build_id', 'build_id'))
 

          else:
 

+             if type == '':
 

+                 raise koji.ParameterError('type option is empty')
 

+             # type could be ID or name
 

              btype = lookup_name('btype', type, strict=False)
 

              if not btype:
 

                  raise koji.GenericError('unsupported build type: %s' % type)

file modified
+87 -2 
@@ -2,6 +2,7 @@ 
 

  

  from unittest import mock
 

  

+ import koji
 

  import kojihub
 

  

  QP = kojihub.QueryProcessor
 
@@ -18,7 +19,7 @@ 
 

  

      def setUp(self):
 

          # defaults
 

-         self.tables= ['build']
 

+         self.tables = ['build']
 

          # note: QueryProcessor reports these sorted by alias
 

          self.columns = [
 

              'build.id',
 
@@ -114,7 +115,7 @@ 
 

          self.get_user.return_value = None
 

          rv = self.exports.listBuilds(userID=user)
 

          self.assertEqual(rv, [])
 

-     

+ 

      def test_draft(self):
 

          package = 'test-package'
 

          package_id = 1
 
@@ -128,3 +129,87 @@ 
 

          self.assertEqual(qp.columns, self.columns)
 

          self.assertEqual(qp.clauses, ['draft IS TRUE'] + self.clauses)
 

          self.assertEqual(qp.joins, self.joins)
 

+ 

+     def test_packageID_is_empty(self):
 

+         with self.assertRaises(koji.ParameterError) as cm:
 

+             self.exports.listBuilds(packageID='')
 

+         self.assertEqual(cm.exception.args[0], 'packageID option is empty')
 

+         self.assertEqual(len(self.queries), 0)
 

+ 

+     def test_userID_is_empty(self):
 

+         with self.assertRaises(koji.ParameterError) as cm:
 

+             self.exports.listBuilds(userID='')
 

+         self.assertEqual(cm.exception.args[0], 'userID option is empty')
 

+         self.assertEqual(len(self.queries), 0)
 

+ 

+     def test_volumeID_is_empty(self):
 

+         with self.assertRaises(koji.ParameterError) as cm:
 

+             self.exports.listBuilds(volumeID='')
 

+         self.assertEqual(cm.exception.args[0], 'volumeID option is empty')
 

+         self.assertEqual(len(self.queries), 0)
 

+ 

+     def test_taskID_is_empty(self):
 

+         with self.assertRaises(koji.ParameterError) as cm:
 

+             self.exports.listBuilds(taskID='')
 

+         self.assertEqual(cm.exception.args[0], 'taskID option is empty')
 

+         self.assertEqual(len(self.queries), 0)
 

+ 

+     def test_source_is_empty(self):
 

+         with self.assertRaises(koji.ParameterError) as cm:
 

+             self.exports.listBuilds(source='')
 

+         self.assertEqual(cm.exception.args[0], 'source option is empty')
 

+         self.assertEqual(len(self.queries), 0)
 

+ 

+     def test_prefix_is_empty(self):
 

+         with self.assertRaises(koji.ParameterError) as cm:
 

+             self.exports.listBuilds(prefix='')
 

+         self.assertEqual(cm.exception.args[0], 'prefix option is empty')
 

+         self.assertEqual(len(self.queries), 0)
 

+ 

+     def test_pattern_is_empty(self):
 

+         with self.assertRaises(koji.ParameterError) as cm:
 

+             self.exports.listBuilds(pattern='')
 

+         self.assertEqual(cm.exception.args[0], 'pattern option is empty')
 

+         self.assertEqual(len(self.queries), 0)
 

+ 

+     def test_state_is_empty(self):
 

+         with self.assertRaises(koji.ParameterError) as cm:
 

+             self.exports.listBuilds(state='')
 

+         self.assertEqual(cm.exception.args[0], 'state option is empty')
 

+         self.assertEqual(len(self.queries), 0)
 

+ 

+     def test_createdBefore_is_empty(self):
 

+         with self.assertRaises(koji.ParameterError) as cm:
 

+             self.exports.listBuilds(createdBefore='')
 

+         self.assertEqual(cm.exception.args[0], 'createdBefore option is empty')
 

+         self.assertEqual(len(self.queries), 0)
 

+ 

+     def test_createdAfter_is_empty(self):
 

+         with self.assertRaises(koji.ParameterError) as cm:
 

+             self.exports.listBuilds(createdAfter='')
 

+         self.assertEqual(cm.exception.args[0], 'createdAfter option is empty')
 

+         self.assertEqual(len(self.queries), 0)
 

+ 

+     def test_completeBefore_is_empty(self):
 

+         with self.assertRaises(koji.ParameterError) as cm:
 

+             self.exports.listBuilds(completeBefore='')
 

+         self.assertEqual(cm.exception.args[0], 'completeBefore option is empty')
 

+         self.assertEqual(len(self.queries), 0)
 

+ 

+     def test_completeAfter_is_empty(self):
 

+         with self.assertRaises(koji.ParameterError) as cm:
 

+             self.exports.listBuilds(completeAfter='')
 

+         self.assertEqual(cm.exception.args[0], 'completeAfter option is empty')
 

+         self.assertEqual(len(self.queries), 0)
 

+ 

+     def test_cgID_is_empty(self):
 

+         with self.assertRaises(koji.ParameterError) as cm:
 

+             self.exports.listBuilds(cgID='')
 

+         self.assertEqual(cm.exception.args[0], 'cgID option is empty')
 

+         self.assertEqual(len(self.queries), 0)
 

+ 

+     def test_type_is_empty(self):
 

+         with self.assertRaises(koji.ParameterError) as cm:
 

+             self.exports.listBuilds(type='')
 

+         self.assertEqual(cm.exception.args[0], 'type option is empty')
 

+         self.assertEqual(len(self.queries), 0)

rebased onto fca44b5
a month ago

You've gone beyond the scope of issue #4402 here, which is only about the pattern parameter. While we could probably stand to improve validation for some of the other parameters, it should probably be more involved than a single special case for the empty string.

Most of these changes change a valid if questionable call to an invalid one, i.e. that add an error case that was not previously an error case. Changing the api like that requires careful thought and justification.

A number of these params are not meant to accept strings at all. In those cases simply checking for an empty string seems wrong. Those cases would be better served by using convert_value, as we do in numerous other calls.

Let's put a hold on this pending further discussion in #4402

Pull-Request has been closed by mikem
10 days ago
Metadata
None
No Tags
Changes Summary 2
+40 -7
file changed
kojihub/kojihub.py
+87 -2
file changed
tests/test_hub/test_list_builds.py
Powered by Pagure 5.14.1
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.