PR#2923: get info for deleted tag entries via api - koji

koji

#2923 get info for deleted tag entries via api

Merged 2 years ago by tkopecek. Opened 2 years ago by tkopecek.

tkopecek/koji issue1506 into master

updates

Tomas Kopecek • 2 years ago

7916b52

get_tag and accepts "auto" event

Tomas Kopecek • 2 years ago

0ee750c

hub/kojihub.py

file modified

+26 -3

		`@@ -3299,6 +3299,9 @@`
		`Note that in order for a tag to 'exist', it must have an active entry`
		`in tag_config. A tag whose name appears in the tag table but has no`
		`active tag_config entry is considered deleted.`
		`+`
		`+ event option can be either event_id or "auto" which will pick last`
		`+ recorded create_event (option for getting deleted tags)`
		`"""`

		`tables = ['tag_config']`
		`@@ -3311,17 +3314,37 @@`
		`'tag_config.arches': 'arches',`
		`'tag_config.locked': 'locked',`
		`'tag_config.maven_support': 'maven_support',`
		`- 'tag_config.maven_include_all': 'maven_include_all'`
		`+ 'tag_config.maven_include_all': 'maven_include_all',`
		`}`
		`- clauses = [eventCondition(event, table='tag_config')]`
		`+ data = {'tagInfo': tagInfo}`
		`+`
		`+ clauses = []`
		`if isinstance(tagInfo, int):`
		`clauses.append("tag.id = %(tagInfo)i")`
		`elif isinstance(tagInfo, str):`
		`clauses.append("tag.name = %(tagInfo)s")`
		`else:`
		`raise koji.GenericError('Invalid type for tagInfo: %s' % type(tagInfo))`
		`+ if event == "auto":`
		`+ # find active event or latest create_event`
		`+ opts = {'order': '-create_event', 'limit': 1}`
		`+ query = QueryProcessor(tables=['tag_config'], columns=['create_event', 'revoke_event'],`
		`+ joins=['tag on tag.id = tag_config.tag_id'],`
		`+ clauses=clauses, values=data, opts=opts)`
		`+ try:`
		`+ event = query.executeOne(strict=True)['revoke_event']`
		`+ except koji.GenericError:`
		`+ event = None`
		`+ if event is not None:`
		`+ # query point instantly before the revoke_event`
		`+ # (to get latest tag_config before deletion)`
		`+ event -= 1`
		`+ fields['tag_config.revoke_event'] = 'revoke_event'`
		`+ else:`
		`+ # if tag is not deleted, query event=None`
		`+ pass`
		`+ clauses.append(eventCondition(event, table='tag_config'))`

		`- data = {'tagInfo': tagInfo}`
		`fields, aliases = zip(*fields.items())`
		`query = QueryProcessor(columns=fields, aliases=aliases, tables=tables,`
		`joins=joins, clauses=clauses, values=data)`

tkopecek commented 2 years ago

New "revoked" value for events. I'm not that happy about the name, maybe something like "all" is better?

julian8628 commented 2 years ago

Is that a case that we need "revoked" items with an event?

tkopecek commented 2 years ago

In such case you can just pass event_id and you'll get revoked ones. In case you would more refined data, maybe history search?

rebased onto 39dbd4ab190b7884a2c2dc0961b3d2fc8410692e

2 years ago

julian8628 commented 2 years ago

yeah, a condition "%(table)screate_event <= %(event)d" % locals() which is similar with query_history. It doesn't look necessary to be covered by eventCondition

Edited 2 years ago by julian8628

mikem commented 2 years ago

The largest issue I see here is that we're also calling get_tag_extra with the same event parameter. The tag_extra table is multivalued at a given event, so we simply can't use the same logic there. At the moment, we're going to get every tag extra value ever set in the deleted tag case.

I believe we'll need to pass the correct event id to get_tag_extra

mikem commented 2 years ago

Other notes:

we shouldn't add the order option in the original case
typo: if event != 'inactive':
we're losing the the single value sanity check in the original case

mikem commented 2 years ago

Given that eventCondition() alone can't return a condition that accomplishes what we want, I'm not sure it make sense to make a change there at all. The calling code already has to have very different logic in the revoked case; it can just skip adding the condition too. TRUE IS TRUE isn't really an event condition.

mikem commented 2 years ago

I think the difficulty in finding a clear name for the special event value might be an indication that this is the wrong approach. It might be better to go the other way and add a new option to the call.

mikem commented 2 years ago

That said, perhaps event = "auto"?

rebased onto 8d1bf116df9780ca311586cc9538e360004cf45a

2 years ago

tkopecek commented 2 years ago

I've cleaned it a lot (and dropped policy part as it needs more work according to #2917).

rebased onto 07a9167d93ef924d7dfa3cfe5f815df1e06080bc

2 years ago

rebased onto 23a9ffa054f9a08b339c3e2248a3320a6d1b602a

2 years ago

mikem commented 2 years ago

The event query doesn't seem to have any clauses?

MIght be cleaner to add revoke_event to fields in the auto case rather than selectively delete it after.

rebased onto 0ee750c

2 years ago

tkopecek commented 2 years ago

Ouch, updated.

tkopecek commented 2 years ago

I've not checked that correct revoke_event is returned in local test. I'll add tests when we settle the behaviour here.

mikem commented 2 years ago

I don't think we're picking the right event. We're getting the most recent create_event, but that might not be the most recent event for the followup tag_extra query.

I think we want to:

query revoke_event in this first query (but still taking the entry with highest create_event)
if revoke_event is null, then the tag is not deleted and we should go with event = None
otherwise we should go with event = revoke_event - 1

The delete_tag() function does everything in one transaction, so all the revokes from that should have the same event id (and even if they did advance, the tag_config entry is revoked first). So, pretty sure revoke_event - 1 is the best representation of the undeleted tag.

Edited 2 years ago by mikem

mikem commented 2 years ago

Also, the code fails with event=auto and a nonexistent tag. Easiest thing to do is set event=None when we don't get a row and fail later (I think using the singleValue method with strict=False will do this in one line), but we could also duplicate the error code.

tkopecek commented 2 years ago

updated

1 new commit added