It could use same logic as previous tests. So, put it two lines ahead and set sufficient=False instead of rewriting rescan.

@tkopecek fixed

+        sys.stdout.write('%(name)s (%(parent_id)i) p%(priority)i\n' % currtag)

inheritance priority is a lower level detail that does not need to be printed in the inheritance tree. The inheritance tree is printed in the correct order and that is all that is relevant there. When an admin needs to know the priority details because they are making inheritance adjustments, they can already see that in the taginfo output.

-        update = UpdateProcessor('tag_inheritance', values=locals(),
-                                 clauses=['tag_id=%(tag_id)s', 'parent_id = %(parent_id)s'])
-        update.make_revoke()

This breaks inheritance updates. When we are replacing an entry, we must revoke the old one. Without this, there will be numerous failures, e.g. when updating the priority of an existing inheritance line.

What was your intent here?

+                if previous['priority'] != link['priority']:
+                    rescan = True

The point of the rescan logic is to avoid redundancy in the inheritance tree. There are many legitimate ways that tag-A might inherit from tag-B through multiple pathways. E.g. tag-A could inherit directly from tag-B, but also indirectly via another tag. Perhaps:

     A (180)
....  ├─C (182)
....  │  └─B (181)
....  └─B (181)

In such a case, there is no point to consider B again, as it's already in the list. Unless, as the logic checks, the earlier link had options that limit the inheritance in some way that the new link does not (hence the new link offers something new).

The priority field does not change anything about the inheritance link itself, just the order of traversal. A change in the priority is not sufficient to rescan here.

CREATE UNIQUE INDEX tag_inheritance_unique ON tag_inheritance (tag_id, priority) WHERE active IS TRUE;

We already have UNIQUE (tag_id,priority,active) in the table. We do not need an additional partial index.

Unique index is too restrictive and can't handle multiple changes in history for same link.
event
1) add parent with priority 1
2) revoke it
3) add it again
4) revoke it - it will fail in this point as there can't be two inactive records with same priority.

Partial index solves this issue limiting constraint on active only. It could be an issue also in some other tables.

Unique index is too restrictive and can't handle multiple changes in history for same link.

This is why the active field must be either True or NULL, never false (note the check constraint on it). This is a key part of how versioning works in the db. Because NULL is not considered equal to NULL in postgres, the uniqueness constraint effectively only applies when active is true. All of our versioned tables have similar uniqueness constraints.

You absolutely can perform the steps you list above with the existing schema.

Ouch, I've had broken testing database - my fault. Should clean it up more often.

@tkopecek @mikem all fixed

This is still not needed?

@tkopecek comment fixed

With this change in place, the force option in the cli command appears to be obsolete.

     # read current data and index
     data = dict([[link['parent_id'], link] for link in readInheritanceData(tag_id)])
     for link in changes:
         link['is_update'] = True
         parent_id = link['parent_id']
+        priority = link['priority']
         orig = data.get(parent_id)

The existing code is indexing the inheritance links by parent_id. That will need to change if we allow multiple direct links to the same parent. The orig that we compare to might not be the right one.

You define the priority variable here in this loop, but don't use it until much later.

     for parent_id, link in data.items():
         if not link.get('is_update'):
             continue
         # revoke old values
         update = UpdateProcessor('tag_inheritance', values=locals(),
-                                 clauses=['tag_id=%(tag_id)s', 'parent_id = %(parent_id)s'])
+                                 clauses=['tag_id=%(tag_id)s', 'parent_id=%(parent_id)s',
+                                          'priority=%(priority)s'])
         update.make_revoke()

At this point the priority value is a remnant from a previous loop and unrelated to the link under consideration.

Extend a bit for migration: DROP CONSTRAINT IF EXISTS

Too long line (fails flake8)

Otherwise :thumbsup:

fixed

         if link.get('delete link'):
-            check_fields = ('parent_id',)
+            check_fields = ('parent_id', 'priority',)
         for f in check_fields:

This is unfortunately a backwards incompatible api change. Previously, this field was not required, and even with this feature it is only really needed when there are multiple links to a given parent (which is expected to be rare).

+
+class TestCancelBuildRootExports(unittest.TestCase):
+

test case class is misnamed

         if link.get('delete link'):
             if orig:
-                data[parent_id] = dslice(link, ['delete link', 'is_update', 'parent_id'])
+                data[parent_id] = dslice(link,
+                                         ['delete link', 'is_update', 'parent_id', 'priority'])
+            break
         elif not orig or clear:

The addition of break here is incorrect. This causes the code to ignore any remaining changes in the list.

The existing code is indexing the inheritance links by parent_id. That will need to change if we allow multiple direct links to the same parent. The orig that we compare to might not be the right one.

Nothing in the updates addresses this point from before. This is actually the core problem for this feature, as noted in #2804.

tbh, it would probably be easier to keep the defacto limitation in place and fix the commands to match. While there is a hypothetical use case in #2804, it's never really come up (and you can manage it using an indirect inheritance if really needed). It seems the the only real issue is the command causing confusion (#3910). It's also entirely possible there are client scripts out there that assume the current uniqueness (e.g. koji-inheritance-replace from koji-tools)

tbh, it would probably be easier to keep the defacto limitation in place and fix the commands to match. While there is a hypothetical use case in #2804, it's never really come up (and you can manage it using an indirect inheritance if really needed). It seems the the only real issue is the command causing confusion (#3910). It's also entirely possible there are client scripts out there that assume the current uniqueness (e.g. koji-inheritance-replace from koji-tools)

Hm, so you want to say that we want to fix #3910 but #2804 close with conclusion that we don't want to do it, right?

so you want to say that we want to fix #3910 but #2804 close with conclusion that we don't want to do it, right?

Yes. I've gone ahead and closed #2804

@mikem ok, I dropped all changes related to #2804. In this PR are changes related to #3985 now.

While it would certainly be nice to have tests for write_tag_inheritance, the tests you have here seem to still be written with multiple links to the same parent involved.

Both remove-tag-inheritance and edit-tag-inheritance share this confusing <tag> <parent> <priority> selection usage. We should be consistent between these. I.e. fix both up in the same way.

Because the parent is always sufficient to specify a link, the priority is never required. For backwards compatibility[1] , we should continue to accept it and treat it as an assertion about the priority of the entry we're operating on.

While we're here, the "no parent arg" case that this code allows could be made clearer. If the parent is not given, then the command should probably indicate the name of the implicit parent it is operating on.

[1] I guess technically, this option has never worked, so I'm not sure how valuable it is to preserve it. Still, I guess it doesn't really hurt to do so.

@mikem all fixed. Parent is in the usage of edit-tag-inheritance and remove-tag-inheritance obligatory. It was mistake :-)

This doesn't quite address everything I mentioned above, but at this point it's probably safer to go with just the simple int cast. Actually fixing the logic in this old code is going to be more invasive.

So, sure. Let's run with this :thumbsup:

this can fail with

TypeError: int() argument must be a string, a bytes-like object or a real number, not 'NoneType'

if options.priority is not specified. It should either have a default value or be checked for existence prior.

@relias-redhat fixed

Commit 3315e3c3 fixes this pull-request