Fix bug in listing owned tags

We were indexing into a map using the docker_image_id, but the ancestors use the *image id*. Also cleans up the code and adds some tests.

Fixes https://jira.prod.coreos.systems/browse/QS-55

endpoints/api/tag.py

@@ -172,14 +172,11 @@ class RepositoryTagImages(RepositoryParamResource):
     if tag_image is None:
       raise NotFound()
 
+    # Find all the parent images for the tag.
     parent_images = model.get_parent_images(namespace, repository, tag_image.docker_image_id)
-    image_map = {str(tag_image.docker_image_id): tag_image}
-
-    for image in parent_images:
-      image_map[str(image.docker_image_id)] = image
-
-    image_map_all = dict(image_map)
     all_images = [tag_image] + list(parent_images)
+    image_map = {image.docker_image_id: image for image in all_images}
+    skip_set = set()
 
     # Filter the images returned to those not found in the ancestry of any of the other tags in
     # the repository.
@@ -189,18 +186,13 @@ class RepositoryTagImages(RepositoryParamResource):
         if current_tag.name == tag:
           continue
 
-        # Remove the tag's image ID.
+        skip_set.add(current_tag.image.ancestor_id)
-        tag_image_id = str(current_tag.docker_image_id)
+        skip_set = skip_set | set(current_tag.image.ancestor_id_list)
-        image_map.pop(tag_image_id, None)
-
-        # Remove any ancestors:
-        for ancestor_id in current_tag.image.ancestors.split('/'):
-          image_map.pop(ancestor_id, None)
 
     return {
       'images': [
-        image.to_dict(image_map_all) for image in all_images
+        image.to_dict(image_map) for image in all_images
-        if not parsed_args['owned'] or (str(image.docker_image_id) in image_map)
+        if not parsed_args['owned'] or (image.ancestor_id not in skip_set)
       ]
     }
 

endpoints/api/tag_models_interface.py

@@ -61,7 +61,7 @@ class Repository(namedtuple('Repository', ['namespace_name', 'repository_name'])
 class Image(
   namedtuple('Image', [
     'docker_image_id', 'created', 'comment', 'command', 'storage_image_size',
-    'storage_uploading', 'ancestor_id_list'
+    'storage_uploading', 'ancestor_id_list', 'ancestor_id'
   ])):
   """
   Image
@@ -72,6 +72,7 @@ class Image(
   :type storage_image_size: int
   :type storage_uploading: boolean
   :type ancestor_id_list: [int]
+  :type ancestor_id: int
   """
 
   def to_dict(self, image_map, include_ancestors=True):

endpoints/api/tag_models_pre_oci.py

@@ -120,11 +120,12 @@ def convert_image(database_image):
                comment=database_image.comment, command=database_image.command,
                storage_image_size=database_image.storage.image_size,
                storage_uploading=database_image.storage.uploading,
-               ancestor_id_list=database_image.ancestor_id_list())
+               ancestor_id_list=database_image.ancestor_id_list(),
+               ancestor_id=database_image.id)
 
 
 def convert_tag(tag, manifest_list=None):
-  return Tag(name=tag.name, image=tag.image, reversion=tag.reversion,
+  return Tag(name=tag.name, image=convert_image(tag.image), reversion=tag.reversion,
              lifetime_start_ts=tag.lifetime_start_ts, lifetime_end_ts=tag.lifetime_end_ts,
              manifest_list=manifest_list, docker_image_id=tag.image.docker_image_id)
 

endpoints/api/test/test_tag.py

@@ -8,7 +8,7 @@ from data.model import DataModelException
 from endpoints.api.tag_models_interface import RepositoryTagHistory, Tag
 from endpoints.api.test.shared import conduct_api_call
 from endpoints.test.shared import client_with_identity
-from endpoints.api.tag import RepositoryTag, RestoreTag, ListRepositoryTags
+from endpoints.api.tag import RepositoryTag, RestoreTag, ListRepositoryTags, RepositoryTagImages
 
 from features import FeatureNameValue
 
@@ -281,3 +281,17 @@ def test_restore_tag(test_manifest, test_tag, manifest_generated, expected_statu
 
   if manifest_generated:
     generate_manifest.assert_called_with('devtable', 'simple', test_tag)
+
+@pytest.mark.parametrize('repository, tag, owned, expect_images', [
+  ('devtable/simple', 'prod', False, True),
+  ('devtable/simple', 'prod', True, False),
+  ('devtable/simple', 'latest', False, True),
+  ('devtable/simple', 'latest', True, False),
+
+  ('devtable/complex', 'prod', False, True),
+  ('devtable/complex', 'prod', True, True),
+])
+def test_list_tag_images(repository, tag, owned, expect_images, authd_client):
+  params = {'repository': repository, 'tag': tag, 'owned': owned}
+  result = conduct_api_call(authd_client, RepositoryTagImages, 'get', params, None, 200).json
+  assert bool(result['images']) == expect_images

endpoints/api/test/test_tag_models_pre_oci.py

@@ -34,78 +34,6 @@ def mock_out_get_repository(monkeypatch, namespace_name, repository_name):
   monkeypatch.setattr(model.repository, 'get_repository', return_function)
 
 
-def create_mock_tag(name, reversion, lifetime_start_ts, lifetime_end_ts, mock_id, docker_image_id,
-                    manifest_list):
-  tag_mock = Mock()
-  tag_mock.name = name
-  image_mock = Mock()
-  image_mock.docker_image_id = docker_image_id
-  tag_mock.image = image_mock
-  tag_mock.reversion = reversion
-  tag_mock.lifetime_start_ts = lifetime_start_ts
-  tag_mock.lifetime_end_ts = lifetime_end_ts
-  tag_mock.id = mock_id
-  tag_mock.manifest_list = manifest_list
-  tag = Tag(name=name, reversion=reversion, image=image_mock, docker_image_id=docker_image_id,
-            lifetime_start_ts=lifetime_start_ts, lifetime_end_ts=lifetime_end_ts,
-            manifest_list=manifest_list)
-  return tag_mock, tag
-
-
-first_mock, first_tag = create_mock_tag('tag1', 'rev1', 'start1', 'end1', 'id1',
-                                        'docker_image_id1', [])
-second_mock, second_tag = create_mock_tag('tag2', 'rev2', 'start2', 'end2', 'id2',
-                                          'docker_image_id2', ['manifest'])
-
-
-def mock_out_list_repository_tag_history(monkeypatch, namespace_name, repository_name, page, size,
-                                         specific_tag):
-  def list_empty_tag_history(repository, page, size, specific_tag):
-    return [], {}, False
-
-  def list_filled_tag_history(repository, page, size, specific_tag):
-    tags = [first_mock, second_mock]
-    return tags, {
-      first_mock.id: first_mock.manifest_list,
-      second_mock.id: second_mock.manifest_list
-    }, len(tags) > size
-
-  def list_only_second_tag(repository, page, size, specific_tag):
-    tags = [second_mock]
-    return tags, {second_mock.id: second_mock.manifest_list}, len(tags) > size
-
-  if namespace_name == EMPTY_NAMESPACE or repository_name == EMPTY_REPOSITORY:
-    return_function = list_empty_tag_history
-  else:
-    if specific_tag == 'tag2':
-      return_function = list_only_second_tag
-    else:
-      return_function = list_filled_tag_history
-
-  monkeypatch.setattr(model.tag, 'list_repository_tag_history', return_function)
-
-
-@pytest.mark.parametrize(
-  'expected, namespace_name,        repository_name,       page,  size, specific_tag', [
-    (None, BAD_NAMESPACE_NAME, 'repository_name', 1, 100, None),
-    (None, 'namespace_name', BAD_REPOSITORY_NAME, 1, 100, None),
-    (RepositoryTagHistory(tags=[], more=False), EMPTY_NAMESPACE, EMPTY_REPOSITORY, 1, 100, None),
-    (RepositoryTagHistory(tags=[first_tag, second_tag], more=False), 'namespace', 'repository', 1,
-     100, None),
-    (RepositoryTagHistory(tags=[first_tag, second_tag], more=True), 'namespace', 'repository', 1,
-     1, None),
-    (RepositoryTagHistory(tags=[second_tag], more=False), 'namespace', 'repository', 1, 100,
-     'tag2'),
-  ])
-def test_list_repository_tag_history(expected, namespace_name, repository_name, page, size,
-                                     specific_tag, get_monkeypatch):
-  mock_out_get_repository(get_monkeypatch, namespace_name, repository_name)
-  mock_out_list_repository_tag_history(get_monkeypatch, namespace_name, repository_name, page,
-                                       size, specific_tag)
-  assert pre_oci_model.list_repository_tag_history(namespace_name, repository_name, page, size,
-                                                   specific_tag) == expected
-
-
 def get_repo_mock(monkeypatch, return_value):
   def return_return_value(namespace_name, repository_name):
     return return_value
@@ -238,60 +166,6 @@ def test_get_parent_images_empty_parent_images(get_monkeypatch):
   assert images == []
 
 
-def compare_images(parent_image, attr_dict):
-  for field in parent_image._fields:
-    assert getattr(parent_image, field) == getattr(attr_dict, field)
-
-
-def test_get_parent_images(get_monkeypatch):
-  get_image_by_id_mock = Mock()
-  get_monkeypatch.setattr(model.image, 'get_image_by_id', get_image_by_id_mock)
-
-  def fake_list():
-    return []
-
-  image_one = AttrDict({
-    'docker_image_id': 'docker_image_id',
-    'created': 'created_one',
-    'comment': 'comment_one',
-    'command': 'command_one',
-    'storage': AttrDict({
-      'image_size': 'image_size_one',
-      'uploading': 'uploading_one'
-    }),
-    'ancestors': 'one/two/three',
-    'ancestor_id_list': fake_list
-  })
-  image_two = AttrDict({
-    'docker_image_id': 'docker_image_id_two',
-    'created': 'created',
-    'comment': 'comment',
-    'command': 'command',
-    'storage': AttrDict({
-      'image_size': 'image_size',
-      'uploading': 'uploading'
-    }),
-    'ancestors': 'four/five/six',
-    'ancestor_id_list': fake_list
-  })
-
-  get_parent_images_mock = Mock(return_value=[image_one, image_two])
-  get_monkeypatch.setattr(model.image, 'get_parent_images', get_parent_images_mock)
-  get_monkeypatch.setattr(model.image, 'get_image_by_id', Mock())
-
-  images = pre_oci_model.get_parent_images('namespace_name', 'repository_name', 'tag_name')
-  image_one.ancestor_id_list = []
-  image_one.storage_image_size = 'image_size_one'
-  image_one.storage_uploading = 'uploading_one'
-  image_one.ancestor_length = 13
-  image_two.ancestor_id_list = []
-  image_two.storage_uploading = 'uploading'
-  image_two.storage_image_size = 'image_size'
-  image_two.ancestor_length = 13
-  compare_images(images[0], image_one)
-  compare_images(images[1], image_two)
-
-
 def test_list_repository_tags(get_monkeypatch):
   mock = Mock(return_value=[])