#4048 Fix seeing the diff on closed PR and deleted forks
Opened 2 years ago by pingou. Modified 10 months ago

file modified
+2
@@ -1957,6 +1957,8 @@ 

          request.project.user.username if request.project.is_fork else None,

          request.id,

      )

+     # Update the start and stop commits

+     pagure.lib.tasks.update_pull_request.delay(request.uid)

  

      return request

  

file modified
+4 -4
@@ -266,7 +266,7 @@ 

      if request.status != "Open":

          commitid = request.commit_stop

          try:

-             for commit in repo_obj.walk(commitid, pygit2.GIT_SORT_NONE):

+             for commit in orig_repo.walk(commitid, pygit2.GIT_SORT_NONE):

                  diff_commits.append(commit)

                  if commit.oid.hex == request.commit_start:

                      break
@@ -287,9 +287,9 @@ 

              if start == diff_commits[0].oid.hex:

                  diff = diff_commits[0].tree.diff_to_tree(swap=True)

              else:

-                 diff = repo_obj.diff(

-                     repo_obj.revparse_single(start),

-                     repo_obj.revparse_single(diff_commits[0].oid.hex),

+                 diff = orig_repo.diff(

+                     orig_repo.revparse_single(start),

+                     orig_repo.revparse_single(diff_commits[0].oid.hex),

                  )

      else:

          try:

@@ -32,7 +32,7 @@ 

  from pagure.lib.repo import PagureRepo

  

  

- class PagureFlaskPrNoSourcestests(tests.Modeltests):

+ class BasePrNoSourcestests(tests.Modeltests):

      """ Tests PR in pagure when the source is gone """

  

      maxDiff = None
@@ -41,7 +41,7 @@ 

      @patch('pagure.lib.notify.fedmsg_publish', MagicMock(return_value=True))

      def setUp(self):

          """ Set up the environnment, ran before every tests. """

-         super(PagureFlaskPrNoSourcestests, self).setUp()

+         super(BasePrNoSourcestests, self).setUp()

  

          tests.create_projects(self.session)

          tests.create_projects_git(
@@ -65,6 +65,7 @@ 

          project = pagure.lib.query.get_authorized_project(self.session, 'test')

          fork = pagure.lib.query.get_authorized_project(

              self.session, 'test', user='foo')

+         print('fork', fork.fullname)

  

          self.set_up_git_repo(repo=project, fork=fork)

  
@@ -76,13 +77,7 @@ 

          path = os.path.join(

              self.path, 'repos', 'test.git',

              'refs', 'pull', '1', 'head')

-         cnt = 0

-         while not os.path.exists(path):

-             time.sleep(0.1)

-             cnt += 1

-             if cnt == 100:

-                 # We're about 10 seconds in, let's bail

-                 raise Exception('Sorry, worker took too long')

+         self.assertTrue(os.path.exists(path))

  

      def set_up_git_repo(self, repo, fork, branch_from='feature'):

          """ Set up the git repo and create the corresponding PullRequest
@@ -174,6 +169,18 @@ 

  

          shutil.rmtree(newpath)

  

+ 

+ class PagureFlaskPrNoSourcestests(BasePrNoSourcestests):

+     """ Tests PR in pagure when the source is gone """

+ 

+     maxDiff = None

+ 

+     @patch('pagure.lib.notify.send_email', MagicMock(return_value=True))

+     @patch('pagure.lib.notify.fedmsg_publish', MagicMock(return_value=True))

+     def setUp(self):

+         """ Set up the environnment, ran before every tests. """

+         super(PagureFlaskPrNoSourcestests, self).setUp()

+ 

      def test_request_pull_reference(self):

          """ Test if there is a reference created for a new PR. """

  
@@ -219,6 +226,11 @@ 

          # View the pull-request

          output2 = self.app.get('/test/pull-request/1')

          self.assertEqual(output2.status_code, 200)

+         output_text = output2.get_data(as_text=True)

+         self.assertIn('''<span>Files Changed&nbsp;</span>

+       <span class="badge badge-secondary badge-pill">

+         1

+       </span>''', output_text)

  

      def test_accessing_pr_patch_fork_deleted(self):

          """ Test accessing the PR's patch if the fork has been deleted. """
@@ -274,6 +286,11 @@ 

          # View the pull-request

          output2 = self.app.get('/test/pull-request/1')

          self.assertEqual(output2.status_code, 200)

+         output_text = output2.get_data(as_text=True)

+         self.assertIn('''<span>Files Changed&nbsp;</span>

+       <span class="badge badge-secondary badge-pill">

+         1

+       </span>''', output_text)

  

      def test_accessing_pr_patch_branch_deleted(self):

          """ Test accessing the PR's patch if branch it originates from has
@@ -307,9 +324,119 @@ 

          # View the pull-request

          output = self.app.get('/test/pull-request/1.patch')

          self.assertEqual(output.status_code, 200)

+         output_text = output.get_data(as_text=True)

          self.assertIn(

              '--- a/sources\n+++ b/sources\n@@ -1,2 +1,4 @@',

-             output.get_data(as_text=True))

+             output_text)

+ 

+     def test_accessing_pr_patch_fork_deleted_recreated(self):

+         """ Test accessing the PR's patch if the fork has been deleted

+         and re-created. """

+ 

+         # Delete fork on disk

+         project = pagure.lib.query.get_authorized_project(

+             self.session, 'test', user='foo')

+         repo_path = os.path.join(self.path, 'repos', project.path)

+         self.assertTrue(os.path.exists(repo_path))

+         shutil.rmtree(repo_path)

+         self.assertFalse(os.path.exists(repo_path))

+ 

+         # Delete fork in the DB

+         self.session.delete(project)

+         self.session.commit()

+ 

+         # Re-create foo's fork of pingou's test project

+         item = pagure.lib.model.Project(

+             user_id=2,  # foo

+             name='test',

+             description='test project #1',

+             hook_token='aaabbb',

+             is_fork=True,

+             parent_id=1,

+         )

+         self.session.add(item)

+         self.session.commit()

+         # Create the fork's git repo

+         repo_path = os.path.join(self.path, 'repos', item.path)

+         pygit2.init_repository(repo_path, bare=True)

+ 

+         # View the pull-request

+         output = self.app.get('/test/pull-request/1')

+         self.assertEqual(output.status_code, 200)

+         output_text = output.get_data(as_text=True)

+         self.assertIn('''<span>Commits&nbsp;</span>

+       <span class="badge badge-secondary badge-pill">

+         1

+       </span>''', output_text)

+         self.assertIn('''<span>Files Changed&nbsp;</span>

+       <span class="badge badge-secondary badge-pill">

+         1

+       </span>''', output_text)

+ 

+ 

+ class PagureFlaskClosedPrNoSourcestests(BasePrNoSourcestests):

+     """ Tests viewing closed PR in pagure when the source is gone """

+ 

+     maxDiff = None

+ 

+     @patch('pagure.lib.notify.send_email', MagicMock(return_value=True))

+     @patch('pagure.lib.notify.fedmsg_publish', MagicMock(return_value=True))

+     def setUp(self):

+         """ Set up the environnment, ran before every tests. """

+         super(PagureFlaskClosedPrNoSourcestests, self).setUp()

+ 

+         # Ensure things got setup straight

+         project = pagure.lib.query.get_authorized_project(self.session, 'test')

+         self.assertEqual(len(project.requests), 1)

+         request = project.requests[0]

+         self.assertEqual(request.status, "Open")

+         request.status = "Closed"

+         self.session.add(request)

+         self.session.commit()

+ 

+     def test_accessing_pr_patch_fork_deleted_recreated(self):

+         """ Test accessing the PR's patch if the fork has been deleted

+         and re-created. """

+ 

+         # Delete fork on disk

+         project = pagure.lib.query.get_authorized_project(

+             self.session, 'test', user='foo')

+         repo_path = os.path.join(self.path, 'repos', project.path)

+         self.assertTrue(os.path.exists(repo_path))

+         shutil.rmtree(repo_path)

+         self.assertFalse(os.path.exists(repo_path))

+ 

+         # Delete fork in the DB

+         self.session.delete(project)

+         self.session.commit()

+ 

+         # Re-create foo's fork of pingou's test project

+         item = pagure.lib.model.Project(

+             user_id=2,  # foo

+             name='test',

+             description='test project #1',

+             hook_token='aaabbb',

+             is_fork=True,

+             parent_id=1,

+         )

+         self.session.add(item)

+         self.session.commit()

+         # Create the fork's git repo

+         fork_path = os.path.join(self.path, 'repos', item.path)

+         pygit2.clone_repository(repo_path, fork_path, bare=True)

+ 

+         # View the pull-request

+         output = self.app.get('/test/pull-request/1')

+         self.assertEqual(output.status_code, 200)

+         output_text = output.get_data(as_text=True)

+         self.assertIn('''<span>Commits&nbsp;</span>

+       <span class="badge badge-secondary badge-pill">

+         1

+       </span>''', output_text)

+         self.assertIn('''<span>Files Changed&nbsp;</span>

+       <span class="badge badge-secondary badge-pill">

+         1

+       </span>''', output_text)

  

  

  if __name__ == '__main__':

Basically, we were looking at the fork to generate the diff of the
PR, but if the forks has been deleted and later re-created the
commits are gone from the forks, so the diff appears empty.
If we switch from using the fork to using the target project, since
we pulled the object in the target repo when creating the refs
refs/pull/<id>/head, it will work just fine.
This is what this commit does, it moves the logic of the diff from
the fork repo to the target repo.

Thanks to Slavek Kabrda to pointing this one out and figuring out
the fix.

Signed-off-by: Pierre-Yves Chibon pingou@pingoured.fr

It'll need tests before we can merge this one

rebased onto 1f442860ba89347e71644f99c39b6583200f9cb2

2 years ago

rebased onto e277b46ffad5e65f76b949c888e66982e162e1d2

2 years ago

Ok, I've spent quite some time trying to get tests for this to run, ie to fail on master and pass on this branch.

This the change I ended up with:

From af0cad9e351375e9be03a5d04ca0e2271d25cf7d Mon Sep 17 00:00:00 2001
From: Pierre-Yves Chibon <pingou@pingoured.fr>
Date: Thu, 29 Nov 2018 12:50:20 +0100
Subject: [PATCH] Try replicating the issue with closed PRs in tests

Signed-off-by: Pierre-Yves Chibon <pingou@pingoured.fr>
---
 pagure/lib/query.py                         |   2 +
 tests/test_pagure_flask_ui_pr_no_sources.py | 147 ++++++++++++++++++++++++++--
 2 files changed, 139 insertions(+), 10 deletions(-)

diff --git a/pagure/lib/query.py b/pagure/lib/query.py
index 03baa608..4238f262 100644
--- a/pagure/lib/query.py
+++ b/pagure/lib/query.py
@@ -1943,6 +1943,8 @@ def new_pull_request(
         request.project.user.username if request.project.is_fork else None,
         request.id,
     )
+    # Update the start and stop commits
+    pagure.lib.tasks.update_pull_request.delay(request.uid)

     return request

diff --git a/tests/test_pagure_flask_ui_pr_no_sources.py b/tests/test_pagure_flask_ui_pr_no_sources.py
index 70a99628..07945ad3 100644
--- a/tests/test_pagure_flask_ui_pr_no_sources.py
+++ b/tests/test_pagure_flask_ui_pr_no_sources.py
@@ -32,7 +32,7 @@ import tests
 from pagure.lib.repo import PagureRepo


-class PagureFlaskPrNoSourcestests(tests.Modeltests):
+class BasePrNoSourcestests(tests.Modeltests):
     """ Tests PR in pagure when the source is gone """

     maxDiff = None
@@ -41,7 +41,7 @@ class PagureFlaskPrNoSourcestests(tests.Modeltests):
     @patch('pagure.lib.notify.fedmsg_publish', MagicMock(return_value=True))
     def setUp(self):
         """ Set up the environnment, ran before every tests. """
-        super(PagureFlaskPrNoSourcestests, self).setUp()
+        super(BasePrNoSourcestests, self).setUp()

         tests.create_projects(self.session)
         tests.create_projects_git(
@@ -65,6 +65,7 @@ class PagureFlaskPrNoSourcestests(tests.Modeltests):
         project = pagure.lib.query.get_authorized_project(self.session, 'test')
         fork = pagure.lib.query.get_authorized_project(
             self.session, 'test', user='foo')
+        print('fork', fork.fullname)

         self.set_up_git_repo(repo=project, fork=fork)

@@ -76,13 +77,7 @@ class PagureFlaskPrNoSourcestests(tests.Modeltests):
         path = os.path.join(
             self.path, 'repos', 'test.git',
             'refs', 'pull', '1', 'head')
-        cnt = 0
-        while not os.path.exists(path):
-            time.sleep(0.1)
-            cnt += 1
-            if cnt == 100:
-                # We're about 10 seconds in, let's bail
-                raise Exception('Sorry, worker took too long')
+        self.assertTrue(os.path.exists(path))

     def set_up_git_repo(self, repo, fork, branch_from='feature'):
         """ Set up the git repo and create the corresponding PullRequest
@@ -174,6 +169,18 @@ class PagureFlaskPrNoSourcestests(tests.Modeltests):

         shutil.rmtree(newpath)

+
+class PagureFlaskPrNoSourcestests(BasePrNoSourcestests):
+    """ Tests PR in pagure when the source is gone """
+
+    maxDiff = None
+
+    @patch('pagure.lib.notify.send_email', MagicMock(return_value=True))
+    @patch('pagure.lib.notify.fedmsg_publish', MagicMock(return_value=True))
+    def setUp(self):
+        """ Set up the environnment, ran before every tests. """
+        super(PagureFlaskPrNoSourcestests, self).setUp()
+
     def test_request_pull_reference(self):
         """ Test if there is a reference created for a new PR. """

@@ -219,6 +226,11 @@ class PagureFlaskPrNoSourcestests(tests.Modeltests):
         # View the pull-request
         output2 = self.app.get('/test/pull-request/1')
         self.assertEqual(output2.status_code, 200)
+        output_text = output2.get_data(as_text=True)
+        self.assertIn('''<span>Files Changed&nbsp;</span>
+      <span class="badge badge-secondary badge-pill">
+        1
+      </span>''', output_text)

     def test_accessing_pr_patch_fork_deleted(self):
         """ Test accessing the PR's patch if the fork has been deleted. """
@@ -274,6 +286,11 @@ class PagureFlaskPrNoSourcestests(tests.Modeltests):
         # View the pull-request
         output2 = self.app.get('/test/pull-request/1')
         self.assertEqual(output2.status_code, 200)
+        output_text = output2.get_data(as_text=True)
+        self.assertIn('''<span>Files Changed&nbsp;</span>
+      <span class="badge badge-secondary badge-pill">
+        1
+      </span>''', output_text)

     def test_accessing_pr_patch_branch_deleted(self):
         """ Test accessing the PR's patch if branch it originates from has
@@ -307,9 +324,119 @@ class PagureFlaskPrNoSourcestests(tests.Modeltests):
         # View the pull-request
         output = self.app.get('/test/pull-request/1.patch')
         self.assertEqual(output.status_code, 200)
+        output_text = output.get_data(as_text=True)
         self.assertIn(
             '--- a/sources\n+++ b/sources\n@@ -1,2 +1,4 @@',
-            output.get_data(as_text=True))
+            output_text)
+
+    def test_accessing_pr_patch_fork_deleted_recreated(self):
+        """ Test accessing the PR's patch if the fork has been deleted
+        and re-created. """
+
+        # Delete fork on disk
+        project = pagure.lib.query.get_authorized_project(
+            self.session, 'test', user='foo')
+        repo_path = os.path.join(self.path, 'repos', project.path)
+        self.assertTrue(os.path.exists(repo_path))
+        shutil.rmtree(repo_path)
+        self.assertFalse(os.path.exists(repo_path))
+
+        # Delete fork in the DB
+        self.session.delete(project)
+        self.session.commit()
+
+        # Re-create foo's fork of pingou's test project
+        item = pagure.lib.model.Project(
+            user_id=2,  # foo
+            name='test',
+            description='test project #1',
+            hook_token='aaabbb',
+            is_fork=True,
+            parent_id=1,
+        )
+        self.session.add(item)
+        self.session.commit()
+        # Create the fork's git repo
+        repo_path = os.path.join(self.path, 'repos', item.path)
+        pygit2.init_repository(repo_path, bare=True)
+
+        # View the pull-request
+        output = self.app.get('/test/pull-request/1')
+        self.assertEqual(output.status_code, 200)
+        output_text = output.get_data(as_text=True)
+        self.assertIn('''<span>Commits&nbsp;</span>
+      <span class="badge badge-secondary badge-pill">
+        1
+      </span>''', output_text)
+        self.assertIn('''<span>Files Changed&nbsp;</span>
+      <span class="badge badge-secondary badge-pill">
+        1
+      </span>''', output_text)
+
+
+class PagureFlaskClosedPrNoSourcestests(BasePrNoSourcestests):
+    """ Tests viewing closed PR in pagure when the source is gone """
+
+    maxDiff = None
+
+    @patch('pagure.lib.notify.send_email', MagicMock(return_value=True))
+    @patch('pagure.lib.notify.fedmsg_publish', MagicMock(return_value=True))
+    def setUp(self):
+        """ Set up the environnment, ran before every tests. """
+        super(PagureFlaskClosedPrNoSourcestests, self).setUp()
+
+        # Ensure things got setup straight
+        project = pagure.lib.query.get_authorized_project(self.session, 'test')
+        self.assertEqual(len(project.requests), 1)
+        request = project.requests[0]
+        self.assertEqual(request.status, "Open")
+        request.status = "Closed"
+        self.session.add(request)
+        self.session.commit()
+
+    def test_accessing_pr_patch_fork_deleted_recreated(self):
+        """ Test accessing the PR's patch if the fork has been deleted
+        and re-created. """
+
+        # Delete fork on disk
+        project = pagure.lib.query.get_authorized_project(
+            self.session, 'test', user='foo')
+        repo_path = os.path.join(self.path, 'repos', project.path)
+        self.assertTrue(os.path.exists(repo_path))
+        shutil.rmtree(repo_path)
+        self.assertFalse(os.path.exists(repo_path))
+
+        # Delete fork in the DB
+        self.session.delete(project)
+        self.session.commit()
+
+        # Re-create foo's fork of pingou's test project
+        item = pagure.lib.model.Project(
+            user_id=2,  # foo
+            name='test',
+            description='test project #1',
+            hook_token='aaabbb',
+            is_fork=True,
+            parent_id=1,
+        )
+        self.session.add(item)
+        self.session.commit()
+        # Create the fork's git repo
+        fork_path = os.path.join(self.path, 'repos', item.path)
+        pygit2.clone_repository(repo_path, fork_path, bare=True)
+
+        # View the pull-request
+        output = self.app.get('/test/pull-request/1')
+        self.assertEqual(output.status_code, 200)
+        output_text = output.get_data(as_text=True)
+        self.assertIn('''<span>Commits&nbsp;</span>
+      <span class="badge badge-secondary badge-pill">
+        1
+      </span>''', output_text)
+        self.assertIn('''<span>Files Changed&nbsp;</span>
+      <span class="badge badge-secondary badge-pill">
+        1
+      </span>''', output_text)


 if __name__ == '__main__':
-- 
2.14.5

Reading it I think it replicates the issue we had in stg, but it passes successfully in master, which means it doesn't trigger the bug :(

rebased onto 5efe838

2 years ago

The issue is that we could replicate this in real life but not in tests, so I'm not sure if we actually fixed the root cause or not

pr view was buggy when pull request's origin repo was deleted due to pull_request.project_form.is_fork when project_form was undefined on the template: https://pagure.io/pagure/blob/240f793ee58320d04f3fb85b12270246d0456bb7/f/pagure/templates/repo_pull_request.html#_87

https://pagure.io/pagure/c/38a4ae1dd5ec9c597de467974b5eca16ef0f2f69?branch=master fixed that.

Could be there part of the underlying problem in real life? I'll try to make some tests around this

Basically, we were looking at the fork to generate the diff of the
PR, but if the forks has been deleted and later re-created the
commits are gone from the forks, so the diff appears empty.

I'm trying to reproduce this on stg.pagure.io without success:

I had an open pr from forks/jlanda/merge-test to merge-test, canceled the pull request, removed my fork, recreated it, and...
https://stg.pagure.io/merge-test/pull-request/2#request_diff

I have the diff there :S Should I wait to the garbage collector or something between removing and recreating?

m, actually this is not necessary if the fork has been effectively removed from the db.

From current /api/fork.py...

    else: # this means that the repo is not a remote one.
        repo_from = request.project_from
        parentpath = pagure.utils.get_repo_path(request.project)
        repopath = parentpath
        if repo_from:
            repopath = pagure.utils.get_repo_path(repo_from)

       repo_obj = pygit2.Repository(repopath)

When fork has been deleted, request.project_from = None, so repo_path = parentpath, so we'll be walking on upstream's commits not the fork ones. The problem here would be when pr.project_form continues on the db but the bare repo is not on disk, but that's an stale state... should we support it someway?

So yep, the 500 that you were seeing was due to the pr.project_form.name that we were trying to paint on pull_request view, but now that is fixed and shows "Unknown source"