Rewrite and simplify view-filter service.

Rewrite the view filter service to make it easier to verify and fix a
bug in the handling of "any" queries:

The input filter object representing the parsed search query is
translated into a tree of filter primitives, where each primitive is
either a TermFilter that tests whether an annotation matches a single
(field, term) or a BinaryOpFilter that combines other filters using AND
or OR operators. The list of annotations are then matched against the
tree's root filter.

This system allows a nicer representation of "any" field queries by
expanding a query such as "foo bar" into:

  (quote:foo OR text:foo OR ...) AND (quote:bar OR text:bar OR ...)

In the process a mistake was uncovered in a test case for the "any"
filter which incorrectly allowed the previous broken implementation to
pass.

src/shared/polyfills.js

 'use strict';
 
-// ES2015 polyfills
+// ES2015
 require('core-js/es6/promise');
 require('core-js/es6/map');
 require('core-js/es6/set');
@@ -14,6 +14,7 @@ require('core-js/fn/string/ends-with');
 require('core-js/fn/string/starts-with');
 
 // ES2017
+require('core-js/fn/object/entries');
 require('core-js/fn/object/values');
 
 // URL constructor, required by IE 10/11,

src/sidebar/test/view-filter-test.coffee

@@ -49,7 +49,7 @@ describe 'viewFilter', ->
           operator: 'and'
 
       viewFilter.filter [], filters
-      assert.calledWith fakeUnicode.fold, 'tiger'
+      assert.calledWith fakeUnicode.fold, 'Tiger'
 
     describe 'filter operators', ->
       annotations = null
@@ -165,9 +165,9 @@ describe 'viewFilter', ->
               "type": "TextQuoteSelector",
               "exact": "The Tiger by William Blake",
             }]
+          ]
           user: "acct:poe@edgar.com"
           tags: ["poem", "Blake", "Tiger"]
-          ]
 
         filters =
           any:

src/sidebar/view-filter.js

@@ -2,89 +2,97 @@
 
 // @ngInject
 function viewFilter(unicode) {
-  function _normalize(e) {
-    if (typeof e === 'string') {
-      var normed = unicode.normalize(e);
-      return unicode.fold(normed);
-    } else {
-      return e;
+  /**
+   * Normalize a field value or query term for comparison.
+   */
+  function normalize(val) {
+    if (typeof val !== 'string') {
+      return val;
     }
+    return unicode.fold(unicode.normalize(val)).toLowerCase();
   }
 
-  function _matches(filter, value, match) {
-    var matches = true;
+  /**
+   * Filter that matches annotations against a single field & term.
+   *
+   * eg. "quote:foo" or "text:bar"
+   */
+  class TermFilter {
+    /**
+     * @param {string} field - Name of field to match
+     * @param {string} term - Query term
+     * @param {Checker} checker - Functions for extracting term values from
+     *   an annotation and checking whether they match a query term.
+     */
+    constructor(field, term, checker) {
+      this.field = field;
+      this.term = term;
+      this.checker = checker;
+    }
 
-    for (var term of filter.terms) {
-      if (!match(term, value)) {
-        matches = false;
-        if (filter.operator === 'and') {
-          break;
+    matches(ann) {
+      var checker = this.checker;
+      if (checker.autofalse && checker.autofalse(ann)) {
+        return false;
       }
+
+      var value = checker.value(ann);
+      if (Array.isArray(value)) {
+        value = value.map(normalize);
       } else {
-        matches = true;
-        if (filter.operator === 'or') {
-          break;
+        value = normalize(value);
       }
+      return checker.match(this.term, value);
     }
   }
-    return matches;
-  }
-
-  function _arrayMatches(filter, value, match) {
-    var matches = true;
-    // Make copy for filtering
-    var copy = filter.terms.slice();
-
-    copy = copy.filter(e => match(value, e));
-
-    if (((filter.operator === 'and') && (copy.length < filter.terms.length)) ||
-        ((filter.operator === 'or') && !copy.length)) {
-      matches = false;
-    }
-    return matches;
-  }
 
-  function _checkMatch(filter, annotation, checker) {
-    var autofalsefn = checker.autofalse;
-    if (autofalsefn && autofalsefn(annotation)) {
-      return false;
+  /**
+   * Filter that combines other filters using AND or OR combinators.
+   */
+  class BinaryOpFilter {
+    /**
+     * @param {'and'|'or'} op - Binary operator
+     * @param {Filter[]} - Array of filters to test against
+     */
+    constructor(op, filters) {
+      this.operator = op;
+      this.filters = filters;
     }
 
-    var value = checker.value(annotation);
-    if (Array.isArray(value)) {
-      value = value.map(e => e.toLowerCase());
-      value = value.map(e => _normalize(e));
-      return _arrayMatches(filter, value, checker.match);
+    matches(ann) {
+      if (this.operator === 'and') {
+        return this.filters.every(filter => filter.matches(ann));
       } else {
-      value = value.toLowerCase();
-      value = _normalize(value);
-      return _matches(filter, value, checker.match);
+        return this.filters.some(filter => filter.matches(ann));
+      }
     }
   }
 
-  // The field configuration
-  //
-  // [facet_name]:
-  //   autofalse: a function for a preliminary false match result
-  //   value: a function to extract to facet value for the annotation.
-  //   match: a function to check if the extracted value matches the facet value
+  /**
+   * Functions for extracting field values from annotations and testing whether
+   * they match a query term.
+   *
+   * [facet_name]:
+   *   autofalse: a function for a preliminary false match result
+   *   value: a function to extract to facet value for the annotation.
+   *   match: a function to check if the extracted value matches the facet value
+   */
   this.fields = {
     quote: {
-      autofalse: ann => !Array.isArray(ann.references),
+      autofalse: ann => (ann.references || []).length > 0,
       value(annotation) {
-        var quotes = (annotation.target || []).map((t) =>
-          (() => {
-            var result = [];
-            for (var s of (t.selector || [])) {
-              if (s.type === 'TextQuoteSelector') {
-                if (!s.exact) { continue; }
-                result.push(s.exact);
-              }
-            }
-            return result;
-          })());
-        quotes = Array.prototype.concat(...quotes);
-        return quotes.join('\n');
+        if (!annotation.target) {
+          // FIXME: All annotations *must* have a target, so this check should
+          // not be required.
+          return '';
+        }
+        var target = annotation.target[0];
+        var selectors = target.selector || [];
+
+        return selectors
+          .filter(s => s.type === 'TextQuoteSelector')
+          .map(s => s.exact)
+          .join('\n');
       },
       match: (term, value) => value.indexOf(term) > -1,
     },
@@ -99,7 +107,7 @@ function viewFilter(unicode) {
     tag: {
       autofalse: ann => !Array.isArray(ann.tags),
       value: ann => ann.tags,
-      match: (term, value) => term.includes(value),
+      match: (term, value) => value.includes(term),
     },
     text: {
       autofalse: ann => typeof ann.text !== 'string',
@@ -116,9 +124,6 @@ function viewFilter(unicode) {
       value: ann => ann.user,
       match: (term, value) => value.indexOf(term) > -1,
     },
-    any: {
-      fields: ['quote', 'text', 'tag', 'user'],
-    },
   };
 
   /**
@@ -130,63 +135,33 @@ function viewFilter(unicode) {
    * @return {string[]} IDs of matching annotations.
    */
   this.filter = (annotations, filters) => {
-    var filter;
-    var limit = Math.min(...((filters.result ? filters.result.terms : undefined) || []) || []);
-    var count = 0;
-
-    // Normalizing the filters, need to do only once.
-    for (var f in filters) {
-      if (!filters.hasOwnProperty(f)) {
-        continue;
+    var limit = annotations.length;
+    if (filters.result) {
+      limit = Math.min(...filters.result.terms.map(parseInt));
+    }
+
+    // Convert the input filter object into a filter tree, expanding "any"
+    // filters.
+    var fieldFilters = Object.entries(filters).map(([field, filter]) => {
+      var terms = filter.terms.map(normalize);
+      var termFilters;
+      if (field === 'any') {
+        var anyFields = ['quote', 'text', 'tag', 'user'];
+        termFilters = terms.map(term => new BinaryOpFilter('or', anyFields.map(field =>
+          new TermFilter(field, term, this.fields[field])
+        )));
+      } else {
+        termFilters = terms.map(term => new TermFilter(field, term, this.fields[field]));
       }
-
-      filter = filters[f];
-      if (filter.terms) {
-        filter.terms = filter.terms.map(e => {
-          e = e.toLowerCase();
-          e = _normalize(e);
-          return e;
+      return new BinaryOpFilter(filter.operator, termFilters);
     });
-      }
-    }
-
-    var result = [];
-    for (var annotation of annotations) {
-      if (count >= limit) { break; }
 
-      var match = true;
-      for (var category in filters) {
-        if (!filters.hasOwnProperty(category)) {
-          continue;
-        }
-        filter = filters[category];
-        if (!match) { break; }
-        if (!filter.terms.length) { continue; }
+    var rootFilter = new BinaryOpFilter('and', fieldFilters);
 
-        switch (category) {
-        case 'any':
-          var categoryMatch = false;
-          for (var field of this.fields.any.fields) {
-            for (var term of filter.terms) {
-              var termFilter = {terms: [term], operator: 'and'};
-              if (_checkMatch(termFilter, annotation, this.fields[field])) {
-                categoryMatch = true;
-                break;
-              }
-            }
-          }
-          match = categoryMatch;
-          break;
-        default:
-          match = _checkMatch(filter, annotation, this.fields[category]);
-        }
-      }
-
-      if (!match) { continue; }
-      count++;
-      result.push(annotation.id);
-    }
-    return result;
+    return annotations
+      .filter(ann => rootFilter.matches(ann))
+      .slice(0, limit)
+      .map(ann => ann.id);
   };
 }