#2791 with_owners options for readPackageList and readTaggedBuilds
Merged 3 years ago by tkopecek. Opened 3 years ago by tkopecek.
tkopecek/koji issue2780  into  master

file modified
+18 -6
@@ -352,8 +352,12 @@ 

      if dsttag is None:

          print("No such tag: %s" % tag)

          sys.exit(1)

-     pkglist = dict([(p['package_name'], p['package_id'])

-                     for p in session.listPackages(tagID=dsttag['id'])])

+     try:

+         pkglist = session.listPackages(tagID=dsttag['id'], with_owners=False)

+     except koji.ParameterError:

+         # performance option added in 1.25

+         pkglist = session.listPackages(tagID=dsttag['id'])

+     pkglist = dict([(p['package_name'], p['package_id']) for p in pkglist])

      to_add = []

      for package in args[1:]:

          package_id = pkglist.get(package, None)
@@ -387,8 +391,12 @@ 

      dsttag = session.getTag(tag)

      if dsttag is None:

          error("No such tag: %s" % tag)

-     pkglist = dict([(p['package_name'], p['package_id'])

-                     for p in session.listPackages(tagID=dsttag['id'], inherited=True)])

+     try:

+         pkglist = session.listPackages(tagID=dsttag['id'], inherited=True, with_owners=False)

+     except koji.ParameterError:

+         # performance option added in 1.25

+         pkglist = session.listPackages(tagID=dsttag['id'], inherited=True)

+     pkglist = dict([(p['package_name'], p['package_id']) for p in pkglist])

      ret = 0

      for package in args[1:]:

          package_id = pkglist.get(package, None)
@@ -424,8 +432,12 @@ 

      dsttag = session.getTag(tag)

      if dsttag is None:

          error("No such tag: %s" % tag)

-     pkglist = dict([(p['package_name'], p['package_id'])

-                     for p in session.listPackages(tagID=dsttag['id'])])

+     try:

+         pkglist = session.listPackages(tagID=dsttag['id'], with_owners=False)

+     except koji.ParameterError:

+         # performance option added in 1.25

+         pkglist = session.listPackages(tagID=dsttag['id'])

+     pkglist = dict([(p['package_name'], p['package_id']) for p in pkglist])

      ret = 0

      for package in args[1:]:

          package_id = pkglist.get(package, None)

file modified
+99 -78
@@ -1069,7 +1069,7 @@ 

      # check pkg list existence

      tag = get_tag(taginfo, strict=True)

      pkg = lookup_package(pkginfo, strict=True)

-     if not readPackageList(tag['id'], pkgID=pkg['id'], inherit=True):

+     if not readPackageList(tag['id'], pkgID=pkg['id'], inherit=True, with_owners=False):

          raise koji.GenericError("Package %s is not in tag listing for %s" %

                                  (pkg['name'], tag['name']))

      pkglist_add(taginfo, pkginfo, block=True, force=force)
@@ -1124,51 +1124,66 @@ 

  

  

  def readPackageList(tagID=None, userID=None, pkgID=None, event=None, inherit=False,

-                     with_dups=False):

+                     with_dups=False, with_owners=True):

      """Returns the package list for the specified tag or user.

  

      One of (tagID,userID,pkgID) must be specified

  

      Note that the returned data includes blocked entries

+ 

+     :param int tagID: filter on tag

+     :param int userID: filter on package owner

+     :param int pkgID: filter on package

+     :param int event: filter on event

+     :param bool inherit: return also inherited packages

+     :param bool with_dups: possible duplicates from inheritance, makes no sense

+                            with inherit=False

+     :param bool with_owners: return also owner info. It needs to be set to True

+                              if userID is not None

+ 

+     :returns [dict]: list of dicts with package info

      """

      if tagID is None and userID is None and pkgID is None:

          raise koji.GenericError('tag,user, and/or pkg must be specified')

  

-     packages = {}

-     fields = (('package.id', 'package_id'), ('package.name', 'package_name'),

-               ('tag.id', 'tag_id'), ('tag.name', 'tag_name'),

-               ('users.id', 'owner_id'), ('users.name', 'owner_name'),

-               ('extra_arches', 'extra_arches'),

-               ('tag_packages.blocked', 'blocked'))

-     flist = ', '.join([pair[0] for pair in fields])

-     cond1 = eventCondition(event, table='tag_packages')

-     cond2 = eventCondition(event, table='tag_package_owners')

-     q = """

-     SELECT %(flist)s

-     FROM tag_packages

-     JOIN tag on tag.id = tag_packages.tag_id

-     JOIN package ON package.id = tag_packages.package_id

-     JOIN tag_package_owners ON

-         tag_packages.tag_id = tag_package_owners.tag_id AND

-         tag_packages.package_id = tag_package_owners.package_id

-     JOIN users ON users.id = tag_package_owners.owner

-     WHERE %(cond1)s AND %(cond2)s"""

+     if userID is not None and not with_owners:

+         raise koji.GenericError("userID and with_owners=False can't be used together")

+ 

+     tables = ['tag_packages']

+     fields = [

+         ('package.id', 'package_id'),

+         ('package.name', 'package_name'),

+         ('tag.id', 'tag_id'),

+         ('tag.name', 'tag_name'),

+         ('extra_arches', 'extra_arches'),

+         ('tag_packages.blocked', 'blocked'),

+     ]

+     joins = ['tag ON tag.id = tag_packages.tag_id',

+              'package ON package.id = tag_packages.package_id']

+     clauses = [eventCondition(event, table='tag_packages')]

      if tagID is not None:

-         q += """

-         AND tag.id = %%(tagID)i"""

+         clauses.append('tag.id = %(tagID)i')

      if userID is not None:

-         q += """

-         AND users.id = %%(userID)i"""

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

      if pkgID is not None:

          if isinstance(pkgID, int):

-             q += """

-             AND package.id = %%(pkgID)i"""

+             clauses.append('package.id = %(pkgID)i')

          else:

-             q += """

-             AND package.name = %%(pkgID)s"""

+             clauses.append('package.name = %(pkgID)s')

+     if with_owners:

+         fields += [('users.id', 'owner_id'), ('users.name', 'owner_name')]

+         joins += [

+             'tag_package_owners ON tag_packages.tag_id = tag_package_owners.tag_id AND \

+                                    tag_packages.package_id = tag_package_owners.package_id',

+             'users ON users.id = tag_package_owners.owner'

+         ]

+         clauses.append(eventCondition(event, table='tag_package_owners'))

+     fields, aliases = zip(*fields)

+     query = QueryProcessor(columns=fields, aliases=aliases, tables=tables, joins=joins,

+                            clauses=clauses, values=locals())

  

-     q = q % locals()

-     for p in _multiRow(q, locals(), [pair[1] for pair in fields]):

+     packages = {}

+     for p in query.execute():

          # things are simpler for the first tag

          pkgid = p['package_id']

          if with_dups:
@@ -1194,7 +1209,8 @@ 

                  re_cache[pat] = prog

              re_list.append(prog)

          # same query as before, with different params

-         for p in _multiRow(q, locals(), [pair[1] for pair in fields]):

+         query.values['tagID'] = tagID

+         for p in query.execute():

              pkgid = p['package_id']

              if not with_dups and pkgid in packages:

                  # previous data supercedes
@@ -1293,13 +1309,15 @@ 

                       type=None):

      """Returns a list of builds for specified tag

  

-     set inherit=True to follow inheritance

-     set event to query at a time in the past

-     set latest=True to get only the latest build per package

-     set latest=N to get only the N latest tagged RPMs

- 

-     If type is not None, restrict the list to builds of the given type.  Currently the supported

-     types are 'maven', 'win', and 'image'.

+     :param int tag: tag ID

+     :param int event: query at a time in the past

+     :param bool inherit: follow inheritance

+     :param bool|int latest: True for latest build per package, N to get N latest builds per package

+     :param int package: filter on package name

+     :param str owner: filter on user name

+     :param str type: restrict the list to builds of the given type.  Currently the supported

+                      types are 'maven', 'win', and 'image'.

+     :returns [dict]: list of buildinfo dicts

      """

      # build - id pkg_id version release epoch

      # tag_listing - id build_id tag_id
@@ -1313,8 +1331,11 @@ 

  

      # regardless of inherit setting, we need to use inheritance to read the

      # package list

-     packages = readPackageList(tagID=tag, event=event, inherit=True, pkgID=package)

+     packages = readPackageList(tagID=tag, event=event, inherit=True, pkgID=package,

+                                with_owners=False)

  

+     st_complete = koji.BUILD_STATES['COMPLETE']

+     tables = ['tag_listing']

      # these values are used for each iteration

      fields = [('tag.id', 'tag_id'), ('tag.name', 'tag_name'), ('build.id', 'id'),

                ('build.id', 'build_id'), ('build.version', 'version'), ('build.release', 'release'),
@@ -1322,64 +1343,63 @@ 

                ('build.completion_time', 'completion_time'),

                ('build.start_time', 'start_time'),

                ('build.task_id', 'task_id'),

+               ('users.id', 'owner_id'), ('users.name', 'owner_name'),

                ('events.id', 'creation_event_id'), ('events.time', 'creation_time'),

                ('volume.id', 'volume_id'), ('volume.name', 'volume_name'),

                ('package.id', 'package_id'), ('package.name', 'package_name'),

                ('package.name', 'name'),

                ("package.name || '-' || build.version || '-' || build.release", 'nvr'),

-               ('users.id', 'owner_id'), ('users.name', 'owner_name')]

-     st_complete = koji.BUILD_STATES['COMPLETE']

+               ('tag_listing.create_event', 'create_event')]

+     joins = [

+         'tag ON tag.id = tag_listing.tag_id',

+         'build ON build.id = tag_listing.build_id',

+         'events ON events.id = build.create_event',

+         'package ON package.id = build.pkg_id',

+         'volume ON volume.id = build.volume_id',

+         'users ON users.id = build.owner',

+     ]

  

-     type_join = ''

      if type is None:

          pass

      elif type == 'maven':

-         type_join = 'JOIN maven_builds ON maven_builds.build_id = tag_listing.build_id'

+         joins.append('maven_builds ON maven_builds.build_id = tag_listing.build_id')

          fields.extend([('maven_builds.group_id', 'maven_group_id'),

                         ('maven_builds.artifact_id', 'maven_artifact_id'),

                         ('maven_builds.version', 'maven_version')])

      elif type == 'win':

-         type_join = 'JOIN win_builds ON win_builds.build_id = tag_listing.build_id'

+         joins.append('win_builds ON win_builds.build_id = tag_listing.build_id')

          fields.append(('win_builds.platform', 'platform'))

      elif type == 'image':

-         type_join = 'JOIN image_builds ON image_builds.build_id = tag_listing.build_id'

+         joins.append('image_builds ON image_builds.build_id = tag_listing.build_id')

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

      else:

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

          if not btype:

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

          btype_id = btype['id']

-         type_join = ('JOIN build_types ON build.id = build_types.build_id '

-                      'AND btype_id = %(btype_id)s')

- 

-     q = """SELECT %s

-     FROM tag_listing

-     JOIN tag ON tag.id = tag_listing.tag_id

-     JOIN build ON build.id = tag_listing.build_id

-     %s

-     JOIN users ON users.id = build.owner

-     JOIN events ON events.id = build.create_event

-     JOIN package ON package.id = build.pkg_id

-     JOIN volume ON volume.id = build.volume_id

-     WHERE %s AND tag_id=%%(tagid)s

-         AND build.state=%%(st_complete)i

-     """ % (', '.join([pair[0] for pair in fields]), type_join,

-            eventCondition(event, 'tag_listing'))

+         joins += ['build_types ON build.id = build_types.build_id',

+                   'btype_id = %(btype_id)s']

+ 

+     clauses = [

+         eventCondition(event, 'tag_listing'),

+         'tag_id = %(tagid)s',

+         'build.state = %(st_complete)i'

+     ]

      if package:

-         q += """AND package.name = %(package)s

-         """

+         clauses.append('package.name = %(package)s')

      if owner:

-         q += """AND users.name = %(owner)s

-         """

-     q += """ORDER BY tag_listing.create_event DESC

-     """

-     # i.e. latest first

+         clauses.append('users.name = %(owner)s')

+     queryOpts = {'order': '-create_event'}  # latest first

+     query = QueryProcessor(columns=[x[0] for x in fields], aliases=[x[1] for x in fields],

+                            tables=tables, joins=joins, clauses=clauses, values=locals(),

+                            opts=queryOpts)

  

      builds = []

      seen = {}   # used to enforce the 'latest' option

      for tagid in taglist:

          # log_error(koji.db._quoteparams(q,locals()))

-         for build in _multiRow(q, locals(), [pair[1] for pair in fields]):

+         query.values['tagid'] = tagid

+         for build in query.execute():

              pkgid = build['package_id']

              pinfo = packages.get(pkgid, None)

              if pinfo is None or pinfo['blocked']:
@@ -2431,7 +2451,7 @@ 

      For any parent tags where 'maven_include_all' is true, include all versions

      of a given groupId:artifactId, not just the most-recently-tagged.

      """

-     packages = readPackageList(tagID=tag_id, event=event_id, inherit=True)

+     packages = readPackageList(tagID=tag_id, event=event_id, inherit=True, with_owners=False)

      taglist = [tag_id]

      if inherit:

          taglist.extend([link['parent_id'] for link in readFullInheritance(tag_id, event_id)])
@@ -2561,7 +2581,8 @@ 

      #       see https://pagure.io/koji/issue/588 for background

      rpms, builds = readTaggedRPMS(tag_id, event=event_id, inherit=True, latest=latest)

      groups = readTagGroups(tag_id, event=event_id, inherit=True)

-     blocks = [pkg for pkg in readPackageList(tag_id, event=event_id, inherit=True).values()

+     blocks = [pkg for pkg in readPackageList(tag_id, event=event_id, inherit=True,

+                                              with_owners=False).values()

                if pkg['blocked']]

      repodir = koji.pathinfo.repo(repo_id, tinfo['name'])

      os.makedirs(repodir)  # should not already exist
@@ -10945,7 +10966,7 @@ 

          if fromtag:

              assert_tag_access(fromtag_id, user_id=None, force=force)

          # package list check

-         pkgs = readPackageList(tagID=tag_id, pkgID=pkg_id, inherit=True)

+         pkgs = readPackageList(tagID=tag_id, pkgID=pkg_id, inherit=True, with_owners=False)

          pkg_error = None

          if pkg_id not in pkgs:

              pkg_error = "Package %s not in list for %s" % (build['name'], tag['name'])
@@ -11028,7 +11049,7 @@ 

          # note: we're just running the quick checks now so we can fail

          #       early if appropriate, rather then waiting for the task

          # Make sure package is on the list for the tag we're adding it to

-         pkgs = readPackageList(tagID=tag2_id, pkgID=pkg_id, inherit=True)

+         pkgs = readPackageList(tagID=tag2_id, pkgID=pkg_id, inherit=True, with_owners=False)

          pkg_error = None

          if pkg_id not in pkgs:

              pkg_error = "Package %s not in list for tag %s" % (package, tag2)
@@ -11848,7 +11869,7 @@ 

      getPackage = staticmethod(lookup_package)

  

      def listPackages(self, tagID=None, userID=None, pkgID=None, prefix=None, inherited=False,

-                      with_dups=False, event=None, queryOpts=None):

+                      with_dups=False, event=None, queryOpts=None, with_owners=True):

          """List if tagID and/or userID is specified, limit the

          list to packages belonging to the given user or with the

          given tag.
@@ -11880,7 +11901,7 @@ 

                  pkgID = get_package_id(pkgID, strict=True)

              result_list = list(readPackageList(tagID=tagID, userID=userID, pkgID=pkgID,

                                                 inherit=inherited, with_dups=with_dups,

-                                                event=event).values())

+                                                event=event, with_owners=with_owners).values())

              if with_dups:

                  # when with_dups=True, readPackageList returns a list of list of dicts

                  # convert it to a list of dicts for consistency
@@ -11928,7 +11949,7 @@ 

          pkg_id = get_package_id(pkg, strict=False)

          if pkg_id is None or tag_id is None:

              return False

-         pkgs = readPackageList(tagID=tag_id, pkgID=pkg_id, inherit=True)

+         pkgs = readPackageList(tagID=tag_id, pkgID=pkg_id, inherit=True, with_owners=False)

          if pkg_id not in pkgs:

              return False

          else:
@@ -14326,7 +14347,7 @@ 

          # don't check policy for admins using force

          assert_policy('tag', policy_data, force=force)

          # package list check

-         pkgs = readPackageList(tagID=tag_id, pkgID=pkg_id, inherit=True)

+         pkgs = readPackageList(tagID=tag_id, pkgID=pkg_id, inherit=True, with_owners=False)

          pkg_error = None

          if pkg_id not in pkgs:

              pkg_error = "Package %s not in list for %s" % (build['name'], tag)

@@ -56,7 +56,7 @@ 

          activate_session_mock.assert_called_once_with(session, options)

          session.getUser.assert_called_once_with(owner)

          session.getTag.assert_called_once_with(tag)

-         session.listPackages.assert_called_once_with(tagID=dsttag['id'])

+         session.listPackages.assert_called_once_with(tagID=dsttag['id'], with_owners=False)

          session.packageListAdd.assert_called_once_with(

              tag, package, owner, **kwargs)

          session.multiCall.assert_called_once_with(strict=True)
@@ -100,7 +100,7 @@ 

          self.assertEqual(session.mock_calls,

                           [call.getUser(owner),

                            call.getTag(tag),

-                           call.listPackages(tagID=dsttag['id']),

+                           call.listPackages(tagID=dsttag['id'], with_owners=False),

                            call.packageListAdd(tag, packages[0], owner, **kwargs),

                            call.packageListAdd(tag, packages[2], owner, **kwargs),

                            call.multiCall(strict=True)])

@@ -42,7 +42,7 @@ 

          activate_session_mock.assert_called_once_with(session, options)

          session.getTag.assert_called_once_with(tag)

          session.listPackages.assert_called_once_with(

-             tagID=dsttag['id'], inherited=True)

+             tagID=dsttag['id'], inherited=True, with_owners=False)

          session.packageListBlock.assert_called_once_with(

              tag, package, force=True)

          session.multiCall.assert_called_once_with(strict=True)
@@ -79,7 +79,7 @@ 

          self.assertEqual(

              session.mock_calls, [

                  call.getTag(tag),

-                 call.listPackages(tagID=dsttag['id'], inherited=True),

+                 call.listPackages(tagID=dsttag['id'], inherited=True, with_owners=False),

                  call.packageListBlock(tag, packages[0]),

                  call.packageListBlock(tag, packages[1]),

                  call.packageListBlock(tag, packages[2]),
@@ -116,7 +116,7 @@ 

          activate_session_mock.assert_called_once_with(session, options)

          session.getTag.assert_called_once_with(tag)

          session.listPackages.assert_called_once_with(

-             tagID=dsttag['id'], inherited=True)

+             tagID=dsttag['id'], inherited=True, with_owners=False)

          session.packageListBlock.assert_not_called()

          session.multiCall.assert_not_called()

  

@@ -43,8 +43,7 @@ 

          # Finally, assert that things were called as we expected.

          activate_session_mock.assert_called_once_with(session, options)

          session.getTag.assert_called_once_with(tag)

-         session.listPackages.assert_called_once_with(

-             tagID=dsttag['id'])

+         session.listPackages.assert_called_once_with(tagID=dsttag['id'], with_owners=False)

          session.packageListRemove.assert_called_once_with(

              tag, package, **kwargs)

          session.multiCall.assert_called_once_with(strict=True)
@@ -80,12 +79,14 @@ 

          activate_session_mock.assert_called_once_with(session, options)

          self.assertEqual(

              session.mock_calls, [

-                 call.getTag(tag), call.listPackages(

-                     tagID=dsttag['id']), call.packageListRemove(

-                     tag, packages[0], **kwargs), call.packageListRemove(

-                     tag, packages[1], **kwargs), call.packageListRemove(

-                         tag, packages[2], **kwargs), call.multiCall(

-                             strict=True)])

+                 call.getTag(tag),

+                 call.listPackages(tagID=dsttag['id'], with_owners=False),

+                 call.packageListRemove(tag, packages[0], **kwargs),

+                 call.packageListRemove(tag, packages[1], **kwargs),

+                 call.packageListRemove(tag, packages[2], **kwargs),

+                 call.multiCall( strict=True)

+             ]

+         )

          self.assertNotEqual(rv, 1)

  

      @mock.patch('sys.stdout', new_callable=six.StringIO)
@@ -119,12 +120,14 @@ 

          activate_session_mock.assert_called_once_with(session, options)

          self.assertEqual(

              session.mock_calls, [

-                 call.getTag(tag), call.listPackages(

-                     tagID=dsttag['id']), call.packageListRemove(

-                     tag, packages[0], **kwargs), call.packageListRemove(

-                     tag, packages[1], **kwargs), call.packageListRemove(

-                         tag, packages[2], **kwargs), call.multiCall(

-                             strict=True)])

+                 call.getTag(tag),

+                 call.listPackages(tagID=dsttag['id'], with_owners=False),

+                 call.packageListRemove(tag, packages[0], **kwargs),

+                 call.packageListRemove(tag, packages[1], **kwargs),

+                 call.packageListRemove(tag, packages[2], **kwargs),

+                 call.multiCall(strict=True)

+             ]

+         )

          self.assertNotEqual(rv, 1)

  

      @mock.patch('sys.stderr', new_callable=six.StringIO)
@@ -156,8 +159,7 @@ 

          # Finally, assert that things were called as we expected.

          activate_session_mock.assert_called_once_with(session, options)

          session.getTag.assert_called_once_with(tag)

-         session.listPackages.assert_called_once_with(

-             tagID=dsttag['id'])

+         session.listPackages.assert_called_once_with(tagID=dsttag['id'], with_owners=False)

          session.packageListRemove.assert_not_called()

          session.multiCall.assert_not_called()

  

file modified
+1 -1
@@ -968,7 +968,7 @@ 

  

      all = int(all)

  

-     numPackages = server.count('listPackages', tagID=tag['id'], inherited=True)

+     numPackages = server.count('listPackages', tagID=tag['id'], inherited=True, with_owners=False)

      numBuilds = server.count('listTagged', tag=tag['id'], inherit=True)

      values['numPackages'] = numPackages

      values['numBuilds'] = numBuilds

I don't think that readTaggedBuilds needs a with_owners option. This is a different owner field -- package owner vs build owner. It should be enough to just pass with_owners=False to readPackageList here.

Cheers for QueryProcessor conversions, but I don't like splitting up fields/aliases declarations. Putting them in separate lists makes it hard to ensure alignment and invites future bugs. Best to keep them together in a list of pairs or a dict and split them before the query, as we do in other places. I'm a fan of using zip (as in readTaggedRPMS), but the approach in _split_fields works too.

The sanity check on the userID param should be if userID is None. It is not the same case if a client passes userID=0 or userID="".

rebased onto c16e382054ffa15964cc6590d1065f535797e7d0

3 years ago

Thanks for the updates!

The listTagged call wrapper is still passing with_owners to readTaggedBuilds even though the option has been removed, and several client calls to listTagged are still passing this option.

1 new commit added

  • cleanup removed options
3 years ago

1 new commit added

  • backward compatibility for CLI
3 years ago

Ouch, cleaned. I've also modified CLI to work with older hub.

rebased onto 0be3b6c9d36421a16ac01edd074d336316c71b5e

3 years ago

Metadata Update from @tkopecek:
- Pull-request tagged with: testing-ready

3 years ago

rebased onto 2169356

3 years ago

11 new commits added

  • backward compatibility for CLI
  • cleanup removed options
  • remove with_owners from readTaggedBuilds
  • updated API docs
  • use listTagged with_owner=False
  • x
  • readTaggedBuilds with_owners option
  • update API docs
  • fix tests for new option
  • use with_owners=False
  • hub: add with_owners option to readPackageList
3 years ago

Commit 0414ff7 fixes this pull-request

Pull-Request has been merged by tkopecek

3 years ago

Metadata Update from @mfilip:
- Pull-request tagged with: testing-done

2 years ago