From a9a076a76373ee32628af18b4eb69ae3aff6f4db Mon Sep 17 00:00:00 2001 From: Ian Bicking Date: Tue, 21 Mar 2017 17:29:27 -0500 Subject: [PATCH] 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 --- .../webextension/selector/documentMetadata.js | 1 - addon/webextension/selector/shooter.js | 2 +- server/db-patches/patch-14-15.sql | 1 + server/db-patches/patch-15-14.sql | 1 + server/src/dbschema.js | 2 +- server/src/pages/shot/view.js | 5 +- server/src/servershot.js | 9 ++- shared/shot.js | 76 +++++++++++++++++-- 8 files changed, 84 insertions(+), 13 deletions(-) create mode 100644 server/db-patches/patch-14-15.sql create mode 100644 server/db-patches/patch-15-14.sql diff --git a/addon/webextension/selector/documentMetadata.js b/addon/webextension/selector/documentMetadata.js index 1a46381280..1ef926547d 100644 --- 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 index 8040d3dbef..3004c195b0 100644 --- 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 new file mode 100644 index 0000000000..4ba988151b --- /dev/null +++ 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 new file mode 100644 index 0000000000..20495286c1 --- /dev/null +++ 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 index 378f9971b6..374dad2567 100644 --- 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 index 1e06a11d29..3409524bd6 100644 --- a/server/src/pages/shot/view.js +++ b/server/src/pages/shot/view.js @@ -295,7 +295,10 @@ class Body extends React.Component { { myShotsText }
-
Saved from  { linkTextShort } { timeDiff } { expiresDiff }
+
+ { linkTextShort ? Saved from  { linkTextShort } : null } + { timeDiff } { expiresDiff } +
diff --git a/server/src/servershot.js b/server/src/servershot.js index 5fa103f24a..c260e7318b 100644 --- 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 index d2fa2859d4..b9bea8b7d3 100644 --- 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,7 +570,7 @@ 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); @@ -515,7 +578,7 @@ fullScreenThumbnail abTests // 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; }