Obtain Chromium file contents asynchronously (#3801)

* Squashed commit of the following: commit 5ee220fded Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon Apr 15 17:11:42 2024 -0700 npm: bump @babel/preset-env from 7.24.3 to 7.24.4 (#3799) Bumps [@babel/preset-env](https://github.com/babel/babel/tree/HEAD/packages/babel-preset-env) from 7.24.3 to 7.24.4. - [Release notes](https://github.com/babel/babel/releases) - [Changelog](https://github.com/babel/babel/blob/main/CHANGELOG.md) - [Commits](https://github.com/babel/babel/commits/v7.24.4/packages/babel-preset-env) --- updated-dependencies: - dependency-name: "@babel/preset-env" dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> commit 66c768273e Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon Apr 15 16:21:28 2024 -0700 npm: bump rollup from 4.14.1 to 4.14.3 (#3797) Bumps [rollup](https://github.com/rollup/rollup) from 4.14.1 to 4.14.3. - [Release notes](https://github.com/rollup/rollup/releases) - [Changelog](https://github.com/rollup/rollup/blob/master/CHANGELOG.md) - [Commits](https://github.com/rollup/rollup/compare/v4.14.1...v4.14.3) --- updated-dependencies: - dependency-name: rollup dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> commit bea0dd3af3 Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon Apr 15 15:47:05 2024 -0700 npm: bump @babel/core from 7.24.3 to 7.24.4 (#3798) Bumps [@babel/core](https://github.com/babel/babel/tree/HEAD/packages/babel-core) from 7.24.3 to 7.24.4. - [Release notes](https://github.com/babel/babel/releases) - [Changelog](https://github.com/babel/babel/blob/main/CHANGELOG.md) - [Commits](https://github.com/babel/babel/commits/v7.24.4/packages/babel-core) --- updated-dependencies: - dependency-name: "@babel/core" dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> commit 4142251f32 Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon Apr 15 15:46:43 2024 -0700 npm: bump tar from 6.2.1 to 7.0.1 (#3796) Bumps [tar](https://github.com/isaacs/node-tar) from 6.2.1 to 7.0.1. - [Release notes](https://github.com/isaacs/node-tar/releases) - [Changelog](https://github.com/isaacs/node-tar/blob/main/CHANGELOG.md) - [Commits](https://github.com/isaacs/node-tar/compare/v6.2.1...v7.0.1) --- updated-dependencies: - dependency-name: tar dependency-type: direct:development update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> commit 57437c166c Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon Apr 15 15:46:15 2024 -0700 npm: bump sass from 1.74.1 to 1.75.0 (#3795) Bumps [sass](https://github.com/sass/dart-sass) from 1.74.1 to 1.75.0. - [Release notes](https://github.com/sass/dart-sass/releases) - [Changelog](https://github.com/sass/dart-sass/blob/main/CHANGELOG.md) - [Commits](https://github.com/sass/dart-sass/compare/1.74.1...1.75.0) --- updated-dependencies: - dependency-name: sass dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> commit 6358a50bfa Author: Jeffrey Yasskin <jyasskin@google.com> Date: Mon Apr 15 12:30:20 2024 -0700 Show feature links in the Consensus section (#3791) Also denormalizes external reviewer positions into the FeatureEntries table. commit cf742510fa Author: Daniel Smith <56164590+DanielRyanSmith@users.noreply.github.com> Date: Mon Apr 15 10:20:25 2024 -0700 Check that OT Chromium name not used by other OTs (#3792) * async calls * update tests * type hinting * remove print * Changes suggested by @jrobbins * lint fix
2024-04-18 11:39:01 -07:00 · 2024-04-18 11:39:01 -07:00 · d8cf7f26f1
--- a/api/origin_trials_api.py
+++ b/api/origin_trials_api.py
@ -12,7 +12,9 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.

+import asyncio
 import json5
+import logging
 import requests
 import validators
 from base64 import b64decode
@ -29,13 +31,16 @@ ENABLED_FEATURES_FILE_URL = 'https://chromium.googlesource.com/chromium/src/+/ma
 GRACE_PERIOD_FILE = 'https://chromium.googlesource.com/chromium/src/+/main/third_party/blink/common/origin_trials/manual_completion_origin_trial_features.cc?format=TEXT'


-def get_chromium_file(url: str) -> str:
+def get_chromium_file(url: str) -> asyncio.Future[requests.Response]:
  """Get chromium file contents from a given URL"""
+  loop = asyncio.get_event_loop()
  try:
-    resp = requests.get(url)
+    file_future = loop.run_in_executor(None, requests.get, url)
  except requests.RequestException as e:
+    logging.exception(
+        f'Failed to get response to obtain Chromium file: {url}')
    raise e
-  return b64decode(resp.text).decode('utf-8')
+  return file_future


 class OriginTrialsAPI(basehandlers.EntitiesAPIHandler):
@ -55,22 +60,36 @@ class OriginTrialsAPI(basehandlers.EntitiesAPIHandler):

    return trials_list

-  def _validate_creation_args(
-      self, body: dict) -> dict:
+  async def _validate_creation_args(
+      self, body: dict) -> dict[str, str]:
    """Check that all provided OT creation arguments are valid."""
+    try:
+      files_async = [
+          get_chromium_file(ENABLED_FEATURES_FILE_URL),
+          get_chromium_file(WEBFEATURE_FILE_URL),
+          get_chromium_file(GRACE_PERIOD_FILE)]
+      responses = await asyncio.gather(*files_async)
+      enabled_features_text, webfeature_file, grace_period_file = [
+          b64decode(resp.text).decode('utf-8') for resp in responses]
+    except requests.exceptions.RequestException:
+      self.abort(500, 'Error obtaining Chromium file for validation')
    validation_errors: dict[str, str] = {}
    chromium_trial_name = body.get(
        'ot_chromium_trial_name', {}).get('value')
    if chromium_trial_name is None:
      self.abort(400, 'Chromium trial name not specified.')

-    trials_list = origin_trials_client.get_trials_list()
+    # Check if a trial already exists with the same name.
+    try:
+      trials_list = origin_trials_client.get_trials_list()
+    except requests.exceptions.RequestException:
+      self.abort(500, 'Error obtaining origin trial data from API')
+
    if (any(trial['origin_trial_feature_name'] == chromium_trial_name
            for trial in trials_list)):
      validation_errors['ot_chromium_trial_name'] = (
          'Chromium trial name is already used by another origin trial')

-    enabled_features_text = get_chromium_file(ENABLED_FEATURES_FILE_URL)
    enabled_features_json = json5.loads(enabled_features_text)
    if (not any(feature.get('origin_trial_feature_name') == chromium_trial_name
                for feature in enabled_features_json['data'])):
@ -82,9 +101,7 @@ class OriginTrialsAPI(basehandlers.EntitiesAPIHandler):
      if use_counter is None:
        validation_errors['ot_webfeature_use_counter'] = (
            'No UseCounter specified for non-deprecation trial.')
-      else:
-        webfeature_file = get_chromium_file(WEBFEATURE_FILE_URL)
-        if f'{use_counter} =' not in webfeature_file:
+      elif f'{use_counter} =' not in webfeature_file:
          validation_errors['ot_webfeature_use_counter'] = (
              'UseCounter not landed in web_feature.mojom')

@ -98,7 +115,6 @@ class OriginTrialsAPI(basehandlers.EntitiesAPIHandler):
              f'{feature["name"]}')

    if body.get('ot_is_critical_trial', {}).get('value', False):
-      grace_period_file = get_chromium_file(GRACE_PERIOD_FILE)
      if (f'blink::mojom::OriginTrialFeature::k{chromium_trial_name}'
          not in grace_period_file):
        validation_errors['ot_is_critical_trial'] = (
@ -132,7 +148,8 @@ class OriginTrialsAPI(basehandlers.EntitiesAPIHandler):
      return redirect_resp

    body = self.get_json_param_dict()
-    validation_errors = self._validate_creation_args(body)
+    loop = asyncio.get_event_loop()
+    validation_errors = loop.run_until_complete(self._validate_creation_args(body))
    if validation_errors:
      return {
          'message': 'Errors found when validating arguments',
--- a/api/origin_trials_api_test.py
+++ b/api/origin_trials_api_test.py
@ -123,7 +123,7 @@ bool FeatureHasExpiryGracePeriod(blink::mojom::OriginTrialFeature feature) {
        entity.key.delete()

  @mock.patch('api.origin_trials_api.get_chromium_file')
-  def test_validate_creation_args__valid(self, mock_get_chromium_file):
+  async def test_validate_creation_args__valid(self, mock_get_chromium_file):
    """No error messages should be returned if all args are valid."""
    mock_get_chromium_file.side_effect = [
      self.mock_features_file,
@ -154,12 +154,12 @@ bool FeatureHasExpiryGracePeriod(blink::mojom::OriginTrialFeature feature) {
    }
    # No exception should be raised.
    with test_app.test_request_context(self.request_path):
-       result = self.handler._validate_creation_args(body)
+       result = await self.handler._validate_creation_args(body)
    expected = {}
    self.assertEqual(expected, result)

  @mock.patch('api.origin_trials_api.get_chromium_file')
-  def test_validate_creation_args__invalid_webfeature_use_counter(
+  async def test_validate_creation_args__invalid_webfeature_use_counter(
      self, mock_get_chromium_file):
    """Error message returned if UseCounter not found in file."""
    mock_get_chromium_file.side_effect = [
@ -195,7 +195,7 @@ bool FeatureHasExpiryGracePeriod(blink::mojom::OriginTrialFeature feature) {
    self.assertEqual(expected, result)

  @mock.patch('api.origin_trials_api.get_chromium_file')
-  def test_validate_creation_args__missing_webfeature_use_counter(
+  async def test_validate_creation_args__missing_webfeature_use_counter(
      self, mock_get_chromium_file):
    """Error message returned if UseCounter is missing."""
    mock_get_chromium_file.side_effect = [
@ -221,13 +221,13 @@ bool FeatureHasExpiryGracePeriod(blink::mojom::OriginTrialFeature feature) {
      },
    }
    with test_app.test_request_context(self.request_path):
-      result = self.handler._validate_creation_args(body)
+      result = await self.handler._validate_creation_args(body)
    expected = {'ot_webfeature_use_counter': (
        'No UseCounter specified for non-deprecation trial.')}
    self.assertEqual(expected, result)

  @mock.patch('api.origin_trials_api.get_chromium_file')
-  def test_validate_creation_args__missing_webfeature_use_counter_deprecation(
+  async def test_validate_creation_args__missing_webfeature_use_counter_deprecation(
      self, mock_get_chromium_file):
    """No error message returned for missing UseCounter if deprecation trial."""
    """Deprecation trial does not need a webfeature use counter value."""
@ -254,12 +254,12 @@ bool FeatureHasExpiryGracePeriod(blink::mojom::OriginTrialFeature feature) {
      },
    }
    with test_app.test_request_context(self.request_path):
-      result = self.handler._validate_creation_args(body)
+      result = await self.handler._validate_creation_args(body)
    expected = {}
    self.assertEqual(expected, result)

  @mock.patch('api.origin_trials_api.get_chromium_file')
-  def test_validate_creation_args__invalid_chromium_trial_name(
+  async def test_validate_creation_args__invalid_chromium_trial_name(
      self, mock_get_chromium_file):
    """Error message returned if Chromium trial name not found in file."""
    mock_get_chromium_file.side_effect = [
@ -289,13 +289,13 @@ bool FeatureHasExpiryGracePeriod(blink::mojom::OriginTrialFeature feature) {
      },
    }
    with test_app.test_request_context(self.request_path):
-      result = self.handler._validate_creation_args(body)
+      result = await self.handler._validate_creation_args(body)
    expected = {'ot_chromium_trial_name': (
        'Origin trial feature name not found in file')}
    self.assertEqual(expected, result)

  @mock.patch('api.origin_trials_api.get_chromium_file')
-  def test_validate_creation_args__missing_chromium_trial_name(
+  async def test_validate_creation_args__missing_chromium_trial_name(
      self, mock_get_chromium_file):
    """Error message returned if Chromium trial is missing from request."""
    mock_get_chromium_file.side_effect = [
@ -322,10 +322,10 @@ bool FeatureHasExpiryGracePeriod(blink::mojom::OriginTrialFeature feature) {
    }
    with test_app.test_request_context(self.request_path):
      with self.assertRaises(werkzeug.exceptions.BadRequest):
-        self.handler._validate_creation_args(body)
+        await self.handler._validate_creation_args(body)

  @mock.patch('api.origin_trials_api.get_chromium_file')
-  def test_validate_creation_args__invalid_third_party_trial(
+  async def test_validate_creation_args__invalid_third_party_trial(
      self, mock_get_chromium_file):
    """Error message returned if third party support not found in file."""
    mock_get_chromium_file.side_effect = [
@ -355,7 +355,7 @@ bool FeatureHasExpiryGracePeriod(blink::mojom::OriginTrialFeature feature) {
      },
    }
    with test_app.test_request_context(self.request_path):
-      result = self.handler._validate_creation_args(body)
+      result = await self.handler._validate_creation_args(body)
    expected = {'ot_has_third_party_support': (
        'One or more features do not have third party '
        'support set in runtime_enabled_features.json5. '
@ -363,7 +363,7 @@ bool FeatureHasExpiryGracePeriod(blink::mojom::OriginTrialFeature feature) {
    self.assertEqual(expected, result)

  @mock.patch('api.origin_trials_api.get_chromium_file')
-  def test_validate_creation_args__invalid_critical_trial(
+  async def test_validate_creation_args__invalid_critical_trial(
      self, mock_get_chromium_file):
    """Error message returned if critical trial name not found in file."""
    mock_get_chromium_file.side_effect = [
@ -393,7 +393,7 @@ bool FeatureHasExpiryGracePeriod(blink::mojom::OriginTrialFeature feature) {
      },
    }
    with test_app.test_request_context(self.request_path):
-      result = self.handler._validate_creation_args(body)
+      result = await self.handler._validate_creation_args(body)
    expected = {'ot_is_critical_trial': (
        'Use counter has not landed in grace period array for critical trial')}
    self.assertEqual(expected, result)
--- a/client-src/elements/chromedash-link.js
+++ b/client-src/elements/chromedash-link.js
@ -569,6 +569,14 @@ export class ChromedashLink extends LitElement {
    }
  }

+  withLabel(link) {
+    if (this.showContentAsLabel) {
+      return html`<slot></slot>: ${link}`;
+    } else {
+      return link;
+    }
+  }
+
  render() {
    if (!this.href) {
      console.error('Missing [href] attribute in', this);
--- a/client-src/elements/chromedash-link_test.js
+++ b/client-src/elements/chromedash-link_test.js
@ -83,6 +83,51 @@ it('shows content as label when requested', async () => {
  );
 });

+it('shows content as label when requested', async () => {
+  const yesterday = new Date(Date.now() - DAY);
+  const featureLinks = [
+    {
+      url: 'https://github.com/GoogleChrome/chromium-dashboard/issues/3007',
+      type: 'github_issue',
+      information: {
+        url: 'https://api.github.com/repos/GoogleChrome/chromium-dashboard/issues/3007',
+        state: 'open',
+        created_at: yesterday.toISOString(),
+      },
+    },
+  ];
+
+  const elWithLabel = await fixture(
+    html`<chromedash-link
+      showContentAsLabel
+      href="https://github.com/GoogleChrome/chromium-dashboard/issues/3007"
+      .featureLinks=${featureLinks}
+      >Content</chromedash-link
+    >`
+  );
+  expect(elWithLabel).shadowDom.to.equal(
+    `<slot>...</slot>:
+     <a href="https://github.com/GoogleChrome/chromium-dashboard/issues/3007"
+        rel="noopener noreferrer"
+        target="_blank">...</a>`,
+    {ignoreChildren: ['slot', 'a']}
+  );
+
+  const elWithoutLabel = await fixture(
+    html`<chromedash-link
+      href="https://github.com/GoogleChrome/chromium-dashboard/issues/3007"
+      .featureLinks=${featureLinks}
+      >Content</chromedash-link
+    >`
+  );
+  expect(elWithoutLabel).shadowDom.to.equal(
+    `<a href="https://github.com/GoogleChrome/chromium-dashboard/issues/3007"
+        rel="noopener noreferrer"
+        target="_blank">...</a>`,
+    {ignoreChildren: ['slot', 'a']}
+  );
+});
+
 describe('Github issue links', () => {
  // Get the 'information' object by running
  // `curl -L https://api.github.com/repos/OWNER/REPO/issues/ISSUE_NUMBER`
--- a/framework/basehandlers.py
+++ b/framework/basehandlers.py
@ -41,15 +41,13 @@ from internals.data_types import CHANGED_FIELDS_LIST_TYPE
 from internals.core_models import FeatureEntry, MilestoneSet, Stage
 from internals import user_models

-from google.auth.transport import requests
 from flask import session
 from flask import render_template
 from flask_cors import CORS
-import sys

 # Our API responses are prefixed with this ro prevent attacks that
 # exploit <script src="...">.  See go/xssi.
-XSSI_PREFIX = ')]}\'\n';
+XSSI_PREFIX = ')]}\'\n'


 # See https://www.regextester.com/93901 for url regex