amp-geo: Fall back to API for country by mdmower · Pull Request #26407 · ampproject/amphtml

mdmower · 2020-01-18T06:11:09Z

If country code is not available in a prerendered body's classes or in
amp-geo-0.1.js itself, support falling back to an API request. The
publisher must provide the API endpoint; cdn.ampproject.org will
not host a general country code API.

Allow case insensitive country code in JSON schema (country is most
often presented in uppercase when ISO 3166-1 table is referenced).

JSON schema of Geo API response - version 0.1:

{
  "$schema": "http://json-schema.org/draft-07/schema#",
  "type": "object",
  "properties": {
    "country": {
      "type": "string",
      "title": "ISO 3166-1 alpha-2 (case insensitive) country code of client request",
      "default": "",
      "pattern": "^[a-zA-Z]{2}$"
    }
  },
  "required": [
    "country"
  ]
}

Sample response:

{
  "country": "de"
}

If country code is not available in a prerendered body's classes or in amp-geo-0.1.js itself, support falling back to an API request if the publisher must provide (cdn.ampproject.org will not host a general country code API). Allow case insensitive country code in JSON schema (country is most often presented in uppercase when ISO 3166-1 table is referenced). JSON schema of Geo API response - version 0.1: { "$schema": "http://json-schema.org/draft-07/schema#", "type": "object", "properties": { "country": { "type": "string", "title": "ISO 3166-1 alpha-2 (case insensitive) country code of client request", "default": "", "pattern": "^[a-zA-Z]{2}$" } }, "required": [ "country" ] } Sample response: { "country": "de" } temp temp

mdmower · 2020-02-01T21:52:35Z

@Gregable @sebastianbenz @zhouyx @jridgewell - I believe this PR matches the amp-geo API design we discussed (relates to #25873). If any of you have time for a review (or could recommend a reviewer) it would be appreciated.

zhouyx · 2020-02-03T19:13:35Z

Thank you @mdmower, as mentioned in the meeting, one thing that we are actively working on is to add more granular geo detection support to amp-geo. Thoughts on supporting ISO 3166-2 format in the future using the same API?

mdmower · 2020-02-04T15:59:51Z

Thoughts on supporting ISO 3166-2 format in the future using the same API?

@zhouyx The JSON schema could either be modified to allow ISO 3166-1 or ISO 3166-2 in a single property (instead of country, it could be called region), or a second optional subdivision property could be introduced. In either case, if only the ISO 3166-1 code is available, then amp-geo works at the country level, and if ISO 3166-2 is available, amp-geo works at the subdivision level. Do you have a preference for one of these schema changes?

zhouyx · 2020-02-06T01:49:12Z

I prefer the second optional subdivision property because in this case it doesn't mix thee required country code vs the optional region code.

Two concerns I have. These concerns are not blocking this change. But I want to come up with a solution to the first concern when we launch US-CA support.

AMP is going to only support US-CA in short term. This means that US without subdivision code can mean 1. subdivision is not supported, or 2. user is not in US-CA. This means that a CA user can be labeled as a non CA user during self host. I want to understand how bad this is given publishers are going to rely on the decision to apply CCPA settings.
When providing the region information through AMP's cdn, there will be a confidence value that also comes with decision. I briefly mentioned this in I2I: amp-geo to add preset-us-ca #26637. Though the generic region support design has not been finalized yet, it's likely that we would also require such a confidence level value from the endpoint.

Thanks

mdmower · 2020-02-06T04:28:41Z

@zhouyx Thanks for the details. I believe this JSON example would satisfy your requirements.

{
    "countries": [
        {
            "country": "US",
            "confidence": 0.9,
            "subdivisions: [
                {
                    "subdivision": "WA",
                    "confidence": 0.6
                },
                {
                    "subdivision": "OR",
                    "confidence": 0.4
                }
            ]
        },
        {
            "country": "CA",
            "confidence": 0.1,
            "subdivisions: [
                {
                    "subdivision": "BC",
                    "confidence": 1
                }
            ]
        }
    ]
}

Special cases:

countries: [] - Country lookup supported but no country was identified
countries[n].subdivisions: null - Subdivision lookup not supported
countries[n].subdivisions: [] - Subdivision lookup supported but no subdivision was identified

Confidence ranges from 0.0 to 1.0 and the sum of all confidences for countries or subdivisions should not exceed 1.0 (but can be less than 1.0).

For example, if an API supports only country lookup and has no support for confidence levels or subdivisions, it would report:

{
    "countries": [
        {
            "country": "US",
            "confidence": 1,
            "subdivisions: null
        }
    ]
}

Let me know if you think a schema that fits this model would be preferable at start, or if it should be introduced later as "Version 0.2".

zhouyx · 2020-02-12T00:05:17Z

Thank you for the proposal! I have reached out to the team who provides the amp-geo service, because I don't feel comfortable finalizing the endpoint design before we have a complete story from them.

A few concerns we have today

support for disputed area, which an ISO code does not exist.
confidence level and how we want to handle it. (Note it's most likely that only one result with a confidence level will be returned). that looks like

{
  country: 'us'
  region 'us-ca'
  confidence level: 0.8
}

there won't be another entry with confidence level 0.2. Also the confidence level will likely only applies to region value.

Because we don't have the design finalized. Let's only focus on country and us-ca for now. What about this

region not supported

{
  "country": "us"
}

region supported, and in us-ca

{
  "country": "us",
  "region": "us-ca"
}

region supported, not in us-ca. or can't verify region

{
  "country": "us",
  "region": null // or any value that's not "us-ca" for now
}

We are still deciding if region value should be "ca" or "us-ca", let's only get "country" support in this PR.

jridgewell

LGTM. Needs @zhouyx's approval, and probably @Gregable's.

zhouyx · 2020-02-21T19:08:44Z

extensions/amp-geo/0.1/amp-geo.js

+   * @return {Promise<(string|null)>}
+   */
+  fetchCountry_() {
+    if (typeof urls.geoApi !== 'string') {


@jridgewell Did we require https in urls? If not we should use assertHttpsUrl

HTTPS sounds like a good requirement to protect user privacy (physical location). assertHttpsUrl is geared more towards AMPHTML elements that have disallowed src attribute values. I opted for isSecureUrlDeprecated, which also makes allowances for localhost testing, since it is a bit more generic and less tied to AmpDoc.

zhouyx · 2020-02-21T19:11:26Z

extensions/amp-geo/0.1/amp-geo.js

+      })
+      .then(res => res.json())
+      .then(json => {
+        if (!/^[a-z]{2}$/i.test(json.country)) {


nit: please use json['country'] to avoid the name minimize.

zhouyx · 2020-02-21T19:11:43Z

extensions/amp-geo/0.1/amp-geo.js

+          );
+          return null;
+        }
+        return json.country.toLowerCase();


nit: same here, json['country']

zhouyx · 2020-02-21T19:23:55Z

extensions/amp-geo/0.1/amp-geo.js

@@ -199,14 +202,97 @@ export class AmpGeo extends AMP.BaseElement {
      this.mode_ = mode.GEO_HOT_PATCH;
      this.country_ = trimmedCountryMatch[1];
    } else if (trimmedCountryMatch[0] === '' && !getMode(this.win).localDev) {


nit: I'd prefer to handle one mode in one if statement block. Can we please change the logic to

else if (...) { this.mode_ = mode.GEO_HOT_PATCH; } else if (trimmedCountryMatch[0] === '' && urls.geoApi) { this.mode_ = mode.GEO_API; } else if (!getMode(this.win).localDev) { this.error_ = true; } ....

zhouyx · 2020-02-21T19:27:30Z

extensions/amp-geo/0.1/amp-geo.js

+        method: 'GET',
+        credentials: 'omit',
+      })
+      .then(res => res.json())


@jridgewell Curious does xhr request timeout by default? Should we add a timeout here?

Timeout introduced in latest commit.

hmmm 60 seconds timeout. @jridgewell what's the typical timeout value applied by AMP runtime?

- Code style preferences - Require HTTPS and non-ampproject-cdn API URL - Timeout API request after 60 seconds - Add timeout test - Cleanup test output of new tests

zhouyx · 2020-02-25T03:20:53Z

extensions/amp-geo/0.1/amp-geo.js

+      return null;
+    }
+
+    if (isProxyOrigin(url)) {


ProxyOrigin doesn't work the same reason any random url doesn't work. I don't think we need a special check for this. Let me know if you think differently.

Admittedly, there's not much reason for this check. It can be removed. I will wait for a final decision on isSecureUrlDeprecated/assertHttpsUrl before pushing a new change.

I don't think it's needed.

zhouyx · 2020-02-25T03:21:12Z

extensions/amp-geo/0.1/amp-geo.js

@@ -199,14 +202,97 @@ export class AmpGeo extends AMP.BaseElement {
      this.mode_ = mode.GEO_HOT_PATCH;
      this.country_ = trimmedCountryMatch[1];
    } else if (trimmedCountryMatch[0] === '' && !getMode(this.win).localDev) {


zhouyx · 2020-02-25T03:33:37Z

extensions/amp-geo/0.1/amp-geo.js

+      return null;
+    }
+
+    if (!isSecureUrlDeprecated(url)) {


Hmmm the function name tells me that it's deprecated : )
#isSecureUrlDeprecated() doesn't handle the relative url case //, but #assertHttpsUrl() does.
The elementContext and sourceName makes the #assertHttpsUrl() looks like it's tied to AMP elements. But they are used only as error message. A better name is probably context instead of elementContext. What about

assertHttpsUrl(url, 'geoApiUrl', 'AMP_CONFIG urls')

I'm not convinced assertHttpsUrl() is the right choice. It's a wrapper for isSecureUrlDeprecated(url) || /^(\/\/)/.test(url). Given the privacy implications of user location, do you really want to allow relative protocol? It seems more reasonable to directly make use of isSecureUrlDeprecated which requires HTTPS even if an AMP page is served from HTTP.

The reason it's deprecated is because it doesn't work properly in the different ampdoc modes. Use Services.urlFor's isSecure() instead.

- Prefer Services.Url.isSecure to test if a string URL is HTTPS - Remove unnecessary check for isProxyOrigin - Remove unreachable code

mdmower · 2020-02-26T04:13:48Z

Thank you both for the reviews. Modifications made.

zhouyx

LGTM with one minor request. Thanks!

zhouyx · 2020-02-26T18:50:48Z

extensions/amp-geo/0.1/amp-geo.js

+   * @param {*} url
+   * @return {?string}
+   */
+  validateApiUrl(url) {


nit: let's make it a private method please. Thanks

mdmower · 2020-02-28T15:41:35Z

Are we ok with waiting to document this feature until #25873 is ready? Availability of self-hosting is when this feature becomes useful.

zhouyx · 2020-02-28T20:41:54Z

That's sounds like the optimal approach. SGTM Thanks

* master: Launch `amp-next-page` v2 & clean up experiment (ampproject#27035) ✨Implement Display Locking on amp-accordion (ampproject#25867) 📖<amp-next-page> documentation and validation (ampproject#26841) Improve visibility event tests (ampproject#26847) amp-geo: Fall back to API for country (ampproject#26407) ✨ Add customization options to <amp-story-quiz> (ampproject#26714) navigation: Minor test improvements (ampproject#26926) ♻️ Alias video.fullscreen action for symmetry with interface names (ampproject#27017) ✨ Implements `amp-analytics` support for `amp-next-page` (ampproject#26451) ✨ amp-video-iframe integration: global jwplayer() singleton by default (ampproject#26969) Fix unit tests broken by ampproject#26687 (ampproject#27000) Filter redirect info from emails (ampproject#27012) 📖 Make amp-bind doc valid, fix amp-form stories filter (ampproject#27003) url-replacements: Minor test improvement (ampproject#26930) viewport: Minor test improvement (ampproject#26931) amp-consent fullscreen warning (ampproject#26467) dep-check: USE CAPS FOR IMPORTANCE (ampproject#26993) fix img url (ampproject#26987) # Conflicts: # extensions/amp-next-page/amp-next-page.md

googlebot added the cla: yes label Jan 18, 2020

mdmower mentioned this pull request Jan 18, 2020

I2I: AMP Runtime Self-Hosting #25873

Open

mdmower changed the title ~~[WIP] amp-geo: Fallback to API for country~~ [WIP] amp-geo: Fall back to API for country Jan 19, 2020

This was referenced Jan 19, 2020

Improve support for self-hosted runtime #25026

Closed

ampdoc: Custom runtime support for singleDoc #25028

Closed

mdmower added 2 commits February 1, 2020 12:59

amp-geo: Add API tests

7ebb3bf

mdmower force-pushed the patch-geo-01 branch from 44c808e to 7ebb3bf Compare February 1, 2020 21:43

mdmower marked this pull request as ready for review February 1, 2020 21:47

mdmower changed the title ~~[WIP] amp-geo: Fall back to API for country~~ amp-geo: Fall back to API for country Feb 1, 2020

amp-bundle-size bot requested a review from jeffjose February 1, 2020 21:51

amp-geo: Define geoApi(Url) to config types

65c41c4

jridgewell reviewed Feb 21, 2020

View reviewed changes

zhouyx suggested changes Feb 21, 2020

View reviewed changes

amp-geo: API revisions based on review

7c2f2c1

- Code style preferences - Require HTTPS and non-ampproject-cdn API URL - Timeout API request after 60 seconds - Add timeout test - Cleanup test output of new tests

mdmower requested a review from zhouyx February 24, 2020 06:52

zhouyx reviewed Feb 25, 2020

View reviewed changes

amp-geo: Revisions from review

b574612

- Prefer Services.Url.isSecure to test if a string URL is HTTPS - Remove unnecessary check for isProxyOrigin - Remove unreachable code

mdmower requested a review from zhouyx February 26, 2020 04:14

amp-geo: Restore test

c458b06

zhouyx approved these changes Feb 26, 2020

View reviewed changes

jridgewell approved these changes Feb 26, 2020

View reviewed changes

amp-geo: Mark methods private

752f769

Merge remote-tracking branch 'origin/master' into patch-geo-01

a9bf5e1

mdmower merged commit 26fde84 into ampproject:master Feb 28, 2020

ampprojectbot added PR use: In Beta / Experimental PR use: In Stable labels Mar 10, 2020

mdmower deleted the patch-geo-01 branch March 16, 2020 01:25

mdmower mentioned this pull request Apr 15, 2020

Add preset-us-ca set to amp-geo #27571

Merged

Conversation

mdmower commented Jan 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdmower commented Feb 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhouyx commented Feb 3, 2020

Uh oh!

mdmower commented Feb 4, 2020

Uh oh!

zhouyx commented Feb 6, 2020

Uh oh!

mdmower commented Feb 6, 2020

Uh oh!

zhouyx commented Feb 12, 2020

Uh oh!

jridgewell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdmower commented Feb 26, 2020

Uh oh!

zhouyx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdmower commented Feb 28, 2020

Uh oh!

zhouyx commented Feb 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mdmower commented Jan 18, 2020 •

edited

Loading

mdmower commented Feb 1, 2020 •

edited

Loading