launchpad-reviewers team mailing list archive

["j.c.sackett" <jcsackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/bad-traverse-changes-file into lp:launchpad.

Commit message:
Fixes a check for changes files that was causing a NotFound error.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #885541 in Launchpad itself: "Badly named .changes files aren't traversable"
  https://bugs.launchpad.net/launchpad/+bug/885541

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/bad-traverse-changes-file/+merge/127536

Summary
=======
Badly named changes files (i.e. those that violate naming conventions) can't
be traversed in LP, leading to 404s even though the file exists.

This is because Archive.getByFileName assumes and changes file will end with
"_source.changes". This check is more restrictive than necessary--we have
checked for all other filetypes at this point, and if the file doesn't exist
we will still return NotFound appropriately. Since at the point of the failure
all we care about is "do we need to setup the query to look for changes files"
just using the ".changes" ending is enough.

Preimp
======
Spoke with Curtis Hovey.

Implementation
==============
The check for the filename ending with _source.changes is changed to just
check for .changes. Subsequent checks and allowances for failures are already
caught later in the method, so this is safe.

Tests
=====
bin/test -vvct test_oddly_named_files_are_found

QA
==
Attempt to go to the file link in the bug.

LoC
===
I have a credit of more than 400 LoC from previous maintenance work.

Lint
====

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/soyuz/tests/test_archive.py
  lib/lp/soyuz/model/archive.py
-- 
https://code.launchpad.net/~jcsackett/launchpad/bad-traverse-changes-file/+merge/127536
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/bad-traverse-changes-file into lp:launchpad.

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2012-09-28 06:25:44 +0000
+++ lib/lp/soyuz/model/archive.py	2012-10-02 16:42:22 +0000
@@ -29,7 +29,6 @@
     Not,
     Or,
     Select,
-    SQL,
     Sum,
     )
 from storm.locals import (
@@ -1540,7 +1539,7 @@
                     BinaryPackageFile.binarypackagereleaseID,
                 BinaryPackageFile.libraryfileID == LibraryFileAlias.id,
                 )
-        elif filename.endswith('_source.changes'):
+        elif filename.endswith('.changes'):
             clauses = (
                 SourcePackagePublishingHistory.archive == self.id,
                 SourcePackagePublishingHistory.sourcepackagereleaseID ==

=== modified file 'lib/lp/soyuz/tests/test_archive.py'
--- lib/lp/soyuz/tests/test_archive.py	2012-09-28 06:25:44 +0000
+++ lib/lp/soyuz/tests/test_archive.py	2012-10-02 16:42:22 +0000
@@ -1998,6 +1998,17 @@
         pub.sourcepackagerelease.addFile(new_dsc)
         self.assertEqual(new_dsc, self.archive.getFileByName(dsc.filename))
 
+    def test_oddly_named_files_are_found(self):
+        pub = self.factory.makeSourcePackagePublishingHistory(
+            archive=self.archive)
+        pu = self.factory.makePackageUpload(
+            changes_filename='foo-bar-baz_amd64.changes')
+        pu.setDone()
+        pu.addSource(pub.sourcepackagerelease)
+        self.assertEqual(
+            pu.changesfile,
+            self.archive.getFileByName(pu.changesfile.filename))
+
 
 class TestGetPublishedSources(TestCaseWithFactory):