koji

Clone

#4000 Fix remove-tag-inheritance with priority
Merged a month ago by tkopecek. Opened 2 months ago by jcupova.
jcupova/koji issue-3985  into  master

file modified
+11 -9 
@@ -5227,7 +5227,7 @@ 
 

  

  def handle_edit_tag_inheritance(goptions, session, args):
 

      """[admin] Edit tag inheritance"""
 

-     usage = "usage: %prog edit-tag-inheritance [options] <tag> <parent> <priority>"
 

+     usage = "usage: %prog edit-tag-inheritance [options] <tag> <parent> [<priority>]"
 

      parser = OptionParser(usage=get_usage_str(usage))
 

      parser.add_option("--priority", help="Specify a new priority")
 

      parser.add_option("--maxdepth", help="Specify max depth")
 
@@ -5256,13 +5256,13 @@ 
 

          if not parent:
 

              parser.error("No such tag: %s" % args[1])
 

          if len(args) > 2:
 

-             priority = args[2]
 

+             priority = int(args[2])
 

  

      data = session.getInheritanceData(tag['id'])
 

      if parent and data:
 

          data = [datum for datum in data if datum['parent_id'] == parent['id']]
 

      if priority and data:
 

-         data = [datum for datum in data if datum['priority'] == priority]
 

+         data = [datum for datum in data if int(datum['priority']) == priority]
 

  

      if len(data) == 0:
 

          error("No inheritance link found to remove.  Please check your arguments")
 
@@ -5278,10 +5278,12 @@ 
 

      data = data[0]
 

  

      inheritanceData = session.getInheritanceData(tag['id'])
 

-     samePriority = [datum for datum in inheritanceData if datum['priority'] == options.priority]
 

-     if samePriority:
 

-         error("Error: There is already an active inheritance with that priority on %s, "
 

-               "please specify a different priority with --priority." % tag['name'])
 

+     if options.priority is not None:
 

+         samePriority = [datum for datum in inheritanceData
 

+                         if datum['priority'] == int(options.priority)]
 

+         if samePriority:
 

+             error("Error: There is already an active inheritance with that priority on %s, "
 

+                   "please specify a different priority with --priority." % tag['name'])
 

  

      new_data = data.copy()
 

      if options.priority is not None and options.priority.isdigit():
 
@@ -5308,7 +5310,7 @@ 
 

  

  def handle_remove_tag_inheritance(goptions, session, args):
 

      """[admin] Remove a tag inheritance link"""
 

-     usage = "usage: %prog remove-tag-inheritance <tag> <parent> <priority>"
 

+     usage = "usage: %prog remove-tag-inheritance <tag> <parent> [<priority>]"
 

      parser = OptionParser(usage=get_usage_str(usage))
 

      (options, args) = parser.parse_args(args)
 

  
@@ -5332,7 +5334,7 @@ 
 

          if not parent:
 

              parser.error("No such tag: %s" % args[1])
 

          if len(args) > 2:
 

-             priority = args[2]
 

+             priority = int(args[2])
 

  

      data = session.getInheritanceData(tag['id'])
 

      if parent and data:

file modified
+6 -3 
@@ -15,7 +15,7 @@ 
 

          self.session = mock.MagicMock()
 

          self.session.getAPIVersion.return_value = koji.API_VERSION
 

          self.activate_session_mock = mock.patch('koji_cli.commands.activate_session').start()
 

-         self.error_format = """Usage: %s edit-tag-inheritance [options] <tag> <parent> <priority>
 

+         self.error_format = """Usage: %s edit-tag-inheritance [options] <tag> <parent> [<priority>]
 

  (Specify the --help global option for a list of other help options)
 

  

  %s: error: {message}
 
@@ -31,7 +31,7 @@ 
 

                                  'noconfig': False,
 

                                  'parent_id': 2,
 

                                  'pkg_filter': '',
 

-                                 'priority': self.priority}
 

+                                 'priority': int(self.priority)}
 

          self.child_tag_info = {'arches': 'x86_64',
 

                                 'extra': {},
 

                                 'id': 1,
 
@@ -51,6 +51,9 @@ 
 

                                  'perm': None,
 

                                  'perm_id': None}
 

  

+     def tearDown(self):
 

+         mock.patch.stopall()
 

+ 

      def test_edit_tag_inheritance_without_option(self):
 

          expected = self.format_error_message(
 

              "This command takes at least one argument: a tag name or ID")
 
@@ -271,7 +274,7 @@ 
 

      def test_edit_tag_inheritance_help(self):
 

          self.assert_help(
 

              handle_edit_tag_inheritance,
 

-             """Usage: %s edit-tag-inheritance [options] <tag> <parent> <priority>
 

+             """Usage: %s edit-tag-inheritance [options] <tag> <parent> [<priority>]
 

  (Specify the --help global option for a list of other help options)
 

  

  Options:

file modified
+5 -5 
@@ -15,7 +15,7 @@ 
 

          self.session = mock.MagicMock()
 

          self.session.getAPIVersion.return_value = koji.API_VERSION
 

          self.activate_session_mock = mock.patch('koji_cli.commands.activate_session').start()
 

-         self.error_format = """Usage: %s remove-tag-inheritance <tag> <parent> <priority>
 

+         self.error_format = """Usage: %s remove-tag-inheritance <tag> <parent> [<priority>]
 

  (Specify the --help global option for a list of other help options)
 

  

  %s: error: {message}
 
@@ -30,7 +30,7 @@ 
 

                                  'noconfig': False,
 

                                  'parent_id': 2,
 

                                  'pkg_filter': '',
 

-                                 'priority': self.priority}
 

+                                 'priority': int(self.priority)}
 

          self.child_tag_info = {'arches': 'x86_64',
 

                                 'extra': {},
 

                                 'id': 1,
 
@@ -206,13 +206,13 @@ 
 

          self.session.getInheritanceData.assert_has_calls([call(1), call(1)])
 

          self.session.setInheritanceData.assert_called_once_with(
 

              1, [{'child_id': 1, 'intransitive': False, 'maxdepth': None, 'name': self.tag,
 

-                  'noconfig': False, 'parent_id': 2, 'pkg_filter': '', 'priority': self.priority,
 

-                  'delete link': True}])
 

+                  'noconfig': False, 'parent_id': 2, 'pkg_filter': '',
 

+                  'priority': int(self.priority), 'delete link': True}])
 

  

      def test_remove_tag_inheritance_help(self):
 

          self.assert_help(
 

              handle_remove_tag_inheritance,
 

-             """Usage: %s remove-tag-inheritance <tag> <parent> <priority>
 

+             """Usage: %s remove-tag-inheritance <tag> <parent> [<priority>]
 

  (Specify the --help global option for a list of other help options)
 

  

  Options:

file added
+128 
@@ -0,0 +1,128 @@ 
 

+ # coding: utf-8
 

+ import unittest
 

+ 

+ import mock
 

+ 

+ import koji
 

+ import kojihub
 

+ 

+ UP = kojihub.UpdateProcessor
 

+ IP = kojihub.InsertProcessor
 

+ 

+ 

+ class TestWriteInheritanceData(unittest.TestCase):
 

+ 

+     def getInsert(self, *args, **kwargs):
 

+         insert = IP(*args, **kwargs)
 

+         insert.execute = mock.MagicMock()
 

+         insert.make_create = mock.MagicMock()
 

+         self.inserts.append(insert)
 

+         return insert
 

+ 

+     def getUpdate(self, *args, **kwargs):
 

+         update = UP(*args, **kwargs)
 

+         update.execute = mock.MagicMock()
 

+         update.make_revoke = mock.MagicMock()
 

+         self.updates.append(update)
 

+         return update
 

+ 

+     def setUp(self):
 

+         self.maxDiff = None
 

+         self.InsertProcessor = mock.patch('kojihub.kojihub.InsertProcessor',
 

+                                           side_effect=self.getInsert).start()
 

+         self.inserts = []
 

+         self.UpdateProcessor = mock.patch('kojihub.kojihub.UpdateProcessor',
 

+                                           side_effect=self.getUpdate).start()
 

+         self.updates = []
 

+         self.read_inheritance_data = mock.patch('kojihub.kojihub.readInheritanceData').start()
 

+         self.get_tag = mock.patch('kojihub.kojihub.get_tag').start()
 

+         self.context = mock.patch('kojihub.kojihub.context').start()
 

+         self.context.session.assertPerm = mock.MagicMock()
 

+         self.tag_id = 5
 

+         self.changes = {'parent_id': 10, 'priority': 7, 'maxdepth': None, 'intransitive': False,
 

+                         'noconfig': False, 'pkg_filter': '', 'child_id': 5, 'is_update': True}
 

+ 

+     def tearDown(self):
 

+         mock.patch.stopall()
 

+ 

+     def test_no_value_check_fields(self):
 

+         changes = self.changes.copy()
 

+         del changes['parent_id']
 

+         with self.assertRaises(koji.GenericError) as cm:
 

+             kojihub.writeInheritanceData(self.tag_id, changes)
 

+         self.assertEqual("No value for parent_id", str(cm.exception))
 

+         self.read_inheritance_data.assert_not_called()
 

+         self.get_tag.assert_not_called()
 

+         self.assertEqual(len(self.inserts), 0)
 

+         self.assertEqual(len(self.updates), 0)
 

+ 

+     def test_valid(self):
 

+         self.read_inheritance_data.return_value = [
 

+             {'intransitive': False, 'maxdepth': None, 'name': 'test-tag', 'noconfig': False,
 

+              'parent_id': 10, 'pkg_filter': '', 'priority': 4, 'child_id': 5}]
 

+         self.get_tag.return_value = {'id': 10, 'name': 'parent_tag'}
 

+         rv = kojihub.writeInheritanceData(self.tag_id, self.changes)
 

+         self.assertEqual(rv, None)
 

+         self.read_inheritance_data.assert_called_once_with(5)
 

+         self.get_tag.assert_called_once_with(10, strict=True)
 

+         self.assertEqual(len(self.inserts), 1)
 

+         self.assertEqual(len(self.updates), 1)
 

+         update = self.updates[0]
 

+         self.assertEqual(update.table, 'tag_inheritance')
 

+         self.assertEqual(update.clauses, ['tag_id=%(tag_id)s', 'parent_id = %(parent_id)s'])
 

+ 

+         insert = self.inserts[0]
 

+         self.assertEqual(insert.table, 'tag_inheritance')
 

+         self.assertEqual(insert.data, {'parent_id': 10, 'priority': 7, 'maxdepth': None,
 

+                                        'intransitive': False, 'noconfig': False,
 

+                                        'pkg_filter': '', 'tag_id': 5})
 

+         self.assertEqual(insert.rawdata, {})
 

+ 

+     def test_delete_link(self):
 

+         changes = self.changes.copy()
 

+         changes['delete link'] = True
 

+         self.read_inheritance_data.return_value = [
 

+             {'intransitive': False, 'maxdepth': None, 'name': 'test-tag', 'noconfig': False,
 

+              'parent_id': 10, 'pkg_filter': '', 'priority': 4, 'child_id': 5}]
 

+         self.get_tag.return_value = {'id': 10, 'name': 'parent_tag'}
 

+         rv = kojihub.writeInheritanceData(self.tag_id, changes)
 

+         self.assertEqual(rv, None)
 

+         self.read_inheritance_data.assert_called_once_with(5)
 

+         self.get_tag.assert_called_once_with(10, strict=True)
 

+         self.assertEqual(len(self.inserts), 0)
 

+         self.assertEqual(len(self.updates), 1)
 

+         update = self.updates[0]
 

+         self.assertEqual(update.table, 'tag_inheritance')
 

+         self.assertEqual(update.clauses, ['tag_id=%(tag_id)s', 'parent_id = %(parent_id)s'])
 

+ 

+     def test_multiple_parent_with_the_same_priority(self):
 

+         changes = self.changes.copy()
 

+         changes = [changes, {'parent_id': 8, 'priority': 7, 'maxdepth': None,
 

+                              'intransitive': False, 'noconfig': False, 'pkg_filter': '',
 

+                              'child_id': 5, 'is_update': True}]
 

+         self.read_inheritance_data.return_value = [
 

+             {'intransitive': False, 'maxdepth': None, 'name': 'test-tag', 'noconfig': False,
 

+              'parent_id': 10, 'pkg_filter': '', 'priority': 4, 'child_id': 5}]
 

+         self.get_tag.return_value = {'id': 10, 'name': 'parent_tag'}
 

+         with self.assertRaises(koji.GenericError) as cm:
 

+             kojihub.writeInheritanceData(self.tag_id, changes, clear=True)
 

+         self.assertEqual(f"Multiple parent tags ([{changes[0]['parent_id']}, "
 

+                          f"{changes[1]['parent_id']}]) cannot have the same priority value "
 

+                          f"({changes[0]['priority']}) on child tag {changes[0]['child_id']}",
 

+                          str(cm.exception))
 

+         self.read_inheritance_data.assert_called_once_with(5)
 

+         self.get_tag.assert_has_calls([mock.call(10, strict=True)], [mock.call(7, strict=True)])
 

+         self.assertEqual(len(self.inserts), 0)
 

+         self.assertEqual(len(self.updates), 0)
 

+ 

+     def test_no_inheritance_changes(self):
 

+         self.read_inheritance_data.return_value = [
 

+             {'intransitive': False, 'maxdepth': None, 'name': 'test-tag', 'noconfig': False,
 

+              'parent_id': 10, 'pkg_filter': '', 'priority': 7, 'child_id': 5}]
 

+         self.get_tag.return_value = {'id': 10, 'name': 'parent_tag'}
 

+         rv = kojihub.writeInheritanceData(self.tag_id, self.changes)
 

+         self.assertEqual(rv, None)
 

+         self.read_inheritance_data.assert_called_once_with(5)
 

+         self.get_tag.assert_called_once_with(10, strict=True)
 

+         self.assertEqual(len(self.inserts), 0)
 

+         self.assertEqual(len(self.updates), 0)

Fix remote-tag-inheritance with priority

Fixes: https://pagure.io/koji/issue/3985

rebased onto b87f4fc2188b29a516726be95df1ebc7746f524e
2 months ago

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

rebased onto a8ab195a7887dea3f127382b4e507aac3c73f0eb
2 months ago

@tkopecek fixed

rebased onto 4318fa07b126bd9ff800197eda8facbac5def9bf
2 months ago 
+        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.

rebased onto 03db5fad1d01dc06835e3fa1c59d3ac0a8b00a5f
2 months ago

rebased onto 18a7a035590028773bd81c1a77afe57c78b0b60d
2 months ago

rebased onto cbad458fe94c4f7236ae30d725ca62f3bdba726f
2 months ago

@tkopecek @mikem all fixed

This is still not needed?

rebased onto ff97b478dd4961f59f353776a5d238c1c5dcd52e
2 months ago

@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.

rebased onto 76c986f196a71eaa284c158af0bf366b603901b2
2 months ago

rebased onto 8e007601cfdbb0668c5e07f26005a85ba6c59c47
2 months ago

Extend a bit for migration: DROP CONSTRAINT IF EXISTS

Too long line (fails flake8)

Otherwise :thumbsup:

rebased onto 4a1e76c
2 months ago

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.
Edited 2 months ago by mikem

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

rebased onto 5933d883701203b9ea04d0500ce443abc9d43d81
2 months ago

rebased onto 1291d28e7f22b99635ed4da32ffc47694709864f
2 months ago

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

rebased onto a3d155791701889b46b649393848b2625a6c0f9a
2 months ago

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.

rebased onto 7fda889
a month ago

@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:

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

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.
Edited a month ago by relias-redhat

rebased onto cfafb31
a month ago

@relias-redhat fixed

Metadata Update from @relias-redhat:
- Pull-request tagged with: testing-done
a month ago

Commit 3315e3c fixes this pull-request

Pull-Request has been merged by tkopecek
a month ago
Metadata
None

testing-ready testing-done

Changes Summary 4
+11 -9
file changed
cli/koji_cli/commands.py
+6 -3
file changed
tests/test_cli/test_edit_tag_inheritance.py
+5 -5
file changed
tests/test_cli/test_remove_tag_inheritance.py
+128
file added
tests/test_hub/test_write_inheritance_data.py
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.