From 9ad860babdc95a49fbbbd1a50f3e16e3c7802f6e Mon Sep 17 00:00:00 2001 From: Pete Richards Date: Fri, 16 Oct 2015 12:34:47 -0700 Subject: [PATCH 01/18] [Search] Rewrite search controller, tidy Rewrite the search controller, making numerous changes and using prototypical style. First, the search controller immediately hides previous results when a new search is started. Secondly, the search controller ensures that search results displayed match the currently entered query, preventing race conditions. Finally, the search controller uses a poor filtering option that means it may not display all results. --- platform/search/res/templates/search.html | 38 +-- .../src/controllers/SearchController.js | 283 +++++++++--------- 2 files changed, 162 insertions(+), 159 deletions(-) diff --git a/platform/search/res/templates/search.html b/platform/search/res/templates/search.html index e65ab072af..225df353b0 100644 --- a/platform/search/res/templates/search.html +++ b/platform/search/res/templates/search.html @@ -21,21 +21,16 @@ -->
- +
- + - @@ -43,20 +38,20 @@ ng-class="{content: !(ngModel.input === '' || ngModel.input === undefined)}"> M
- + - + v - +
- +
- + Filtered by: {{ ngModel.filtersString }} - - +
- +
- +
Loading...
- +
+ ng-click="controller.loadMore()"> More Results
- +
- - \ No newline at end of file + + diff --git a/platform/search/src/controllers/SearchController.js b/platform/search/src/controllers/SearchController.js index 10cf056b4f..88c9663f51 100644 --- a/platform/search/src/controllers/SearchController.js +++ b/platform/search/src/controllers/SearchController.js @@ -26,146 +26,157 @@ */ define(function () { "use strict"; - + var INITIAL_LOAD_NUMBER = 20, LOAD_INCREMENT = 20; - + + /** + * Controller for search in Tree View. + * + * Filtering is currently buggy; it filters after receiving results from + * search providers, the downside of this is that it requires search + * providers to provide objects for all possible results, which is + * potentially a hit to persistence, thus can be very very slow. + * + * Ideally, filtering should be handled before loading objects from the persistence + * store, the downside to this is that filters must be applied to object + * models, not object instances. + * + * @constructor + * @param $scope + * @param searchService + */ function SearchController($scope, searchService) { - // numResults is the amount of results to display. Will get increased. - // fullResults holds the most recent complete searchService response object - var numResults = INITIAL_LOAD_NUMBER, - fullResults = {hits: []}; - - // Scope variables are: - // Variables used only in SearchController: - // results, an array of searchResult objects - // loading, whether search() is loading - // ngModel.input, the text of the search query - // ngModel.search, a boolean of whether to display search or the tree - // Variables used also in SearchMenuController: - // ngModel.filter, the function filter defined below - // ngModel.types, an array of type objects - // ngModel.checked, a dictionary of which type filter options are checked - // ngModel.checkAll, a boolean of whether to search all types - // ngModel.filtersString, a string list of what filters on the results are active - $scope.results = []; - $scope.loading = false; - - - // Filters searchResult objects by type. Allows types that are - // checked. (ngModel.checked['typekey'] === true) - // If hits is not provided, will use fullResults.hits - function filter(hits) { - var newResults = [], - i = 0; - - if (!hits) { - hits = fullResults.hits; - } - - // If checkAll is checked, search everything no matter what the other - // checkboxes' statuses are. Otherwise filter the search by types. - if ($scope.ngModel.checkAll) { - newResults = fullResults.hits.slice(0, numResults); - } else { - while (newResults.length < numResults && i < hits.length) { - // If this is of an acceptable type, add it to the list - if ($scope.ngModel.checked[hits[i].object.getModel().type]) { - newResults.push(fullResults.hits[i]); - } - i += 1; - } - } - - $scope.results = newResults; - return newResults; - } - - // Make function accessible from SearchMenuController - $scope.ngModel.filter = filter; - - // For documentation, see search below - function search(maxResults) { - var inputText = $scope.ngModel.input; - - if (inputText !== '' && inputText !== undefined) { - // We are starting to load. - $scope.loading = true; - - // Update whether the file tree should be displayed - // Hide tree only when starting search - $scope.ngModel.search = true; - } - - if (!maxResults) { - // Reset 'load more' - numResults = INITIAL_LOAD_NUMBER; - } - - // Send the query - searchService.query(inputText, maxResults).then(function (result) { - // Store all the results before splicing off the front, so that - // we can load more to display later. - fullResults = result; - $scope.results = filter(result.hits); - - // Update whether the file tree should be displayed - // Reveal tree only when finishing search - if (inputText === '' || inputText === undefined) { - $scope.ngModel.search = false; - } - - // Now we are done loading. - $scope.loading = false; - }); - } - - return { - /** - * Search the filetree. Assumes that any search text will - * be in ngModel.input - * - * @param maxResults (optional) The maximum number of results - * that this function should return. If not provided, search - * service default will be used. - */ - search: search, - - /** - * Checks to see if there are more search results to display. If the answer is - * unclear, this function will err toward saying that there are more results. - */ - areMore: function () { - var i; - - // Check to see if any of the not displayed results are of an allowed type - for (i = numResults; i < fullResults.hits.length; i += 1) { - if ($scope.ngModel.checkAll || $scope.ngModel.checked[fullResults.hits[i].object.getModel().type]) { - return true; - } - } - - // If none of the ones at hand are correct, there still may be more if we - // re-search with a larger maxResults - return fullResults.hits.length < fullResults.total; - }, - - /** - * Increases the number of search results to display, and then - * loads them, adding to the displayed results. - */ - loadMore: function () { - numResults += LOAD_INCREMENT; - - if (numResults > fullResults.hits.length && fullResults.hits.length < fullResults.total) { - // Resend the query if we are out of items to display, but there are more to get - search(numResults); - } else { - // Otherwise just take from what we already have - $scope.results = filter(fullResults.hits); - } - } + var controller = this; + this.$scope = $scope; + this.searchService = searchService; + this.numberToDisplay = INITIAL_LOAD_NUMBER; + this.fullResults = []; + this.filteredResults = []; + this.$scope.results = []; + this.$scope.loading = false; + this.pendingQuery = undefined; + this.$scope.ngModel.filter = function () { + return controller.onFilterChange.apply(controller, arguments); }; } + + /** + * Returns true if there are more results than currently displayed for the + * for the current query and filters. + */ + SearchController.prototype.areMore = function () { + return this.$scope.results.length < this.filteredResults.length; + }; + + /** + * Display more results for the currently displayed query and filters. + */ + SearchController.prototype.loadMore = function () { + this.numberToDisplay += LOAD_INCREMENT; + this.updateResults(); + }; + + /** + * Search for the query string specified in scope. + */ + SearchController.prototype.search = function () { + var inputText = this.$scope.ngModel.input, + controller = this; + + this.clearResults(); + + if (inputText) { + this.$scope.loading = true; + this.$scope.ngModel.search = true; + } else { + this.pendingQuery = undefined; + this.$scope.ngModel.search = false; + this.$scope.loading = false; + return; + } + + if (this.pendingQuery === inputText) { + return; // don't issue multiple queries for the same term. + } + + this.pendingQuery = inputText; + + this + .searchService + .query(inputText, 60) // TODO: allow filter in search service. + .then(function (results) { + if (controller.pendingQuery !== inputText) { + return; // another query in progress, so skip this one. + } + controller.onSearchComplete(results); + }); + }; + + /** + * Refilter results and update visible results when filters have changed. + */ + SearchController.prototype.onFilterChange = function () { + this.filter(); + this.updateVisibleResults(); + }; + + /** + * Filter `fullResults` based on currenly active filters, storing the result + * in `filteredResults`. + * + * @private + */ + SearchController.prototype.filter = function () { + var includeTypes = this.$scope.ngModel.checked; + + if (this.$scope.ngModel.checkAll) { + this.filteredResults = this.fullResults; + return; + } + + this.filteredResults = this.fullResults.filter(function (hit) { + return includeTypes[hit.object.getModel().type]; + }); + }; + + + /** + * Clear the search results. + * + * @private + */ + SearchController.prototype.clearResults = function () { + this.$scope.results = []; + this.fullResults = []; + this.filteredResults = []; + this.numberToDisplay = INITIAL_LOAD_NUMBER; + }; + + + + /** + * Update search results from given `results`. + * + * @private + */ + SearchController.prototype.onSearchComplete = function (results) { + this.fullResults = results.hits; + this.filter(); + this.updateVisibleResults(); + this.$scope.loading = false; + this.pendingQuery = undefined; + }; + + /** + * Update visible results from filtered results. + * + * @private + */ + SearchController.prototype.updateVisibleResults = function () { + this.$scope.results = + this.filteredResults.slice(0, this.numberToDisplay); + }; + return SearchController; }); From b5505f372f6a737a2a00a1f987aee97ee5facecf Mon Sep 17 00:00:00 2001 From: Pete Richards Date: Fri, 16 Oct 2015 12:39:41 -0700 Subject: [PATCH 02/18] [Search] Generic Worker Performance Tweaks The generic search worker now does indexing work during the index operation, ensuring that queries do not have to do extraneous or repeat calculations. Change the return format slightly and fixed a bug in the GenericSearchProvider which caused more objects than intended to be returned from the provider. --- .../src/services/GenericSearchProvider.js | 39 ++-- .../src/services/GenericSearchWorker.js | 205 +++++++++--------- 2 files changed, 123 insertions(+), 121 deletions(-) diff --git a/platform/search/src/services/GenericSearchProvider.js b/platform/search/src/services/GenericSearchProvider.js index 9574d156fb..d38326bb9d 100644 --- a/platform/search/src/services/GenericSearchProvider.js +++ b/platform/search/src/services/GenericSearchProvider.js @@ -96,36 +96,33 @@ define( // Handles responses from the web worker. Namely, the results of // a search request. function handleResponse(event) { - var ids = [], - id; + if (event.data.request !== 'search') { + return; // no idea how to handle anything else. + } - // If we have the results from a search - if (event.data.request === 'search') { - // Convert the ids given from the web worker into domain objects - for (id in event.data.results) { - ids.push(id); - } - objectService.getObjects(ids).then(function (objects) { - var searchResults = [], - id; + var workerResults = event.data.results, + ids = Object.keys(workerResults); - // Create searchResult objects - for (id in objects) { - searchResults.push({ - object: objects[id], - id: id, - score: event.data.results[id] + objectService + .getObjects(ids) + .then(function (objects) { + var searchResults = Object + .keys(objects) + .map(function (id) { + return { + object: objects[id], + id: id, + score: workerResults[id].matchCount + }; }); - } // Resove the promise corresponding to this pendingQueries[event.data.timestamp].resolve({ hits: searchResults, - total: event.data.total, + total: searchResults.length, timedOut: event.data.timedOut }); }); - } } function requestAndIndex(id) { @@ -212,7 +209,7 @@ define( var message = { request: 'search', input: searchInput, - maxNumber: maxResults, + maxResults: maxResults, timestamp: timestamp, timeout: timeout }; diff --git a/platform/search/src/services/GenericSearchWorker.js b/platform/search/src/services/GenericSearchWorker.js index 57be98b423..cd45929d4f 100644 --- a/platform/search/src/services/GenericSearchWorker.js +++ b/platform/search/src/services/GenericSearchWorker.js @@ -26,78 +26,55 @@ */ (function () { "use strict"; - + // An array of objects composed of domain object IDs and models // {id: domainObject's ID, model: domainObject's model} - var indexedItems = []; - - // Helper function for serach() - function convertToTerms(input) { - var terms = input; - // Shave any spaces off of the ends of the input - while (terms.substr(0, 1) === ' ') { - terms = terms.substring(1, terms.length); - } - while (terms.substr(terms.length - 1, 1) === ' ') { - terms = terms.substring(0, terms.length - 1); - } - - // Then split it at spaces and asterisks - terms = terms.split(/ |\*/); - - // Remove any empty strings from the terms - while (terms.indexOf('') !== -1) { - terms.splice(terms.indexOf(''), 1); - } - - return terms; + var indexedItems = [], + TERM_SPLITTER = /[ _\*]/; + + function indexItem(id, model) { + var vector = { + name: model.name + }; + vector.cleanName = model.name.trim(); + vector.lowerCaseName = vector.cleanName.toLocaleLowerCase(); + vector.terms = vector.lowerCaseName.split(TERM_SPLITTER); + + indexedItems.push({ + id: id, + vector: vector, + model: model + }); } - + // Helper function for search() - function scoreItem(item, input, terms) { - var name = item.model.name.toLocaleLowerCase(), - weight = 0.65, - score = 0.0, - i; - - // Make the score really big if the item name and - // the original search input are the same - if (name === input) { - score = 42; - } - - for (i = 0; i < terms.length; i += 1) { - // Increase the score if the term is in the item name - if (name.indexOf(terms[i]) !== -1) { - score += 1; - - // Add extra to the score if the search term exists - // as its own term within the items - if (name.split(' ').indexOf(terms[i]) !== -1) { - score += 0.5; - } - } - } - - return score * weight; + function convertToTerms(input) { + var query = { + exactInput: input + }; + query.inputClean = input.trim(); + query.inputLowerCase = query.inputClean.toLocaleLowerCase(); + query.terms = query.inputLowerCase.split(TERM_SPLITTER); + query.exactTerms = query.inputClean.split(TERM_SPLITTER); + return query; } - - /** + + /** * Gets search results from the indexedItems based on provided search * input. Returns matching results from indexedItems, as well as the * timestamp that was passed to it. - * + * * @param data An object which contains: * * input: The original string which we are searching with - * * maxNumber: The maximum number of search results desired + * * maxResults: The maximum number of search results desired * * timestamp: The time identifier from when the query was made */ function search(data) { - // This results dictionary will have domain object ID keys which - // point to the value the domain object's score. - var results = {}, - input = data.input.toLocaleLowerCase(), - terms = convertToTerms(input), + // This results dictionary will have domain object ID keys which + // point to the value the domain object's score. + var results, + input = data.input, + query = convertToTerms(input), message = { request: 'search', results: {}, @@ -105,54 +82,82 @@ timestamp: data.timestamp, timedOut: false }, - score, - i, - id; - - // If the user input is empty, we want to have no search results. - if (input !== '') { - for (i = 0; i < indexedItems.length; i += 1) { - // If this is taking too long, then stop - if (Date.now() > data.timestamp + data.timeout) { - message.timedOut = true; - break; - } - - // Score and add items - score = scoreItem(indexedItems[i], input, terms); - if (score > 0) { - results[indexedItems[i].id] = score; - message.total += 1; - } - } + matches = {}; + + if (!query.inputClean) { + // No search terms, no results; + return message; } - - // Truncate results if there are more than maxResults - if (message.total > data.maxResults) { - i = 0; - for (id in results) { - message.results[id] = results[id]; - i += 1; - if (i >= data.maxResults) { - break; + + // Two phases: find matches, then score matches. + // Idea being that match finding should be fast, so that future scoring + // operations process fewer objects. + + query.terms.forEach(function findMatchingItems(term) { + indexedItems + .filter(function matchesItem(item) { + return item.vector.lowerCaseName.indexOf(term) !== -1; + }) + .forEach(function trackMatch(matchedItem) { + if (!matches[matchedItem.id]) { + matches[matchedItem.id] = { + matchCount: 0, + item: matchedItem + }; + } + matches[matchedItem.id].matchCount += 1; + }); + }); + + // Then, score matching items. + results = Object + .keys(matches) + .map(function asMatches(matchId) { + return matches[matchId]; + }) + .map(function prioritizeExactMatches(match) { + if (match.item.vector.name === query.exactInput) { + match.matchCount += 100; + } else if (match.item.vector.lowerCaseName === + query.inputLowerCase) { + match.matchCount += 50; } - } - // TODO: This seems inefficient. - } else { - message.results = results; - } - + return match; + }) + .map(function prioritizeCompleteTermMatches(match) { + match.item.vector.terms.forEach(function (term) { + if (query.terms.indexOf(term) !== -1) { + match.matchCount += 0.5; + } + }); + return match; + }) + .sort(function compare(a, b) { + if (a.matchCount > b.matchCount) { + return -1; + } + if (a.matchCount < b.matchCount) { + return 1; + } + return 0; + }); + + message.total = results.length; + message.results = results + .slice(0, data.maxResults) + .reduce(function arrayToObject(obj, match) { + obj[match.item.id] = match; + return obj; + }, {}); + return message; } - + self.onmessage = function (event) { if (event.data.request === 'index') { - indexedItems.push({ - id: event.data.id, - model: event.data.model - }); + indexItem(event.data.id, event.data.model); } else if (event.data.request === 'search') { self.postMessage(search(event.data)); } }; -}()); \ No newline at end of file +}()); From 099591ad2eb427ecde5d3dbde11a5c0c52b974c7 Mon Sep 17 00:00:00 2001 From: Pete Richards Date: Fri, 16 Oct 2015 15:26:04 -0700 Subject: [PATCH 03/18] [Search] Aggregator returns objects, providers return models Search providers return search results as models for domain objects, as the actual number of max results is enforced by the aggregator, and because the individual providers store and return the models for their objects already. This lowers the amount of resources consumed instantiating domain objects, and also allows the individual search providers to implement function-based filtering on domain object models, which is beneficial as it allows the search filtering in the search controller to be done before paginating of results. --- platform/search/bundle.json | 2 +- .../search/src/services/SearchAggregator.js | 290 +++++++++++------- 2 files changed, 182 insertions(+), 110 deletions(-) diff --git a/platform/search/bundle.json b/platform/search/bundle.json index 67dc058ed9..1117c3d161 100644 --- a/platform/search/bundle.json +++ b/platform/search/bundle.json @@ -59,7 +59,7 @@ "provides": "searchService", "type": "aggregator", "implementation": "services/SearchAggregator.js", - "depends": [ "$q" ] + "depends": [ "$q", "objectService" ] } ], "workers": [ diff --git a/platform/search/src/services/SearchAggregator.js b/platform/search/src/services/SearchAggregator.js index 2324090595..abb6478936 100644 --- a/platform/search/src/services/SearchAggregator.js +++ b/platform/search/src/services/SearchAggregator.js @@ -24,122 +24,194 @@ /** * Module defining SearchAggregator. Created by shale on 07/16/2015. */ -define( - [], - function () { - "use strict"; +define([ - var DEFUALT_TIMEOUT = 1000, - DEFAULT_MAX_RESULTS = 100; - - /** - * Allows multiple services which provide search functionality - * to be treated as one. - * - * @constructor - * @param $q Angular's $q, for promise consolidation. - * @param {SearchProvider[]} providers The search providers to be - * aggregated. - */ - function SearchAggregator($q, providers) { - this.$q = $q; - this.providers = providers; +], function ( + +) { + "use strict"; + + var DEFAULT_TIMEOUT = 1000, + DEFAULT_MAX_RESULTS = 100; + + /** + * Aggregates multiple search providers as a singular search provider. + * Search providers are expected to implement a `query` method which returns + * a promise for a `modelResults` object. + * + * The search aggregator combines the results from multiple providers, + * removes aggregates, and converts the results to domain objects. + * + * @constructor + * @param $q Angular's $q, for promise consolidation. + * @param objectService + * @param {SearchProvider[]} providers The search providers to be + * aggregated. + */ + function SearchAggregator($q, objectService, providers) { + this.$q = $q; + this.objectService = objectService; + this.providers = providers; + } + + /** + * Sends a query to each of the providers. Returns a promise for + * a result object that has the format + * {hits: searchResult[], total: number, timedOut: boolean} + * where a searchResult has the format + * {id: string, object: domainObject, score: number} + * + * @param {String} inputText The text input that is the query. + * @param {Number} maxResults (optional) The maximum number of results + * that this function should return. If not provided, a + * default of 100 will be used. + * @param {Function} [filter] if provided, will be called for every + * potential modelResult. If it returns false, the model result will be + * excluded from the search results. + * @returns {Promise} A Promise for a search result object. + */ + SearchAggregator.prototype.query = function ( + inputText, + maxResults, + filter + ) { + + var aggregator = this, + timestamp = Date.now(), + resultPromises; + + if (!maxResults) { + maxResults = DEFAULT_MAX_RESULTS; } - /** - * Sends a query to each of the providers. Returns a promise for - * a result object that has the format - * {hits: searchResult[], total: number, timedOut: boolean} - * where a searchResult has the format - * {id: string, object: domainObject, score: number} - * - * @param inputText The text input that is the query. - * @param maxResults (optional) The maximum number of results - * that this function should return. If not provided, a - * default of 100 will be used. - */ - SearchAggregator.prototype.query = function queryAll(inputText, maxResults) { - var $q = this.$q, - providers = this.providers, - i, - timestamp = Date.now(), - resultPromises = []; + resultPromises = this.providers.map(function (provider) { + return provider.query( + inputText, + timestamp, + maxResults, + DEFAULT_TIMEOUT + ); + }); - // Remove duplicate objects that have the same ID. Modifies the passed - // array, and returns the number that were removed. - function filterDuplicates(results, total) { - var ids = {}, - numRemoved = 0, - i; + return this.$q + .all(resultPromises) + .then(function (providerResults) { + var modelResults = { + hits: [], + totals: 0, + timedOut: false + }; - for (i = 0; i < results.length; i += 1) { - if (ids[results[i].id]) { - // If this result's ID is already there, remove the object - results.splice(i, 1); - numRemoved += 1; - - // Reduce loop index because we shortened the array - i -= 1; - } else { - // Otherwise add the ID to the list of the ones we have seen - ids[results[i].id] = true; - } - } - - return numRemoved; - } - - // Order the objects from highest to lowest score in the array. - // Modifies the passed array, as well as returns the modified array. - function orderByScore(results) { - results.sort(function (a, b) { - if (a.score > b.score) { - return -1; - } else if (b.score > a.score) { - return 1; - } else { - return 0; - } + providerResults.forEach(function (providerResult) { + modelResults.hits = + modelResults.hits.concat(providerResult.hits); + modelResults.totals += providerResult.totals; + modelResults.timedOut = + modelResults.timedOut || providerResult.timedOut; }); - return results; - } - if (!maxResults) { - maxResults = DEFAULT_MAX_RESULTS; - } + aggregator.orderByScore(modelResults); + aggregator.applyFilter(modelResults, filter); + aggregator.removeDuplicates(modelResults); - // Send the query to all the providers - for (i = 0; i < providers.length; i += 1) { - resultPromises.push( - providers[i].query(inputText, timestamp, maxResults, DEFUALT_TIMEOUT) - ); - } - - // Get promises for results arrays - return $q.all(resultPromises).then(function (resultObjects) { - var results = [], - totalSum = 0, - i; - - // Merge results - for (i = 0; i < resultObjects.length; i += 1) { - results = results.concat(resultObjects[i].hits); - totalSum += resultObjects[i].total; - } - // Order by score first, so that when removing repeats we keep the higher scored ones - orderByScore(results); - totalSum -= filterDuplicates(results, totalSum); - - return { - hits: results, - total: totalSum, - timedOut: resultObjects.some(function (obj) { - return obj.timedOut; - }) - }; + return aggregator.asObjectResults(modelResults); }); - }; + }; - return SearchAggregator; - } -); \ No newline at end of file + /** + * Order model results by score descending and return them. + */ + SearchAggregator.prototype.orderByScore = function (modelResults) { + modelResults.hits.sort(function (a, b) { + if (a.score > b.score) { + return -1; + } else if (b.score > a.score) { + return 1; + } else { + return 0; + } + }); + return modelResults; + }; + + /** + * Apply a filter to each model result, removing it from search results + * if it does not match. + */ + SearchAggregator.prototype.applyFilter = function (modelResults, filter) { + if (!filter) { + return modelResults; + } + var initialLength = modelResults.hits.length, + finalLength, + removedByFilter; + + modelResults.hits = modelResults.hits.filter(function (hit) { + return filter(hit.model); + }); + + finalLength = modelResults.hits; + removedByFilter = initialLength - finalLength; + modelResults.totals -= removedByFilter; + + return modelResults; + }; + + /** + * Remove duplicate hits in a modelResults object, and decrement `totals` + * each time a duplicate is removed. + */ + SearchAggregator.prototype.removeDuplicates = function (modelResults) { + var includedIds = {}; + + modelResults.hits = modelResults + .hits + .filter(function alreadyInResults(hit) { + if (includedIds[hit.id]) { + modelResults.totals -= 1; + return false; + } + includedIds[hit.id] = true; + return true; + }); + + return modelResults; + }; + + /** + * Convert modelResults to objectResults by fetching them from the object + * service. + * + * @returns {Promise} for an objectResults object. + */ + SearchAggregator.prototype.asObjectResults = function (modelResults) { + var objectIds = modelResults.hits.map(function (modelResult) { + return modelResult.id; + }); + + return this + .objectService + .getObjects(objectIds) + .then(function (objects) { + + var objectResults = { + totals: modelResults.totals, + timedOut: modelResults.timedOut + }; + + objectResults.hits = modelResults + .hits + .map(function asObjectResult(hit) { + return { + id: hit.id, + object: objects[hit.id], + score: hit.score + }; + }); + + return objectResults; + }); + }; + + return SearchAggregator; +}); From 78e5c0143b67c48c00adb40702e54829839eeda0 Mon Sep 17 00:00:00 2001 From: Pete Richards Date: Fri, 16 Oct 2015 15:26:46 -0700 Subject: [PATCH 04/18] [Search] Overhaul generic search provider Rewrite the generic search provider to use prototypes. Increase performance by utilizing the model service instead of the object service, and use a simplified method of request queueing. --- platform/search/bundle.json | 3 +- .../src/services/GenericSearchProvider.js | 442 ++++++++++-------- .../src/services/GenericSearchWorker.js | 10 +- 3 files changed, 241 insertions(+), 214 deletions(-) diff --git a/platform/search/bundle.json b/platform/search/bundle.json index 1117c3d161..9e39e28d03 100644 --- a/platform/search/bundle.json +++ b/platform/search/bundle.json @@ -48,8 +48,7 @@ "depends": [ "$q", "$log", - "throttle", - "objectService", + "modelService", "workerService", "topic", "GENERIC_SEARCH_ROOTS" diff --git a/platform/search/src/services/GenericSearchProvider.js b/platform/search/src/services/GenericSearchProvider.js index d38326bb9d..907ab33189 100644 --- a/platform/search/src/services/GenericSearchProvider.js +++ b/platform/search/src/services/GenericSearchProvider.js @@ -24,226 +24,258 @@ /** * Module defining GenericSearchProvider. Created by shale on 07/16/2015. */ -define( - [], - function () { - "use strict"; +define([ - var DEFAULT_MAX_RESULTS = 100, - DEFAULT_TIMEOUT = 1000, - MAX_CONCURRENT_REQUESTS = 100, - FLUSH_INTERVAL = 0, - stopTime; +], function ( - /** - * A search service which searches through domain objects in - * the filetree without using external search implementations. - * - * @constructor - * @param $q Angular's $q, for promise consolidation. - * @param $log Anglar's $log, for logging. - * @param {Function} throttle a function to throttle function invocations - * @param {ObjectService} objectService The service from which - * domain objects can be gotten. - * @param {WorkerService} workerService The service which allows - * more easy creation of web workers. - * @param {GENERIC_SEARCH_ROOTS} ROOTS An array of the root - * domain objects' IDs. - */ - function GenericSearchProvider($q, $log, throttle, objectService, workerService, topic, ROOTS) { - var indexed = {}, - pendingIndex = {}, - pendingQueries = {}, - toRequest = [], - worker = workerService.run('genericSearchWorker'), - mutationTopic = topic("mutation"), - indexingStarted = Date.now(), - pendingRequests = 0, - scheduleFlush; +) { + "use strict"; - this.worker = worker; - this.pendingQueries = pendingQueries; - this.$q = $q; - // pendingQueries is a dictionary with the key value pairs st - // the key is the timestamp and the value is the promise + var DEFAULT_MAX_RESULTS = 100, + MAX_CONCURRENT_REQUESTS = 100; - function scheduleIdsForIndexing(ids) { - ids.forEach(function (id) { - if (!indexed[id] && !pendingIndex[id]) { - indexed[id] = true; - pendingIndex[id] = true; - toRequest.push(id); - } - }); - scheduleFlush(); - } + /** + * A search service which searches through domain objects in + * the filetree without using external search implementations. + * + * @constructor + * @param $q Angular's $q, for promise consolidation. + * @param $log Anglar's $log, for logging. + * @param {ModelService} modelService the model service. + * @param {WorkerService} workerService the workerService. + * @param {TopicService} topic the topic service. + * @param {Array} ROOTS An array of object Ids to begin indexing. + */ + function GenericSearchProvider($q, $log, modelService, workerService, topic, ROOTS) { + var provider = this; + this.$q = $q; + this.$log = $log; + this.modelService = modelService; - // Tell the web worker to add a domain object's model to its list of items. - function indexItem(domainObject) { - var model = domainObject.getModel(); + this.indexedIds = {}; + this.idsToIndex = []; + this.pendingIndex = {}; + this.pendingRequests = 0; - worker.postMessage({ - request: 'index', - model: model, - id: domainObject.getId() - }); + this.pendingQueries = {}; - if (Array.isArray(model.composition)) { - scheduleIdsForIndexing(model.composition); - } - } + this.worker = this.startWorker(workerService); - // Handles responses from the web worker. Namely, the results of - // a search request. - function handleResponse(event) { - if (event.data.request !== 'search') { - return; // no idea how to handle anything else. - } + ROOTS.forEach(function indexRoot(rootId) { + provider.scheduleForIndexing(rootId); + }); + } - var workerResults = event.data.results, - ids = Object.keys(workerResults); - - objectService - .getObjects(ids) - .then(function (objects) { - var searchResults = Object - .keys(objects) - .map(function (id) { - return { - object: objects[id], - id: id, - score: workerResults[id].matchCount - }; - }); - - // Resove the promise corresponding to this - pendingQueries[event.data.timestamp].resolve({ - hits: searchResults, - total: searchResults.length, - timedOut: event.data.timedOut - }); - }); - } - - function requestAndIndex(id) { - pendingRequests += 1; - objectService.getObjects([id]).then(function (objects) { - delete pendingIndex[id]; - if (objects[id]) { - indexItem(objects[id]); - } - }, function () { - $log.warn("Failed to index domain object " + id); - }).then(function () { - pendingRequests -= 1; - scheduleFlush(); - }); - } - - scheduleFlush = throttle(function flush() { - var batchSize = - Math.max(MAX_CONCURRENT_REQUESTS - pendingRequests, 0); - - if (toRequest.length + pendingRequests < 1) { - $log.info([ - 'GenericSearch finished indexing after ', - ((Date.now() - indexingStarted) / 1000).toFixed(2), - ' seconds.' - ].join('')); - } else { - toRequest.splice(-batchSize, batchSize) - .forEach(requestAndIndex); - } - }, FLUSH_INTERVAL); - - worker.onmessage = handleResponse; - - // Index the tree's contents once at the beginning - scheduleIdsForIndexing(ROOTS); - - // Re-index items when they are mutated - mutationTopic.listen(function (domainObject) { - var id = domainObject.getId(); - indexed[id] = false; - scheduleIdsForIndexing([id]); - }); + /** + * Query the search provider for results. + * + * @param {String} input the string to search by. + * @param {Number} timestamp part of the SearchProvider interface, ignored. + * @param {Number} maxResults max number of results to return. + * @returns {Promise} a promise for a modelResults object. + */ + GenericSearchProvider.prototype.query = function ( + input, + timestamp, + maxResults + ) { + if (!maxResults) { + maxResults = DEFAULT_MAX_RESULTS; } - /** - * Searches through the filetree for domain objects which match - * the search term. This function is to be used as a fallback - * in the case where other search services are not avaliable. - * Returns a promise for a result object that has the format - * {hits: searchResult[], total: number, timedOut: boolean} - * where a searchResult has the format - * {id: string, object: domainObject, score: number} - * - * Notes: - * * The order of the results is not guarenteed. - * * A domain object qualifies as a match for a search input if - * the object's name property contains any of the search terms - * (which are generated by splitting the input at spaces). - * * Scores are higher for matches that have more of the terms - * as substrings. - * - * @param input The text input that is the query. - * @param timestamp The time at which this function was called. - * This timestamp is used as a unique identifier for this - * query and the corresponding results. - * @param maxResults (optional) The maximum number of results - * that this function should return. - * @param timeout (optional) The time after which the search should - * stop calculations and return partial results. - */ - GenericSearchProvider.prototype.query = function query(input, timestamp, maxResults, timeout) { - var terms = [], - searchResults = [], - pendingQueries = this.pendingQueries, - worker = this.worker, - defer = this.$q.defer(); + var queryId = this.dispatchSearch(input, maxResults), + pendingQuery = this.$q.defer(); - // Tell the worker to search for items it has that match this searchInput. - // Takes the searchInput, as well as a max number of results (will return - // less than that if there are fewer matches). - function workerSearch(searchInput, maxResults, timestamp, timeout) { - var message = { - request: 'search', - input: searchInput, - maxResults: maxResults, - timestamp: timestamp, - timeout: timeout - }; - worker.postMessage(message); - } + this.pendingQueries[queryId] = pendingQuery; - // If the input is nonempty, do a search - if (input !== '' && input !== undefined) { + return pendingQuery.promise; + }; - // Allow us to access this promise later to resolve it later - pendingQueries[timestamp] = defer; + /** + * Creates a search worker and attaches handlers. + * + * @private + * @param workerService + * @returns worker the created search worker. + */ + GenericSearchProvider.prototype.startWorker = function (workerService) { + var worker = workerService.run('genericSearchWorker'), + provider = this; - // Check to see if the user provided a maximum - // number of results to display - if (!maxResults) { - // Else, we provide a default value - maxResults = DEFAULT_MAX_RESULTS; - } - // Similarly, check if timeout was provided - if (!timeout) { - timeout = DEFAULT_TIMEOUT; - } - - // Send the query to the worker - workerSearch(input, maxResults, timestamp, timeout); - - return defer.promise; - } else { - // Otherwise return an empty result - return { hits: [], total: 0 }; - } + worker.onmessage = function (messageEvent) { + provider.onWorkerMessage(messageEvent); }; + return worker; + }; - return GenericSearchProvider; - } -); + /** + * Listen to the mutation topic and re-index objects when they are + * mutated. + * + * @private + * @param topic the topicService. + */ + GenericSearchProvider.prototype.indexOnMutation = function (topic) { + var mutationTopic = topic('mutation'), + provider = this; + + mutationTopic.listen(function (mutatedObject) { + var id = mutatedObject.getId(); + provider.indexed[id] = false; + provider.scheduleForIndexing(id); + }); + }; + + /** + * Schedule an id to be indexed at a later date. If there are less + * pending requests then allowed, will kick off an indexing request. + * + * @private + * @param {String} id to be indexed. + */ + GenericSearchProvider.prototype.scheduleForIndexing = function (id) { + if (!this.indexedIds[id] && !this.pendingIndex[id]) { + this.indexedIds[id] = true; + this.pendingIndex[id] = true; + this.idsToIndex.push(id); + } + this.keepIndexing(); + }; + + /** + * If there are less pending requests than concurrent requests, keep + * firing requests. + * + * @private + */ + GenericSearchProvider.prototype.keepIndexing = function () { + if (this.pendingRequests < MAX_CONCURRENT_REQUESTS) { + this.beginIndexRequest(); + } + }; + + /** + * Pass an id and model to the worker to be indexed. If the model has + * composition, schedule those ids for later indexing. + * + * @private + * @param id a model id + * @param model a model + */ + GenericSearchProvider.prototype.index = function (id, model) { + var provider = this; + + this.worker.postMessage({ + request: 'index', + model: model, + id: id + }); + + if (Array.isArray(model.composition)) { + model.composition.forEach(function (id) { + provider.scheduleForIndexing(id); + }); + } + }; + + /** + * Pulls an id from the indexing queue, loads it from the model service, + * and indexes it. Upon completion, tells the provider to keep + * indexing. + * + * @private + */ + GenericSearchProvider.prototype.beginIndexRequest = function () { + var idToIndex = this.idsToIndex.shift(), + provider = this; + + if (!idToIndex) { + return; + } + + this.pendingRequests += 1; + this.modelService + .getModels([idToIndex]) + .then(function (models) { + delete provider.pendingIndex[idToIndex]; + if (models[idToIndex]) { + provider.index(idToIndex, models[idToIndex]); + } + }, function () { + provider + .$log + .warn('Failed to index domain object ' + idToIndex); + }) + .then(function () { + provider.keepIndexing(); + }); + }; + + /** + * Handle messages from the worker. Only really knows how to handle search + * results, which are parsed, transformed into a modelResult object, which + * is used to resolve the corresponding promise. + * @private + */ + GenericSearchProvider.prototype.onWorkerMessage = function (event) { + if (event.data.request !== 'search') { + return; + } + + var pendingQuery = this.pendingQueries[event.data.queryId], + modelResults = { + timedOut: event.data.timedOut, + total: event.data.total + }; + + modelResults.hits = event.data.results.map(function (hit) { + return { + id: hit.item.id, + model: hit.item.model, + score: hit.matchCount + }; + }); + + + pendingQuery.resolve(modelResults); + delete this.pendingQueries[event.data.queryId]; + }; + + /** + * @private + * @returns {Number} a unique, unusued query Id. + */ + GenericSearchProvider.prototype.makeQueryId = function () { + var queryId = Math.ceil(Math.random() * 100000); + while (this.pendingQueries[queryId]) { + queryId = Math.ceil(Math.random() * 100000); + } + return queryId; + }; + + /** + * Dispatch a search query to the worker and return a queryId. + * + * @private + * @returns {Number} a unique query Id for the query. + */ + GenericSearchProvider.prototype.dispatchSearch = function ( + searchInput, + maxResults + ) { + var queryId = this.makeQueryId(); + + this.worker.postMessage({ + request: 'search', + input: searchInput, + maxResults: maxResults, + queryId: queryId + }); + + return queryId; + }; + + + return GenericSearchProvider; +}); diff --git a/platform/search/src/services/GenericSearchWorker.js b/platform/search/src/services/GenericSearchWorker.js index cd45929d4f..4ef44e29ab 100644 --- a/platform/search/src/services/GenericSearchWorker.js +++ b/platform/search/src/services/GenericSearchWorker.js @@ -79,8 +79,8 @@ request: 'search', results: {}, total: 0, - timestamp: data.timestamp, - timedOut: false + timedOut: false, + queryId: data.queryId }, matches = {}; @@ -144,11 +144,7 @@ message.total = results.length; message.results = results - .slice(0, data.maxResults) - .reduce(function arrayToObject(obj, match) { - obj[match.item.id] = match; - return obj; - }, {}); + .slice(0, data.maxResults); return message; } From a2fce8e56c824c458fb625155abc92c086438c08 Mon Sep 17 00:00:00 2001 From: Pete Richards Date: Fri, 16 Oct 2015 16:05:31 -0700 Subject: [PATCH 05/18] [Search] Rewrite elasticsearch provider with prototype Rewrite the elasticsearch provider to use prototypes and clean up the implementation. Now returns a modelResults object to keep it in line with the general search provider. --- .../elastic/src/ElasticSearchProvider.js | 287 +++++++----------- 1 file changed, 112 insertions(+), 175 deletions(-) diff --git a/platform/persistence/elastic/src/ElasticSearchProvider.js b/platform/persistence/elastic/src/ElasticSearchProvider.js index 604e3d0ed3..bc313f4deb 100644 --- a/platform/persistence/elastic/src/ElasticSearchProvider.js +++ b/platform/persistence/elastic/src/ElasticSearchProvider.js @@ -24,190 +24,127 @@ /** * Module defining ElasticSearchProvider. Created by shale on 07/16/2015. */ -define( - [], - function () { - "use strict"; +define([ - // JSLint doesn't like underscore-prefixed properties, - // so hide them here. - var ID = "_id", - SCORE = "_score", - DEFAULT_MAX_RESULTS = 100; - - /** - * A search service which searches through domain objects in - * the filetree using ElasticSearch. - * - * @constructor - * @param $http Angular's $http service, for working with urls. - * @param {ObjectService} objectService the service from which - * domain objects can be gotten. - * @param ROOT the constant `ELASTIC_ROOT` which allows us to - * interact with ElasticSearch. - */ - function ElasticSearchProvider($http, objectService, ROOT) { - this.$http = $http; - this.objectService = objectService; - this.root = ROOT; +], function ( + +) { + "use strict"; + + var DEFAULT_MAX_RESULTS = 100; + + /** + * A search service which searches through domain objects in + * the filetree using ElasticSearch. + * + * @constructor + * @param $http Angular's $http service, for working with urls. + * @param {ObjectService} objectService the service from which + * domain objects can be gotten. + * @param ROOT the constant `ELASTIC_ROOT` which allows us to + * interact with ElasticSearch. + */ + function ElasticSearchProvider($http, objectService, ROOT) { + this.$http = $http; + this.objectService = objectService; + this.root = ROOT; + } + + /** + * Search for domain objects using elasticsearch as a search provider. + * + * @param {String} searchTerm the term to search by. + * @param {Number} [maxResults] the max numer of results to return. + * @returns {Promise} promise for a modelResults object. + */ + ElasticSearchProvider.prototype.query = function (searchTerm, maxResults) { + var searchUrl = this.root + '/_search/', + params = {}, + provider = this; + + if (!maxResults) { + maxResults = DEFAULT_MAX_RESULTS; } - /** - * Searches through the filetree for domain objects using a search - * term. This is done through querying elasticsearch. Returns a - * promise for a result object that has the format - * {hits: searchResult[], total: number, timedOut: boolean} - * where a searchResult has the format - * {id: string, object: domainObject, score: number} - * - * Notes: - * * The order of the results is from highest to lowest score, - * as elsaticsearch determines them to be. - * * Uses the fuzziness operator to get more results. - * * More about this search's behavior at - * https://www.elastic.co/guide/en/elasticsearch/reference/current/search-uri-request.html - * - * @param searchTerm The text input that is the query. - * @param timestamp The time at which this function was called. - * This timestamp is used as a unique identifier for this - * query and the corresponding results. - * @param maxResults (optional) The maximum number of results - * that this function should return. - * @param timeout (optional) The time after which the search should - * stop calculations and return partial results. Elasticsearch - * does not guarentee that this timeout will be strictly followed. - */ - ElasticSearchProvider.prototype.query = function query(searchTerm, timestamp, maxResults, timeout) { - var $http = this.$http, - objectService = this.objectService, - root = this.root, - esQuery; - - function addFuzziness(searchTerm, editDistance) { - if (!editDistance) { - editDistance = ''; - } + searchTerm = this.cleanTerm(searchTerm); + searchTerm = this.fuzzyMatchUnquotedTerms(searchTerm); - return searchTerm.split(' ').map(function (s) { - // Don't add fuzziness for quoted strings - if (s.indexOf('"') !== -1) { - return s; - } else { - return s + '~' + editDistance; - } - }).join(' '); - } + params.q = searchTerm; + params.size = maxResults; - // Currently specific to elasticsearch - function processSearchTerm(searchTerm) { - var spaceIndex; - - // Cut out any extra spaces - while (searchTerm.substr(0, 1) === ' ') { - searchTerm = searchTerm.substring(1, searchTerm.length); - } - while (searchTerm.substr(searchTerm.length - 1, 1) === ' ') { - searchTerm = searchTerm.substring(0, searchTerm.length - 1); - } - spaceIndex = searchTerm.indexOf(' '); - while (spaceIndex !== -1) { - searchTerm = searchTerm.substring(0, spaceIndex) + - searchTerm.substring(spaceIndex + 1, searchTerm.length); - spaceIndex = searchTerm.indexOf(' '); - } - - // Add fuzziness for completeness - searchTerm = addFuzziness(searchTerm); - - return searchTerm; - } - - // Processes results from the format that elasticsearch returns to - // a list of searchResult objects, then returns a result object - // (See documentation for query for object descriptions) - function processResults(rawResults, timestamp) { - var results = rawResults.data.hits.hits, - resultsLength = results.length, - ids = [], - scores = {}, - searchResults = [], - i; - - // Get the result objects' IDs - for (i = 0; i < resultsLength; i += 1) { - ids.push(results[i][ID]); - } - - // Get the result objects' scores - for (i = 0; i < resultsLength; i += 1) { - scores[ids[i]] = results[i][SCORE]; - } - - // Get the domain objects from their IDs - return objectService.getObjects(ids).then(function (objects) { - var j, - id; - - for (j = 0; j < resultsLength; j += 1) { - id = ids[j]; - - // Include items we can get models for - if (objects[id].getModel) { - // Format the results as searchResult objects - searchResults.push({ - id: id, - object: objects[id], - score: scores[id] - }); - } - } - - return { - hits: searchResults, - total: rawResults.data.hits.total, - timedOut: rawResults.data.timed_out - }; - }); - } + return this + .$http({ + method: "GET", + url: searchUrl, + params: params + }) + .then(function success(succesResponse) { + return provider.parseResponse(succesResponse); + }, function error(errorResponse) { + // Gracefully fail. + return { + hits: [], + total: 0 + }; + }); + }; - // Check to see if the user provided a maximum - // number of results to display - if (!maxResults) { - // Else, we provide a default value. - maxResults = DEFAULT_MAX_RESULTS; - } + /** + * Clean excess whitespace from a search term and return the cleaned + * version. + * + * @private + * @param {string} the search term to clean. + * @returns {string} search terms cleaned of excess whitespace. + */ + ElasticSearchProvider.prototype.cleanTerm = function (term) { + return term.trim().replace(/ +/, ' '); + }; - // If the user input is empty, we want to have no search results. - if (searchTerm !== '' && searchTerm !== undefined) { - // Process the search term - searchTerm = processSearchTerm(searchTerm); + /** + * Add fuzzy matching markup to search terms that are not quoted. + * + * The following: + * hello welcome "to quoted village" have fun + * will become + * hello~ welcome~ "to quoted village" have~ fun~ + * + * @private + */ + ElasticSearchProvider.prototype.fuzzyMatchUnquotedTerms = function (query) { + var matchUnquotedSpaces = '\\s+(?=([^"]*"[^"]*")*[^"]*$)', + matcher = new RegExp(matchUnquotedSpaces, 'g'); - // Create the query to elasticsearch - esQuery = root + "/_search/?q=" + searchTerm + - "&size=" + maxResults; - if (timeout) { - esQuery += "&timeout=" + timeout; - } + return query + .replace(matcher, '~ ') + .replace('"~', '"'); + }; - // Get the data... - return this.$http({ - method: "GET", - url: esQuery - }).then(function (rawResults) { - // ...then process the data - return processResults(rawResults, timestamp); - }, function (err) { - // In case of error, return nothing. (To prevent - // infinite loading time.) - return {hits: [], total: 0}; - }); - } else { - return {hits: [], total: 0}; - } + /** + * Parse the response from ElasticSearch and convert it to a + * modelResults object. + * + * @private + * @param response a ES response object from $http + * @returns modelResults + */ + ElasticSearchProvider.prototype.parseResponse = function (response) { + var results = response.data.hits.hits, + searchResults = results.map(function (result) { + return { + id: result['_id'], + model: result['_source'], + score: result['_score'] + }; + }); + + return { + hits: searchResults, + total: response.data.hits.total, + timedOut: response.data.timed_out }; + }; - - return ElasticSearchProvider; - } -); \ No newline at end of file + return ElasticSearchProvider; +}); From 12efb47be74bc52ca67a7600e588c08ad8c67590 Mon Sep 17 00:00:00 2001 From: Pete Richards Date: Fri, 16 Oct 2015 16:09:51 -0700 Subject: [PATCH 06/18] [Search] Remove timeouts and timestamps Remove timeouts and timestamps which were not effectively doing anything. --- .../elastic/src/ElasticSearchProvider.js | 1 - .../src/services/GenericSearchProvider.js | 3 --- .../search/src/services/GenericSearchWorker.js | 6 ++---- .../search/src/services/SearchAggregator.js | 17 +++++------------ 4 files changed, 7 insertions(+), 20 deletions(-) diff --git a/platform/persistence/elastic/src/ElasticSearchProvider.js b/platform/persistence/elastic/src/ElasticSearchProvider.js index bc313f4deb..e42cd6553d 100644 --- a/platform/persistence/elastic/src/ElasticSearchProvider.js +++ b/platform/persistence/elastic/src/ElasticSearchProvider.js @@ -142,7 +142,6 @@ define([ return { hits: searchResults, total: response.data.hits.total, - timedOut: response.data.timed_out }; }; diff --git a/platform/search/src/services/GenericSearchProvider.js b/platform/search/src/services/GenericSearchProvider.js index 907ab33189..6e7a7168a0 100644 --- a/platform/search/src/services/GenericSearchProvider.js +++ b/platform/search/src/services/GenericSearchProvider.js @@ -70,13 +70,11 @@ define([ * Query the search provider for results. * * @param {String} input the string to search by. - * @param {Number} timestamp part of the SearchProvider interface, ignored. * @param {Number} maxResults max number of results to return. * @returns {Promise} a promise for a modelResults object. */ GenericSearchProvider.prototype.query = function ( input, - timestamp, maxResults ) { if (!maxResults) { @@ -225,7 +223,6 @@ define([ var pendingQuery = this.pendingQueries[event.data.queryId], modelResults = { - timedOut: event.data.timedOut, total: event.data.total }; diff --git a/platform/search/src/services/GenericSearchWorker.js b/platform/search/src/services/GenericSearchWorker.js index 4ef44e29ab..928f66cab8 100644 --- a/platform/search/src/services/GenericSearchWorker.js +++ b/platform/search/src/services/GenericSearchWorker.js @@ -61,13 +61,12 @@ /** * Gets search results from the indexedItems based on provided search - * input. Returns matching results from indexedItems, as well as the - * timestamp that was passed to it. + * input. Returns matching results from indexedItems * * @param data An object which contains: * * input: The original string which we are searching with * * maxResults: The maximum number of search results desired - * * timestamp: The time identifier from when the query was made + * * queryId: an id identifying this query, will be returned. */ function search(data) { // This results dictionary will have domain object ID keys which @@ -79,7 +78,6 @@ request: 'search', results: {}, total: 0, - timedOut: false, queryId: data.queryId }, matches = {}; diff --git a/platform/search/src/services/SearchAggregator.js b/platform/search/src/services/SearchAggregator.js index abb6478936..b22bfbae18 100644 --- a/platform/search/src/services/SearchAggregator.js +++ b/platform/search/src/services/SearchAggregator.js @@ -31,8 +31,7 @@ define([ ) { "use strict"; - var DEFAULT_TIMEOUT = 1000, - DEFAULT_MAX_RESULTS = 100; + var DEFAULT_MAX_RESULTS = 100; /** * Aggregates multiple search providers as a singular search provider. @@ -57,7 +56,7 @@ define([ /** * Sends a query to each of the providers. Returns a promise for * a result object that has the format - * {hits: searchResult[], total: number, timedOut: boolean} + * {hits: searchResult[], total: number} * where a searchResult has the format * {id: string, object: domainObject, score: number} * @@ -87,9 +86,7 @@ define([ resultPromises = this.providers.map(function (provider) { return provider.query( inputText, - timestamp, - maxResults, - DEFAULT_TIMEOUT + maxResults ); }); @@ -98,16 +95,13 @@ define([ .then(function (providerResults) { var modelResults = { hits: [], - totals: 0, - timedOut: false + totals: 0 }; providerResults.forEach(function (providerResult) { modelResults.hits = modelResults.hits.concat(providerResult.hits); modelResults.totals += providerResult.totals; - modelResults.timedOut = - modelResults.timedOut || providerResult.timedOut; }); aggregator.orderByScore(modelResults); @@ -195,8 +189,7 @@ define([ .then(function (objects) { var objectResults = { - totals: modelResults.totals, - timedOut: modelResults.timedOut + totals: modelResults.totals }; objectResults.hits = modelResults From 0f63e4dde9673c7edd85c0ca187a74a4add1fe53 Mon Sep 17 00:00:00 2001 From: Pete Richards Date: Fri, 16 Oct 2015 17:06:23 -0700 Subject: [PATCH 07/18] [Tests] Rewrite search aggregator specs --- .../search/src/services/SearchAggregator.js | 21 +- .../test/services/SearchAggregatorSpec.js | 293 +++++++++++++----- 2 files changed, 230 insertions(+), 84 deletions(-) diff --git a/platform/search/src/services/SearchAggregator.js b/platform/search/src/services/SearchAggregator.js index b22bfbae18..0cdcc5b922 100644 --- a/platform/search/src/services/SearchAggregator.js +++ b/platform/search/src/services/SearchAggregator.js @@ -76,7 +76,6 @@ define([ ) { var aggregator = this, - timestamp = Date.now(), resultPromises; if (!maxResults) { @@ -95,18 +94,18 @@ define([ .then(function (providerResults) { var modelResults = { hits: [], - totals: 0 + total: 0 }; providerResults.forEach(function (providerResult) { modelResults.hits = modelResults.hits.concat(providerResult.hits); - modelResults.totals += providerResult.totals; + modelResults.total += providerResult.total; }); - aggregator.orderByScore(modelResults); - aggregator.applyFilter(modelResults, filter); - aggregator.removeDuplicates(modelResults); + modelResults = aggregator.orderByScore(modelResults); + modelResults = aggregator.applyFilter(modelResults, filter); + modelResults = aggregator.removeDuplicates(modelResults); return aggregator.asObjectResults(modelResults); }); @@ -144,15 +143,15 @@ define([ return filter(hit.model); }); - finalLength = modelResults.hits; + finalLength = modelResults.hits.length; removedByFilter = initialLength - finalLength; - modelResults.totals -= removedByFilter; + modelResults.total -= removedByFilter; return modelResults; }; /** - * Remove duplicate hits in a modelResults object, and decrement `totals` + * Remove duplicate hits in a modelResults object, and decrement `total` * each time a duplicate is removed. */ SearchAggregator.prototype.removeDuplicates = function (modelResults) { @@ -162,7 +161,7 @@ define([ .hits .filter(function alreadyInResults(hit) { if (includedIds[hit.id]) { - modelResults.totals -= 1; + modelResults.total -= 1; return false; } includedIds[hit.id] = true; @@ -189,7 +188,7 @@ define([ .then(function (objects) { var objectResults = { - totals: modelResults.totals + total: modelResults.total }; objectResults.hits = modelResults diff --git a/platform/search/test/services/SearchAggregatorSpec.js b/platform/search/test/services/SearchAggregatorSpec.js index 3205f0f9ec..044a5f4609 100644 --- a/platform/search/test/services/SearchAggregatorSpec.js +++ b/platform/search/test/services/SearchAggregatorSpec.js @@ -19,83 +19,230 @@ * this source code distribution or the Licensing information page available * at runtime from the About dialog for additional information. *****************************************************************************/ -/*global define,describe,it,expect,beforeEach,jasmine*/ +/*global define,describe,it,expect,beforeEach,jasmine,Promise,waitsFor,spyOn*/ /** * SearchSpec. Created by shale on 07/31/2015. */ -define( - ["../../src/services/SearchAggregator"], - function (SearchAggregator) { - "use strict"; +define([ + "../../src/services/SearchAggregator" +], function (SearchAggregator) { + "use strict"; - describe("The search aggregator ", function () { - var mockQ, - mockPromise, - mockProviders = [], - aggregator, - mockProviderResults = [], - mockAggregatorResults, - i; + describe("SearchAggregator", function () { + var $q, + objectService, + providers, + aggregator; - beforeEach(function () { - mockQ = jasmine.createSpyObj( - "$q", - [ "all" ] - ); - mockPromise = jasmine.createSpyObj( - "promise", - [ "then" ] - ); - for (i = 0; i < 3; i += 1) { - mockProviders.push( - jasmine.createSpyObj( - "mockProvider" + i, - [ "query" ] - ) - ); - mockProviders[i].query.andReturn(mockPromise); - } - mockQ.all.andReturn(mockPromise); - - aggregator = new SearchAggregator(mockQ, mockProviders); - aggregator.query(); - - for (i = 0; i < mockProviders.length; i += 1) { - mockProviderResults.push({ - hits: [ - { - id: i, - score: 42 - i - }, - { - id: i + 1, - score: 42 - (2 * i) - } - ] - }); - } - mockAggregatorResults = mockPromise.then.mostRecentCall.args[0](mockProviderResults); - }); - - it("sends queries to all providers", function () { - for (i = 0; i < mockProviders.length; i += 1) { - expect(mockProviders[i].query).toHaveBeenCalled(); - } - }); - - it("filters out duplicate objects", function () { - expect(mockAggregatorResults.hits.length).toEqual(mockProviders.length + 1); - expect(mockAggregatorResults.total).not.toBeLessThan(mockAggregatorResults.hits.length); - }); - - it("orders results by score", function () { - for (i = 1; i < mockAggregatorResults.hits.length; i += 1) { - expect(mockAggregatorResults.hits[i].score) - .not.toBeGreaterThan(mockAggregatorResults.hits[i - 1].score); - } - }); - + beforeEach(function () { + $q = jasmine.createSpyObj( + '$q', + ['all'] + ); + $q.all.andReturn(Promise.resolve([])); + objectService = jasmine.createSpyObj( + 'objectService', + ['getObjects'] + ); + providers = [], + aggregator = new SearchAggregator($q, objectService, providers); }); - } -); \ No newline at end of file + + + it("can order model results by score", function () { + var modelResults = { + hits: [ + {score: 1}, + {score: 23}, + {score: 11} + ] + }, + sorted = aggregator.orderByScore(modelResults); + + expect(sorted.hits).toEqual([ + {score: 23}, + {score: 11}, + {score: 1} + ]); + }); + + it('filters results without a function', function () { + var modelResults = { + hits: [ + {thing: 1}, + {thing: 2} + ], + total: 2 + }, + filtered = aggregator.applyFilter(modelResults); + + expect(filtered.hits).toEqual([ + {thing: 1}, + {thing: 2} + ]); + + expect(filtered.total).toBe(2); + }); + + it('filters results with a function', function () { + var modelResults = { + hits: [ + {model: {thing: 1}}, + {model: {thing: 2}}, + {model: {thing: 3}} + ], + total: 3 + }, + filterFunc = function (model) { + return model.thing < 2; + }, + filtered = aggregator.applyFilter(modelResults, filterFunc); + + expect(filtered.hits).toEqual([ + {model: {thing: 1}} + ]); + expect(filtered.total).toBe(1); + }); + + it('can remove duplicates', function () { + var modelResults = { + hits: [ + {id: 15}, + {id: 23}, + {id: 14}, + {id: 23} + ], + total: 4 + }, + deduped = aggregator.removeDuplicates(modelResults); + + expect(deduped.hits).toEqual([ + {id: 15}, + {id: 23}, + {id: 14} + ]); + expect(deduped.total).toBe(3); + }); + + it('can convert model results to object results', function () { + var modelResults = { + hits: [ + {id: 123, score: 5}, + {id: 234, score: 1} + ], + total: 2 + }, + objects = { + 123: '123-object-hey', + 234: '234-object-hello' + }, + promiseChainComplete = false; + + objectService.getObjects.andReturn(Promise.resolve(objects)); + + aggregator + .asObjectResults(modelResults) + .then(function (objectResults) { + expect(objectResults).toEqual({ + hits: [ + {id: 123, score: 5, object: '123-object-hey'}, + {id: 234, score: 1, object: '234-object-hello'} + ], + total: 2 + }); + }) + .then(function () { + promiseChainComplete = true; + }); + + waitsFor(function () { + return promiseChainComplete; + }); + }); + + it('can send queries to providers', function () { + var provider = jasmine.createSpyObj( + 'provider', + ['query'] + ); + provider.query.andReturn('i prooomise!'); + providers.push(provider); + + aggregator.query('find me', 123, 'filter'); + expect(provider.query).toHaveBeenCalledWith('find me', 123); + expect($q.all).toHaveBeenCalledWith(['i prooomise!']); + }); + + it('supplies max results when none is provided', function () { + var provider = jasmine.createSpyObj( + 'provider', + ['query'] + ); + providers.push(provider); + aggregator.query('find me'); + expect(provider.query).toHaveBeenCalledWith('find me', 100); + }); + + it('can combine responses from multiple providers', function () { + var providerResponses = [ + { + hits: [ + 'oneHit', + 'twoHit' + ], + total: 2 + }, + { + hits: [ + 'redHit', + 'blueHit', + 'by', + 'Pete' + ], + total: 4 + } + ], + promiseChainResolved = false; + + $q.all.andReturn(Promise.resolve(providerResponses)); + spyOn(aggregator, 'orderByScore').andReturn('orderedByScore!'); + spyOn(aggregator, 'applyFilter').andReturn('filterApplied!'); + spyOn(aggregator, 'removeDuplicates') + .andReturn('duplicatesRemoved!'); + spyOn(aggregator, 'asObjectResults').andReturn('objectResults'); + + aggregator + .query('something', 10, 'filter') + .then(function (objectResults) { + expect(aggregator.orderByScore).toHaveBeenCalledWith({ + hits: [ + 'oneHit', + 'twoHit', + 'redHit', + 'blueHit', + 'by', + 'Pete' + ], + total: 6 + }); + expect(aggregator.applyFilter) + .toHaveBeenCalledWith('orderedByScore!', 'filter'); + expect(aggregator.removeDuplicates) + .toHaveBeenCalledWith('filterApplied!'); + expect(aggregator.asObjectResults) + .toHaveBeenCalledWith('duplicatesRemoved!'); + + expect(objectResults).toBe('objectResults'); + }) + .then(function () { + promiseChainResolved = true; + }); + + waitsFor(function () { + return promiseChainResolved; + }); + }); + + }); +}); From 14094a48fc542eca54d02f6db84cd4233305ffa5 Mon Sep 17 00:00:00 2001 From: Pete Richards Date: Fri, 16 Oct 2015 17:33:23 -0700 Subject: [PATCH 08/18] [Search] Remove old specs in prep for rewrite Remove old specs in prep for rewrite. --- .../elastic/test/ElasticSearchProviderSpec.js | 93 +----- .../services/GenericSearchProviderSpec.js | 283 ++---------------- .../test/services/GenericSearchWorkerSpec.js | 32 +- 3 files changed, 42 insertions(+), 366 deletions(-) diff --git a/platform/persistence/elastic/test/ElasticSearchProviderSpec.js b/platform/persistence/elastic/test/ElasticSearchProviderSpec.js index decb1a5c98..af515e9431 100644 --- a/platform/persistence/elastic/test/ElasticSearchProviderSpec.js +++ b/platform/persistence/elastic/test/ElasticSearchProviderSpec.js @@ -24,92 +24,9 @@ /** * SearchSpec. Created by shale on 07/31/2015. */ -define( - ["../src/ElasticSearchProvider"], - function (ElasticSearchProvider) { - "use strict"; +define([ + "../src/ElasticSearchProvider" +], function (ElasticSearchProvider) { + "use strict"; - // JSLint doesn't like underscore-prefixed properties, - // so hide them here. - var ID = "_id", - SCORE = "_score"; - - describe("The ElasticSearch search provider ", function () { - var mockHttp, - mockHttpPromise, - mockObjectPromise, - mockObjectService, - mockDomainObject, - provider, - mockProviderResults; - - beforeEach(function () { - mockHttp = jasmine.createSpy("$http"); - mockHttpPromise = jasmine.createSpyObj( - "promise", - [ "then" ] - ); - mockHttp.andReturn(mockHttpPromise); - // allow chaining of promise.then().catch(); - mockHttpPromise.then.andReturn(mockHttpPromise); - - mockObjectService = jasmine.createSpyObj( - "objectService", - [ "getObjects" ] - ); - mockObjectPromise = jasmine.createSpyObj( - "promise", - [ "then" ] - ); - mockObjectService.getObjects.andReturn(mockObjectPromise); - - mockDomainObject = jasmine.createSpyObj( - "domainObject", - [ "getId", "getModel" ] - ); - - provider = new ElasticSearchProvider(mockHttp, mockObjectService, ""); - provider.query(' test "query" ', 0, undefined, 1000); - }); - - it("sends a query to ElasticSearch", function () { - expect(mockHttp).toHaveBeenCalled(); - }); - - it("gets data from ElasticSearch", function () { - var data = { - hits: { - hits: [ - {}, - {} - ], - total: 0 - }, - timed_out: false - }; - data.hits.hits[0][ID] = 1; - data.hits.hits[0][SCORE] = 1; - data.hits.hits[1][ID] = 2; - data.hits.hits[1][SCORE] = 2; - - mockProviderResults = mockHttpPromise.then.mostRecentCall.args[0]({data: data}); - - expect( - mockObjectPromise.then.mostRecentCall.args[0]({ - 1: mockDomainObject, - 2: mockDomainObject - }).hits.length - ).toEqual(2); - }); - - it("returns nothing for an empty string query", function () { - expect(provider.query("").hits).toEqual([]); - }); - - it("returns something when there is an ElasticSearch error", function () { - mockProviderResults = mockHttpPromise.then.mostRecentCall.args[1](); - expect(mockProviderResults).toBeDefined(); - }); - }); - } -); \ No newline at end of file +}); diff --git a/platform/search/test/services/GenericSearchProviderSpec.js b/platform/search/test/services/GenericSearchProviderSpec.js index e3ee0a97ba..5c48eb2969 100644 --- a/platform/search/test/services/GenericSearchProviderSpec.js +++ b/platform/search/test/services/GenericSearchProviderSpec.js @@ -24,270 +24,29 @@ /** * SearchSpec. Created by shale on 07/31/2015. */ -define( - ["../../src/services/GenericSearchProvider"], - function (GenericSearchProvider) { - "use strict"; +define([ + "../../src/services/GenericSearchProvider" +], function (GenericSearchProvider) { + "use strict"; - describe("The generic search provider ", function () { - var mockQ, - mockLog, - mockThrottle, - mockDeferred, - mockObjectService, - mockObjectPromise, - mockChainedPromise, - mockDomainObjects, - mockCapability, - mockCapabilityPromise, - mockWorkerService, - mockWorker, - mockTopic, - mockMutationTopic, - mockRoots = ['root1', 'root2'], - mockThrottledFn, - throttledCallCount, - provider, - mockProviderResults; + describe('GenericSearchProvider', function () { + var $q, + $log, + modelService, + workerService, + topic, + ROOTS; - function resolveObjectPromises() { - var i; - for (i = 0; i < mockObjectPromise.then.calls.length; i += 1) { - mockChainedPromise.then.calls[i].args[0]( - mockObjectPromise.then.calls[i] - .args[0](mockDomainObjects) - ); - } - } + beforeEach(function () { + $q = jasmine.createSpyObj( + '$q', + ['defer'] + ); + // TODO: continue - function resolveThrottledFn() { - if (mockThrottledFn.calls.length > throttledCallCount) { - mockThrottle.mostRecentCall.args[0](); - throttledCallCount = mockThrottledFn.calls.length; - } - } - - function resolveAsyncTasks() { - resolveThrottledFn(); - resolveObjectPromises(); - } - - beforeEach(function () { - mockQ = jasmine.createSpyObj( - "$q", - [ "defer" ] - ); - mockLog = jasmine.createSpyObj( - "$log", - [ "error", "warn", "info", "debug" ] - ); - mockDeferred = jasmine.createSpyObj( - "deferred", - [ "resolve", "reject"] - ); - mockDeferred.promise = "mock promise"; - mockQ.defer.andReturn(mockDeferred); - - mockThrottle = jasmine.createSpy("throttle"); - mockThrottledFn = jasmine.createSpy("throttledFn"); - throttledCallCount = 0; - - mockObjectService = jasmine.createSpyObj( - "objectService", - [ "getObjects" ] - ); - mockObjectPromise = jasmine.createSpyObj( - "promise", - [ "then", "catch" ] - ); - mockChainedPromise = jasmine.createSpyObj( - "chainedPromise", - [ "then" ] - ); - mockObjectService.getObjects.andReturn(mockObjectPromise); - - mockTopic = jasmine.createSpy('topic'); - - mockWorkerService = jasmine.createSpyObj( - "workerService", - [ "run" ] - ); - mockWorker = jasmine.createSpyObj( - "worker", - [ "postMessage" ] - ); - mockWorkerService.run.andReturn(mockWorker); - - mockCapabilityPromise = jasmine.createSpyObj( - "promise", - [ "then", "catch" ] - ); - - mockDomainObjects = {}; - ['a', 'root1', 'root2'].forEach(function (id) { - mockDomainObjects[id] = ( - jasmine.createSpyObj( - "domainObject", - [ - "getId", - "getModel", - "hasCapability", - "getCapability", - "useCapability" - ] - ) - ); - mockDomainObjects[id].getId.andReturn(id); - mockDomainObjects[id].getCapability.andReturn(mockCapability); - mockDomainObjects[id].useCapability.andReturn(mockCapabilityPromise); - mockDomainObjects[id].getModel.andReturn({}); - }); - - mockCapability = jasmine.createSpyObj( - "capability", - [ "invoke", "listen" ] - ); - mockCapability.invoke.andReturn(mockCapabilityPromise); - mockDomainObjects.a.getCapability.andReturn(mockCapability); - mockMutationTopic = jasmine.createSpyObj( - 'mutationTopic', - [ 'listen' ] - ); - mockTopic.andCallFake(function (key) { - return key === 'mutation' && mockMutationTopic; - }); - mockThrottle.andReturn(mockThrottledFn); - mockObjectPromise.then.andReturn(mockChainedPromise); - - provider = new GenericSearchProvider( - mockQ, - mockLog, - mockThrottle, - mockObjectService, - mockWorkerService, - mockTopic, - mockRoots - ); - }); - - it("indexes tree on initialization", function () { - var i; - - resolveThrottledFn(); - - expect(mockObjectService.getObjects).toHaveBeenCalled(); - expect(mockObjectPromise.then).toHaveBeenCalled(); - - // Call through the root-getting part - resolveObjectPromises(); - - mockRoots.forEach(function (id) { - expect(mockWorker.postMessage).toHaveBeenCalledWith({ - request: 'index', - model: mockDomainObjects[id].getModel(), - id: id - }); - }); - }); - - it("indexes members of composition", function () { - mockDomainObjects.root1.getModel.andReturn({ - composition: ['a'] - }); - - resolveAsyncTasks(); - resolveAsyncTasks(); - - expect(mockWorker.postMessage).toHaveBeenCalledWith({ - request: 'index', - model: mockDomainObjects.a.getModel(), - id: 'a' - }); - }); - - it("listens for changes to mutation", function () { - expect(mockMutationTopic.listen) - .toHaveBeenCalledWith(jasmine.any(Function)); - mockMutationTopic.listen.mostRecentCall - .args[0](mockDomainObjects.a); - - resolveAsyncTasks(); - - expect(mockWorker.postMessage).toHaveBeenCalledWith({ - request: 'index', - model: mockDomainObjects.a.getModel(), - id: mockDomainObjects.a.getId() - }); - }); - - it("sends search queries to the worker", function () { - var timestamp = Date.now(); - provider.query(' test "query" ', timestamp, 1, 2); - expect(mockWorker.postMessage).toHaveBeenCalledWith({ - request: "search", - input: ' test "query" ', - timestamp: timestamp, - maxNumber: 1, - timeout: 2 - }); - }); - - it("gives an empty result for an empty query", function () { - var timestamp = Date.now(), - queryOutput; - - queryOutput = provider.query('', timestamp, 1, 2); - expect(queryOutput.hits).toEqual([]); - expect(queryOutput.total).toEqual(0); - - queryOutput = provider.query(); - expect(queryOutput.hits).toEqual([]); - expect(queryOutput.total).toEqual(0); - }); - - it("handles responses from the worker", function () { - var timestamp = Date.now(), - event = { - data: { - request: "search", - results: { - 1: 1, - 2: 2 - }, - total: 2, - timedOut: false, - timestamp: timestamp - } - }; - - provider.query(' test "query" ', timestamp); - mockWorker.onmessage(event); - mockObjectPromise.then.mostRecentCall.args[0](mockDomainObjects); - expect(mockDeferred.resolve).toHaveBeenCalled(); - }); - - it("warns when objects are unavailable", function () { - resolveAsyncTasks(); - expect(mockLog.warn).not.toHaveBeenCalled(); - mockChainedPromise.then.mostRecentCall.args[0]( - mockObjectPromise.then.mostRecentCall.args[1]() - ); - expect(mockLog.warn).toHaveBeenCalled(); - }); - - it("throttles the loading of objects to index", function () { - expect(mockObjectService.getObjects).not.toHaveBeenCalled(); - resolveThrottledFn(); - expect(mockObjectService.getObjects).toHaveBeenCalled(); - }); - - it("logs when all objects have been processed", function () { - expect(mockLog.info).not.toHaveBeenCalled(); - resolveAsyncTasks(); - resolveThrottledFn(); - expect(mockLog.info).toHaveBeenCalled(); - }); }); - } -); + + }); + +}); diff --git a/platform/search/test/services/GenericSearchWorkerSpec.js b/platform/search/test/services/GenericSearchWorkerSpec.js index b95ec5a1bb..07e8cf4eb6 100644 --- a/platform/search/test/services/GenericSearchWorkerSpec.js +++ b/platform/search/test/services/GenericSearchWorkerSpec.js @@ -33,7 +33,7 @@ define( // If this test fails, make sure this path is correct var worker = new Worker(require.toUrl('platform/search/src/services/GenericSearchWorker.js')), numObjects = 5; - + beforeEach(function () { var i; for (i = 0; i < numObjects; i += 1) { @@ -50,77 +50,77 @@ define( ); } }); - + it("searches can reach all objects", function () { var flag = false, workerOutput, resultsLength = 0; - + // Search something that should return all objects runs(function () { worker.postMessage( { request: "search", input: "object", - maxNumber: 100, + maxResults: 100, timestamp: Date.now(), timeout: 1000 } ); }); - + worker.onmessage = function (event) { var id; - + workerOutput = event.data; for (id in workerOutput.results) { resultsLength += 1; } flag = true; }; - + waitsFor(function () { return flag; }, "The worker should be searching", 1000); - + runs(function () { expect(workerOutput).toBeDefined(); expect(resultsLength).toEqual(numObjects); }); }); - + it("searches return only matches", function () { var flag = false, workerOutput, resultsLength = 0; - + // Search something that should return 1 object runs(function () { worker.postMessage( { request: "search", input: "2", - maxNumber: 100, + maxResults: 100, timestamp: Date.now(), timeout: 1000 } ); }); - + worker.onmessage = function (event) { var id; - + workerOutput = event.data; for (id in workerOutput.results) { resultsLength += 1; } flag = true; }; - + waitsFor(function () { return flag; }, "The worker should be searching", 1000); - + runs(function () { expect(workerOutput).toBeDefined(); expect(resultsLength).toEqual(1); @@ -129,4 +129,4 @@ define( }); }); } -); \ No newline at end of file +); From 98b5ff3c77bb6fe88b6bcbd13d0db840e7bc33df Mon Sep 17 00:00:00 2001 From: Pete Richards Date: Fri, 16 Oct 2015 18:14:33 -0700 Subject: [PATCH 09/18] [Search] Decrement number of pending requests --- platform/search/src/services/GenericSearchProvider.js | 1 + 1 file changed, 1 insertion(+) diff --git a/platform/search/src/services/GenericSearchProvider.js b/platform/search/src/services/GenericSearchProvider.js index 6e7a7168a0..c34a0a2646 100644 --- a/platform/search/src/services/GenericSearchProvider.js +++ b/platform/search/src/services/GenericSearchProvider.js @@ -206,6 +206,7 @@ define([ .warn('Failed to index domain object ' + idToIndex); }) .then(function () { + provider.pendingRequests -=1; provider.keepIndexing(); }); }; From 1ddce48f7ec6b58627c7fea3e66cf8b2d5d94e6b Mon Sep 17 00:00:00 2001 From: Pete Richards Date: Tue, 20 Oct 2015 13:12:04 -0700 Subject: [PATCH 10/18] [Search] Specs for GenericSearchProvider Write specs for GenericSearchProvider and resolve some implementation bugs they uncovered. --- .../src/services/GenericSearchProvider.js | 20 +- .../services/GenericSearchProviderSpec.js | 271 +++++++++++++++++- 2 files changed, 277 insertions(+), 14 deletions(-) diff --git a/platform/search/src/services/GenericSearchProvider.js b/platform/search/src/services/GenericSearchProvider.js index c34a0a2646..e1c90f3f51 100644 --- a/platform/search/src/services/GenericSearchProvider.js +++ b/platform/search/src/services/GenericSearchProvider.js @@ -31,9 +31,6 @@ define([ ) { "use strict"; - var DEFAULT_MAX_RESULTS = 100, - MAX_CONCURRENT_REQUESTS = 100; - /** * A search service which searches through domain objects in * the filetree without using external search implementations. @@ -60,12 +57,18 @@ define([ this.pendingQueries = {}; this.worker = this.startWorker(workerService); + this.indexOnMutation(topic); ROOTS.forEach(function indexRoot(rootId) { provider.scheduleForIndexing(rootId); }); } + /** + * Maximum number of concurrent index requests to allow. + */ + GenericSearchProvider.prototype.MAX_CONCURRENT_REQUESTS = 100; + /** * Query the search provider for results. * @@ -77,9 +80,6 @@ define([ input, maxResults ) { - if (!maxResults) { - maxResults = DEFAULT_MAX_RESULTS; - } var queryId = this.dispatchSearch(input, maxResults), pendingQuery = this.$q.defer(); @@ -100,9 +100,9 @@ define([ var worker = workerService.run('genericSearchWorker'), provider = this; - worker.onmessage = function (messageEvent) { + worker.addEventListener('message', function (messageEvent) { provider.onWorkerMessage(messageEvent); - }; + }); return worker; }; @@ -148,7 +148,7 @@ define([ * @private */ GenericSearchProvider.prototype.keepIndexing = function () { - if (this.pendingRequests < MAX_CONCURRENT_REQUESTS) { + if (this.pendingRequests < this.MAX_CONCURRENT_REQUESTS) { this.beginIndexRequest(); } }; @@ -206,7 +206,7 @@ define([ .warn('Failed to index domain object ' + idToIndex); }) .then(function () { - provider.pendingRequests -=1; + provider.pendingRequests -= 1; provider.keepIndexing(); }); }; diff --git a/platform/search/test/services/GenericSearchProviderSpec.js b/platform/search/test/services/GenericSearchProviderSpec.js index 5c48eb2969..fb3d595345 100644 --- a/platform/search/test/services/GenericSearchProviderSpec.js +++ b/platform/search/test/services/GenericSearchProviderSpec.js @@ -26,27 +26,290 @@ */ define([ "../../src/services/GenericSearchProvider" -], function (GenericSearchProvider) { +], function ( + GenericSearchProvider +) { "use strict"; describe('GenericSearchProvider', function () { var $q, $log, modelService, + models, workerService, + worker, topic, - ROOTS; + mutationTopic, + ROOTS, + provider; beforeEach(function () { $q = jasmine.createSpyObj( '$q', ['defer'] ); - // TODO: continue + $log = jasmine.createSpyObj( + '$log', + ['warn'] + ); + models = {}; + modelService = jasmine.createSpyObj( + 'modelService', + ['getModels'] + ); + modelService.getModels.andReturn(Promise.resolve(models)); + workerService = jasmine.createSpyObj( + 'workerService', + ['run'] + ); + worker = jasmine.createSpyObj( + 'worker', + [ + 'postMessage', + 'addEventListener' + ] + ); + workerService.run.andReturn(worker); + topic = jasmine.createSpy('topic'); + mutationTopic = jasmine.createSpyObj( + 'mutationTopic', + ['listen'] + ); + topic.andReturn(mutationTopic); + ROOTS = [ + 'mine' + ]; + spyOn(GenericSearchProvider.prototype, 'scheduleForIndexing'); + + provider = new GenericSearchProvider( + $q, + $log, + modelService, + workerService, + topic, + ROOTS + ); + }); + + it('listens for general mutation', function () { + expect(topic).toHaveBeenCalledWith('mutation'); + expect(mutationTopic.listen) + .toHaveBeenCalledWith(jasmine.any(Function)); + }); + + it('starts indexing roots', function () { + expect(provider.scheduleForIndexing).toHaveBeenCalledWith('mine'); + }); + + it('runs a worker', function () { + expect(workerService.run) + .toHaveBeenCalledWith('genericSearchWorker'); + }); + + it('listens for messages from worker', function () { + expect(worker.addEventListener) + .toHaveBeenCalledWith('message', jasmine.any(Function)); + spyOn(provider, 'onWorkerMessage'); + worker.addEventListener.mostRecentCall.args[1]('mymessage'); + expect(provider.onWorkerMessage).toHaveBeenCalledWith('mymessage'); + }); + + it('has a maximum number of concurrent requests', function () { + expect(provider.MAX_CONCURRENT_REQUESTS).toBe(100); + }); + + describe('scheduleForIndexing', function () { + beforeEach(function () { + provider.scheduleForIndexing.andCallThrough(); + spyOn(provider, 'keepIndexing'); + }); + + it('tracks ids to index', function () { + expect(provider.indexedIds['a']).not.toBeDefined(); + expect(provider.pendingIndex['a']).not.toBeDefined(); + expect(provider.idsToIndex).not.toContain('a'); + provider.scheduleForIndexing('a'); + expect(provider.indexedIds['a']).toBeDefined(); + expect(provider.pendingIndex['a']).toBeDefined(); + expect(provider.idsToIndex).toContain('a'); + }); + + it('calls keep indexing', function () { + provider.scheduleForIndexing('a'); + expect(provider.keepIndexing).toHaveBeenCalled(); + }); + }); + + describe('keepIndexing', function () { + it('kicks off an index request when not at maximum', function () { + spyOn(provider, 'beginIndexRequest'); + provider.pendingRequests = 0; + provider.MAX_CONCURRENT_REQUESTS = 10; + provider.keepIndexing(); + expect(provider.beginIndexRequest).toHaveBeenCalled(); + }); + + it('does not index when at capacity', function () { + spyOn(provider, 'beginIndexRequest'); + provider.pendingRequests = 10; + provider.MAX_CONCURRENT_REQUESTS = 10; + provider.keepIndexing(); + expect(provider.beginIndexRequest).not.toHaveBeenCalled(); + }); + }); + + describe('index', function () { + it('sends index message to worker', function () { + var id = 'anId', + model = {}; + + provider.index(id, model); + expect(worker.postMessage).toHaveBeenCalledWith({ + request: 'index', + id: id, + model: model + }); + }); + + it('schedules composed ids for indexing', function () { + var id = 'anId', + model = {composition: ['abc', 'def']}; + + provider.index(id, model); + expect(provider.scheduleForIndexing) + .toHaveBeenCalledWith('abc'); + expect(provider.scheduleForIndexing) + .toHaveBeenCalledWith('def'); + }); + }); + + describe('beginIndexRequest', function () { + + beforeEach(function () { + provider.pendingRequests = 0; + provider.pendingIds = {'abc': true}; + provider.idsToIndex = ['abc']; + models.abc = {}; + spyOn(provider, 'index'); + }); + + it('removes items from queue', function () { + provider.beginIndexRequest(); + expect(provider.idsToIndex.length).toBe(0); + }); + + it('tracks number of pending requests', function () { + provider.beginIndexRequest(); + expect(provider.pendingRequests).toBe(1); + waitsFor(function () { + return provider.pendingRequests === 0; + }); + runs(function () { + expect(provider.pendingRequests).toBe(0); + }); + }); + + it('indexes objects', function () { + provider.beginIndexRequest(); + waitsFor(function () { + return provider.pendingRequests === 0; + }) + runs(function () { + expect(provider.index) + .toHaveBeenCalledWith('abc', models.abc); + }); + }); + + it('does not error if no objects queued', function () { + provider.idsToIndex = []; + expect(function () { + provider.beginIndexRequest() + }).not.toThrow(); + }); + }); + + + it('can dispatch searches to worker', function () { + spyOn(provider, 'makeQueryId').andReturn(428); + expect(provider.dispatchSearch('searchTerm', 100)) + .toBe(428); + + expect(worker.postMessage).toHaveBeenCalledWith({ + request: 'search', + input: 'searchTerm', + maxResults: 100, + queryId: 428 + }); + }); + + it('can generate queryIds', function () { + expect(provider.makeQueryId()).toEqual(jasmine.any(Number)); + }); + + it('can query for terms', function () { + var deferred = {promise: {}}; + spyOn(provider, 'dispatchSearch').andReturn(303); + $q.defer.andReturn(deferred); + + expect(provider.query('someTerm', 100)).toBe(deferred.promise); + expect(provider.pendingQueries[303]).toBe(deferred); + }); + + describe('onWorkerMessage', function () { + var pendingQuery; + beforeEach(function () { + pendingQuery = jasmine.createSpyObj( + 'pendingQuery', + ['resolve'] + ); + provider.pendingQueries[143] = pendingQuery; + }); + + it('resolves pending searches', function () { + provider.onWorkerMessage({ + data: { + request: 'search', + total: 2, + results: [ + { + item: { + id: 'abc', + model: {id: 'abc'} + }, + matchCount: 4 + }, + { + item: { + id: 'def', + model: {id: 'def'} + }, + matchCount: 2 + } + ], + queryId: 143 + } + }); + + expect(pendingQuery.resolve) + .toHaveBeenCalledWith({ + total: 2, + hits: [{ + id: 'abc', + model: {id: 'abc'}, + score: 4 + }, { + id: 'def', + model: {id: 'def'}, + score: 2 + }] + }); + + expect(provider.pendingQueries[143]).not.toBeDefined(); + + }); }); }); - }); From ec7e6cc5b445439630ce56121383f47eb8849cde Mon Sep 17 00:00:00 2001 From: Pete Richards Date: Tue, 20 Oct 2015 13:55:46 -0700 Subject: [PATCH 11/18] [Search] Update spec for Generic Search Worker --- .../test/services/GenericSearchWorkerSpec.js | 281 ++++++++++++------ 1 file changed, 186 insertions(+), 95 deletions(-) diff --git a/platform/search/test/services/GenericSearchWorkerSpec.js b/platform/search/test/services/GenericSearchWorkerSpec.js index 07e8cf4eb6..20afb4c781 100644 --- a/platform/search/test/services/GenericSearchWorkerSpec.js +++ b/platform/search/test/services/GenericSearchWorkerSpec.js @@ -4,12 +4,12 @@ * Administration. All rights reserved. * * Open MCT Web is licensed under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance with the License. + * 'License'); you may not use this file except in compliance with the License. * You may obtain a copy of the License at * http://www.apache.org/licenses/LICENSE-2.0. * * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * distributed under the License is distributed on an 'AS IS' BASIS, WITHOUT * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the * License for the specific language governing permissions and limitations * under the License. @@ -19,114 +19,205 @@ * this source code distribution or the Licensing information page available * at runtime from the About dialog for additional information. *****************************************************************************/ -/*global define,describe,it,expect,runs,waitsFor,beforeEach,jasmine,Worker,require*/ +/*global define,describe,it,expect,runs,waitsFor,beforeEach,jasmine,Worker, + require,afterEach*/ /** * SearchSpec. Created by shale on 07/31/2015. */ -define( - [], - function () { - "use strict"; +define([ - describe("The generic search worker ", function () { - // If this test fails, make sure this path is correct - var worker = new Worker(require.toUrl('platform/search/src/services/GenericSearchWorker.js')), - numObjects = 5; +], function ( - beforeEach(function () { - var i; - for (i = 0; i < numObjects; i += 1) { - worker.postMessage( - { - request: "index", - id: i, - model: { - name: "object " + i, - id: i, - type: "something" - } - } - ); - } - }); +) { + 'use strict'; - it("searches can reach all objects", function () { - var flag = false, - workerOutput, - resultsLength = 0; + describe('GenericSearchWorker', function () { + // If this test fails, make sure this path is correct + var worker, + objectX, + objectY, + objectZ, + itemsToIndex, + onMessage, + data, + waitForResult; - // Search something that should return all objects - runs(function () { - worker.postMessage( - { - request: "search", - input: "object", - maxResults: 100, - timestamp: Date.now(), - timeout: 1000 - } - ); - }); + beforeEach(function () { + worker = new Worker( + require.toUrl('platform/search/src/services/GenericSearchWorker.js') + ); - worker.onmessage = function (event) { - var id; + objectX = { + id: 'x', + model: {name: 'object xx'} + }; + objectY = { + id: 'y', + model: {name: 'object yy'} + }; + objectZ = { + id: 'z', + model: {name: 'object zz'} + }; + itemsToIndex = [ + objectX, + objectY, + objectZ + ]; - workerOutput = event.data; - for (id in workerOutput.results) { - resultsLength += 1; - } - flag = true; - }; - - waitsFor(function () { - return flag; - }, "The worker should be searching", 1000); - - runs(function () { - expect(workerOutput).toBeDefined(); - expect(resultsLength).toEqual(numObjects); + itemsToIndex.forEach(function (item) { + worker.postMessage({ + request: 'index', + id: item.id, + model: item.model }); }); - it("searches return only matches", function () { - var flag = false, - workerOutput, - resultsLength = 0; - - // Search something that should return 1 object - runs(function () { - worker.postMessage( - { - request: "search", - input: "2", - maxResults: 100, - timestamp: Date.now(), - timeout: 1000 - } - ); - }); - - worker.onmessage = function (event) { - var id; - - workerOutput = event.data; - for (id in workerOutput.results) { - resultsLength += 1; - } - flag = true; - }; + onMessage = jasmine.createSpy('onMessage'); + worker.addEventListener('message', onMessage); + waitForResult = function () { waitsFor(function () { - return flag; - }, "The worker should be searching", 1000); - - runs(function () { - expect(workerOutput).toBeDefined(); - expect(resultsLength).toEqual(1); - expect(workerOutput.results[2]).toBeDefined(); + if (onMessage.calls.length > 0) { + data = onMessage.calls[0].args[0].data; + return true; + } + return false; }); + }; + }); + + afterEach(function () { + worker.terminate(); + }); + + it('returns search results for partial term matches', function () { + + worker.postMessage({ + request: 'search', + input: 'obj', + maxResults: 100, + queryId: 123 + }); + + waitForResult(); + + runs(function () { + expect(onMessage).toHaveBeenCalled(); + + expect(data.request).toBe('search'); + expect(data.total).toBe(3); + expect(data.queryId).toBe(123); + expect(data.results.length).toBe(3); + expect(data.results[0].item.id).toBe('x'); + expect(data.results[0].item.model).toEqual(objectX.model); + expect(data.results[0].matchCount).toBe(1); + expect(data.results[1].item.id).toBe('y'); + expect(data.results[1].item.model).toEqual(objectY.model); + expect(data.results[1].matchCount).toBe(1); + expect(data.results[2].item.id).toBe('z'); + expect(data.results[2].item.model).toEqual(objectZ.model); + expect(data.results[2].matchCount).toBe(1); }); }); - } -); + + it('scores exact term matches higher', function () { + worker.postMessage({ + request: 'search', + input: 'object', + maxResults: 100, + queryId: 234 + }); + + waitForResult(); + + runs(function () { + expect(data.queryId).toBe(234); + expect(data.results.length).toBe(3); + expect(data.results[0].item.id).toBe('x'); + expect(data.results[0].matchCount).toBe(1.5); + }); + }); + + it('can find partial term matches', function () { + worker.postMessage({ + request: 'search', + input: 'x', + maxResults: 100, + queryId: 345 + }); + + waitForResult(); + + runs(function () { + expect(data.queryId).toBe(345); + expect(data.results.length).toBe(1); + expect(data.results[0].item.id).toBe('x'); + expect(data.results[0].matchCount).toBe(1); + }); + }); + + it('matches individual terms', function () { + worker.postMessage({ + request: 'search', + input: 'x y z', + maxResults: 100, + queryId: 456 + }); + + waitForResult(); + + runs(function () { + expect(data.queryId).toBe(456); + expect(data.results.length).toBe(3); + expect(data.results[0].item.id).toBe('x'); + expect(data.results[0].matchCount).toBe(1); + expect(data.results[1].item.id).toBe('y'); + expect(data.results[1].matchCount).toBe(1); + expect(data.results[2].item.id).toBe('z'); + expect(data.results[1].matchCount).toBe(1); + }); + }); + + it('scores exact matches highest', function () { + worker.postMessage({ + request: 'search', + input: 'object xx', + maxResults: 100, + queryId: 567 + }); + + waitForResult(); + + runs(function () { + expect(data.queryId).toBe(567); + expect(data.results.length).toBe(3); + expect(data.results[0].item.id).toBe('x'); + expect(data.results[0].matchCount).toBe(103); + expect(data.results[1].matchCount).toBe(1.5); + expect(data.results[2].matchCount).toBe(1.5); + }); + }); + + it('scores multiple term match above single match', function () { + worker.postMessage({ + request: 'search', + input: 'obj x', + maxResults: 100, + queryId: 678 + }); + + waitForResult(); + + runs(function () { + expect(data.queryId).toBe(678); + expect(data.results.length).toBe(3); + expect(data.results[0].item.id).toBe('x'); + expect(data.results[0].matchCount).toBe(2); + expect(data.results[1].matchCount).toBe(1); + expect(data.results[2].matchCount).toBe(1); + }); + }); + }); +}); From 76151d09a0a589fb646494079ebf8a483ecb2d11 Mon Sep 17 00:00:00 2001 From: Pete Richards Date: Tue, 20 Oct 2015 15:13:37 -0700 Subject: [PATCH 12/18] [Search] use service for filters, add spec Add a spec for the SearchController, and use the SearchService to execute filters by supplying a filterPredicate. --- .../src/controllers/SearchController.js | 92 +++-- .../test/controllers/SearchControllerSpec.js | 380 ++++++++++-------- 2 files changed, 250 insertions(+), 222 deletions(-) diff --git a/platform/search/src/controllers/SearchController.js b/platform/search/src/controllers/SearchController.js index 88c9663f51..629e495331 100644 --- a/platform/search/src/controllers/SearchController.js +++ b/platform/search/src/controllers/SearchController.js @@ -27,9 +27,6 @@ define(function () { "use strict"; - var INITIAL_LOAD_NUMBER = 20, - LOAD_INCREMENT = 20; - /** * Controller for search in Tree View. * @@ -50,9 +47,8 @@ define(function () { var controller = this; this.$scope = $scope; this.searchService = searchService; - this.numberToDisplay = INITIAL_LOAD_NUMBER; - this.fullResults = []; - this.filteredResults = []; + this.numberToDisplay = this.RESULTS_PER_PAGE; + this.availabileResults = 0; this.$scope.results = []; this.$scope.loading = false; this.pendingQuery = undefined; @@ -61,28 +57,30 @@ define(function () { }; } + SearchController.prototype.RESULTS_PER_PAGE = 20; + /** * Returns true if there are more results than currently displayed for the * for the current query and filters. */ SearchController.prototype.areMore = function () { - return this.$scope.results.length < this.filteredResults.length; + return this.$scope.results.length < this.availableResults; }; /** * Display more results for the currently displayed query and filters. */ SearchController.prototype.loadMore = function () { - this.numberToDisplay += LOAD_INCREMENT; - this.updateResults(); + this.numberToDisplay += this.RESULTS_PER_PAGE; + this.dispatchSearch(); }; /** - * Search for the query string specified in scope. + * Reset search results, then search for the query string specified in + * scope. */ SearchController.prototype.search = function () { - var inputText = this.$scope.ngModel.input, - controller = this; + var inputText = this.$scope.ngModel.input; this.clearResults(); @@ -96,51 +94,64 @@ define(function () { return; } - if (this.pendingQuery === inputText) { + this.dispatchSearch(); + }; + + /** + * Dispatch a search to the search service if it hasn't already been + * dispatched. + * + * @private + */ + SearchController.prototype.dispatchSearch = function () { + var inputText = this.$scope.ngModel.input, + controller = this, + queryId = inputText + this.numberToDisplay; + + if (this.pendingQuery === queryId) { return; // don't issue multiple queries for the same term. } - this.pendingQuery = inputText; + this.pendingQuery = queryId; this .searchService - .query(inputText, 60) // TODO: allow filter in search service. + .query(inputText, this.numberToDisplay, this.filterPredicate()) .then(function (results) { - if (controller.pendingQuery !== inputText) { + if (controller.pendingQuery !== queryId) { return; // another query in progress, so skip this one. } controller.onSearchComplete(results); }); }; + SearchController.prototype.filter = SearchController.prototype.onFilterChange; + /** * Refilter results and update visible results when filters have changed. */ SearchController.prototype.onFilterChange = function () { - this.filter(); - this.updateVisibleResults(); + this.pendingQuery = undefined; + this.search(); }; /** - * Filter `fullResults` based on currenly active filters, storing the result - * in `filteredResults`. + * Returns a predicate function that can be used to filter object models. * * @private */ - SearchController.prototype.filter = function () { - var includeTypes = this.$scope.ngModel.checked; - + SearchController.prototype.filterPredicate = function () { if (this.$scope.ngModel.checkAll) { - this.filteredResults = this.fullResults; - return; + return function () { + return true; + }; } - - this.filteredResults = this.fullResults.filter(function (hit) { - return includeTypes[hit.object.getModel().type]; - }); + var includeTypes = this.$scope.ngModel.checked; + return function (model) { + return !!includeTypes[model.type]; + }; }; - /** * Clear the search results. * @@ -148,35 +159,22 @@ define(function () { */ SearchController.prototype.clearResults = function () { this.$scope.results = []; - this.fullResults = []; - this.filteredResults = []; - this.numberToDisplay = INITIAL_LOAD_NUMBER; + this.availableResults = 0; + this.numberToDisplay = this.RESULTS_PER_PAGE; }; - /** * Update search results from given `results`. * * @private */ SearchController.prototype.onSearchComplete = function (results) { - this.fullResults = results.hits; - this.filter(); - this.updateVisibleResults(); + this.availableResults = results.total; + this.$scope.results = results.hits; this.$scope.loading = false; this.pendingQuery = undefined; }; - /** - * Update visible results from filtered results. - * - * @private - */ - SearchController.prototype.updateVisibleResults = function () { - this.$scope.results = - this.filteredResults.slice(0, this.numberToDisplay); - }; - return SearchController; }); diff --git a/platform/search/test/controllers/SearchControllerSpec.js b/platform/search/test/controllers/SearchControllerSpec.js index 720d9bd64a..c28d2c540f 100644 --- a/platform/search/test/controllers/SearchControllerSpec.js +++ b/platform/search/test/controllers/SearchControllerSpec.js @@ -4,12 +4,12 @@ * Administration. All rights reserved. * * Open MCT Web is licensed under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance with the License. + * 'License'); you may not use this file except in compliance with the License. * You may obtain a copy of the License at * http://www.apache.org/licenses/LICENSE-2.0. * * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * distributed under the License is distributed on an 'AS IS' BASIS, WITHOUT * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the * License for the specific language governing permissions and limitations * under the License. @@ -24,185 +24,215 @@ /** * SearchSpec. Created by shale on 07/31/2015. */ -define( - ["../../src/controllers/SearchController"], - function (SearchController) { - "use strict"; +define([ + '../../src/controllers/SearchController' +], function ( + SearchController +) { + 'use strict'; - // These should be the same as the ones on the top of the search controller - var INITIAL_LOAD_NUMBER = 20, - LOAD_INCREMENT = 20; - - describe("The search controller", function () { - var mockScope, - mockSearchService, - mockPromise, - mockSearchResult, - mockDomainObject, - mockTypes, - controller; + describe('The search controller', function () { + var mockScope, + mockSearchService, + mockPromise, + mockSearchResult, + mockDomainObject, + mockTypes, + controller; - function bigArray(size) { - var array = [], - i; - for (i = 0; i < size; i += 1) { - array.push(mockSearchResult); - } - return array; + function bigArray(size) { + var array = [], + i; + for (i = 0; i < size; i += 1) { + array.push(mockSearchResult); } - - - beforeEach(function () { - mockScope = jasmine.createSpyObj( - "$scope", - [ "$watch" ] - ); - mockScope.ngModel = {}; - mockScope.ngModel.input = "test input"; - mockScope.ngModel.checked = {}; - mockScope.ngModel.checked['mock.type'] = true; + return array; + } + + + beforeEach(function () { + mockScope = jasmine.createSpyObj( + '$scope', + [ '$watch' ] + ); + mockScope.ngModel = {}; + mockScope.ngModel.input = 'test input'; + mockScope.ngModel.checked = {}; + mockScope.ngModel.checked['mock.type'] = true; + mockScope.ngModel.checkAll = true; + + mockSearchService = jasmine.createSpyObj( + 'searchService', + [ 'query' ] + ); + mockPromise = jasmine.createSpyObj( + 'promise', + [ 'then' ] + ); + mockSearchService.query.andReturn(mockPromise); + + mockTypes = [{key: 'mock.type', name: 'Mock Type', glyph: '?'}]; + + mockSearchResult = jasmine.createSpyObj( + 'searchResult', + [ '' ] + ); + mockDomainObject = jasmine.createSpyObj( + 'domainObject', + [ 'getModel' ] + ); + mockSearchResult.object = mockDomainObject; + mockDomainObject.getModel.andReturn({name: 'Mock Object', type: 'mock.type'}); + + controller = new SearchController(mockScope, mockSearchService, mockTypes); + controller.search(); + }); + + it('has a default number of results per page', function () { + expect(controller.RESULTS_PER_PAGE).toBe(20); + }); + + it('sends queries to the search service', function () { + expect(mockSearchService.query).toHaveBeenCalledWith( + 'test input', + controller.RESULTS_PER_PAGE, + jasmine.any(Function) + ); + }); + + describe('filter query function', function () { + it('returns true when all types allowed', function () { mockScope.ngModel.checkAll = true; - - mockSearchService = jasmine.createSpyObj( - "searchService", - [ "query" ] - ); - mockPromise = jasmine.createSpyObj( - "promise", - [ "then" ] - ); - mockSearchService.query.andReturn(mockPromise); - - mockTypes = [{key: 'mock.type', name: 'Mock Type', glyph: '?'}]; - - mockSearchResult = jasmine.createSpyObj( - "searchResult", - [ "" ] - ); - mockDomainObject = jasmine.createSpyObj( - "domainObject", - [ "getModel" ] - ); - mockSearchResult.object = mockDomainObject; - mockDomainObject.getModel.andReturn({name: 'Mock Object', type: 'mock.type'}); - - controller = new SearchController(mockScope, mockSearchService, mockTypes); - controller.search(); - }); - - it("sends queries to the search service", function () { - expect(mockSearchService.query).toHaveBeenCalled(); - }); - - it("populates the results with results from the search service", function () { - expect(mockPromise.then).toHaveBeenCalledWith(jasmine.any(Function)); - mockPromise.then.mostRecentCall.args[0]({hits: []}); - - expect(mockScope.results).toBeDefined(); - }); - - it("is loading until the service's promise fufills", function () { - // Send query - controller.search(); - expect(mockScope.loading).toBeTruthy(); - - // Then resolve the promises - mockPromise.then.mostRecentCall.args[0]({hits: []}); - expect(mockScope.loading).toBeFalsy(); + controller.onFilterChange(); + var filterFn = mockSearchService.query.mostRecentCall.args[2]; + expect(filterFn('askbfa')).toBe(true); }); - - it("displays only some results when there are many", function () { - expect(mockPromise.then).toHaveBeenCalledWith(jasmine.any(Function)); - mockPromise.then.mostRecentCall.args[0]({hits: bigArray(100)}); - - expect(mockScope.results).toBeDefined(); - expect(mockScope.results.length).toBeLessThan(100); - }); - - it("detects when there are more results", function () { + it('returns true only for matching checked types', function () { mockScope.ngModel.checkAll = false; - - expect(mockPromise.then).toHaveBeenCalledWith(jasmine.any(Function)); - mockPromise.then.mostRecentCall.args[0]({ - hits: bigArray(INITIAL_LOAD_NUMBER + 5), - total: INITIAL_LOAD_NUMBER + 5 - }); - // bigArray gives searchResults of type 'mock.type' - mockScope.ngModel.checked['mock.type'] = false; - mockScope.ngModel.checked['mock.type.2'] = true; - - expect(controller.areMore()).toBeFalsy(); - - mockScope.ngModel.checked['mock.type'] = true; - - expect(controller.areMore()).toBeTruthy(); - }); - - it("can load more results", function () { - var oldSize; - - expect(mockPromise.then).toHaveBeenCalled(); - mockPromise.then.mostRecentCall.args[0]({ - hits: bigArray(INITIAL_LOAD_NUMBER + LOAD_INCREMENT + 1), - total: INITIAL_LOAD_NUMBER + LOAD_INCREMENT + 1 - }); - // These hits and total lengths are the case where the controller - // DOES NOT have to re-search to load more results - oldSize = mockScope.results.length; - - expect(controller.areMore()).toBeTruthy(); - - controller.loadMore(); - expect(mockScope.results.length).toBeGreaterThan(oldSize); - }); - - it("can re-search to load more results", function () { - var oldSize, - oldCallCount; - - expect(mockPromise.then).toHaveBeenCalled(); - mockPromise.then.mostRecentCall.args[0]({ - hits: bigArray(INITIAL_LOAD_NUMBER + LOAD_INCREMENT - 1), - total: INITIAL_LOAD_NUMBER + LOAD_INCREMENT + 1 - }); - // These hits and total lengths are the case where the controller - // DOES have to re-search to load more results - oldSize = mockScope.results.length; - oldCallCount = mockPromise.then.callCount; - expect(controller.areMore()).toBeTruthy(); - - controller.loadMore(); - expect(mockPromise.then).toHaveBeenCalled(); - // Make sure that a NEW call to search has been made - expect(oldCallCount).toBeLessThan(mockPromise.then.callCount); - mockPromise.then.mostRecentCall.args[0]({ - hits: bigArray(INITIAL_LOAD_NUMBER + LOAD_INCREMENT + 1), - total: INITIAL_LOAD_NUMBER + LOAD_INCREMENT + 1 - }); - expect(mockScope.results.length).toBeGreaterThan(oldSize); - }); - - it("sets the ngModel.search flag", function () { - // Flag should be true with nonempty input - expect(mockScope.ngModel.search).toEqual(true); - - // Flag should be flase with empty input - mockScope.ngModel.input = ""; - controller.search(); - mockPromise.then.mostRecentCall.args[0]({hits: [], total: 0}); - expect(mockScope.ngModel.search).toEqual(false); - - // Both the empty string and undefined should be 'empty input' - mockScope.ngModel.input = undefined; - controller.search(); - mockPromise.then.mostRecentCall.args[0]({hits: [], total: 0}); - expect(mockScope.ngModel.search).toEqual(false); - }); - - it("has a default results list to filter from", function () { - expect(mockScope.ngModel.filter()).toBeDefined(); + controller.onFilterChange(); + var filterFn = mockSearchService.query.mostRecentCall.args[2]; + expect(filterFn({type: 'mock.type'})).toBe(true); + expect(filterFn({type: 'other.type'})).toBe(false); }); }); - } -); \ No newline at end of file + + it('populates the results with results from the search service', function () { + expect(mockPromise.then).toHaveBeenCalledWith(jasmine.any(Function)); + mockPromise.then.mostRecentCall.args[0]({hits: ['a']}); + + expect(mockScope.results.length).toBe(1); + expect(mockScope.results).toContain('a'); + }); + + it('is loading until the service\'s promise fufills', function () { + expect(mockScope.loading).toBeTruthy(); + + // Then resolve the promises + mockPromise.then.mostRecentCall.args[0]({hits: []}); + expect(mockScope.loading).toBeFalsy(); + }); + + + xit('displays only some results when there are many', function () { + expect(mockPromise.then).toHaveBeenCalledWith(jasmine.any(Function)); + mockPromise.then.mostRecentCall.args[0]({hits: bigArray(100)}); + + expect(mockScope.results).toBeDefined(); + expect(mockScope.results.length).toBeLessThan(100); + }); + + it('detects when there are more results', function () { + mockPromise.then.mostRecentCall.args[0]({ + hits: bigArray(controller.RESULTS_PER_PAGE), + total: controller.RESULTS_PER_PAGE + 5 + }); + + expect(mockScope.results.length).toBe(controller.RESULTS_PER_PAGE); + expect(controller.areMore()).toBeTruthy; + + controller.loadMore(); + + expect(mockSearchService.query).toHaveBeenCalledWith( + 'test input', + controller.RESULTS_PER_PAGE * 2, + jasmine.any(Function) + ); + + mockPromise.then.mostRecentCall.args[0]({ + hits: bigArray(controller.RESULTS_PER_PAGE + 5), + total: controller.RESULTS_PER_PAGE + 5 + }); + + expect(mockScope.results.length) + .toBe(controller.RESULTS_PER_PAGE + 5); + + expect(controller.areMore()).toBe(false); + }); + + xit('can load more results', function () { + var oldSize; + + expect(mockPromise.then).toHaveBeenCalled(); + mockPromise.then.mostRecentCall.args[0]({ + hits: bigArray(INITIAL_LOAD_NUMBER + LOAD_INCREMENT + 1), + total: INITIAL_LOAD_NUMBER + LOAD_INCREMENT + 1 + }); + // These hits and total lengths are the case where the controller + // DOES NOT have to re-search to load more results + oldSize = mockScope.results.length; + + expect(controller.areMore()).toBeTruthy(); + + controller.loadMore(); + expect(mockScope.results.length).toBeGreaterThan(oldSize); + }); + + xit('can re-search to load more results', function () { + var oldSize, + oldCallCount; + + expect(mockPromise.then).toHaveBeenCalled(); + mockPromise.then.mostRecentCall.args[0]({ + hits: bigArray(INITIAL_LOAD_NUMBER + LOAD_INCREMENT - 1), + total: INITIAL_LOAD_NUMBER + LOAD_INCREMENT + 1 + }); + // These hits and total lengths are the case where the controller + // DOES have to re-search to load more results + oldSize = mockScope.results.length; + oldCallCount = mockPromise.then.callCount; + expect(controller.areMore()).toBeTruthy(); + + controller.loadMore(); + expect(mockPromise.then).toHaveBeenCalled(); + // Make sure that a NEW call to search has been made + expect(oldCallCount).toBeLessThan(mockPromise.then.callCount); + mockPromise.then.mostRecentCall.args[0]({ + hits: bigArray(INITIAL_LOAD_NUMBER + LOAD_INCREMENT + 1), + total: INITIAL_LOAD_NUMBER + LOAD_INCREMENT + 1 + }); + expect(mockScope.results.length).toBeGreaterThan(oldSize); + }); + + it('sets the ngModel.search flag', function () { + // Flag should be true with nonempty input + expect(mockScope.ngModel.search).toEqual(true); + + // Flag should be flase with empty input + mockScope.ngModel.input = ''; + controller.search(); + mockPromise.then.mostRecentCall.args[0]({hits: [], total: 0}); + expect(mockScope.ngModel.search).toEqual(false); + + // Both the empty string and undefined should be 'empty input' + mockScope.ngModel.input = undefined; + controller.search(); + mockPromise.then.mostRecentCall.args[0]({hits: [], total: 0}); + expect(mockScope.ngModel.search).toEqual(false); + }); + + it('attaches a filter function to scope', function () { + expect(mockScope.ngModel.filter).toEqual(jasmine.any(Function)); + }); + }); +}); From ce42429fbdd23bc5eb52cb542e0f3c1be87398f0 Mon Sep 17 00:00:00 2001 From: Pete Richards Date: Tue, 20 Oct 2015 15:14:43 -0700 Subject: [PATCH 13/18] [Search] expose constants, add fudge factor The SearchAggregator exposes it's constants to add stability to tests. It also has a fudge factor which increaases the number of results it requests from providers to better support pagination when using client side filtering. --- .../search/src/services/SearchAggregator.js | 23 +++++++++++++++---- .../test/services/SearchAggregatorSpec.js | 18 +++++++++++++-- 2 files changed, 35 insertions(+), 6 deletions(-) diff --git a/platform/search/src/services/SearchAggregator.js b/platform/search/src/services/SearchAggregator.js index 0cdcc5b922..40e77c271b 100644 --- a/platform/search/src/services/SearchAggregator.js +++ b/platform/search/src/services/SearchAggregator.js @@ -31,8 +31,6 @@ define([ ) { "use strict"; - var DEFAULT_MAX_RESULTS = 100; - /** * Aggregates multiple search providers as a singular search provider. * Search providers are expected to implement a `query` method which returns @@ -53,6 +51,23 @@ define([ this.providers = providers; } + /** + * If max results is not specified in query, use this as default. + */ + SearchAggregator.prototype.DEFAULT_MAX_RESULTS = 100; + + /** + * Because filtering isn't implemented inside each provider, the fudge + * factor is a multiplier on the number of results returned-- more results + * than requested will be fetched, and then will be fetched. This helps + * provide more predictable pagination when large numbers of matches exist + * but very few matches match filters. + * + * If a provider level filter implementation is implemented in the future, + * remove this. + */ + SearchAggregator.prototype.FUDGE_FACTOR = 5; + /** * Sends a query to each of the providers. Returns a promise for * a result object that has the format @@ -79,13 +94,13 @@ define([ resultPromises; if (!maxResults) { - maxResults = DEFAULT_MAX_RESULTS; + maxResults = this.DEFAULT_MAX_RESULTS; } resultPromises = this.providers.map(function (provider) { return provider.query( inputText, - maxResults + maxResults * aggregator.FUDGE_FACTOR ); }); diff --git a/platform/search/test/services/SearchAggregatorSpec.js b/platform/search/test/services/SearchAggregatorSpec.js index 044a5f4609..d63b672b2b 100644 --- a/platform/search/test/services/SearchAggregatorSpec.js +++ b/platform/search/test/services/SearchAggregatorSpec.js @@ -49,6 +49,13 @@ define([ aggregator = new SearchAggregator($q, objectService, providers); }); + it("has a fudge factor", function () { + expect(aggregator.FUDGE_FACTOR).toBe(5); + }); + + it("has default max results", function () { + expect(aggregator.DEFAULT_MAX_RESULTS).toBe(100); + }); it("can order model results by score", function () { var modelResults = { @@ -170,7 +177,11 @@ define([ providers.push(provider); aggregator.query('find me', 123, 'filter'); - expect(provider.query).toHaveBeenCalledWith('find me', 123); + expect(provider.query) + .toHaveBeenCalledWith( + 'find me', + 123 * aggregator.FUDGE_FACTOR + ); expect($q.all).toHaveBeenCalledWith(['i prooomise!']); }); @@ -181,7 +192,10 @@ define([ ); providers.push(provider); aggregator.query('find me'); - expect(provider.query).toHaveBeenCalledWith('find me', 100); + expect(provider.query).toHaveBeenCalledWith( + 'find me', + aggregator.DEFAULT_MAX_RESULTS * aggregator.FUDGE_FACTOR + ); }); it('can combine responses from multiple providers', function () { From fe3263fdfe8a1af779a37a4664ddd30c4e2380f4 Mon Sep 17 00:00:00 2001 From: Pete Richards Date: Tue, 20 Oct 2015 15:27:46 -0700 Subject: [PATCH 14/18] [Search] Remove invalid specs --- .../test/controllers/SearchControllerSpec.js | 55 +------------------ 1 file changed, 1 insertion(+), 54 deletions(-) diff --git a/platform/search/test/controllers/SearchControllerSpec.js b/platform/search/test/controllers/SearchControllerSpec.js index c28d2c540f..a755594d58 100644 --- a/platform/search/test/controllers/SearchControllerSpec.js +++ b/platform/search/test/controllers/SearchControllerSpec.js @@ -133,15 +133,6 @@ define([ expect(mockScope.loading).toBeFalsy(); }); - - xit('displays only some results when there are many', function () { - expect(mockPromise.then).toHaveBeenCalledWith(jasmine.any(Function)); - mockPromise.then.mostRecentCall.args[0]({hits: bigArray(100)}); - - expect(mockScope.results).toBeDefined(); - expect(mockScope.results.length).toBeLessThan(100); - }); - it('detects when there are more results', function () { mockPromise.then.mostRecentCall.args[0]({ hits: bigArray(controller.RESULTS_PER_PAGE), @@ -149,7 +140,7 @@ define([ }); expect(mockScope.results.length).toBe(controller.RESULTS_PER_PAGE); - expect(controller.areMore()).toBeTruthy; + expect(controller.areMore()).toBeTruthy(); controller.loadMore(); @@ -170,50 +161,6 @@ define([ expect(controller.areMore()).toBe(false); }); - xit('can load more results', function () { - var oldSize; - - expect(mockPromise.then).toHaveBeenCalled(); - mockPromise.then.mostRecentCall.args[0]({ - hits: bigArray(INITIAL_LOAD_NUMBER + LOAD_INCREMENT + 1), - total: INITIAL_LOAD_NUMBER + LOAD_INCREMENT + 1 - }); - // These hits and total lengths are the case where the controller - // DOES NOT have to re-search to load more results - oldSize = mockScope.results.length; - - expect(controller.areMore()).toBeTruthy(); - - controller.loadMore(); - expect(mockScope.results.length).toBeGreaterThan(oldSize); - }); - - xit('can re-search to load more results', function () { - var oldSize, - oldCallCount; - - expect(mockPromise.then).toHaveBeenCalled(); - mockPromise.then.mostRecentCall.args[0]({ - hits: bigArray(INITIAL_LOAD_NUMBER + LOAD_INCREMENT - 1), - total: INITIAL_LOAD_NUMBER + LOAD_INCREMENT + 1 - }); - // These hits and total lengths are the case where the controller - // DOES have to re-search to load more results - oldSize = mockScope.results.length; - oldCallCount = mockPromise.then.callCount; - expect(controller.areMore()).toBeTruthy(); - - controller.loadMore(); - expect(mockPromise.then).toHaveBeenCalled(); - // Make sure that a NEW call to search has been made - expect(oldCallCount).toBeLessThan(mockPromise.then.callCount); - mockPromise.then.mostRecentCall.args[0]({ - hits: bigArray(INITIAL_LOAD_NUMBER + LOAD_INCREMENT + 1), - total: INITIAL_LOAD_NUMBER + LOAD_INCREMENT + 1 - }); - expect(mockScope.results.length).toBeGreaterThan(oldSize); - }); - it('sets the ngModel.search flag', function () { // Flag should be true with nonempty input expect(mockScope.ngModel.search).toEqual(true); From 77d81f899bf2c1fb91d03bfc24399d8ba0f505a8 Mon Sep 17 00:00:00 2001 From: Pete Richards Date: Tue, 20 Oct 2015 15:31:33 -0700 Subject: [PATCH 15/18] [Style] JSLint compliance --- .../elastic/src/ElasticSearchProvider.js | 13 ++++++++----- .../test/services/GenericSearchProviderSpec.js | 15 ++++++++------- .../search/test/services/SearchAggregatorSpec.js | 2 +- 3 files changed, 17 insertions(+), 13 deletions(-) diff --git a/platform/persistence/elastic/src/ElasticSearchProvider.js b/platform/persistence/elastic/src/ElasticSearchProvider.js index e42cd6553d..5523ebe7bd 100644 --- a/platform/persistence/elastic/src/ElasticSearchProvider.js +++ b/platform/persistence/elastic/src/ElasticSearchProvider.js @@ -31,7 +31,10 @@ define([ ) { "use strict"; - var DEFAULT_MAX_RESULTS = 100; + var DEFAULT_MAX_RESULTS = 100, + ID_PROPERTY = '_id', + SOURCE_PROPERTY = '_source', + SCORE_PROPERTY = '_score'; /** * A search service which searches through domain objects in @@ -133,15 +136,15 @@ define([ var results = response.data.hits.hits, searchResults = results.map(function (result) { return { - id: result['_id'], - model: result['_source'], - score: result['_score'] + id: result[ID_PROPERTY], + model: result[SOURCE_PROPERTY], + score: result[SCORE_PROPERTY] }; }); return { hits: searchResults, - total: response.data.hits.total, + total: response.data.hits.total }; }; diff --git a/platform/search/test/services/GenericSearchProviderSpec.js b/platform/search/test/services/GenericSearchProviderSpec.js index fb3d595345..b958657e01 100644 --- a/platform/search/test/services/GenericSearchProviderSpec.js +++ b/platform/search/test/services/GenericSearchProviderSpec.js @@ -19,7 +19,8 @@ * this source code distribution or the Licensing information page available * at runtime from the About dialog for additional information. *****************************************************************************/ -/*global define,describe,it,expect,beforeEach,jasmine*/ +/*global define,describe,it,expect,beforeEach,jasmine,Promise,spyOn,waitsFor, + runs*/ /** * SearchSpec. Created by shale on 07/31/2015. @@ -126,12 +127,12 @@ define([ }); it('tracks ids to index', function () { - expect(provider.indexedIds['a']).not.toBeDefined(); - expect(provider.pendingIndex['a']).not.toBeDefined(); + expect(provider.indexedIds.a).not.toBeDefined(); + expect(provider.pendingIndex.a).not.toBeDefined(); expect(provider.idsToIndex).not.toContain('a'); provider.scheduleForIndexing('a'); - expect(provider.indexedIds['a']).toBeDefined(); - expect(provider.pendingIndex['a']).toBeDefined(); + expect(provider.indexedIds.a).toBeDefined(); + expect(provider.pendingIndex.a).toBeDefined(); expect(provider.idsToIndex).toContain('a'); }); @@ -214,7 +215,7 @@ define([ provider.beginIndexRequest(); waitsFor(function () { return provider.pendingRequests === 0; - }) + }); runs(function () { expect(provider.index) .toHaveBeenCalledWith('abc', models.abc); @@ -224,7 +225,7 @@ define([ it('does not error if no objects queued', function () { provider.idsToIndex = []; expect(function () { - provider.beginIndexRequest() + provider.beginIndexRequest(); }).not.toThrow(); }); }); diff --git a/platform/search/test/services/SearchAggregatorSpec.js b/platform/search/test/services/SearchAggregatorSpec.js index d63b672b2b..f8bee0dcc0 100644 --- a/platform/search/test/services/SearchAggregatorSpec.js +++ b/platform/search/test/services/SearchAggregatorSpec.js @@ -45,7 +45,7 @@ define([ 'objectService', ['getObjects'] ); - providers = [], + providers = []; aggregator = new SearchAggregator($q, objectService, providers); }); From 9a63e997108000278b973c2128afc91309bac2b8 Mon Sep 17 00:00:00 2001 From: Pete Richards Date: Tue, 20 Oct 2015 16:01:42 -0700 Subject: [PATCH 16/18] [Search] Add spec for ElasticSearchProvider Add spec coverage for ElasticSearchProvider. Also remove unneeded guards for max number of results, as the aggregator will always provide a max number of results. --- platform/persistence/elastic/bundle.json | 2 +- .../elastic/src/ElasticSearchProvider.js | 17 +- .../elastic/test/ElasticSearchProviderSpec.js | 145 +++++++++++++++++- 3 files changed, 147 insertions(+), 17 deletions(-) diff --git a/platform/persistence/elastic/bundle.json b/platform/persistence/elastic/bundle.json index f78d8504f2..3e1383351e 100644 --- a/platform/persistence/elastic/bundle.json +++ b/platform/persistence/elastic/bundle.json @@ -13,7 +13,7 @@ "provides": "searchService", "type": "provider", "implementation": "ElasticSearchProvider.js", - "depends": [ "$http", "objectService", "ELASTIC_ROOT" ] + "depends": [ "$http", "ELASTIC_ROOT" ] } ], "constants": [ diff --git a/platform/persistence/elastic/src/ElasticSearchProvider.js b/platform/persistence/elastic/src/ElasticSearchProvider.js index 5523ebe7bd..84290d999a 100644 --- a/platform/persistence/elastic/src/ElasticSearchProvider.js +++ b/platform/persistence/elastic/src/ElasticSearchProvider.js @@ -31,8 +31,7 @@ define([ ) { "use strict"; - var DEFAULT_MAX_RESULTS = 100, - ID_PROPERTY = '_id', + var ID_PROPERTY = '_id', SOURCE_PROPERTY = '_source', SCORE_PROPERTY = '_score'; @@ -42,14 +41,11 @@ define([ * * @constructor * @param $http Angular's $http service, for working with urls. - * @param {ObjectService} objectService the service from which - * domain objects can be gotten. * @param ROOT the constant `ELASTIC_ROOT` which allows us to * interact with ElasticSearch. */ - function ElasticSearchProvider($http, objectService, ROOT) { + function ElasticSearchProvider($http, ROOT) { this.$http = $http; - this.objectService = objectService; this.root = ROOT; } @@ -65,10 +61,6 @@ define([ params = {}, provider = this; - if (!maxResults) { - maxResults = DEFAULT_MAX_RESULTS; - } - searchTerm = this.cleanTerm(searchTerm); searchTerm = this.fuzzyMatchUnquotedTerms(searchTerm); @@ -102,7 +94,7 @@ define([ * @returns {string} search terms cleaned of excess whitespace. */ ElasticSearchProvider.prototype.cleanTerm = function (term) { - return term.trim().replace(/ +/, ' '); + return term.trim().replace(/ +/g, ' '); }; /** @@ -121,7 +113,8 @@ define([ return query .replace(matcher, '~ ') - .replace('"~', '"'); + .replace(/$/, '~') + .replace(/"~+/, '"'); }; /** diff --git a/platform/persistence/elastic/test/ElasticSearchProviderSpec.js b/platform/persistence/elastic/test/ElasticSearchProviderSpec.js index af515e9431..f8337e0862 100644 --- a/platform/persistence/elastic/test/ElasticSearchProviderSpec.js +++ b/platform/persistence/elastic/test/ElasticSearchProviderSpec.js @@ -19,14 +19,151 @@ * this source code distribution or the Licensing information page available * at runtime from the About dialog for additional information. *****************************************************************************/ -/*global define,describe,it,expect,beforeEach,jasmine*/ +/*global define,describe,it,expect,beforeEach,jasmine,spyOn,Promise,waitsFor*/ /** * SearchSpec. Created by shale on 07/31/2015. */ define([ - "../src/ElasticSearchProvider" -], function (ElasticSearchProvider) { - "use strict"; + '../src/ElasticSearchProvider' +], function ( + ElasticSearchProvider +) { + 'use strict'; + + describe('ElasticSearchProvider', function () { + var $http, + ROOT, + provider; + + beforeEach(function () { + $http = jasmine.createSpy('$http'); + ROOT = 'http://localhost:9200'; + + provider = new ElasticSearchProvider($http, ROOT); + }); + + describe('query', function () { + beforeEach(function () { + spyOn(provider, 'cleanTerm').andReturn('cleanedTerm'); + spyOn(provider, 'fuzzyMatchUnquotedTerms').andReturn('fuzzy'); + spyOn(provider, 'parseResponse').andReturn('parsedResponse'); + $http.andReturn(Promise.resolve({})); + }); + + it('cleans terms and adds fuzzyness', function () { + provider.query('hello', 10); + expect(provider.cleanTerm).toHaveBeenCalledWith('hello'); + expect(provider.fuzzyMatchUnquotedTerms) + .toHaveBeenCalledWith('cleanedTerm'); + }); + + it('calls through to $http', function () { + provider.query('hello', 10); + expect($http).toHaveBeenCalledWith({ + method: 'GET', + params: { + q: 'fuzzy', + size: 10 + }, + url: 'http://localhost:9200/_search/' + }); + }); + + it('gracefully fails when http fails', function () { + var promiseChainResolved = false; + $http.andReturn(Promise.reject()); + + provider + .query('hello', 10) + .then(function (results) { + expect(results).toEqual({ + hits: [], + total: 0 + }); + promiseChainResolved = true; + }); + + waitsFor(function () { + return promiseChainResolved; + }); + }); + + it('parses and returns when http succeeds', function () { + var promiseChainResolved = false; + $http.andReturn(Promise.resolve('successResponse')); + + provider + .query('hello', 10) + .then(function (results) { + expect(provider.parseResponse) + .toHaveBeenCalledWith('successResponse'); + expect(results).toBe('parsedResponse'); + promiseChainResolved = true; + }); + + waitsFor(function () { + return promiseChainResolved; + }); + }); + }); + + it('can clean terms', function () { + expect(provider.cleanTerm(' asdhs ')).toBe('asdhs'); + expect(provider.cleanTerm(' and some words')) + .toBe('and some words'); + expect(provider.cleanTerm('Nice input')).toBe('Nice input'); + }); + + it('can create fuzzy term matchers', function () { + expect(provider.fuzzyMatchUnquotedTerms('pwr dvc 43')) + .toBe('pwr~ dvc~ 43~'); + + expect(provider.fuzzyMatchUnquotedTerms( + 'hello welcome "to quoted village" have fun' + )).toBe( + 'hello~ welcome~ "to quoted village" have~ fun~' + ); + }); + + it('can parse responses', function () { + var elasticSearchResponse = { + data: { + hits: { + total: 2, + hits: [ + { + '_id': 'hit1Id', + '_source': 'hit1Model', + '_score': 0.56 + }, + { + '_id': 'hit2Id', + '_source': 'hit2Model', + '_score': 0.34 + } + ] + } + } + }; + + expect(provider.parseResponse(elasticSearchResponse)) + .toEqual({ + hits: [ + { + id: 'hit1Id', + model: 'hit1Model', + score: 0.56 + }, + { + id: 'hit2Id', + model: 'hit2Model', + score: 0.34 + } + ], + total: 2 + }); + }); + }); }); From 833f57e284d3f68cb70dd49ab7e27f20c7610c4c Mon Sep 17 00:00:00 2001 From: Pete Richards Date: Wed, 21 Oct 2015 07:39:59 -0700 Subject: [PATCH 17/18] [Search] Don't block UI between requests Timeout subsequent calls to keepIndexing at the end of a indexRequest, so that UI operations are not blocked. --- .../src/services/GenericSearchProvider.js | 21 ++++++------ .../services/GenericSearchProviderSpec.js | 33 ++++++++++++++----- 2 files changed, 35 insertions(+), 19 deletions(-) diff --git a/platform/search/src/services/GenericSearchProvider.js b/platform/search/src/services/GenericSearchProvider.js index e1c90f3f51..2b99abcae2 100644 --- a/platform/search/src/services/GenericSearchProvider.js +++ b/platform/search/src/services/GenericSearchProvider.js @@ -19,7 +19,7 @@ * this source code distribution or the Licensing information page available * at runtime from the About dialog for additional information. *****************************************************************************/ -/*global define*/ +/*global define,setTimeout*/ /** * Module defining GenericSearchProvider. Created by shale on 07/16/2015. @@ -62,6 +62,8 @@ define([ ROOTS.forEach(function indexRoot(rootId) { provider.scheduleForIndexing(rootId); }); + + } /** @@ -148,8 +150,10 @@ define([ * @private */ GenericSearchProvider.prototype.keepIndexing = function () { - if (this.pendingRequests < this.MAX_CONCURRENT_REQUESTS) { - this.beginIndexRequest(); + while (this.pendingRequests < this.MAX_CONCURRENT_REQUESTS && + this.idsToIndex.length + ) { + this.beginIndexRequest(); } }; @@ -188,10 +192,6 @@ define([ var idToIndex = this.idsToIndex.shift(), provider = this; - if (!idToIndex) { - return; - } - this.pendingRequests += 1; this.modelService .getModels([idToIndex]) @@ -206,8 +206,10 @@ define([ .warn('Failed to index domain object ' + idToIndex); }) .then(function () { - provider.pendingRequests -= 1; - provider.keepIndexing(); + setTimeout(function () { + provider.pendingRequests -= 1; + provider.keepIndexing(); + }, 0); }); }; @@ -235,7 +237,6 @@ define([ }; }); - pendingQuery.resolve(modelResults); delete this.pendingQueries[event.data.queryId]; }; diff --git a/platform/search/test/services/GenericSearchProviderSpec.js b/platform/search/test/services/GenericSearchProviderSpec.js index b958657e01..7b8e52274b 100644 --- a/platform/search/test/services/GenericSearchProviderSpec.js +++ b/platform/search/test/services/GenericSearchProviderSpec.js @@ -143,17 +143,38 @@ define([ }); describe('keepIndexing', function () { - it('kicks off an index request when not at maximum', function () { - spyOn(provider, 'beginIndexRequest'); - provider.pendingRequests = 0; + it('calls beginIndexRequest until at maximum', function () { + spyOn(provider, 'beginIndexRequest').andCallThrough(); + provider.pendingRequests = 9; + provider.idsToIndex = ['a', 'b', 'c']; provider.MAX_CONCURRENT_REQUESTS = 10; provider.keepIndexing(); expect(provider.beginIndexRequest).toHaveBeenCalled(); + expect(provider.beginIndexRequest.calls.length).toBe(1); + }); + + it('calls beginIndexRequest for all ids to index', function () { + spyOn(provider, 'beginIndexRequest').andCallThrough(); + provider.pendingRequests = 0; + provider.idsToIndex = ['a', 'b', 'c']; + provider.MAX_CONCURRENT_REQUESTS = 10; + provider.keepIndexing(); + expect(provider.beginIndexRequest).toHaveBeenCalled(); + expect(provider.beginIndexRequest.calls.length).toBe(3); }); it('does not index when at capacity', function () { spyOn(provider, 'beginIndexRequest'); provider.pendingRequests = 10; + provider.idsToIndex.push('a'); + provider.MAX_CONCURRENT_REQUESTS = 10; + provider.keepIndexing(); + expect(provider.beginIndexRequest).not.toHaveBeenCalled(); + }); + + it('does not index when no ids to index', function () { + spyOn(provider, 'beginIndexRequest'); + provider.pendingRequests = 0; provider.MAX_CONCURRENT_REQUESTS = 10; provider.keepIndexing(); expect(provider.beginIndexRequest).not.toHaveBeenCalled(); @@ -222,12 +243,6 @@ define([ }); }); - it('does not error if no objects queued', function () { - provider.idsToIndex = []; - expect(function () { - provider.beginIndexRequest(); - }).not.toThrow(); - }); }); From 496cf85b7e8b55f5b432444bd63058e0b0a93d5a Mon Sep 17 00:00:00 2001 From: Pete Richards Date: Wed, 21 Oct 2015 09:46:32 -0700 Subject: [PATCH 18/18] [JSDoc] Correct mistake --- platform/search/src/services/SearchAggregator.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/platform/search/src/services/SearchAggregator.js b/platform/search/src/services/SearchAggregator.js index 40e77c271b..00988f81a8 100644 --- a/platform/search/src/services/SearchAggregator.js +++ b/platform/search/src/services/SearchAggregator.js @@ -59,9 +59,9 @@ define([ /** * Because filtering isn't implemented inside each provider, the fudge * factor is a multiplier on the number of results returned-- more results - * than requested will be fetched, and then will be fetched. This helps - * provide more predictable pagination when large numbers of matches exist - * but very few matches match filters. + * than requested will be fetched, and then will be filtered. This helps + * provide more predictable pagination when large numbers of results are + * returned but very few results match filters. * * If a provider level filter implementation is implemented in the future, * remove this.