refactor(data+endpoints): code review changes

this puts the view logic on the object and adds a parameter for logging

[TESTING->locally with docker compose]

Issue: https://coreosdev.atlassian.net/browse/QUAY-632

- [ ] It works!
- [ ] Comments provide sufficient explanations for the next contributor
- [ ] Tests cover changes and corner cases
- [ ] Follows Quay syntax patterns and format

data/model/log.py

@@ -108,11 +108,6 @@ def log_action(kind_name, user_or_organization_name, performer=None, repository=
     performer = performer.id
 
   if repository is not None:
-    if hasattr(repository, 'namespace_name') and hasattr(repository, 'repository_name'):
-      maybe_repo = data.model.repository.get_repository(repository.namespace_name,
-                                                        repository.repository_name)
-      repository = maybe_repo.id if maybe_repo else None
-    elif hasattr(repository, 'id'):
     repository = repository.id
 
   kind = _get_log_entry_kind(kind_name)

endpoints/api/__init__.py

@@ -353,7 +353,7 @@ def request_error(exception=None, **kwargs):
   raise InvalidRequest(message, data)
 
 
-def log_action(kind, user_or_orgname, metadata=None, repo=None):
+def log_action(kind, user_or_orgname, metadata=None, repo=None, repo_name=None):
   if not metadata:
     metadata = {}
 
@@ -364,8 +364,14 @@ def log_action(kind, user_or_orgname, metadata=None, repo=None):
     metadata['oauth_token_application'] = oauth_token.application.name
 
   performer = get_authenticated_user()
+
+  if repo_name:
+    repository = model.repository.get_repository(user_or_orgname, repo_name)
+  else:
+    repository = repo
+
   model.log.log_action(kind, user_or_orgname, performer=performer, ip=request.remote_addr,
-                       metadata=metadata, repository=repo)
+                       metadata=metadata, repository=repository)
 
 
 def define_json_response(schema_name):

endpoints/api/image.py

@@ -9,33 +9,6 @@ from endpoints.exception import NotFound
 from data import model
 
 
-def image_view_pre_oci(image, image_map, include_ancestors=True):
-  command = image.command
-
-  def docker_id(aid):
-    if aid not in image_map:
-      return ''
-
-    return image_map[aid].docker_image_id
-
-  image_data = {
-    'id': image.docker_image_id,
-    'created': format_date(image.created),
-    'comment': image.comment,
-    'command': json.loads(command) if command else None,
-    'size': image.storage_image_size,
-    'uploading': image.storage_uploading,
-    'sort_index': image.ancestor_length,
-  }
-
-  if include_ancestors:
-    # Calculate the ancestors string, with the DBID's replaced with the docker IDs.
-    ancestors = [docker_id(a) for a in image.ancestor_id_list]
-    image_data['ancestors'] = '/{0}/'.format('/'.join(ancestors))
-
-  return image_data
-
-
 def image_view(image, image_map, include_ancestors=True):
   command = image.command
 

endpoints/api/tag.py

@@ -7,7 +7,6 @@ from data.model import DataModelException
 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_pre_oci
 from endpoints.api.tag_models_interface import Repository
 from endpoints.api.tag_models_pre_oci import pre_oci_model as model
 from endpoints.exception import NotFound
@@ -15,25 +14,6 @@ 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):
@@ -60,7 +40,7 @@ class ListRepositoryTags(RepositoryParamResource):
       raise NotFound()
 
     return {
-      'tags': [tag_view(tag) for tag in tag_history.tags],
+      'tags': [tag.to_dict() for tag in tag_history.tags],
       'page': page,
       'has_additional': tag_history.more,
     }
@@ -111,7 +91,7 @@ class RepositoryTag(RepositoryParamResource):
       'namespace': namespace,
       'image': image_id,
       'original_image': original_image_id
-    }, repo=repo)
+    }, repo_name=repository)
 
     _generate_and_store_manifest(namespace, repository, tag)
 
@@ -129,7 +109,7 @@ class RepositoryTag(RepositoryParamResource):
                {'username': username,
                 'repo': repository,
                 'namespace': namespace,
-                'tag': tag}, repo=Repository(namespace_name=namespace, repository_name=repository))
+                'tag': tag}, repo_name=repository)
 
     return '', 204
 
@@ -154,6 +134,9 @@ class RepositoryTagImages(RepositoryParamResource):
     except DataModelException:
       raise NotFound()
 
+    if tag_image is None:
+      raise NotFound()
+
     parent_images = model.get_parent_images(namespace, repository, tag_image.docker_image_id)
     image_map = {str(tag_image.docker_image_id): tag_image}
 
@@ -181,7 +164,7 @@ class RepositoryTagImages(RepositoryParamResource):
 
     return {
       'images': [
-        image_view_pre_oci(image, image_map_all) for image in all_images
+        image.to_dict(image_map_all) for image in all_images
         if not parsed_args['owned'] or (str(image.docker_image_id) in image_map)
       ]
     }
@@ -239,7 +222,7 @@ 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=repo)
+    log_action('revert_tag', namespace, log_data, repo_name=repository)
 
     return {
       'image_id': image_id,

endpoints/api/tag_models_interface.py

@@ -1,8 +1,11 @@
+import json
 from abc import ABCMeta, abstractmethod
 from collections import namedtuple
 
 from six import add_metaclass
 
+from endpoints.api import format_date
+
 
 class Tag(
   namedtuple('Tag', [
@@ -20,6 +23,24 @@ class Tag(
   :type docker_image_id: string
   """
 
+  def to_dict(self):
+    tag_info = {
+      'name': self.name,
+      'docker_image_id': self.docker_image_id,
+      'reversion': self.reversion,
+    }
+
+    if self.lifetime_start_ts > 0:
+      tag_info['start_ts'] = self.lifetime_start_ts
+
+    if self.lifetime_end_ts > 0:
+      tag_info['end_ts'] = self.lifetime_end_ts
+
+    if self.manifest_list:
+      tag_info['manifest_digest'] = self.manifest_list
+
+    return tag_info
+
 
 class RepositoryTagHistory(namedtuple('RepositoryTagHistory', ['tags', 'more'])):
   """
@@ -52,9 +73,34 @@ class Image(
   :type storage_uploading: boolean
   :type ancestor_length: int
   :type ancestor_id_list: [int]
-
   """
 
+  def to_dict(self, image_map, include_ancestors=True):
+    command = self.command
+
+    def docker_id(aid):
+      if aid not in image_map:
+        return ''
+
+      return image_map[aid].docker_image_id
+
+    image_data = {
+      'id': self.docker_image_id,
+      'created': format_date(self.created),
+      'comment': self.comment,
+      'command': json.loads(command) if command else None,
+      'size': self.storage_image_size,
+      'uploading': self.storage_uploading,
+      'sort_index': self.ancestor_length,
+    }
+
+    if include_ancestors:
+      # Calculate the ancestors string, with the DBID's replaced with the docker IDs.
+      ancestors = [docker_id(a) for a in self.ancestor_id_list]
+      image_data['ancestors'] = '/{0}/'.format('/'.join(ancestors))
+
+    return image_data
+
 
 @add_metaclass(ABCMeta)
 class TagDataInterface(object):

endpoints/api/tag_models_pre_oci.py

@@ -35,7 +35,7 @@ class PreOCIModel(TagDataInterface):
     return Repository(image.repository.namespace_user, image.repository.name)
 
   def get_repo_tag_image(self, repository, tag_name):
-    repo = model.repository.get_repository(repository.namespace_name, repository.repository_name)
+    repo = model.repository.get_repository(str(repository.namespace_name), str(repository.repository_name))
     if repo is None:
       return None