#2923 get info for deleted tag entries via api
Merged 9 months ago by tkopecek. Opened 11 months ago by tkopecek.
tkopecek/koji issue1506  into  master

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")


          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)

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

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

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

11 months 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

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

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

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.

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.

That said, perhaps event = "auto"?

rebased onto 8d1bf116df9780ca311586cc9538e360004cf45a

11 months ago

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

rebased onto 07a9167d93ef924d7dfa3cfe5f815df1e06080bc

11 months ago

rebased onto 23a9ffa054f9a08b339c3e2248a3320a6d1b602a

11 months 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

11 months ago

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

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.

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.

1 new commit added

  • updates
11 months ago

Note that this fixes #1506 but not #2917

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

9 months ago

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

9 months ago

Commit e7b6e78 fixes this pull-request

Pull-Request has been merged by tkopecek

9 months ago