Merge lp:~jelmer/launchpad/621778-homepage-field into lp:launchpad/db-devel

[Jelmer Vernooij]

Add SourcePackageRelease.homepage, BinaryPackageRelease.homepage.

This adds a "Homepage" field to SourcePackageRelease and BinaryPackageRelease.

This field was added to the Debian policy a fairly recently, see http://www.debian.org/doc/debian-policy/ch-controlfields.html#s-f-Homepage

I'm adding this field so that it can be used as the default value for the upstream homepage URL when creating an upstream project from an Ubuntu source package.

See: https://bugs.edge.launchpad.net/launchpad-registry/+bug/621778

[Robert Collins (lifeless)]

If you're adding in python support for this, I think a couple of tests would be sensible: e.g. that you want it nullable.

DB side looks fine.

review: Approve (db)

[Stuart Bishop (stub)]

Is homepage one word or two? Both forms are in common use and I don't know if LP Style prefers homepage or home_page.

DB patch is fine. It would be lovely if we could enforce the format of the string using a CHECK constraint, but I don't think we can - we have to accept whatever rubbish the user put in the package that they uploaded. You might want to mention this in the comment before anyone attempts to naively mark it up (possible attack vector there if someone sticks Javascript in the homepage field and we don't validate the URL before marking it up).

patch-2208-05-0.sql

review: Approve (db)

[Robert Collins (lifeless)]

On Mon, Aug 23, 2010 at 6:25 PM, Stuart Bishop
<email address hidden> wrote:
> Review: Approve db
> Is homepage one word or two? Both forms are in common use and I don't know if LP Style prefers homepage or home_page.
>
> DB patch is fine. It would be lovely if we could enforce the format of the string using a CHECK constraint, but I don't think we can - we have to accept whatever rubbish the user put in the package that they uploaded. You might want to mention this in the comment before anyone attempts to naively mark it up (possible attack vector there if someone sticks Javascript in the homepage field and we don't validate the URL before marking it up).

Good catch: I suggest making the factory function return just such a
javascript function by default, to make it blindingly obvious :)

-Rob

[Jelmer Vernooij (jelmer)]

> Is homepage one word or two? Both forms are in common use and I don't know if
> LP Style prefers homepage or home_page.
Both "homepage" and "home page" are allowed according to my dictionary. "homepage" appears to be used more often in the Launchpad source code.

> DB patch is fine. It would be lovely if we could enforce the format of the
> string using a CHECK constraint, but I don't think we can - we have to accept
> whatever rubbish the user put in the package that they uploaded. You might
> want to mention this in the comment before anyone attempts to naively mark it
> up (possible attack vector there if someone sticks Javascript in the homepage
> field and we don't validate the URL before marking it up).
Makes sense, updated in the database comment and the API interface.

[Jelmer Vernooij (jelmer)]

> > Is homepage one word or two? Both forms are in common use and I don't know
> if
> > LP Style prefers homepage or home_page.
> Both "homepage" and "home page" are allowed according to my dictionary.
> "homepage" appears to be used more often in the Launchpad source code.
"Homepage" is also the name of the field in the Debian control file from which this field will be set.

[Steve Kowalik (stevenk)]

Hi,

These changes look pretty good, I just have some concerns:

* There exists lp.services.fields.URIField that allows for validation of URIs that are passed to it. Does that sound like a better fit than TextField?
* Why is homepage not exported over the API?

review: Needs Information (code*)

[Steve Kowalik (stevenk)]

One thing I forgot to mention is the you aren't using the patch filename that stub mentioned.

[Jelmer Vernooij (jelmer)]

> These changes look pretty good, I just have some concerns:
>
> * There exists lp.services.fields.URIField that allows for validation of URIs
> that are passed to it. Does that sound like a better fit than TextField?
Since we're importing the homepage field from the user-provided Debian package control fields, we can't rely on the homepage field containing a valid URL.

> * Why is homepage not exported over the API?
Mainly because the rest of SourcePackageRelease/BinaryPackageRelease isn't exported either, it seemed out of scope for this branch.

[Steve Kowalik (stevenk)]

Okay, this looks good to me!

review: Approve (code*)

[Tim Penhey (thumper)]

review: Approve

=== modified file 'database/schema/comments.sql'
--- database/schema/comments.sql	2010-08-23 04:51:48 +0000
+++ database/schema/comments.sql	2010-08-25 12:22:47 +0000
@@ -1339,6 +1339,7 @@
 COMMENT ON COLUMN SourcePackageRelease.build_conflicts_indep IS 'The list of packages that will conflict with this source while building in architecture independent environment, as mentioned in the control file "Build-Conflicts-Indep:" field.';
 COMMENT ON COLUMN SourcePackageRelease.changelog IS 'The LibraryFileAlias ID of changelog associated with this sourcepackage.  Often in the case of debian packages and will be found after the installation in /usr/share/doc/<binarypackagename>/changelog.Debian.gz';
 COMMENT ON COLUMN SourcePackageRelease.user_defined_fields IS 'A JSON struct containing a sequence of key-value pairs with user defined fields in the control file.';
+COMMENT ON COLUMN SourcePackageRelease.homepage IS 'Upstream project homepage URL, not checked for validity.';
 
 -- SourcePackageName
 
@@ -1480,6 +1481,7 @@
 COMMENT ON COLUMN BinaryPackageRelease.breaks IS 'The list of packages which will be broken by the installtion of this package, as it is in the control file "Breaks:" field.';
 COMMENT ON COLUMN BinaryPackageRelease.debug_package IS 'The corresponding binary package release containing debug symbols for this binary, if any.';
 COMMENT ON COLUMN BinaryPackageRelease.user_defined_fields IS 'A JSON struct containing a sequence of key-value pairs with user defined fields in the control file.';
+COMMENT ON COLUMN BinaryPackageRelease.homepage IS 'Upstream project homepage URL, not checked for validity.';
 
 
 -- BinaryPackageFile
 
=== added file 'database/schema/patch-2208-05-0.sql'
--- database/schema/patch-2208-05-0.sql	1970-01-01 00:00:00 +0000
+++ database/schema/patch-2208-05-0.sql	2010-08-25 12:22:47 +0000
@@ -0,0 +1,11 @@
+-- Copyright 2010 Canonical Ltd.  This software is licensed under the
+-- GNU Affero General Public License version 3 (see the file LICENSE).
+SET client_min_messages=ERROR;
+
+ALTER TABLE BinaryPackageRelease
+    ADD COLUMN homepage TEXT;
+
+ALTER TABLE SourcePackageRelease
+    ADD COLUMN homepage TEXT;
+
+INSERT INTO LaunchpadDatabaseRevision VALUES (2208, 05, 0);
 
=== modified file 'lib/lp/registry/interfaces/distroseries.py'
--- lib/lp/registry/interfaces/distroseries.py	2010-08-21 13:54:20 +0000
+++ lib/lp/registry/interfaces/distroseries.py	2010-08-25 12:22:47 +0000
@@ -602,7 +602,8 @@
         dsc_maintainer_rfc822, dsc_standards_version, dsc_format,
         dsc_binaries, archive, copyright, build_conflicts,
         build_conflicts_indep, dateuploaded=None,
-        source_package_recipe_build=None, user_defined_fields=None):
+        source_package_recipe_build=None, user_defined_fields=None,
+        homepage=None):
         """Create an uploads `SourcePackageRelease`.
 
         Set this distroseries set to be the uploadeddistroseries.
@@ -640,6 +641,8 @@
          :param source_package_recipe_build: optional SourcePackageRecipeBuild
          :param user_defined_fields: optional sequence of key-value pairs with
                                      user defined fields.
+         :param homepage: optional string with (unchecked) upstream homepage
+                          URL
          :return: the just creates `SourcePackageRelease`
         """
 
 
=== modified file 'lib/lp/registry/model/distroseries.py'
--- lib/lp/registry/model/distroseries.py	2010-08-24 20:30:48 +0000
+++ lib/lp/registry/model/distroseries.py	2010-08-25 12:22:47 +0000
@@ -1186,7 +1186,8 @@
         dsc_maintainer_rfc822, dsc_standards_version, dsc_format,
         dsc_binaries, archive, copyright, build_conflicts,
         build_conflicts_indep, dateuploaded=DEFAULT,
-        source_package_recipe_build=None, user_defined_fields=None):
+        source_package_recipe_build=None, user_defined_fields=None,
+        homepage=None):
         """See `IDistroSeries`."""
         return SourcePackageRelease(
             upload_distroseries=self, sourcepackagename=sourcepackagename,
@@ -1203,7 +1204,7 @@
             build_conflicts=build_conflicts,
             build_conflicts_indep=build_conflicts_indep,
             source_package_recipe_build=source_package_recipe_build,
-            user_defined_fields=user_defined_fields)
+            user_defined_fields=user_defined_fields, homepage=homepage)
 
     def getComponentByName(self, name):
         """See `IDistroSeries`."""
 
=== modified file 'lib/lp/soyuz/interfaces/binarypackagebuild.py'
--- lib/lp/soyuz/interfaces/binarypackagebuild.py	2010-08-21 13:54:20 +0000
+++ lib/lp/soyuz/interfaces/binarypackagebuild.py	2010-08-25 12:22:47 +0000
@@ -140,7 +140,7 @@
         shlibdeps=None, depends=None, recommends=None, suggests=None,
         conflicts=None, replaces=None, provides=None, pre_depends=None,
         enhances=None, breaks=None, essential=False, debug_package=None,
-        user_defined_fields=None):
+        user_defined_fields=None, homepage=None):
         """Create and return a `BinaryPackageRelease`.
 
         The binarypackagerelease will be attached to this specific build.
 
=== modified file 'lib/lp/soyuz/interfaces/binarypackagerelease.py'
--- lib/lp/soyuz/interfaces/binarypackagerelease.py	2010-08-21 13:56:34 +0000
+++ lib/lp/soyuz/interfaces/binarypackagerelease.py	2010-08-25 12:22:47 +0000
@@ -80,6 +80,13 @@
     user_defined_fields = List(
         title=_("Sequence of user-defined fields as key-value pairs."))
 
+    homepage = TextLine(
+        title=_("Homepage"),
+        description=_(
+        "Upstream project homepage as set in the package. This URL is not "
+        "sanitized."),
+        required=False)
+
     files = Attribute("Related list of IBinaryPackageFile entries")
 
     title = TextLine(required=True, readonly=True)
 
=== modified file 'lib/lp/soyuz/interfaces/sourcepackagerelease.py'
--- lib/lp/soyuz/interfaces/sourcepackagerelease.py	2010-08-21 13:56:34 +0000
+++ lib/lp/soyuz/interfaces/sourcepackagerelease.py	2010-08-25 12:22:47 +0000
@@ -104,6 +104,14 @@
 
     user_defined_fields = List(
         title=_("Sequence of user-defined fields as key-value pairs."))
+
+    homepage = TextLine(
+        title=_("Homepage"),
+        description=_(
+        "Upstream project homepage as set in the package. This URL is not "
+        "sanitized."),
+        required=False)
+
     # read-only properties
     name = Attribute('The sourcepackagename for this release, as text')
     title = Attribute('The title of this sourcepackagerelease')
 
=== modified file 'lib/lp/soyuz/model/binarypackagebuild.py'
--- lib/lp/soyuz/model/binarypackagebuild.py	2010-08-25 00:30:21 +0000
+++ lib/lp/soyuz/model/binarypackagebuild.py	2010-08-25 12:22:47 +0000
@@ -485,7 +485,7 @@
         architecturespecific, shlibdeps=None, depends=None, recommends=None,
         suggests=None, conflicts=None, replaces=None, provides=None,
         pre_depends=None, enhances=None, breaks=None, essential=False,
-        debug_package=None, user_defined_fields=None):
+        debug_package=None, user_defined_fields=None, homepage=None):
         """See IBuild."""
         return BinaryPackageRelease(
             build=self, binarypackagename=binarypackagename, version=version,
@@ -498,7 +498,7 @@
             breaks=breaks, essential=essential, installedsize=installedsize,
             architecturespecific=architecturespecific,
             debug_package=debug_package,
-            user_defined_fields=user_defined_fields)
+            user_defined_fields=user_defined_fields, homepage=homepage)
 
     def estimateDuration(self):
         """See `IPackageBuild`."""
 
=== modified file 'lib/lp/soyuz/model/binarypackagerelease.py'
--- lib/lp/soyuz/model/binarypackagerelease.py	2010-08-21 13:56:34 +0000
+++ lib/lp/soyuz/model/binarypackagerelease.py	2010-08-25 12:22:47 +0000
@@ -81,6 +81,7 @@
     installedsize = IntCol(dbName='installedsize')
     architecturespecific = BoolCol(dbName='architecturespecific',
                                    notNull=True)
+    homepage = StringCol(dbName='homepage')
     datecreated = UtcDateTimeCol(notNull=True, default=UTC_NOW)
     debug_package = ForeignKey(dbName='debug_package',
                               foreignKey='BinaryPackageRelease')
 
=== modified file 'lib/lp/soyuz/model/sourcepackagerelease.py'
--- lib/lp/soyuz/model/sourcepackagerelease.py	2010-08-21 13:56:34 +0000
+++ lib/lp/soyuz/model/sourcepackagerelease.py	2010-08-25 12:22:47 +0000
@@ -146,6 +146,7 @@
     build_conflicts = StringCol(dbName='build_conflicts')
     build_conflicts_indep = StringCol(dbName='build_conflicts_indep')
     architecturehintlist = StringCol(dbName='architecturehintlist')
+    homepage = StringCol(dbName='homepage')
     format = EnumCol(dbName='format', schema=SourcePackageType,
         default=SourcePackageType.DPKG, notNull=True)
     upload_distroseries = ForeignKey(foreignKey='DistroSeries',
 
=== modified file 'lib/lp/soyuz/tests/test_binarypackagerelease.py'
--- lib/lp/soyuz/tests/test_binarypackagerelease.py	2010-08-21 13:56:34 +0000
+++ lib/lp/soyuz/tests/test_binarypackagerelease.py	2010-08-25 12:22:47 +0000
@@ -49,3 +49,19 @@
         self.assertEquals([
             ["Python-Version", ">= 2.4"],
             ["Other", "Bla"]], release.user_defined_fields)
+
+    def test_homepage_default(self):
+        # By default, no homepage is set.
+        bpr = self.factory.makeBinaryPackageRelease()
+        self.assertEquals(None, bpr.homepage)
+
+    def test_homepage_empty(self):
+        # The homepage field can be empty.
+        bpr = self.factory.makeBinaryPackageRelease(homepage="")
+        self.assertEquals("", bpr.homepage)
+
+    def test_homepage_set_invalid(self):
+        # As the homepage field is inherited from the .deb, the URL
+        # does not have to be valid.
+        bpr = self.factory.makeBinaryPackageRelease(homepage="<invalid<url")
+        self.assertEquals("<invalid<url", bpr.homepage)
 
=== modified file 'lib/lp/soyuz/tests/test_sourcepackagerelease.py'
--- lib/lp/soyuz/tests/test_sourcepackagerelease.py	2010-08-21 13:54:20 +0000
+++ lib/lp/soyuz/tests/test_sourcepackagerelease.py	2010-08-25 12:22:47 +0000
@@ -38,3 +38,19 @@
         self.assertEquals([
             ["Python-Version", ">= 2.4"],
             ["Other", "Bla"]], release.user_defined_fields)
+
+    def test_homepage_default(self):
+        # By default, no homepage is set.
+        spr = self.factory.makeSourcePackageRelease()
+        self.assertEquals(None, spr.homepage)
+
+    def test_homepage_empty(self):
+        # The homepage field can be empty.
+        spr = self.factory.makeSourcePackageRelease(homepage="")
+        self.assertEquals("", spr.homepage)
+
+    def test_homepage_set_invalid(self):
+        # As the homepage field is inherited from the DSCFile, the URL
+        # does not have to be valid.
+        spr = self.factory.makeSourcePackageRelease(homepage="<invalid<url")
+        self.assertEquals("<invalid<url", spr.homepage)
 
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2010-08-23 08:12:39 +0000
+++ lib/lp/testing/factory.py	2010-08-25 12:22:47 +0000
@@ -2408,7 +2408,8 @@
                                  date_uploaded=UTC_NOW,
                                  source_package_recipe_build=None,
                                  dscsigningkey=None,
-                                 user_defined_fields=None):
+                                 user_defined_fields=None,
+                                 homepage=None):
         """Make a `SourcePackageRelease`."""
         if distroseries is None:
             if source_package_recipe_build is not None:
@@ -2476,7 +2477,8 @@
             archive=archive,
             dateuploaded=date_uploaded,
             source_package_recipe_build=source_package_recipe_build,
-            user_defined_fields=user_defined_fields)
+            user_defined_fields=user_defined_fields,
+            homepage=homepage)
 
     def makeSourcePackageReleaseFile(self, sourcepackagerelease=None,
                                      library_file=None, filetype=None):
@@ -2704,7 +2706,8 @@
                                  provides=None, pre_depends=None,
                                  enhances=None, breaks=None,
                                  essential=False, installed_size=None,
-                                 date_created=None, debug_package=None):
+                                 date_created=None, debug_package=None,
+                                 homepage=None):
         """Make a `BinaryPackageRelease`."""
         if build is None:
             build = self.makeBinaryPackageBuild()
@@ -2735,7 +2738,8 @@
                 suggests=suggests, conflicts=conflicts, replaces=replaces,
                 provides=provides, pre_depends=pre_depends,
                 enhances=enhances, breaks=breaks, essential=essential,
-                installedsize=installed_size, debug_package=debug_package)
+                installedsize=installed_size, debug_package=debug_package,
+                homepage=homepage)
         if date_created is not None:
             removeSecurityProxy(bpr).datecreated = date_created
         return bpr