launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/remove-webservice-get-commit into lp:launchpad.

Commit message:
Remove the post-webservice-GET commit.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/remove-webservice-get-commit/+merge/336604

It's been disabled for everyone since 2018-01-02 with no reported ill effects, so it's time for it to go.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/remove-webservice-get-commit into lp:launchpad.

=== modified file 'lib/lp/services/webapp/servers.py'
--- lib/lp/services/webapp/servers.py	2017-12-17 00:56:02 +0000
+++ lib/lp/services/webapp/servers.py	2018-01-25 13:17:27 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Definition of the internet servers that Launchpad uses."""
@@ -31,7 +31,6 @@
 from zope.app.server import wsgi
 from zope.app.wsgi import WSGIPublisherApplication
 from zope.component import getUtility
-from zope.event import notify
 from zope.formlib.itemswidgets import MultiDataHelper
 from zope.formlib.widget import SimpleInputWidget
 from zope.interface import (
@@ -64,10 +63,7 @@
 from lp.app.errors import UnexpectedFormData
 import lp.layers
 from lp.services.config import config
-from lp.services.features import (
-    get_relevant_feature_controller,
-    getFeatureFlag,
-    )
+from lp.services.features import get_relevant_feature_controller
 from lp.services.features.flags import NullFeatureController
 from lp.services.feeds.interfaces.application import IFeedsApplication
 from lp.services.feeds.interfaces.feed import IFeed
@@ -87,7 +83,6 @@
     )
 from lp.services.webapp.errorlog import ErrorReportRequest
 from lp.services.webapp.interfaces import (
-    FinishReadOnlyRequestEvent,
     IBasicLaunchpadRequest,
     IBrowserFormNG,
     IFavicon,
@@ -1241,24 +1236,6 @@
         else:
             return super(WebServicePublication, self).getResource(request, ob)
 
-    def finishReadOnlyRequest(self, request, ob, txn):
-        """Commit the transaction even though there should be no writes."""
-        if getFeatureFlag('webservice.read_only_commit.disabled'):
-            super(WebServicePublication, self).finishReadOnlyRequest(
-                request, ob, txn)
-        else:
-            # WebServicePublication used to commit on every request to store
-            # OAuthNonces to prevent replay attacks. But TLS prevents replay
-            # attacks too, so we don't bother with nonces any more. However,
-            # this commit will stay here until we can switch it off in a
-            # controlled test to ensure that nothing depends on it on prod.
-            notify(FinishReadOnlyRequestEvent(ob, request))
-            # Transaction commits usually need to be aware of the
-            # possibility of a doomed transaction.  We do not expect that
-            # this code will encounter doomed transactions.  If it does,
-            # this will need to be revisited.
-            txn.commit()
-
     def getPrincipal(self, request):
         """See `LaunchpadBrowserPublication`.
 

=== modified file 'lib/lp/services/webapp/tests/test_servers.py'
--- lib/lp/services/webapp/tests/test_servers.py	2018-01-02 16:10:26 +0000
+++ lib/lp/services/webapp/tests/test_servers.py	2018-01-25 13:17:27 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -31,7 +31,6 @@
     )
 
 from lp.app import versioninfo
-from lp.services.features.testing import FeatureFixture
 from lp.services.webapp.interfaces import IFinishReadOnlyRequestEvent
 from lp.services.webapp.publication import LaunchpadBrowserPublication
 from lp.services.webapp.servers import (
@@ -53,10 +52,7 @@
     EventRecorder,
     TestCase,
     )
-from lp.testing.layers import (
-    FunctionalLayer,
-    ZopelessDatabaseLayer,
-    )
+from lp.testing.layers import FunctionalLayer
 
 
 class SetInWSGIEnvironmentTestCase(TestCase):
@@ -699,7 +695,7 @@
         self.log.append("ABORT")
 
 
-class TestFinishReadOnlyRequestMixin:
+class TestFinishReadOnlyRequest(TestCase):
     # Publications that have a finishReadOnlyRequest() method are obliged to
     # fire an IFinishReadOnlyRequestEvent.
 
@@ -730,13 +726,11 @@
         self.assertIs(fake_request, finish_event.request)
         self.assertIs(fake_object, finish_event.object)
 
-class TestFinishReadOnlyRequest(TestFinishReadOnlyRequestMixin, TestCase):
-
     def test_WebServicePub_fires_FinishReadOnlyRequestEvent(self):
         # WebServicePublication.finishReadOnlyRequest() issues an
-        # IFinishReadOnlyRequestEvent and commits the transaction.
+        # IFinishReadOnlyRequestEvent and aborts the transaction.
         publication = WebServicePublication(None)
-        self._test_publication(publication, ["COMMIT"])
+        self._test_publication(publication, ["ABORT"])
 
     def test_LaunchpadBrowserPub_fires_FinishReadOnlyRequestEvent(self):
         # LaunchpadBrowserPublication.finishReadOnlyRequest() issues an
@@ -745,23 +739,6 @@
         self._test_publication(publication, ["ABORT"])
 
 
-class TestFinishReadOnlyRequestFeatureFlag(
-        TestFinishReadOnlyRequestMixin, TestCase):
-
-    layer = ZopelessDatabaseLayer
-
-    def test_WebServicePub_aborts_if_read_only_commit_disabled(self):
-        # If the webservice.read_only_commit.disabled feature flag is set,
-        # WebServicePublication.finishReadOnlyRequest() behaves like
-        # LaunchpadBrowserPublication.finishReadOnlyRequest() and aborts the
-        # transaction.  (This is intended to be the default behaviour in
-        # future.)
-        self.useFixture(FeatureFixture(
-            {'webservice.read_only_commit.disabled': 'on'}))
-        publication = WebServicePublication(None)
-        self._test_publication(publication, ["ABORT"])
-
-
 def test_suite():
     suite = unittest.TestSuite()
     suite.addTest(DocTestSuite(