Repository: cofacts/rumors-api
Pull request #128 (2019-05-25 07:48:36)

add fbclid filter funcation
Added fbclid filter, remove fbclid in query parameter.


Patch:
diff --git a/.travis.yml b/.travis.yml
index d7a9424..b1a13a9 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -5,7 +5,7 @@ services:
 
 language: node_js
 
-node_js: '8'
+node_js: '10'
 
 before_install:
   # Startup test db
diff --git a/src/graphql/models/Hyperlink.js b/src/graphql/models/Hyperlink.js
index a7e923a..817858d 100644
--- a/src/graphql/models/Hyperlink.js
+++ b/src/graphql/models/Hyperlink.js
@@ -1,9 +1,19 @@
 import { GraphQLObjectType, GraphQLString } from 'graphql';
 
+/**
+ *
+ * @param {string} field
+ * @returns {resolveFn} Resolves URL entry using url or normalizedUrl
+ */
 function resolveUrl(field) {
-  return async function({ url }, args, { loaders }) {
-    const urlEntry = await loaders.urlLoader.load(url);
-    return urlEntry[field];
+  return async function({ url, normalizedUrl }, args, { loaders }) {
+    const urls = [url];
+    if (normalizedUrl) {
+      urls.push(normalizedUrl);
+    }
+    const urlEnties = await loaders.urlLoader.loadMany(urls);
+    const firstEntry = urlEnties.find(urlEntry => urlEntry) || {};
+    return firstEntry[field];
   };
 }
 
diff --git a/src/graphql/mutations/CreateArticle.js b/src/graphql/mutations/CreateArticle.js
index 55c7582..bb24825 100644
--- a/src/graphql/mutations/CreateArticle.js
+++ b/src/graphql/mutations/CreateArticle.js
@@ -104,11 +104,14 @@ export function updateArticleHyperlinks(articleId, scrapResults) {
     id: articleId,
     body: {
       doc: {
-        hyperlinks: scrapResults.map(({ url, title, summary }) => ({
-          url,
-          title,
-          summary,
-        })),
+        hyperlinks: scrapResults.map(
+          ({ url, normalizedUrl, title, summary }) => ({
+            url,
+            normalizedUrl,
+            title,
+            summary,
+          })
+        ),
       },
     },
   });
@@ -130,7 +133,6 @@ export default {
   },
   resolve(rootValue, { text, reference, reason }, { appId, userId, loaders }) {
     assertUser({ appId, userId });
-
     const newArticlePromise = createNewArticle({
       text,
       reference,
@@ -145,9 +147,11 @@ export default {
 
     // Dependencies
     //
-    // newArticlePromise --> replyRequestPromise -.
-    //                  \                          >-> done
-    // scrapPromise -----`-> hyperlinkPromise ----'
+    // newArticlePromise* --> replyRequestPromise* -.
+    //                  \                            \ (ensure no parallel update to article)
+    // scrapPromise -----`----------------------------`--> hyperlinkPromise* --> done
+    //
+    // *: Updates article. Will trigger version_conflict_engine_exception if run in parallel.
     //
 
     const replyRequestPromise = newArticlePromise.then(articleId =>
@@ -157,6 +161,7 @@ export default {
     const hyperlinkPromise = Promise.all([
       newArticlePromise,
       scrapPromise,
+      replyRequestPromise, // Not used, just to avoid parallel update to article
     ]).then(([articleId, scrapResults]) => {
       return updateArticleHyperlinks(articleId, scrapResults);
     });
diff --git a/src/graphql/mutations/CreateReply.js b/src/graphql/mutations/CreateReply.js
index eeccb42..202a8f5 100644
--- a/src/graphql/mutations/CreateReply.js
+++ b/src/graphql/mutations/CreateReply.js
@@ -23,11 +23,14 @@ export function updateReplyHyperlinks(replyId, scrapResults) {
     id: replyId,
     body: {
       doc: {
-        hyperlinks: scrapResults.map(({ url, title, summary }) => ({
-          url,
-          title,
-          summary,
-        })),
+        hyperlinks: scrapResults.map(
+          ({ url, normalizedUrl, title, summary }) => ({
+            url,
+            normalizedUrl,
+            title,
+            summary,
+          })
+        ),
       },
     },
   });
diff --git a/src/graphql/mutations/__fixtures__/CreateReply.js b/src/graphql/mutations/__fixtures__/CreateReply.js
index b8c3ea2..f5b993a 100644
--- a/src/graphql/mutations/__fixtures__/CreateReply.js
+++ b/src/graphql/mutations/__fixtures__/CreateReply.js
@@ -5,9 +5,9 @@ export default {
     references: [{ type: 'LINE' }],
   },
   '/urls/doc/hyperlink1': {
-    canonical: 'http://google.com',
+    canonical: 'http://google.com/',
     title: 'Google google',
     summary: 'Gooooooogle',
-    url: 'http://google.com',
+    url: 'http://google.com/',
   },
 };
diff --git a/src/graphql/queries/__fixtures__/ListArticles.js b/src/graphql/queries/__fixtures__/ListArticles.js
index 51fa3f2..661bbbe 100644
--- a/src/graphql/queries/__fixtures__/ListArticles.js
+++ b/src/graphql/queries/__fixtures__/ListArticles.js
@@ -39,6 +39,7 @@ export default {
     hyperlinks: [
       {
         url: 'http://gohome.com',
+        normalizedUrl: 'http://gohome.com/',
         title: '馮諼很餓',
         summary:
           '居有頃，倚柱彈其劍，歌曰：「長鋏歸來乎！食無魚。」左右以告。孟嘗君曰：「食之，比門下之客。」',
@@ -46,14 +47,14 @@ export default {
     ],
   },
   '/urls/doc/gohome': {
-    url: 'http://gohome.com',
+    url: 'http://gohome.com/',
     title: '馮諼很餓',
     summary:
       '居有頃，倚柱彈其劍，歌曰：「長鋏歸來乎！食無魚。」左右以告。孟嘗君曰：「食之，比門下之客。」',
     topImageUrl: 'http://gohome.com/image.jpg',
   },
   '/urls/doc/biau': {
-    url: 'http://出師表.com',
+    url: 'http://出師表.com/',
     title: '出師表',
     summary:
       '臣亮言：先帝創業未半，而中道崩殂。今天下三分，益州 疲弊，此誠危急存亡之秋也。',
diff --git a/src/graphql/queries/__fixtures__/ListReplies.js b/src/graphql/queries/__fixtures__/ListReplies.js
index 6b4fa85..f9ccf55 100644
--- a/src/graphql/queries/__fixtures__/ListReplies.js
+++ b/src/graphql/queries/__fixtures__/ListReplies.js
@@ -31,6 +31,7 @@ export default {
     hyperlinks: [
       {
         url: 'http://gohome.com',
+        normalizedUrl: 'http://gohome.com/',
         title: '馮諼很餓',
         summary:
           '居有頃，倚柱彈其劍，歌曰：「長鋏歸來乎！食無魚。」左右以告。孟嘗君曰：「食之，比門下之客。」',
@@ -40,14 +41,14 @@ export default {
     createdAt: 1,
   },
   '/urls/doc/gohome': {
-    url: 'http://gohome.com',
+    url: 'http://gohome.com/',
     title: '馮諼很餓',
     summary:
       '居有頃，倚柱彈其劍，歌曰：「長鋏歸來乎！食無魚。」左右以告。孟嘗君曰：「食之，比門下之客。」',
     topImageUrl: 'http://gohome.com/image.jpg',
   },
   '/urls/doc/foobar': {
-    url: 'http://foo.com',
+    url: 'http://foo.com/',
     title: 'bar',
     summary: 'bar',
     topImageUrl: 'http://foo.com/image.jpg',
diff --git a/src/util/scrapUrls.js b/src/util/scrapUrls.js
index aafff39..b3beaff 100644
--- a/src/util/scrapUrls.js
+++ b/src/util/scrapUrls.js
@@ -1,8 +1,16 @@
 import rollbar from 'rollbarInstance';
 import urlRegex from 'url-regex';
 import DataLoader from 'dataloader';
+import url from 'url';
 
 import gql from './gql';
+/**
+ * ScrapResult:
+ * Refer to https://github.com/cofacts/url-resolver/blob/master/src/typeDefs/UrlResolveResult.graphql
+ * with new fields:
+ * @param {string} normalizedUrl - URL normalized in scrapUrl process
+ * @param {boolean} fromUrlsCache - If the entry is from cacheLoader
+ */
 
 /**
  * Extracts urls from a string.
@@ -24,8 +32,12 @@ async function scrapUrls(
   text,
   { cacheLoader, client, noFetch = false, scrapLimit = 5 } = {}
 ) {
-  const urls = text.match(urlRegex()) || [];
-  if (urls.length === 0) return [];
+  const originalUrls = text.match(urlRegex()) || [];
+  if (originalUrls.length === 0) return [];
+
+  // Normalize URLs before sending to cache or scrapper to increase cache hit
+  //
+  const normalizedUrls = removeFBCLIDIfExist(originalUrls);
 
   const scrapLoader = new DataLoader(urls =>
     gql`
@@ -44,28 +56,32 @@ async function scrapUrls(
     `({ urls }).then(({ data }) => data.resolvedUrls)
   );
 
+  // result: list of ScrapResult, with its `url` being the url in text,
+  // but canonical url may be normalized
+  //
   const result = await Promise.all(
     // 1st pass: resolve cache
-    urls.map(async url => {
+    normalizedUrls.map(async (normalizedUrl, i) => {
       if (cacheLoader) {
-        const result = await cacheLoader.load(url);
+        const result = await cacheLoader.load(normalizedUrl);
 
         if (result)
           return {
             ...result,
-            url, // Overwrite the URL from cache with the actual url in text, because cacheLoader may match canonical URLs
+            url: originalUrls[i],
+            normalizedUrl,
             fromUrlsCache: true,
           };
       }
 
-      return url;
+      return normalizedUrl;
     })
   ).then(results => {
     // 2nd pass: scrap when needed
     let scrappingCount = 0;
 
     return Promise.all(
-      results.map(async result => {
+      results.map(async (result, i) => {
         if (typeof result !== 'string') {
           return result;
         }
@@ -74,7 +90,11 @@ async function scrapUrls(
 
         if (noFetch || scrappingCount >= scrapLimit) return null;
         scrappingCount += 1;
-        return scrapLoader.load(result);
+        return scrapLoader.load(result).then(scrapped => ({
+          ...scrapped,
+          url: originalUrls[i],
+          normalizedUrl: scrapped.url,
+        }));
       })
     );
   });
@@ -135,4 +155,25 @@ async function scrapUrls(
   return result;
 }
 
+/**
+ * Remove fbclid if it exist, handling the url comes from FB
+ *
+ * @param {string[]} inputTexts - raw urls
+ * @return {string[]} - urls without fbclid query parameter
+ */
+export function removeFBCLIDIfExist(inputTexts) {
+  return inputTexts.map(text => {
+    try {
+      const myURL = new URL(text);
+      myURL.searchParams.delete('fbclid');
+      return url.format(myURL, { unicode: true }); // keep unicode URLs as-is
+    } catch (e) {
+      console.error('[scrapUrls][removeFBCLIDIfExist]', e); // eslint-disable-line no-console
+
+      // URL patterns not recognized by URL constructor, just skip
+      return text;
+    }
+  });
+}
+
 export default scrapUrls;


Test patch:
diff --git a/src/graphql/mutations/__tests__/__snapshots__/CreateArticle.js.snap b/src/graphql/mutations/__tests__/__snapshots__/CreateArticle.js.snap
index 9f218b0..6b7c8a9 100644
--- a/src/graphql/mutations/__tests__/__snapshots__/CreateArticle.js.snap
+++ b/src/graphql/mutations/__tests__/__snapshots__/CreateArticle.js.snap
@@ -41,6 +41,7 @@ Object {
   "createdAt": "2017-01-28T08:45:57.011Z",
   "hyperlinks": Array [
     Object {
+      "normalizedUrl": "http://foo.com/article/1",
       "summary": "FOO article content",
       "title": "FOO title",
       "url": "http://foo.com/article/1",
diff --git a/src/graphql/mutations/__tests__/__snapshots__/CreateReply.js.snap b/src/graphql/mutations/__tests__/__snapshots__/CreateReply.js.snap
index 790fd7a..fbcf9dc 100644
--- a/src/graphql/mutations/__tests__/__snapshots__/CreateReply.js.snap
+++ b/src/graphql/mutations/__tests__/__snapshots__/CreateReply.js.snap
@@ -6,6 +6,7 @@ Object {
   "createdAt": "2017-01-28T08:45:57.011Z",
   "hyperlinks": Array [
     Object {
+      "normalizedUrl": "http://google.com/",
       "summary": "Gooooooogle",
       "title": "Google google",
       "url": "http://google.com",
diff --git a/src/util/__tests__/__snapshots__/scrapUrls.js.snap b/src/util/__tests__/__snapshots__/scrapUrls.js.snap
index b436623..8d62de2 100644
--- a/src/util/__tests__/__snapshots__/scrapUrls.js.snap
+++ b/src/util/__tests__/__snapshots__/scrapUrls.js.snap
@@ -11,6 +11,7 @@ Array [
     "fetchedAt": "2017-02-01T00:00:00.000Z",
     "fromUrlsCache": true,
     "id": "someUrl-2nd-fetch",
+    "normalizedUrl": "http://example.com/article/1111-aaaaa-bbb-ccc",
     "summary": "Changed summary",
     "title": "Changed title",
     "url": "http://example.com/article/1111-aaaaa-bbb-ccc",
@@ -24,6 +25,7 @@ Array [
     "fetchedAt": "2017-02-01T00:00:00.000Z",
     "fromUrlsCache": true,
     "id": "someUrl-2nd-fetch",
+    "normalizedUrl": "http://example.com/article/1111",
     "summary": "Changed summary",
     "title": "Changed title",
     "url": "http://example.com/article/1111",
@@ -87,6 +89,7 @@ exports[`scrapping & storage scraps from Internet and handles error: foundResult
 Object {
   "canonical": "http://example.com/index.html",
   "html": "<html><head></head><body>Hello world</body></html>",
+  "normalizedUrl": "http://example.com/index.html",
   "status": 200,
   "summary": "Some text as summary",
   "title": "Some title",
@@ -99,6 +102,7 @@ exports[`scrapping & storage scraps from Internet and handles error: notFoundRes
 Object {
   "canonical": "http://example.com/not-found",
   "html": "<html><head></head><body>Not Found</body></html>",
+  "normalizedUrl": "http://example.com/not-found",
   "status": 404,
   "summary": "Not Found",
   "title": "",
diff --git a/src/util/__tests__/scrapUrls.js b/src/util/__tests__/scrapUrls.js
index 5497cc5..1340adf 100644
--- a/src/util/__tests__/scrapUrls.js
+++ b/src/util/__tests__/scrapUrls.js
@@ -4,7 +4,7 @@ import MockDate from 'mockdate';
 
 import { loadFixtures, unloadFixtures } from 'util/fixtures';
 import fixtures from '../__fixtures__/scrapUrls';
-import scrapUrls from '../scrapUrls';
+import scrapUrls, { removeFBCLIDIfExist } from '../scrapUrls';
 import DataLoaders from 'graphql/dataLoaders';
 import client from 'util/client';
 import gql from '../gql';
@@ -112,3 +112,21 @@ describe('caching', () => {
     ).toMatchSnapshot();
   });
 });
+
+describe('remove fbclid', () => {
+  it('removeFBCLID', async () => {
+    const url =
+      'https://www.youtube.com/playlist?list=PLdwQWxpS513DrHuUQ356zK6pLoJ8NcVP2&fbclid=IwAR2Yiso_SArcTCmCEKKnUuIpTS-y_fyPbl_X19OG0eaAhVxdJxGh8AMicwE';
+
+    expect(removeFBCLIDIfExist([url])).toEqual([
+      'https://www.youtube.com/playlist?list=PLdwQWxpS513DrHuUQ356zK6pLoJ8NcVP2',
+    ]);
+  });
+  it('keep origin url if fbclid is not existed', async () => {
+    const url =
+      'https://www.youtube.com/playlist?list=PLdwQWxpS513DrHuUQ356zK6pLoJ8NcVP2';
+    expect(removeFBCLIDIfExist([url])).toEqual([
+      'https://www.youtube.com/playlist?list=PLdwQWxpS513DrHuUQ356zK6pLoJ8NcVP2',
+    ]);
+  });
+});