From 1e35e60c21dea89466702e543a9792655f9bad74 Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Aug 29 2019 13:44:28 +0000 Subject: PR#1473: move tag/package owners to separate table Merges #1473 https://pagure.io/koji/pull-request/1473 Fixes: #956 https://pagure.io/koji/issue/956 package ownership changes should not trigger repo regens --- diff --git a/cli/koji_cli/commands.py b/cli/koji_cli/commands.py index 19058ea..2d54606 100644 --- a/cli/koji_cli/commands.py +++ b/cli/koji_cli/commands.py @@ -4200,6 +4200,13 @@ def _print_histline(entry, **kwargs): fmt = "package list entry created: %(package.name)s in %(tag.name)s" else: fmt = "package list entry revoked: %(package.name)s in %(tag.name)s" + elif table == 'tag_package_owners': + if edit: + fmt = "package owner changed for %(package.name)s in %(tag.name)s" + elif create: + fmt = "package owner %(owner.name)s set for %(package.name)s in %(tag.name)s" + else: + fmt = "package owner %(owner.name)s revoked for %(package.name)s in %(tag.name)s" elif table == 'tag_inheritance': if edit: fmt = "inheritance line %(tag.name)s->%(parent.name)s updated" @@ -4352,6 +4359,7 @@ _table_keys = { 'tag_external_repos' : ['tag_id', 'external_repo_id'], 'tag_listing' : ['build_id', 'tag_id'], 'tag_packages' : ['package_id', 'tag_id'], + 'tag_package_owners' : ['package_id', 'tag_id'], 'group_config' : ['group_id', 'tag_id'], 'group_req_listing' : ['group_id', 'tag_id', 'req_id'], 'group_package_listing' : ['group_id', 'tag_id', 'package'], diff --git a/docs/schema-upgrade-1.18-1.19.sql b/docs/schema-upgrade-1.18-1.19.sql index eccce78..9ca4763 100644 --- a/docs/schema-upgrade-1.18-1.19.sql +++ b/docs/schema-upgrade-1.18-1.19.sql @@ -4,5 +4,55 @@ BEGIN; +CREATE TABLE tag_package_owners ( + package_id INTEGER NOT NULL REFERENCES package(id), + tag_id INTEGER NOT NULL REFERENCES tag (id), + owner INTEGER NOT NULL REFERENCES users(id), +-- versioned - see earlier description of versioning + create_event INTEGER NOT NULL REFERENCES events(id) DEFAULT get_event(), + revoke_event INTEGER REFERENCES events(id), + creator_id INTEGER NOT NULL REFERENCES users(id), + revoker_id INTEGER REFERENCES users(id), + active BOOLEAN DEFAULT 'true' CHECK (active), + CONSTRAINT active_revoke_sane CHECK ( + (active IS NULL AND revoke_event IS NOT NULL AND revoker_id IS NOT NULL) + OR (active IS NOT NULL AND revoke_event IS NULL AND revoker_id IS NULL)), + PRIMARY KEY (create_event, package_id, tag_id), + UNIQUE (package_id,tag_id,active) +) WITHOUT OIDS; + +CREATE OR REPLACE FUNCTION convert_owners() RETURNS SETOF tag_packages AS +$BODY$ +DECLARE + r tag_packages%rowtype; + r2 tag_packages%rowtype; + last_owner int; +BEGIN + FOR r IN SELECT package_id, tag_id FROM tag_packages GROUP BY package_id, tag_id ORDER BY package_id, tag_id + LOOP + last_owner := 0; + FOR r2 IN SELECT * FROM tag_packages WHERE package_id = r.package_id AND tag_id = r.tag_id ORDER BY create_event + LOOP + -- always use first and last (active) row + IF last_owner = 0 OR r2.active IS TRUE THEN + last_owner := r2.owner; + RETURN NEXT r2; -- return current row of SELECT + ELSE + -- copy others only if owner changed + IF last_owner <> r2.owner THEN + RETURN NEXT r2; + last_owner := r2.owner; + END IF; + END IF; + END LOOP; + END LOOP; + RETURN; +END +$BODY$ +LANGUAGE plpgsql; + +INSERT INTO tag_package_owners (SELECT package_id, tag_id, owner create_event revoke_event creator_id revoker_id active FROM convert_owners()); +ALTER TABLE tag_packages DROP COLUMN owner; +DROP FUNCTION convert_owners(); COMMIT; diff --git a/docs/schema.sql b/docs/schema.sql index d8d8f25..d9d46c6 100644 --- a/docs/schema.sql +++ b/docs/schema.sql @@ -604,6 +604,23 @@ CREATE INDEX tag_packages_create_event ON tag_packages(create_event); CREATE INDEX tag_packages_revoke_event ON tag_packages(revoke_event); CREATE INDEX tag_packages_owner ON tag_packages(owner); +CREATE TABLE tag_package_owners ( + package_id INTEGER NOT NULL REFERENCES package(id), + tag_id INTEGER NOT NULL REFERENCES tag (id), + owner INTEGER NOT NULL REFERENCES users(id), +-- versioned - see earlier description of versioning + create_event INTEGER NOT NULL REFERENCES events(id) DEFAULT get_event(), + revoke_event INTEGER REFERENCES events(id), + creator_id INTEGER NOT NULL REFERENCES users(id), + revoker_id INTEGER REFERENCES users(id), + active BOOLEAN DEFAULT 'true' CHECK (active), + CONSTRAINT active_revoke_sane CHECK ( + (active IS NULL AND revoke_event IS NOT NULL AND revoker_id IS NOT NULL) + OR (active IS NOT NULL AND revoke_event IS NULL AND revoker_id IS NULL)), + PRIMARY KEY (create_event, package_id, tag_id), + UNIQUE (package_id,tag_id,active) +) WITHOUT OIDS; + -- package groups (per tag). used for generating comps for the tag repos CREATE TABLE groups ( id SERIAL NOT NULL PRIMARY KEY, diff --git a/hub/kojihub.py b/hub/kojihub.py index eeb2b7a..4b2fce1 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -903,16 +903,32 @@ def _pkglist_remove(tag_id, pkg_id): update.make_revoke() #XXX user_id? update.execute() +def _pkglist_owner_remove(tag_id, pkg_id): + clauses = ('package_id=%(pkg_id)i', 'tag_id=%(tag_id)i') + update = UpdateProcessor('tag_package_owners', values=locals(), clauses=clauses) + update.make_revoke() #XXX user_id? + update.execute() + +def _pkglist_owner_add(tag_id, pkg_id, owner): + _pkglist_owner_remove(tag_id, pkg_id) + data = {'tag_id': tag_id, 'package_id': pkg_id, 'owner': owner} + insert = InsertProcessor('tag_package_owners', data=data) + insert.make_create() #XXX user_id? + insert.execute() + def _pkglist_add(tag_id, pkg_id, owner, block, extra_arches): - #revoke old entry (if present) - data = dslice(locals(), ('tag_id', 'owner', 'extra_arches')) - data['package_id'] = pkg_id - data['blocked'] = block - data['extra_arches'] = koji.parse_arches(data['extra_arches'], strict=True, allow_none=True) + # revoke old entry (if present) _pkglist_remove(tag_id, pkg_id) + data = { + 'tag_id': tag_id, + 'package_id': pkg_id, + 'blocked': block, + 'extra_arches': koji.parse_arches(extra_arches, strict=True, allow_none=True) + } insert = InsertProcessor('tag_packages', data=data) insert.make_create() #XXX user_id? insert.execute() + _pkglist_owner_add(tag_id, pkg_id, owner) def pkglist_add(taginfo, pkginfo, owner=None, block=None, extra_arches=None, force=False, update=False): """Add to (or update) package list for tag""" @@ -955,6 +971,8 @@ def _direct_pkglist_add(taginfo, pkginfo, owner, block, extra_arches, force, # blocked pkglist = readPackageList(tag_id, pkgID=pkg['id'], inherit=True) previous = pkglist.get(pkg['id'], None) + changed = False + changed_owner = False if previous is None: block = bool(block) if update and not force: @@ -965,6 +983,7 @@ def _direct_pkglist_add(taginfo, pkginfo, owner, block, extra_arches, force, #already there (possibly via inheritance) if owner is None: owner = previous['owner_id'] + changed_owner = previous['owner_id'] != owner if block is None: block = previous['blocked'] else: @@ -972,14 +991,12 @@ def _direct_pkglist_add(taginfo, pkginfo, owner, block, extra_arches, force, if extra_arches is None: extra_arches = previous['extra_arches'] #see if the data is the same - changed = False - for key, value in (('owner_id', owner), - ('blocked', block), - ('extra_arches', extra_arches)): + for key, value in (('blocked', block), + ('extra_arches', extra_arches)): if previous[key] != value: changed = True break - if not changed and not force: + if not changed and not changed_owner and not force: #no point in adding it again with the same data return if previous['blocked'] and not block and not force: @@ -989,7 +1006,10 @@ def _direct_pkglist_add(taginfo, pkginfo, owner, block, extra_arches, force, owner = context.session.user_id else: raise koji.GenericError("owner not specified") - _pkglist_add(tag_id, pkg['id'], owner, block, extra_arches) + if not previous or changed: + _pkglist_add(tag_id, pkg['id'], owner, block, extra_arches) + elif changed_owner: + _pkglist_owner_add(tag_id, pkg['id'], owner) koji.plugin.run_callbacks('postPackageListChange', action=action, tag=tag, package=pkg, owner=owner, block=block, extra_arches=extra_arches, force=force, update=update) @@ -1083,14 +1103,18 @@ def readPackageList(tagID=None, userID=None, pkgID=None, event=None, inherit=Fal ('extra_arches', 'extra_arches'), ('tag_packages.blocked', 'blocked')) flist = ', '.join([pair[0] for pair in fields]) - cond = eventCondition(event) + 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 users ON users.id = tag_packages.owner - WHERE %(cond)s""" + JOIN tag_package_owners ON + tag_packages.tag_id = tag_package_owners.tag_id AND + tag_packages.package_id = tag_packages.package_id + JOIN users ON users.id = tag_package_owners.owner + WHERE %(cond1)s AND %(cond2)s""" if tagID != None: q += """ AND tag.id = %%(tagID)i""" @@ -1205,7 +1229,11 @@ def list_tags(build=None, package=None, queryOpts=None): joins.append('tag_packages ON tag.id = tag_packages.tag_id') clauses.append('tag_packages.active = true') clauses.append('tag_packages.package_id = %(packageID)i') - joins.append('users ON tag_packages.owner = users.id') + joins.append("tag_package_owners ON\n" + " tag_packages.tag_id = tag_package_owners.tag_id AND\n" + " tag_packages.package_id = tag_package_owners.package_id AND\n" + " tag_package_owners.active IS TRUE") + joins.append('users ON tag_package_owners.owner = users.id') packageID = packageinfo['id'] query = QueryProcessor(columns=fields, aliases=aliases, tables=tables, @@ -3349,6 +3377,7 @@ def _delete_tag(tagInfo): _tagDelete('build_target_config', tagID, 'dest_tag') _tagDelete('tag_listing', tagID) _tagDelete('tag_packages', tagID) + _tagDelete('tag_package_owners', tagID) _tagDelete('tag_external_repos', tagID) _tagDelete('group_config', tagID) _tagDelete('group_req_listing', tagID) @@ -6938,8 +6967,8 @@ def query_history(tables=None, **kwargs): tables: list of versioned tables to search, no value implies all tables valid entries: user_perms, user_groups, tag_inheritance, tag_config, build_target_config, external_repo_config, tag_external_repos, - tag_listing, tag_packages, group_config, group_req_listing, - group_package_listing + tag_listing, tag_packages, tag_package_owners, group_config, + group_req_listing, group_package_listing - Time options - times are specified as an integer event or a string timestamp @@ -6998,7 +7027,8 @@ def query_history(tables=None, **kwargs): 'host_channels': ['host_id', 'channel_id'], 'tag_external_repos': ['tag_id', 'external_repo_id', 'priority', 'merge_mode'], 'tag_listing': ['build_id', 'tag_id'], - 'tag_packages': ['package_id', 'tag_id', 'owner', 'blocked', 'extra_arches'], + 'tag_packages': ['package_id', 'tag_id', 'blocked', 'extra_arches'], + 'tag_package_owners': ['package_id', 'tag_id', 'owner'], 'group_config': ['group_id', 'tag_id', 'blocked', 'exported', 'display_name', 'is_default', 'uservisible', 'description', 'langonly', 'biarchonly'], 'group_req_listing': ['group_id', 'tag_id', 'req_id', 'blocked', 'type', 'is_metapkg'],