From b1b0da7afd378d8601a6a7c1bb2eb66806a64312 Mon Sep 17 00:00:00 2001
From: Joseph Schorr <josephschorr@users.noreply.github.com>
Date: Tue, 2 Aug 2016 14:17:33 -0400
Subject: [PATCH] Fix off-by-one error in repo tags pagination

Fixes #1665
---
 data/model/tag.py      |  6 ++++--
 endpoints/api/tag.py   | 12 ++++-------
 test/test_api_usage.py | 48 ++++++++++++++++++++++++++++++++++++------
 3 files changed, 50 insertions(+), 16 deletions(-)

diff --git a/data/model/tag.py b/data/model/tag.py
index 007cd2e71..4b048e748 100644
--- a/data/model/tag.py
+++ b/data/model/tag.py
@@ -203,12 +203,14 @@ def list_repository_tag_history(repo_obj, page=1, size=100, specific_tag=None):
            .where(RepositoryTag.repository == repo_obj)
            .where(RepositoryTag.hidden == False)
            .order_by(RepositoryTag.lifetime_start_ts.desc(), RepositoryTag.name)
-           .paginate(page, size))
+           .limit(size + 1)
+           .offset(size * (page - 1)))
 
   if specific_tag:
     query = query.where(RepositoryTag.name == specific_tag)
 
-  return query
+  tags = list(query)
+  return tags[0:size], len(tags) > size
 
 
 def revert_tag(repo_obj, tag_name, docker_image_id):
diff --git a/endpoints/api/tag.py b/endpoints/api/tag.py
index 9aa462611..eeca246df 100644
--- a/endpoints/api/tag.py
+++ b/endpoints/api/tag.py
@@ -47,17 +47,13 @@ class ListRepositoryTags(RepositoryParamResource):
 
     page = max(1, parsed_args.get('page', 1))
     limit = min(100, max(1, parsed_args.get('limit', 50)))
+    tags, has_additional = model.tag.list_repository_tag_history(repo, page=page, size=limit,
+                                                                 specific_tag=specific_tag)
 
-    # Note: We ask for limit+1 here, so we can check to see if there are
-    # additional pages of results.
-    tags = model.tag.list_repository_tag_history(repo, page=page, size=limit+1,
-                                                 specific_tag=specific_tag)
-
-    tags = list(tags)
     return {
-      'tags': [tag_view(tag) for tag in tags[0:limit]],
+      'tags': [tag_view(tag) for tag in tags],
       'page': page,
-      'has_additional': len(tags) >= limit
+      'has_additional': has_additional,
     }
 
 
diff --git a/test/test_api_usage.py b/test/test_api_usage.py
index d0cf90589..d3a537343 100644
--- a/test/test_api_usage.py
+++ b/test/test_api_usage.py
@@ -2411,15 +2411,51 @@ class TestListAndDeleteTag(ApiTestCase):
 
     latest_image = model.tag.get_tag_image(ADMIN_ACCESS_USER, "complex", "prod")
 
-    for i in xrange(1, 100):
-      model.tag.create_or_update_tag(ADMIN_ACCESS_USER, "complex", "tag" + str(i), latest_image.docker_image_id)
+    # Create 10 tags in an empty repo.
+    user = model.user.get_user_or_org(ADMIN_ACCESS_USER)
+    repo = model.repository.create_repository(ADMIN_ACCESS_USER, "empty", user)
+
+    image = model.image.find_create_or_link_image(latest_image.docker_image_id, repo,
+                                                  ADMIN_ACCESS_USER, {}, ['local_us'])
+    remaining_tags = set()
+    for i in xrange(1, 11):
+      tag_name = "tag" + str(i)
+      remaining_tags.add(tag_name)
+      model.tag.create_or_update_tag(ADMIN_ACCESS_USER, "empty", tag_name,
+                                     image.docker_image_id)
+
+    # Make sure we can iterate over all of them.
+    json = self.getJsonResponse(ListRepositoryTags,
+                                params=dict(repository=ADMIN_ACCESS_USER + '/empty', page=1,
+                                            limit=5))
+    self.assertEquals(1, json['page'])
+    self.assertEquals(5, len(json['tags']))
+    self.assertTrue(json['has_additional'])
+
+    names = {tag['name'] for tag in json['tags']}
+    remaining_tags = remaining_tags - names
+    self.assertEquals(5, len(remaining_tags))
 
     json = self.getJsonResponse(ListRepositoryTags,
-                                params=dict(repository=ADMIN_ACCESS_USER + '/complex', page=2))
+                                params=dict(repository=ADMIN_ACCESS_USER + '/empty', page=2,
+                                            limit=5))
+
+    self.assertEquals(2, json['page'])
+    self.assertEquals(5, len(json['tags']))
+    self.assertFalse(json['has_additional'])
+
+    names = {tag['name'] for tag in json['tags']}
+    remaining_tags = remaining_tags - names
+    self.assertEquals(0, len(remaining_tags))
+
+    json = self.getJsonResponse(ListRepositoryTags,
+                                params=dict(repository=ADMIN_ACCESS_USER + '/empty', page=3,
+                                            limit=5))
+
+    self.assertEquals(3, json['page'])
+    self.assertEquals(0, len(json['tags']))
+    self.assertFalse(json['has_additional'])
 
-    # Make sure that we're able to see the second page of results.
-    assert json['page'] == 2
-    assert len(json['tags']) == 50
 
 class TestRepoPermissions(ApiTestCase):
   def listUserPermissions(self, namespace=ADMIN_ACCESS_USER, repo='simple'):