Attachment 341939 Details for Bug 186291 – Work-in-progress - First pass

[patch] Work-in-progress - First pass

WIP.patch (text/plain), 17.20 KB, created by Daniel Bates on 2018-06-04 16:51:55 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Daniel Bates

Created: 2018-06-04 16:51:55 PDT

Size: 17.20 KB

patch

obsolete

>From cee3627b2c1c21da927821190984609d1bebf749 Mon Sep 17 00:00:00 2001
>From: Daniel Bates <dabates@apple.com>
>Date: Mon, 4 Jun 2018 16:46:37 -0700
>Subject: [PATCH] First pass at supporting out-of-band patch upload and
> processing.
>
>---
> .../handlers/releasepatch.py                  |  2 +
> .../QueueStatusServer/handlers/submittoews.py |  6 ++-
> Tools/QueueStatusServer/main.py               |  2 +
> .../templates/submittoews.html                |  6 ++-
> .../webkitpy/common/net/bugzilla/bugzilla.py  | 44 +++++++++++++++----
> .../common/net/bugzilla/bugzilla_mock.py      | 18 +++++++-
> .../webkitpy/common/net/statusserver.py       | 16 +++++--
> .../webkitpy/common/net/statusserver_mock.py  | 17 ++++++-
> Tools/Scripts/webkitpy/tool/bot/feeders.py    | 11 ++++-
> .../Scripts/webkitpy/tool/commands/queues.py  |  8 +++-
> 10 files changed, 109 insertions(+), 21 deletions(-)
>
>diff --git a/Tools/QueueStatusServer/handlers/releasepatch.py b/Tools/QueueStatusServer/handlers/releasepatch.py
>index 3a36370a6a9..0a4c22fd9fe 100644
>--- a/Tools/QueueStatusServer/handlers/releasepatch.py
>+++ b/Tools/QueueStatusServer/handlers/releasepatch.py
>@@ -32,6 +32,7 @@ from google.appengine.ext.webapp import template
> from handlers.updatebase import UpdateBase
> from loggers.recordpatchevent import RecordPatchEvent
> from model.attachment import Attachment
>+from model.attachmentdata import AttachmentData
> from model.queues import Queue
> 
> 
>@@ -58,3 +59,4 @@ class ReleasePatch(UpdateBase):
>         RecordPatchEvent.stopped(attachment_id, queue_name, last_status.message)
> 
>         queue.active_work_items().expire_item(attachment_id)
>+        AttachmentData.remove_attachment_data(attachment_id)
>diff --git a/Tools/QueueStatusServer/handlers/submittoews.py b/Tools/QueueStatusServer/handlers/submittoews.py
>index 5a79d0cbc04..3e832c5950b 100644
>--- a/Tools/QueueStatusServer/handlers/submittoews.py
>+++ b/Tools/QueueStatusServer/handlers/submittoews.py
>@@ -32,6 +32,7 @@ from google.appengine.ext.webapp import template
> from handlers.updatebase import UpdateBase
> from loggers.recordpatchevent import RecordPatchEvent
> from model.attachment import Attachment
>+from model.attachmentdata import AttachmentData
> from model.queues import Queue
> 
> 
>@@ -41,7 +42,7 @@ class SubmitToEWS(UpdateBase):
> 
>     def _should_add_to_ews_queue(self, queue, attachment):
>         # This assert() is here to make sure we're not submitting to the commit-queue.
>-        # The commit-queue clients check each patch anyway, but there is not sense
>+        # The commit-queue clients check each patch anyway, but there is no sense
>         # in adding things to the commit-queue when they won't be processed by it.
>         assert(queue.is_ews())
>         latest_status = attachment.status_for_queue(queue)
>@@ -59,6 +60,9 @@ class SubmitToEWS(UpdateBase):
> 
>     def post(self):
>         attachment_id = self._int_from_request("attachment_id")
>+        attachment_data = self.request.get("attachment_data")
>+        if attachment_data:
>+            AttachmentData.add_attachment_data(attachment_id, str(attachment_data))
>         attachment = Attachment(attachment_id)
>         self._add_attachment_to_ews_queues(attachment)
>         if self.request.get("next_action") == "return_to_bubbles":
>diff --git a/Tools/QueueStatusServer/main.py b/Tools/QueueStatusServer/main.py
>index 085cdba37b6..0aa49468a7d 100644
>--- a/Tools/QueueStatusServer/main.py
>+++ b/Tools/QueueStatusServer/main.py
>@@ -35,6 +35,7 @@ from google.appengine.ext import webapp
> from google.appengine.ext.webapp.util import run_wsgi_app
> 
> from handlers.activebots import ActiveBots
>+from handlers.fetchattachmentdata import FetchAttachmentData
> from handlers.gc import GC
> from handlers.nextpatch import NextPatch
> from handlers.patch import Patch
>@@ -76,6 +77,7 @@ routes = [
>     (r'/queue-status/(.*)', QueueStatus),
>     (r'/queue-status-json/(.*)', QueueStatusJSON),
>     (r'/next-patch/(.*)', NextPatch),
>+    (r'/attachment/(.*)', FetchAttachmentData),
>     ('/release-patch', ReleasePatch),
>     ('/release-lock', ReleaseLock),
>     ('/update-status', UpdateStatus),
>diff --git a/Tools/QueueStatusServer/templates/submittoews.html b/Tools/QueueStatusServer/templates/submittoews.html
>index 25ad2bc7094..9f8dd445c4f 100644
>--- a/Tools/QueueStatusServer/templates/submittoews.html
>+++ b/Tools/QueueStatusServer/templates/submittoews.html
>@@ -1,3 +1,5 @@
>-<form name="submit_to_ews" method="POST">
>-Attachment id of patch to submit: <input name="attachment_id"><input type="submit" value="Submit for EWS Processing">
>+<form name="submit_to_ews" enctype="multipart/form-data" method="POST">
>+Attachment id of patch to submit: <input name="attachment_id"><br>
>+Patch (used if attachment id is restricted): <input type="file" name="attachment_data"><br>
>+<input type="submit" value="Submit for EWS Processing">
> </form>
>diff --git a/Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py b/Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py
>index 510ec062423..117e492e4d1 100644
>--- a/Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py
>+++ b/Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py
>@@ -278,8 +278,10 @@ class BugzillaQueries(object):
> 
>     # NOTE: This is the only client of _fetch_attachment_ids_request_query
>     # This method only makes one request to bugzilla.
>-    def fetch_attachment_ids_from_review_queue(self, since=None):
>+    def fetch_attachment_ids_from_review_queue(self, since=None, only_security_bugs=False):
>         review_queue_url = "request.cgi?action=queue&type=review&group=type"
>+        if only_security_bugs:
>+            review_queue_url += '&product=Security'
>         return self._fetch_attachment_ids_request_query(review_queue_url, since)
> 
>     # This only works if your account has edituser privileges.
>@@ -493,9 +495,12 @@ class Bugzilla(object):
>         self.authenticate()
>         return self.open_url(attachment_url).read()
> 
>+    def _parse_bug_title_from_attachment_page(self, page):
>+        return BeautifulSoup(page).find('div', attrs={'id':'bug_title'})
>+
>     def _parse_bug_id_from_attachment_page(self, page):
>         # The "Up" relation happens to point to the bug.
>-        title = BeautifulSoup(page).find('div', attrs={'id':'bug_title'})
>+        title = self._parse_bug_title_from_attachment_page(page)
>         if not title :
>             _log.warning("This attachment does not exist (or you don't have permissions to view it).")
>             return None
>@@ -505,25 +510,48 @@ class Bugzilla(object):
>             return None
>         return int(match.group('bug_id'))
> 
>-    def bug_id_for_attachment_id(self, attachment_id):
>-        return NetworkTransaction().run(lambda: self.get_bug_id_for_attachment_id(attachment_id))
>+    def bug_id_for_attachment_id(self, attachment_id, throw_on_access_error=False):
>+        return NetworkTransaction().run(lambda: self.get_bug_id_for_attachment_id(attachment_id, throw_on_access_error))
>+
>+
>+    class AccessError(Exception):
>+        NOT_PERMITTED = 1 << 0
>+        OTHER = 1 << 1
>+
>+        def __init__(self, attachment_id, error_code, bug_title):
>+            super(Bugzilla.AccessError, self).__init__('Failed to access {}'.format(attachment_id))
>+            self.attachment_id = attachment_id
>+            self.error_code = error_code
>+            self.bug_title = bug_title
> 
>-    def get_bug_id_for_attachment_id(self, attachment_id):
>+
>+    def _parse_attachment_page_for_title_and_error(self, page):
>+        title = self._parse_bug_title_from_attachment_page(page)
>+        if title == 'Bug Access Denied':
>+            return (title, Bugzilla.AccessError.NOT_PERMITTED_ERROR)
>+        return (title, Bugzilla.AccessError.OTHER_ERROR)
>+
>+    def get_bug_id_for_attachment_id(self, attachment_id, throw_on_access_error=False):
>         self.authenticate()
> 
>         attachment_url = self.attachment_url_for_id(attachment_id, 'edit')
>         _log.info("Fetching: %s" % attachment_url)
>         page = self.open_url(attachment_url)
>-        return self._parse_bug_id_from_attachment_page(page)
>+        bug_id = self._parse_bug_id_from_attachment_page(page)
>+        if bug_id:
>+            return bug_id
>+        if throw_on_access_error:
>+            raise AccessError(attachment_id, *self._parse_attachment_page_for_title_and_error(page))
>+        return None
> 
>     # FIXME: This should just return Attachment(id), which should be able to
>     # lazily fetch needed data.
> 
>-    def fetch_attachment(self, attachment_id):
>+    def fetch_attachment(self, attachment_id, throw_on_access_error=False):
>         # We could grab all the attachment details off of the attachment edit
>         # page but we already have working code to do so off of the bugs page,
>         # so re-use that.
>-        bug_id = self.bug_id_for_attachment_id(attachment_id)
>+        bug_id = self.bug_id_for_attachment_id(attachment_id, throw_on_access_error)
>         if not bug_id:
>             _log.warning("Unable to parse bug_id from attachment {}".format(attachment_id))
>             return None
>diff --git a/Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla_mock.py b/Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla_mock.py
>index fd889cdcdf7..fb9b71cf70e 100644
>--- a/Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla_mock.py
>+++ b/Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla_mock.py
>@@ -293,7 +293,7 @@ class MockBugzillaQueries(object):
>                 self._all_bugs())
>         return map(lambda bug: bug.id(), bugs_with_commit_queued_patches)
> 
>-    def fetch_attachment_ids_from_review_queue(self, since=None):
>+    def fetch_attachment_ids_from_review_queue(self, since=None, only_security_bugs=False):
>         unreviewed_patches = sum([bug.unreviewed_patches()
>                                   for bug in self._all_bugs()], [])
>         if since:
>@@ -395,7 +395,7 @@ class MockBugzilla(object):
>     def set_override_patch(self, patch):
>         self._override_patch = patch
> 
>-    def fetch_attachment(self, attachment_id):
>+    def fetch_attachment(self, attachment_id, throw_on_access_error=False):
>         if self._override_patch:
>             return self._override_patch
> 
>@@ -408,6 +408,20 @@ class MockBugzilla(object):
>             if attachment.id() == int(attachment_id):
>                 return attachment
> 
>+    def fetch_attachment_contents(self, attachment_id):
>+        return """
>+diff --git a/Makefile b/Makefile
>+index d30c90acb49..70a5284e56a 100644
>+--- a/Makefile
>++++ b/Makefile
>+@@ -1,4 +1,4 @@
>+-TOOLS_MODULE = Tools
>++ TOOLS_MODULE = Tools
>+ 
>+ ifneq (,$(DISABLE_WEBKIT_TOOLS))
>+        TOOLS_MODULE =
>+"""
>+
>     def bug_url_for_bug_id(self, bug_id):
>         return "%s/%s" % (self.bug_server_url, bug_id)
> 
>diff --git a/Tools/Scripts/webkitpy/common/net/statusserver.py b/Tools/Scripts/webkitpy/common/net/statusserver.py
>index c07ab65e3e3..eb07f6a1c3a 100644
>--- a/Tools/Scripts/webkitpy/common/net/statusserver.py
>+++ b/Tools/Scripts/webkitpy/common/net/statusserver.py
>@@ -32,6 +32,7 @@ from webkitpy.common.net.networktransaction import NetworkTransaction
> from webkitpy.thirdparty.BeautifulSoup import BeautifulSoup
> from webkitpy.common.config.urls import statusserver_default_host
> 
>+import StringIO
> import logging
> import urllib2
> 
>@@ -111,16 +112,21 @@ class StatusServer:
>         self._browser["high_priority_work_items"] = " ".join(high_priority_work_items)
>         return self._browser.submit().read()
> 
>-    def _post_work_item_to_ews(self, attachment_id):
>+    def _post_work_item_to_ews(self, attachment_id, attachment_data):
>         submit_to_ews_url = "%s/submit-to-ews" % self.url
>         self._browser.open(submit_to_ews_url)
>         self._browser.select_form(name="submit_to_ews")
>         self._browser["attachment_id"] = unicode(attachment_id)
>+        if attachment_data:
>+            if isinstance(attachment_data, unicode):
>+                attachment_data = attachment_data.encode('utf-8')
>+            attachment_data_filename = 'attachment-{}.patch'.format(attachment_id)
>+            self._browser.add_file(StringIO.StringIO(attachment_data), 'text/plain', attachment_data_filename, 'attachment_data')
>         self._browser.submit()
> 
>-    def submit_to_ews(self, attachment_id):
>+    def submit_to_ews(self, attachment_id, attachment_data=None):
>         _log.info("Submitting attachment %s to EWS queues" % attachment_id)
>-        return NetworkTransaction().run(lambda: self._post_work_item_to_ews(attachment_id))
>+        return NetworkTransaction().run(lambda: self._post_work_item_to_ews(attachment_id, attachment_data))
> 
>     def next_work_item(self, queue_name):
>         _log.info("Fetching next work item for %s" % queue_name)
>@@ -163,6 +169,10 @@ class StatusServer:
>         _log.info("SVN revision: %s broke %s" % (svn_revision_number, broken_bot))
>         return NetworkTransaction().run(lambda: self._post_svn_revision_to_server(svn_revision_number, broken_bot))
> 
>+    def fetch_attachment(patch_id):
>+        attachment_data_url = '{}/attachment/{}'.format(self.url, patch_id)
>+        return self._fetch_url(attachment_data_url)
>+
>     def _fetch_url(self, url):
>         # FIXME: This should use NetworkTransaction's 404 handling instead.
>         try:
>diff --git a/Tools/Scripts/webkitpy/common/net/statusserver_mock.py b/Tools/Scripts/webkitpy/common/net/statusserver_mock.py
>index 0fdd0ebe4f6..7fe3591259c 100644
>--- a/Tools/Scripts/webkitpy/common/net/statusserver_mock.py
>+++ b/Tools/Scripts/webkitpy/common/net/statusserver_mock.py
>@@ -59,7 +59,7 @@ class MockStatusServer(object):
>         self._work_items = work_items
>         _log.info("MOCK: update_work_items: %s %s" % (queue_name, high_priority_work_items + work_items))
> 
>-    def submit_to_ews(self, patch_id):
>+    def submit_to_ews(self, patch_id, attachment_data=None):
>         _log.info("MOCK: submit_to_ews: %s" % (patch_id))
> 
>     def update_status(self, queue_name, status, patch=None, results_file=None):
>@@ -71,3 +71,18 @@ class MockStatusServer(object):
> 
>     def results_url_for_status(self, status_id):
>         return "http://dummy_url"
>+
>+    def fetch_attachment(self, patch_id):
>+        return """
>+diff --git a/Makefile b/Makefile
>+index d30c90acb49..70a5284e56a 100644
>+--- a/Makefile
>++++ b/Makefile
>+@@ -1,4 +1,4 @@
>+-TOOLS_MODULE = Tools
>++ TOOLS_MODULE = Tools
>+ 
>+ ifneq (,$(DISABLE_WEBKIT_TOOLS))
>+        TOOLS_MODULE =
>+"""
>+
>diff --git a/Tools/Scripts/webkitpy/tool/bot/feeders.py b/Tools/Scripts/webkitpy/tool/bot/feeders.py
>index 7d673a2760e..32ac495e7cc 100644
>--- a/Tools/Scripts/webkitpy/tool/bot/feeders.py
>+++ b/Tools/Scripts/webkitpy/tool/bot/feeders.py
>@@ -84,9 +84,16 @@ class EWSFeeder(AbstractFeeder):
>         AbstractFeeder.__init__(self, tool)
> 
>     def feed(self):
>-        ids_needing_review = set(self._tool.bugs.queries.fetch_attachment_ids_from_review_queue(datetime.today() - timedelta(7)))
>+        current_time = datetime.today()
>+        ids_needing_review = set(self._tool.bugs.queries.fetch_attachment_ids_from_review_queue(current_time - timedelta(7)))
>+        security_ids_needing_review = frozenset(self._tool.bugs.queries.fetch_attachment_ids_from_review_queue(current_time - timedelta(7), only_security_bugs=True))
>         new_ids = ids_needing_review.difference(self._ids_sent_to_server)
>         _log.info("Feeding EWS (%s, %s new)" % (pluralize(len(ids_needing_review), "r? patch"), len(new_ids)))
>         for attachment_id in new_ids:  # Order doesn't really matter for the EWS.
>-            self._tool.status_server.submit_to_ews(attachment_id)
>+            # Submit patches for security bugs to the status server since the EWS queue machines may not
>+            # have permission to fetch them directly from Bugzilla.
>+            attachment_data = None
>+            if attachment_id in security_ids_needing_review:
>+                attachment_data = self._tool.bugs.fetch_attachment_contents(attachment_id)
>+            self._tool.status_server.submit_to_ews(attachment_id, attachment_data)
>             self._ids_sent_to_server.add(attachment_id)
>diff --git a/Tools/Scripts/webkitpy/tool/commands/queues.py b/Tools/Scripts/webkitpy/tool/commands/queues.py
>index 77c2d8827bc..21fdf5e273d 100644
>--- a/Tools/Scripts/webkitpy/tool/commands/queues.py
>+++ b/Tools/Scripts/webkitpy/tool/commands/queues.py
>@@ -40,7 +40,7 @@ from StringIO import StringIO
> 
> from webkitpy.common.config.committervalidator import CommitterValidator
> from webkitpy.common.config.ports import DeprecatedPort
>-from webkitpy.common.net.bugzilla import Attachment
>+from webkitpy.common.net.bugzilla import Bugzilla, Attachment
> from webkitpy.common.system.executive import ScriptError
> from webkitpy.tool.bot.botinfo import BotInfo
> from webkitpy.tool.bot.commitqueuetask import CommitQueueTask, CommitQueueTaskDelegate
>@@ -217,7 +217,11 @@ class AbstractPatchQueue(AbstractQueue):
>             patch_id = self._tool.status_server.next_work_item(self.name)
>             if not patch_id:
>                 return None
>-            patch = self._tool.bugs.fetch_attachment(patch_id)
>+            try:
>+                patch = self._tool.bugs.fetch_attachment(patch_id, throw_on_access_error=True)
>+            except Bugzilla.AccessError as e:
>+                if e.error_code == Bugzilla.AccessError.NOT_PERMITTED:
>+                    patch = self._tool.status_server.fetch_attachment(patch_id)
>             if not patch:
>                 # FIXME: Using a fake patch because release_work_item has the wrong API.
>                 # We also don't really need to release the lock (although that's fine),
>-- 
>2.17.0 (Apple Git-105)
>

Actions: View | Formatted Diff | Diff

Attachments on bug 186291: 341938 | 341939 | 342058 | 342741 | 342744 | 342765 | 342766 | 342767 | 342787 | 342789 | 342819 | 342836 | 342844 | 342858 | 342892 | 342907 | 343010