← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] ~lgp171188/launchpad:fix-snaps-description-new-snap-page into launchpad:master

 

Guruprasad has proposed merging ~lgp171188/launchpad:fix-snaps-description-new-snap-page into launchpad:master.

Commit message:
Update the description for snaps in the new snap page
    
Use the description that is currently in https://snapcraft.io/docs.
    
LP: #2037033

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #2037033 in Launchpad itself: "The +new-snap text does mention snappy and points to an invalid (404) URL for documentation"
  https://bugs.launchpad.net/launchpad/+bug/2037033

For more details, see:
https://code.launchpad.net/~lgp171188/launchpad/+git/launchpad/+merge/451926
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~lgp171188/launchpad:fix-snaps-description-new-snap-page into launchpad:master.
diff --git a/doc/reference/python.rst b/doc/reference/python.rst
index a616784..5d877b3 100644
--- a/doc/reference/python.rst
+++ b/doc/reference/python.rst
@@ -264,6 +264,21 @@ passes and returns them easier to debug.
 Database-related
 ================
 
+Storm
+-----
+
+We use two database ORM (object-relational mapper) APIs in Launchpad, the
+older and deprecated SQLObject API and the new and improved `Storm
+<https://storm.canonical.com>`_ API.  All new code should use the Storm API,
+and you are encouraged to convert existing code to Storm as part of your
+tech-debt payments.
+
+.. note::
+
+    The SQLObject and Storm ``ResultSet`` interfaces are not compatible, so
+    e.g. if you need to ``UNION`` between these two, you will run into
+    trouble.  We are looking into ways to address this.
+
 Field attributes
 ----------------
 
diff --git a/lib/lp/app/doc/batch-navigation.rst b/lib/lp/app/doc/batch-navigation.rst
index ab762aa..25274f1 100644
--- a/lib/lp/app/doc/batch-navigation.rst
+++ b/lib/lp/app/doc/batch-navigation.rst
@@ -8,7 +8,8 @@ This documents and tests the Launchpad-specific elements of its usage.
 
 Note that our use of the batching code relies on the registration of
 lp.services.webapp.batching.FiniteSequenceAdapter for
-storm.zope.interfaces.IResultSet.
+storm.zope.interfaces.IResultSet and
+storm.zope.interfaces.ISQLObjectResultSet.
 
 Batch navigation provides a way to navigate batch results in a web
 page by providing URL links to the next, previous and numbered pages
diff --git a/lib/lp/blueprints/model/sprint.py b/lib/lp/blueprints/model/sprint.py
index 15f20ea..3a45f35 100644
--- a/lib/lp/blueprints/model/sprint.py
+++ b/lib/lp/blueprints/model/sprint.py
@@ -425,7 +425,12 @@ class HasSprintsMixin:
 
         Subclasses must overwrite this method if it doesn't suit them.
         """
-        table = getattr(self, "__storm_table__")
+        try:
+            table = getattr(self, "__storm_table__")
+        except AttributeError:
+            # XXX cjwatson 2020-09-10: Remove this once all inheritors have
+            # been converted from SQLObject to Storm.
+            table = getattr(self, "_table")
         return [
             getattr(Specification, table.lower()) == self,
             Specification.id == SprintSpecification.specification_id,
diff --git a/lib/lp/services/database/interfaces.py b/lib/lp/services/database/interfaces.py
index f619539..808e25b 100644
--- a/lib/lp/services/database/interfaces.py
+++ b/lib/lp/services/database/interfaces.py
@@ -9,6 +9,7 @@ __all__ = [
     "IPrimaryObject",
     "IPrimaryStore",
     "IRequestExpired",
+    "ISQLBase",
     "IStandbyStore",
     "IStore",
     "IStoreSelector",
@@ -20,6 +21,7 @@ __all__ = [
 
 from zope.interface import Interface
 from zope.interface.common.interfaces import IRuntimeError
+from zope.schema import Int
 
 
 class IRequestExpired(IRuntimeError):
@@ -28,6 +30,15 @@ class IRequestExpired(IRuntimeError):
     """
 
 
+# XXX 2007-02-09 jamesh:
+# This derived from sqlos.interfaces.ISQLObject before hand.  I don't
+# think it is ever used though ...
+class ISQLBase(Interface):
+    """An extension of ISQLObject that provides an ID."""
+
+    id = Int(title="The integer ID for the instance")
+
+
 #
 # Database policies
 #
diff --git a/lib/lp/services/database/sqlbase.py b/lib/lp/services/database/sqlbase.py
index f604f4c..10a2e1f 100644
--- a/lib/lp/services/database/sqlbase.py
+++ b/lib/lp/services/database/sqlbase.py
@@ -19,13 +19,19 @@ __all__ = [
     "quote_identifier",
     "reset_store",
     "session_store",
+    "SQLBase",
     "sqlvalues",
     "StupidCache",
 ]
 
+<<<<<<< lib/lp/services/database/sqlbase.py
+=======
+
+>>>>>>> lib/lp/services/database/sqlbase.py
 from datetime import datetime, timezone
 
 import psycopg2
+import storm
 import transaction
 from psycopg2.extensions import (
     ISOLATION_LEVEL_AUTOCOMMIT,
@@ -38,18 +44,27 @@ from psycopg2.extensions import (
 from storm.databases.postgres import compile as postgres_compile
 from storm.expr import State
 from storm.expr import compile as storm_compile
+from storm.locals import Storm  # noqa: B1
+from storm.locals import Store
 from storm.zope.interfaces import IZStorm
 from twisted.python.util import mergeFunctionMetadata
 from zope.component import getUtility
+from zope.interface import implementer
+from zope.security.proxy import removeSecurityProxy
 
 from lp.services.config import dbconfig
 from lp.services.database.interfaces import (
     DEFAULT_FLAVOR,
     MAIN_STORE,
     DisallowedStore,
+    IPrimaryObject,
+    IPrimaryStore,
+    ISQLBase,
+    IStore,
     IStoreSelector,
 )
 from lp.services.database.sqlobject import sqlrepr
+from lp.services.propertycache import clear_property_cache
 
 # Default we want for scripts, and the PostgreSQL default. Note psycopg1 will
 # use SERIALIZABLE unless we override, but psycopg2 will not.
@@ -96,15 +111,168 @@ class StupidCache:
         return self._cache.keys()
 
 
-def _get_main_default_store():
-    """Return the main store using the default flavor.
+def _get_sqlobject_store():
+    """Return the store used by the SQLObject compatibility layer."""
+    return getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
+
+
+class LaunchpadStyle(storm.sqlobject.SQLObjectStyle):
+    """A SQLObject style for launchpad.
 
-    For web requests, the default flavor uses a primary or standby database
-    depending on the type of request (see
-    `lp.services.database.policy.LaunchpadDatabasePolicy`); in all other
-    situations, it uses the primary database.
+    Python attributes and database columns are lowercase.
+    Class names and database tables are MixedCase. Using this style should
+    simplify SQLBase class definitions since more defaults will be correct.
     """
-    return getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
+
+    def pythonAttrToDBColumn(self, attr):
+        return attr
+
+    def dbColumnToPythonAttr(self, col):
+        return col
+
+    def pythonClassToDBTable(self, className):
+        return className
+
+    def dbTableToPythonClass(self, table):
+        return table
+
+    def idForTable(self, table):
+        return "id"
+
+    def pythonClassToAttr(self, className):
+        return className.lower()
+
+    # dsilvers: 20050322: If you take this method out; then RelativeJoin
+    # instances in our SQLObject classes cause the following error:
+    # AttributeError: 'LaunchpadStyle' object has no attribute
+    # 'tableReference'
+    def tableReference(self, table):
+        """Return the tablename mapped for use in RelativeJoin statements."""
+        return table.__str__()
+
+
+@implementer(ISQLBase)
+class SQLBase(storm.sqlobject.SQLObjectBase):
+    """Base class emulating SQLObject for legacy database classes."""
+
+    _style = LaunchpadStyle()
+
+    # Silence warnings in linter script, which complains about all
+    # SQLBase-derived objects missing an id.
+    id = None
+
+    def __init__(self, *args, **kwargs):
+        """Extended version of the SQLObjectBase constructor.
+
+        We force use of the primary Store.
+
+        We refetch any parameters from different stores from the
+        correct primary Store.
+        """
+        # Make it simple to write dumb-invalidators - initialized
+        # _cached_properties to a valid list rather than just-in-time
+        # creation.
+        self._cached_properties = []
+        store = IPrimaryStore(self.__class__)
+
+        # The constructor will fail if objects from a different Store
+        # are passed in. We need to refetch these objects from the correct
+        # primary Store if necessary so the foreign key references can be
+        # constructed.
+        # XXX StuartBishop 2009-03-02 bug=336867: We probably want to remove
+        # this code - there are enough other places developers have to be
+        # aware of the replication # set boundaries. Why should
+        # Person(..., account=an_account) work but
+        # some_person.account = an_account fail?
+        for key, argument in kwargs.items():
+            argument = removeSecurityProxy(argument)
+            if not isinstance(argument, Storm):  # noqa: B1
+                continue
+            argument_store = Store.of(argument)
+            if argument_store is not store:
+                new_argument = store.find(
+                    argument.__class__, id=argument.id
+                ).one()
+                assert (
+                    new_argument is not None
+                ), "%s not yet synced to this store" % repr(argument)
+                kwargs[key] = new_argument
+
+        store.add(self)
+        try:
+            self._create(None, **kwargs)
+        except Exception:
+            store.remove(self)
+            raise
+
+    @classmethod
+    def _get_store(cls):
+        return IStore(cls)
+
+    def __repr__(self):
+        return "<%s object>" % (self.__class__.__name__)
+
+    def destroySelf(self):
+        my_primary = IPrimaryObject(self)
+        if self is my_primary:
+            super().destroySelf()
+        else:
+            my_primary.destroySelf()
+
+    def __eq__(self, other):
+        """Equality operator.
+
+        Objects compare equal if they have the same class and id, and the id
+        is not None.
+
+        This rule allows objects retrieved from different stores to compare
+        equal.  Newly-created objects may not yet have an id; in such cases
+        we flush the store so that we can find out their id.
+        """
+        naked_self = removeSecurityProxy(self)
+        naked_other = removeSecurityProxy(other)
+        if naked_self.__class__ != naked_other.__class__:
+            return False
+        try:
+            self_id = naked_self.id
+        except KeyError:
+            self.syncUpdate()
+            self_id = naked_self.id
+        if self_id is None:
+            return False
+        try:
+            other_id = naked_other.id
+        except KeyError:
+            other.syncUpdate()
+            other_id = naked_other.id
+        return self_id == other_id
+
+    def __ne__(self, other):
+        """Inverse of __eq__."""
+        return not (self == other)
+
+    def __hash__(self):
+        """Hash operator.
+
+        We must define __hash__ since we define __eq__ (Python 3 requires
+        this), but we need to take care to preserve the invariant that
+        objects that compare equal have the same hash value.  Newly-created
+        objects may not yet have an id; in such cases we flush the store so
+        that we can find out their id.
+        """
+        try:
+            id = self.id
+        except KeyError:
+            self.syncUpdate()
+            id = self.id
+        return hash((self.__class__, id))
+
+    def __storm_invalidated__(self):
+        """Flush cached properties."""
+        # XXX: RobertCollins 2010-08-16 bug=622648: Note this is not directly
+        # tested, but the entire test suite blows up awesomely if it's broken.
+        # It's entirely unclear where tests for this should be.
+        clear_property_cache(self)
 
 
 def get_transaction_timestamp(store):
@@ -150,14 +318,26 @@ def quote(x):
     >>> from datetime import datetime, date, time
     >>> quote(datetime(2003, 12, 4, 13, 45, 50))
     "'2003-12-04 13:45:50'"
+<<<<<<< lib/lp/services/database/sqlbase.py
     >>> quote(datetime(2003, 12, 4, 13, 45, 50, 123456))
     "'2003-12-04 13:45:50.123456'"
+=======
+>>>>>>> lib/lp/services/database/sqlbase.py
     >>> quote(date(2003, 12, 4))
     "'2003-12-04'"
     >>> quote(time(13, 45, 50))
     "'13:45:50'"
 
-    sqlrepr() also special-cases set objects.
+    This function special cases datetime objects, due to a bug that has
+    since been fixed in SQLOS (it installed an SQLObject converter that
+    stripped the time component from the value).  By itself, the sqlrepr
+    function has the following output:
+
+    >>> sqlrepr(datetime(2003, 12, 4, 13, 45, 50), "postgres")
+    "'2003-12-04T13:45:50'"
+
+    This function also special cases set objects, which SQLObject's
+    sqlrepr() doesn't know how to handle.
 
     >>> quote(set([1, 2, 3]))
     '(1, 2, 3)'
@@ -167,6 +347,15 @@ def quote(x):
     """
     if isinstance(x, datetime):
         return "'%s'" % x
+<<<<<<< lib/lp/services/database/sqlbase.py
+=======
+    elif ISQLBase(x, None) is not None:
+        return str(x.id)
+    elif isinstance(x, (set, frozenset)):
+        # SQLObject can't cope with sets, so convert to a list, which it
+        # /does/ know how to handle.
+        x = list(x)
+>>>>>>> lib/lp/services/database/sqlbase.py
     return sqlrepr(x, "postgres")
 
 
@@ -354,7 +543,7 @@ def block_implicit_flushes(func):
 
     def block_implicit_flushes_decorator(*args, **kwargs):
         try:
-            store = _get_main_default_store()
+            store = _get_sqlobject_store()
         except DisallowedStore:
             return func(*args, **kwargs)
         store.block_implicit_flushes()
@@ -373,7 +562,7 @@ def reset_store(func):
         try:
             return func(*args, **kwargs)
         finally:
-            _get_main_default_store().reset()
+            _get_sqlobject_store().reset()
 
     return mergeFunctionMetadata(func, reset_store_decorator)
 
@@ -425,7 +614,7 @@ class cursor:
     """
 
     def __init__(self):
-        self._connection = _get_main_default_store()._connection
+        self._connection = _get_sqlobject_store()._connection
         self._result = None
 
     def execute(self, query, params=None):
diff --git a/lib/lp/services/database/sqlobject/__init__.py b/lib/lp/services/database/sqlobject/__init__.py
index bd5e645..13fabdc 100644
--- a/lib/lp/services/database/sqlobject/__init__.py
+++ b/lib/lp/services/database/sqlobject/__init__.py
@@ -7,6 +7,30 @@
 import datetime
 
 from storm.expr import SQL
+from storm.sqlobject import (  # noqa: F401
+    AND,
+    CONTAINSSTRING,
+    DESC,
+    IN,
+    LIKE,
+    NOT,
+    OR,
+    BoolCol,
+    DateCol,
+    FloatCol,
+    ForeignKey,
+    IntCol,
+    IntervalCol,
+    SingleJoin,
+    SQLConstant,
+    SQLMultipleJoin,
+    SQLObjectBase,
+    SQLObjectMoreThanOneResultError,
+    SQLObjectResultSet,
+    SQLRelatedJoin,
+    StringCol,
+    UtcDateTimeCol,
+)
 
 _sqlStringReplace = [
     ("\\", "\\\\"),
@@ -45,7 +69,7 @@ def sqlrepr(value, dbname=None):
         return repr(value)
     elif value is None:
         return "NULL"
-    elif isinstance(value, (frozenset, list, set, tuple)):
+    elif isinstance(value, (list, set, tuple)):
         return "(%s)" % ", ".join(sqlrepr(v, dbname) for v in value)
     elif isinstance(value, datetime.datetime):
         return value.strftime("'%Y-%m-%dT%H:%M:%S'")
diff --git a/lib/lp/services/webapp/configure.zcml b/lib/lp/services/webapp/configure.zcml
index 5b61d74..ad25c53 100644
--- a/lib/lp/services/webapp/configure.zcml
+++ b/lib/lp/services/webapp/configure.zcml
@@ -50,6 +50,10 @@
         factory='.batching.FiniteSequenceAdapter' />
 
     <adapter
+        factory='.batching.FiniteSequenceAdapter'
+        for='storm.zope.interfaces.ISQLObjectResultSet' />
+
+    <adapter
         factory='.batching.BoundReferenceSetAdapter'
         for='storm.references.BoundReferenceSet' />
 
@@ -242,6 +246,10 @@
 
     <adapter
         factory="lp.services.webapp.snapshot.snapshot_sql_result" />
+    <!-- It also works for the legacy SQLObject interface. -->
+    <adapter
+        factory="lp.services.webapp.snapshot.snapshot_sql_result"
+        for="storm.zope.interfaces.ISQLObjectResultSet" />
 
     <class class="lp.services.webapp.publisher.RenamedView">
         <allow interface="zope.publisher.interfaces.browser.IBrowserPublisher"
diff --git a/lib/lp/services/webapp/database.zcml b/lib/lp/services/webapp/database.zcml
index 7a5ae12..b7130a2 100644
--- a/lib/lp/services/webapp/database.zcml
+++ b/lib/lp/services/webapp/database.zcml
@@ -60,6 +60,9 @@
         <implements interface="lp.services.database.interfaces.IStore" />
         <allow attributes="get" />
     </class>
+    <class class="lp.services.database.sqlbase.SQLBase">
+        <implements interface="lp.services.database.interfaces.IDBObject" />
+    </class>
     <class class="lp.services.database.stormbase.StormBase">
         <implements interface="lp.services.database.interfaces.IDBObject" />
     </class>
diff --git a/lib/lp/services/webapp/snapshot.py b/lib/lp/services/webapp/snapshot.py
index fd6aaa9..2d46805 100644
--- a/lib/lp/services/webapp/snapshot.py
+++ b/lib/lp/services/webapp/snapshot.py
@@ -19,12 +19,13 @@ HARD_LIMIT_FOR_SNAPSHOT = 1000
 
 
 @implementer(ISnapshotValueFactory)
-@adapter(IResultSet)
+@adapter(IResultSet)  # And ISQLObjectResultSet.
 def snapshot_sql_result(value):
     """Snapshot adapter for the Storm result set."""
-    # ReferenceSet returns ResultSets, which doesn't really help the
-    # Snapshot object. We therefore list()ify the values; this isn't perfect
-    # but allows deltas to be generated reliably.
+    # SQLMultipleJoin and SQLRelatedJoin return
+    # SelectResults, which doesn't really help the Snapshot
+    # object. We therefore list()ify the values; this isn't
+    # perfect but allows deltas to be generated reliably.
     return shortlist(
         value, longest_expected=100, hardlimit=HARD_LIMIT_FOR_SNAPSHOT
     )
diff --git a/lib/lp/snappy/templates/snap-new.pt b/lib/lp/snappy/templates/snap-new.pt
index 29e8f64..1f15132 100644
--- a/lib/lp/snappy/templates/snap-new.pt
+++ b/lib/lp/snappy/templates/snap-new.pt
@@ -10,9 +10,9 @@
 <div metal:fill-slot="main">
   <div>
     <p>
-      A snap package is a self-contained application that can be installed
-      on <a href="https://developer.ubuntu.com/en/snappy/";>snappy Ubuntu
-      Core</a>.  Launchpad can build snap packages using <a
+      Snaps are Linux app packages for desktop, cloud and IoT that are
+      simple to install, secure, cross-platform, and dependency-free.
+      Launchpad can build snap packages using <a
       href="https://developer.ubuntu.com/en/snappy/snapcraft/";>snapcraft</a>,
       from any Git or Bazaar branch on Launchpad that has a
       <tt>snap/snapcraft.yaml</tt>, <tt>build-aux/snap/snapcraft.yaml</tt>,
diff --git a/lib/lp/soyuz/model/publishing.py b/lib/lp/soyuz/model/publishing.py
index 78749c0..5c32866 100644
--- a/lib/lp/soyuz/model/publishing.py
+++ b/lib/lp/soyuz/model/publishing.py
@@ -34,6 +34,8 @@ from storm.info import ClassAlias
 from storm.properties import DateTime, Int, Unicode
 from storm.references import Reference
 from storm.store import Store
+from storm.zope import IResultSet
+from storm.zope.interfaces import ISQLObjectResultSet
 from zope.component import getUtility
 from zope.interface import implementer
 from zope.security.proxy import isinstance as zope_isinstance
@@ -1584,6 +1586,8 @@ class PublishingSet:
             if len(bpphs) == 0:
                 return
         else:
+            if ISQLObjectResultSet.providedBy(bpphs):
+                bpphs = IResultSet(bpphs)
             if bpphs.is_empty():
                 return
 

Follow ups