Fix #2376, remove URL collection

Adds and calculates shot.origin Makes some logic that used shot.url conditional Database migration to make data.url nullable
mozilla-services · Mar 21, 2017 · a9a076a · a9a076a
1 parent b5c828b
commit a9a076a
Show file tree

Hide file tree

Showing 8 changed files with 84 additions and 13 deletions.
diff --git a/addon/webextension/selector/documentMetadata.js b/addon/webextension/selector/documentMetadata.js
@@ -74,7 +74,6 @@ window.documentMetadata = (function () {
 
   return function documentMetadata() {
     let result = {};
-    result.url = location.href;
     result.docTitle = document.title;
     result.siteName = findSiteName();
     result.openGraph = getOpenGraph();

diff --git a/addon/webextension/selector/shooter.js b/addon/webextension/selector/shooter.js
@@ -107,7 +107,7 @@ window.shooter = (function () { // eslint-disable-line no-unused-vars
     backend,
     randomString(RANDOM_STRING_LENGTH) + "/" + domainFromUrl(location),
     {
-      url: location.href
+      origin: window.shot.originFromUrl(location.href)
     }
   );
   shot.update(documentMetadata());

diff --git a/server/db-patches/patch-14-15.sql b/server/db-patches/patch-14-15.sql
@@ -0,0 +1 @@
+ALTER TABLE data ALTER COLUMN url DROP NOT NULL;
diff --git a/server/db-patches/patch-15-14.sql b/server/db-patches/patch-15-14.sql
@@ -0,0 +1 @@
+ALTER TABLE data ALTER COLUMN url SET NOT NULL;
diff --git a/server/src/dbschema.js b/server/src/dbschema.js
@@ -4,7 +4,7 @@ const pgpatcher = require("pg-patcher");
 const path = require("path");
 const mozlog = require("mozlog")("dbschema");
 
-const MAX_DB_LEVEL = exports.MAX_DB_LEVEL = 14;
+const MAX_DB_LEVEL = exports.MAX_DB_LEVEL = 15;
 
 exports.forceDbVersion = function (version) {
   mozlog.info("forcing-db-version", {db: db.constr, version});

diff --git a/server/src/pages/shot/view.js b/server/src/pages/shot/view.js
@@ -295,7 +295,10 @@ class Body extends React.Component {
             <a className="block-button button secondary" href={ myShotsHref } onClick={this.onClickMyShots.bind(this)}>{ myShotsText }</a>
             <div className="shot-info">
               <EditableTitle title={shot.title} isOwner={this.props.isOwner} />
-              <div className="shot-subtitle">Saved from &nbsp;<a className="subtitle-link" href={ shotRedirectUrl } onClick={ this.onClickOrigUrl.bind(this, "navbar") }>{ linkTextShort }</a> <span className="clock-icon"/> { timeDiff } { expiresDiff } </div>
+              <div className="shot-subtitle">
+                { linkTextShort ? <span>Saved from &nbsp;<a className="subtitle-link" href={ shotRedirectUrl } onClick={ this.onClickOrigUrl.bind(this, "navbar") }>{ linkTextShort }</a></span> : null }
+                <span className="clock-icon"/> { timeDiff } { expiresDiff }
+              </div>
             </div>
           </div>
           <div className="more-shot-actions right">

diff --git a/server/src/servershot.js b/server/src/servershot.js
@@ -168,11 +168,12 @@ class Shot extends AbstractShot {
         oks.push({setHead: null});
         oks.push({setBody: null});
         let searchable = this._makeSearchableText(7);
+        let url = json.fullUrl || json.url || json.origin;
         return db.queryWithClient(
           client,
           `INSERT INTO data (id, deviceid, value, url, title, searchable_version, searchable_text)
            VALUES ($1, $2, $3, $4, $5, $6, ${searchable.query})`,
-          [this.id, this.ownerId, JSON.stringify(json), json.url, title, searchable.version].concat(searchable.args)
+          [this.id, this.ownerId, JSON.stringify(json), url, title, searchable.version].concat(searchable.args)
         ).then((rowCount) => {
           return clipRewrites.commit(client);
         }).then(() => {
@@ -245,9 +246,11 @@ class Shot extends AbstractShot {
       }
       queryParts.push(`setweight(to_tsvector(${addText(t)}), '${weight}') /* ${name} */`);
     }
-    let domain = this.url.replace(/^.*:/, "").replace(/\/.*$/, "");
+    if (this.url) {
+      let domain = this.url.replace(/^.*:/, "").replace(/\/.*$/, "");
+      addWeight(domain, 'B', 'domain');
+    }
     addWeight(this.title, 'A', 'title');
-    addWeight(domain, 'B', 'domain');
     if (this.openGraph) {
       let openGraphProps = `
         site_name description

diff --git a/shared/shot.js b/shared/shot.js
@@ -36,6 +36,42 @@ function isUrl(url) {
   return (/^https?:\/\/[a-z0-9\.\-]+[a-z0-9](:[0-9]+)?\/?/i).test(url);
 }
 
+function assertUrl(url) {
+  if (! url) {
+    throw new Error("Empty value is not URL");
+  }
+  if (! isUrl(url)) {
+    let exc = new Error("Not a URL");
+    exc.scheme = url.split(":")[0];
+    throw exc;
+  }
+}
+
+function assertOrigin(url) {
+  assertUrl(url);
+  if (url.search(/^https?:/i) != -1) {
+    let match = (/^https?:\/\/[^/:]+\/?$/i).exec(url);
+    if (! match) {
+      throw new Error("Bad origin, might include path");
+    }
+  }
+}
+
+function originFromUrl(url) {
+  if (! url) {
+    return null;
+  }
+  if (url.search(/^https?:/i) == -1) {
+    // Non-HTTP URLs don't have an origin
+    return null;
+  }
+  let match = (/^https?:\/\/[^/:]+/i).exec(url);
+  if (match) {
+    return match[0];
+  }
+  return null;
+}
+
 /** Check if the given object has all of the required attributes, and no extra
     attributes exception those in optional */
 function checkObject(obj, required, optional) {
@@ -168,7 +204,12 @@ class AbstractShot {
     assert((/^[a-zA-Z0-9]+\/[a-z0-9\.-]+$/).test(id), "Bad ID (should be alphanumeric):", JSON.stringify(id));
     this._backend = backend;
     this._id = id;
-    this.url = attrs.url;
+    this.origin = attrs.origin || null;
+    this.fullUrl = attrs.fullUrl || null;
+    if ((! attrs.fullUrl) && attrs.url) {
+      console.warn("Received deprecated attribute .url");
+      this.fullUrl = attrs.url;
+    }
     this.docTitle = attrs.docTitle || null;
     this.userTitle = attrs.userTitle || null;
     this.createdDate = attrs.createdDate || Date.now();
@@ -281,11 +322,30 @@ class AbstractShot {
   }
 
   get url() {
-    return this._url;
+    return this.fullUrl || this.origin;
   }
   set url(val) {
-    assert(val && isUrl(val), "Bad URL:", val);
-    this._url = val;
+    throw new Error(".url is read-only");
+  }
+
+  get fullUrl() {
+    return this._fullUrl;
+  }
+  set fullUrl(val) {
+    if (val) {
+      assertUrl(val);
+    }
+    this._url = val || undefined;
+  }
+
+  get origin() {
+    return this._origin;
+  }
+  set origin(val) {
+    if (val) {
+      assertOrigin(val);
+    }
+    this._origin = val || undefined;
   }
 
   get filename() {
@@ -304,6 +364,9 @@ class AbstractShot {
   }
 
   get urlDisplay() {
+    if (! this.url) {
+      return null;
+    }
     if (this.url.search(/^https?/i) != -1) {
       let txt = this.url;
       txt = txt.replace(/^[a-z]+:\/\//i, "");
@@ -507,15 +570,15 @@ class AbstractShot {
 }
 
 AbstractShot.prototype.REGULAR_ATTRS = (`
-url docTitle userTitle createdDate favicon images
+origin fullUrl docTitle userTitle createdDate favicon images
 siteName openGraph twitterCard documentSize
 fullScreenThumbnail abTests
 `).split(/\s+/g);
 
 // Attributes that will be accepted in the constructor, but ignored/dropped
 AbstractShot.prototype.DEPRECATED_ATTRS = (`
 microdata history ogTitle createdDevice head body htmlAttrs bodyAttrs headAttrs
-readable hashtags comments showPage isPublic resources deviceId
+readable hashtags comments showPage isPublic resources deviceId url
 `).split(/\s+/g);
 
 AbstractShot.prototype.RECALL_ATTRS = (`
@@ -659,4 +722,5 @@ AbstractShot.prototype.Clip = _Clip;
 
 if (typeof exports != "undefined") {
   exports.AbstractShot = AbstractShot;
+  exports.originFromUrl = originFromUrl;
 }