Merge pull request #2733 from charltonaustin/create_data_interface_for_subsystem_api/tag_632

refactor(data/model/tag_interface): adding in new support for tags api

endpoints/api/tag.py

@@ -2,18 +2,37 @@
 
 from flask import request, abort
 
-from endpoints.api import (
-  resource, nickname, require_repo_read, require_repo_write, RepositoryParamResource, log_action,
-  validate_json_request, path_param, parse_args, query_param, truthy_bool,
-  disallow_for_app_repositories)
-from endpoints.exception import NotFound
-from endpoints.api.image import image_view
-from endpoints.v2.manifest import _generate_and_store_manifest
-from data import model
 from auth.auth_context import get_authenticated_user
+from data import model
+from endpoints.api import (resource, nickname, require_repo_read, require_repo_write,
+                           RepositoryParamResource, log_action, validate_json_request, path_param,
+                           parse_args, query_param, truthy_bool, disallow_for_app_repositories)
+from endpoints.api.image import image_view
+from endpoints.api.tag_models_pre_oci import pre_oci_model
+from endpoints.exception import NotFound
+from endpoints.v2.manifest import _generate_and_store_manifest
 from util.names import TAG_ERROR, TAG_REGEX
 
 
+def tag_view(tag):
+  tag_info = {
+    'name': tag.name,
+    'docker_image_id': tag.docker_image_id,
+    'reversion': tag.reversion,
+  }
+
+  if tag.lifetime_start_ts > 0:
+    tag_info['start_ts'] = tag.lifetime_start_ts
+
+  if tag.lifetime_end_ts > 0:
+    tag_info['end_ts'] = tag.lifetime_end_ts
+
+  if tag.manifest_list:
+    tag_info['manifest_digest'] = tag.manifest_list
+
+  return tag_info
+
+
 @resource('/v1/repository/<apirepopath:repository>/tag/')
 @path_param('repository', 'The full path of the repository. e.g. namespace/name')
 class ListRepositoryTags(RepositoryParamResource):
@@ -28,39 +47,21 @@ class ListRepositoryTags(RepositoryParamResource):
   @query_param('page', 'Page index for the results. Default 1.', type=int, default=1)
   @nickname('listRepoTags')
   def get(self, namespace, repository, parsed_args):
-    repo = model.repository.get_repository(namespace, repository)
-    if not repo:
-      raise NotFound()
-
-    def tag_view(tag):
-      tag_info = {
-        'name': tag.name,
-        'docker_image_id': tag.image.docker_image_id,
-        'reversion': tag.reversion,
-      }
-
-      if tag.lifetime_start_ts > 0:
-        tag_info['start_ts'] = tag.lifetime_start_ts
-
-      if tag.lifetime_end_ts > 0:
-        tag_info['end_ts'] = tag.lifetime_end_ts
-
-      if tag.id in manifest_map:
-        tag_info['manifest_digest'] = manifest_map[tag.id]
-
-      return tag_info
-
     specific_tag = parsed_args.get('specificTag') or None
-
     page = max(1, parsed_args.get('page', 1))
     limit = min(100, max(1, parsed_args.get('limit', 50)))
-    tags, manifest_map, more = model.tag.list_repository_tag_history(repo, page=page, size=limit,
+
-                                                                     specific_tag=specific_tag)
+    tag_history = pre_oci_model.list_repository_tag_history(namespace_name=namespace,
+                                                            repository_name=repository, page=page,
+                                                            size=limit, specific_tag=specific_tag)
+
+    if not tag_history:
+      raise NotFound()
 
     return {
-      'tags': [tag_view(tag) for tag in tags],
+      'tags': [tag_view(tag) for tag in tag_history.tags],
       'page': page,
-      'has_additional': more,
+      'has_additional': tag_history.more,
     }
 
 
@@ -73,9 +74,7 @@ class RepositoryTag(RepositoryParamResource):
     'MoveTag': {
       'type': 'object',
       'description': 'Description of to which image a new or existing tag should point',
-      'required': [
+      'required': ['image',],
-        'image',
-      ],
       'properties': {
         'image': {
           'type': 'string',
@@ -205,9 +204,7 @@ class RestoreTag(RepositoryParamResource):
     'RestoreTag': {
       'type': 'object',
       'description': 'Restores a tag to a specific image',
-      'required': [
+      'required': ['image',],
-        'image',
-      ],
       'properties': {
         'image': {
           'type': 'string',
@@ -251,8 +248,8 @@ class RestoreTag(RepositoryParamResource):
     if existing_image is not None:
       log_data['original_image'] = existing_image.docker_image_id
 
-    log_action('revert_tag', namespace, log_data, repo=model.repository.get_repository(
+    log_action('revert_tag', namespace, log_data, repo=model.repository.get_repository(namespace,
-      namespace, repository))
+                                                                                       repository))
 
     return {
       'image_id': image_id,

endpoints/api/tag_models_interface.py

@@ -0,0 +1,43 @@
+from abc import ABCMeta, abstractmethod
+from collections import namedtuple
+
+from six import add_metaclass
+
+
+class Tag(
+    namedtuple('Tag', [
+      'name', 'image', 'reversion', 'lifetime_start_ts', 'lifetime_end_ts', 'manifest_list',
+      'docker_image_id'
+    ])):
+  """
+  Tag represents a name to an image.
+  :type name: string
+  :type image: Image
+  :type reversion: boolean
+  :type lifetime_start_ts: int
+  :type lifetime_end_ts: int
+  :type manifest_list: [manifest_digest]
+  :type docker_image_id: string
+  """
+
+
+class RepositoryTagHistory(namedtuple('RepositoryTagHistory', ['tags', 'more'])):
+  """
+  Tag represents a name to an image.
+  :type tags: [Tag]
+  :type more: boolean
+  """
+
+
+@add_metaclass(ABCMeta)
+class TagDataInterface(object):
+  """
+  Interface that represents all data store interactions required by a Tag.
+  """
+
+  @abstractmethod
+  def list_repository_tag_history(self, namespace_name, repository_name, page=1, size=100,
+                                  specific_tag=None):
+    """
+    Returns a RepositoryTagHistory with a list of historic tags and whether there are more tags then returned.
+    """

endpoints/api/tag_models_pre_oci.py

@@ -0,0 +1,30 @@
+from data import model
+from endpoints.api.tag_models_interface import TagDataInterface, Tag, RepositoryTagHistory
+
+
+class PreOCIModel(TagDataInterface):
+  """
+  PreOCIModel implements the data model for the Tags using a database schema
+  before it was changed to support the OCI specification.
+  """
+
+  def list_repository_tag_history(self, namespace_name, repository_name, page=1, size=100,
+                                  specific_tag=None):
+    repository = model.repository.get_repository(namespace_name, repository_name)
+    if repository is None:
+      return None
+    tags, manifest_map, more = model.tag.list_repository_tag_history(repository, page, size,
+                                                                     specific_tag)
+    repository_tag_history = []
+    for tag in tags:
+      manifest_list = None
+      if tag.id in manifest_map:
+        manifest_list = manifest_map[tag.id]
+      repository_tag_history.append(
+        Tag(name=tag.name, 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))
+    return RepositoryTagHistory(tags=repository_tag_history, more=more)
+
+
+pre_oci_model = PreOCIModel()

endpoints/api/test/test_models_pre_oci.py

@@ -0,0 +1,103 @@
+import pytest
+from endpoints.api.tag_models_interface import RepositoryTagHistory, Tag
+from mock import Mock
+
+from data import model
+from endpoints.api.tag_models_pre_oci import pre_oci_model
+
+EMPTY_REPOSITORY = 'empty_repository'
+EMPTY_NAMESPACE = 'empty_namespace'
+BAD_REPOSITORY_NAME = 'bad_repository_name'
+BAD_NAMESPACE_NAME = 'bad_namespace_name'
+
+
+@pytest.fixture
+def get_monkeypatch(monkeypatch):
+  return monkeypatch
+
+
+def mock_out_get_repository(monkeypatch, namespace_name, repository_name):
+  def return_none(namespace_name, repository_name):
+    return None
+
+  def return_repository(namespace_name, repository_name):
+    return 'repository'
+
+  if namespace_name == BAD_NAMESPACE_NAME or repository_name == BAD_REPOSITORY_NAME:
+    return_function = return_none
+  else:
+    return_function = return_repository
+
+  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

endpoints/api/test/test_tag.py

@@ -1,10 +1,14 @@
+import json
+
 import pytest
 
-from mock import patch, Mock
+from mock import patch, Mock, MagicMock, call
 
+
+from endpoints.api.tag_models_interface import RepositoryTagHistory, Tag
 from endpoints.api.test.shared import conduct_api_call
-from endpoints.api.tag import RepositoryTag, RestoreTag
 from endpoints.test.shared import client_with_identity
+from endpoints.api.tag import RepositoryTag, RestoreTag, ListRepositoryTags
 
 from features import FeatureNameValue
 
@@ -82,6 +86,28 @@ def authd_client(client):
     yield cl
 
 
+@pytest.fixture()
+def list_repository_tag_history():
+  def list_repository_tag_history(namespace_name, repository_name, page, size, specific_tag):
+    return RepositoryTagHistory(tags=[
+      Tag(name='First Tag', image='image', reversion=False, lifetime_start_ts=0, lifetime_end_ts=0, manifest_list=[],
+          docker_image_id='first docker image id'),
+      Tag(name='Second Tag', image='second image', reversion=True, lifetime_start_ts=10, lifetime_end_ts=100,
+          manifest_list=[], docker_image_id='second docker image id')], more=False)
+
+  with patch('endpoints.api.tag.pre_oci_model.list_repository_tag_history', side_effect=list_repository_tag_history):
+    yield
+
+
+@pytest.fixture()
+def find_no_repo_tag_history():
+  def list_repository_tag_history(namespace_name, repository_name, page, size, specific_tag):
+    return None
+
+  with patch('endpoints.api.tag.pre_oci_model.list_repository_tag_history', side_effect=list_repository_tag_history):
+    yield
+
+
 @pytest.mark.parametrize('test_image,test_tag,expected_status', [
   ('image1', '-INVALID-TAG-NAME', 400),
   ('image1', '.INVALID-TAG-NAME', 400),
@@ -95,7 +121,7 @@ def authd_client(client):
 ])
 def test_move_tag(test_image, test_tag, expected_status, get_repo_image, get_repo_tag_image,
                   create_or_update_tag, generate_manifest, authd_client):
-  params = {'repository': 'devtable/repo', 'tag': test_tag}
+  params = {'repository': 'devtable/simple', 'tag': test_tag}
   request_body = {'image': test_image}
   if expected_status is None:
     with pytest.raises(Exception):
@@ -104,6 +130,62 @@ def test_move_tag(test_image, test_tag, expected_status, get_repo_image, get_rep
     conduct_api_call(authd_client, RepositoryTag, 'put', params, request_body, expected_status)
 
 
+@pytest.mark.parametrize('namespace, repository, specific_tag, page, limit, expected_response_code, expected', [
+  ('devtable', 'simple', None, 1, 10, 200, {'has_additional': False}),
+  ('devtable', 'simple', None, 1, 10, 200, {'page': 1}),
+  ('devtable', 'simple', None, 1, 10, 200, {'tags': [{'docker_image_id': 'first docker image id',
+                                                      'name': 'First Tag',
+                                                      'reversion': False},
+                                                     {'docker_image_id': 'second docker image id',
+                                                      'end_ts': 100,
+                                                      'name': 'Second Tag',
+                                                      'reversion': True,
+                                                      'start_ts': 10}]}),
+])
+def test_list_repository_tags_view_is_correct(namespace, repository, specific_tag, page, limit,
+                                              list_repository_tag_history, expected_response_code, expected,
+                                              authd_client):
+  params = {'repository': namespace + '/' + repository, 'specificTag': specific_tag, 'page': page, 'limit': limit}
+  response = conduct_api_call(authd_client, ListRepositoryTags, 'get', params, expected_code=expected_response_code)
+  compare_list_history_tags_response(expected, response.json)
+
+
+def compare_list_history_tags_response(expected, actual):
+  if 'has_additional' in expected:
+    assert expected['has_additional'] == actual['has_additional']
+
+  if 'page' in expected:
+    assert expected['page'] == actual['page']
+
+  if 'tags' in expected:
+    assert expected['tags'] == actual['tags']
+
+
+def test_no_repo_tag_history(find_no_repo_tag_history, authd_client):
+  params = {'repository': 'devtable/simple', 'specificTag': None, 'page': 1, 'limit': 10}
+  conduct_api_call(authd_client, ListRepositoryTags, 'get', params, expected_code=404)
+
+
+@pytest.mark.parametrize(
+  'specific_tag, page, limit, expected_specific_tag, expected_page, expected_limit', [
+    (None, None, None, None, 1, 50),
+    ('specific_tag', 12, 13, 'specific_tag', 12, 13),
+    ('specific_tag', -1, 101, 'specific_tag', 1, 100),
+    ('specific_tag', 0, 0, 'specific_tag', 1, 1),
+  ])
+def test_repo_tag_history_param_parse(specific_tag, page, limit, expected_specific_tag, expected_page, expected_limit,
+                                      authd_client):
+  mock = MagicMock()
+  mock.return_value = RepositoryTagHistory(tags=[], more=False)
+
+  with patch('endpoints.api.tag.pre_oci_model.list_repository_tag_history', side_effect=mock):
+    params = {'repository': 'devtable/simple', 'specificTag': specific_tag, 'page': page, 'limit': limit}
+    conduct_api_call(authd_client, ListRepositoryTags, 'get', params)
+
+  assert mock.call_args == call(namespace_name='devtable', repository_name='simple',
+                                page=expected_page, size=expected_limit, specific_tag=expected_specific_tag)
+
+
 @pytest.mark.parametrize('test_manifest,test_tag,manifest_generated,expected_status', [
   (None, 'newtag', True, 200),
   (None, 'generatemanifestfail', True, None),
@@ -112,7 +194,7 @@ def test_move_tag(test_image, test_tag, expected_status, get_repo_image, get_rep
 def test_restore_tag(test_manifest, test_tag, manifest_generated, expected_status, get_repository,
                      restore_tag_to_manifest, restore_tag_to_image, generate_manifest,
                      authd_client):
-  params = {'repository': 'devtable/repo', 'tag': test_tag}
+  params = {'repository': 'devtable/simple', 'tag': test_tag}
   request_body = {'image': 'image1'}
   if test_manifest is not None:
     request_body['manifest_digest'] = test_manifest
@@ -123,4 +205,4 @@ def test_restore_tag(test_manifest, test_tag, manifest_generated, expected_statu
     conduct_api_call(authd_client, RestoreTag, 'post', params, request_body, expected_status)
 
   if manifest_generated:
-    generate_manifest.assert_called_with('devtable', 'repo', test_tag)
+    generate_manifest.assert_called_with('devtable', 'simple', test_tag)