Add search #352

jimlambie · 2017-10-31T18:31:47Z

Description

This PR introduces a search endpoint and indexing routine.

Configuration

A search block must exist in the configuration file:

"search": {
  "enabled": true,
  "minQueryLength": 3,
  "datastore": "@dadi/api-mongodb",
  "database": "search"
}

minQueryLength

The number of characters required before a search can be executed

datastore

The data connector to use to store search collections. Currently supports @dadi/api-mongodb only.

database

The database to store words and search instances

Indexable fields

To enable indexing you must specify a search block for each field you'd like indexed within the collection schema, including the weight:

{
  "fields": {
    "title": {
      "type": "String",
      "label": "Title",
      "search": {
        "weight": 2
      }
  }
}

Running a query

Query an indexed collection by adding /search to the collection's endpoint and include a q parameter in the querystring:

https://somedomain.com/1.0/my-db/books/search?q=harry wizard

Field filters can be applied in the same way as collection filtering"

https://somedomain.com/1.0/my-db/books/search?q=harry wizard&fields={"title": 1}

…arch # Conflicts: # dadi/lib/controller/index.js # dadi/lib/model/index.js # dadi/lib/search/index.js # package.json # test/acceptance/db-connection.js # test/acceptance/search_collections.js # workspace/collections/vjoin/testdb/collection.books.json

…arch # Conflicts: # README.md # dadi/lib/index.js # dadi/lib/model/delete.js # dadi/lib/model/index.js # dadi/lib/model/update.js # dadi/lib/search/index.js # package.json # test/acceptance/search_collections.js # test/test-connector/index.js

eduardoboucas · 2018-07-17T09:35:02Z

config.js

+      default: 3
+    },
+    wordCollection: {
+      doc: '',


eduardoboucas · 2018-07-17T09:35:12Z

config.js

+      default: 'words'
+    },
+    datastore: {
+      doc: "",


eduardoboucas · 2018-07-17T09:35:20Z

config.js

+      default: '@dadi/api-mongodb'
+    },
+    database: {
+      doc: '',


eduardoboucas · 2018-07-17T09:36:08Z

dadi/lib/controller/index.js

+  let queryOptions = this._prepareQueryOptions(options)
+
+  if (queryOptions.errors.length !== 0) {
+    sendBackJSON(400, res, next)(null, queryOptions)


In this case, don't we want to return?

eduardoboucas · 2018-07-17T09:37:13Z

dadi/lib/controller/index.js

+      })
+
+      return help.sendBackJSON(200, res, next)(null, results)
+    }).catch(error => {


I believe you don't need this catch, since you have one on the parent Promise.

eduardoboucas · 2018-07-17T09:41:47Z

dadi/lib/search/index.js

+ * @param {Object} options - options to use in the query
+ * @return {Promise}
+ */
+// Search.prototype.insert = function (datastore, data, collection, schema, options = {}) {


Is this meant to be commented out?

eduardoboucas · 2018-07-17T09:42:07Z

dadi/lib/search/index.js

+  if (!Object.keys(this.indexableFields).length) return
+
+  let skip = (page - 1) * limit
+  console.log(`Indexing page ${page} (${limit} per page)`)


Should this be a debug()?

eduardoboucas · 2018-07-17T09:42:38Z

dadi/lib/search/index.js

+    settings: this.model.settings
+  }).then(results => {
+    if (results.results && results.results.length) {
+      console.log(`Indexed ${results.results.length} ${results.results.length === 1 ? 'record' : 'records'} for ${this.model.name}`)


Should this be a debug()?

eduardoboucas · 2018-07-17T09:42:43Z

dadi/lib/search/index.js

+
+      if (results.results.length > 0) {
+        this.index(results.results).then(response => {
+          console.log(`Indexed page ${options.page}/${results.metadata.totalPages}`)


Should this be a debug()?

eduardoboucas · 2018-07-17T09:44:48Z

test/mocha.opts

@@ -1,4 +1,3 @@
--bail


Can we not commit this change? I think it's useful if tests fail early in the CI.

…arch # Conflicts: # CHANGELOG.md

coveralls · 2018-07-17T16:28:24Z

Pull Request Test Coverage Report for Build 1472

257 of 279 (92.11%) changed or added relevant lines in 9 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.06%) to 89.018%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
dadi/lib/controller/index.js	20	21	95.24%
dadi/lib/search/analysers/standard.js	49	51	96.08%
dadi/lib/search/index.js	154	160	96.25%
dadi/lib/controller/searchIndex.js	8	21	38.1%

Totals
Change from base Build 1460:	0.06%
Covered Lines:	3507
Relevant Lines:	3830

💛 - Coveralls

…arch # Conflicts: # CHANGELOG.md # config/mongodb.test.json # package.json

eduardoboucas · 2018-07-25T12:12:39Z

dadi/lib/model/search.js

+  let err
+
+  if (typeof options === 'function') {
+    // done = options


This can go, right?

eduardoboucas · 2018-07-25T12:14:05Z

dadi/lib/search/analysers/standard.js

+
+  areValidWords (words) {
+    return Array.isArray(words) &&
+    words.every(word => {


Indentation here looks a bit off?

eduardoboucas · 2018-07-25T12:15:03Z

dadi/lib/search/index.js

+ * N.B. May only be used with the MongoDB Data Connector.
+ */
+const Search = function (model) {
+  if (!model || model.constructor.name !== 'Model') throw new Error('model should be an instance of Model')


New line? 😬

eduardoboucas · 2018-07-25T12:16:51Z

dadi/lib/search/index.js

+        }
+      }
+    }).catch(err => {
+      console.log(err)


What happens if we go into this catch? We're swallowing the error, so doesn't the request hang?

return Promise.reject(err) <- ?

Unless we want to handle this error in a special way, I would just remove the catch and let the error be handled upstream. But it needs confirmation (and ideally a unit test), I'm just being that annoying guy that points potential issues just by looking at the code.

eduardoboucas · 2018-07-25T12:18:39Z

dadi/lib/search/index.js

+  }
+
+  if (!Array.isArray(documents)) {
+    return


Ignore if this doesn't make sense, but is it okay to be returning undefined here where the other exit routes return a Promise? i.e. isn't there a risk that we'll add a .then() somewhere upstream, which will throw an error if the subject is undefined?

I don't actually see us calling this method from anywhere.

@mingard any pointers to where I might find this being called?

eduardoboucas · 2018-07-25T12:18:52Z

dadi/lib/search/index.js

+
+  debug('deleting documents from the %s index', this.searchCollection)
+
+  let deleteQueue = documents.map(document => this.clearDocumentInstances(document._id.toString()))


New line? 😬

Presumably you mean:

let deleteQueue = documents.map(document => { this.clearDocumentInstances(document._id.toString()) })

let deleteQueue = documents.map(document => { return this.clearDocumentInstances(document._id.toString()) })

(You were missing the return)

eduardoboucas · 2018-07-25T12:20:17Z

dadi/lib/search/index.js

+Search.prototype.getIndexableFields = function () {
+  let schema = this.model.schema
+
+  return Object.assign({}, ...Object.keys(schema)


I find this return statement super confusing 😞

eduardoboucas · 2018-07-25T12:22:19Z

dadi/lib/search/index.js

+Search.prototype.removeNonIndexableFields = function (document) {
+  if (typeof document !== 'object') return {}
+
+  return Object.assign({}, ...Object.keys(document)


I find this return statement super confusing 😞

eduardoboucas · 2018-07-25T12:23:53Z

dadi/lib/search/index.js

+    settings: this.getWordSchema().settings
+  }).then(results => {
+    // Get all word instances from Analyser
+    this.clearDocumentInstances(docId).then(response => {


Shouldn't we return this.clearDocumentInstances(docId)?

eduardoboucas · 2018-07-25T12:27:10Z

dadi/lib/search/index.js

+    options: options,
+    schema: this.model.schema,
+    settings: this.model.settings
+  }).then(results => {


Minor thing: perhaps we could use object destructuring here to avoid writing results.results, so:

// (...) }).then(({metadata, results}) => { if (results && results.length) { // (...)

…arch

…ture/search

eduardoboucas · 2018-08-01T10:35:27Z

dadi/lib/controller/searchIndex.js

+  }
+
+  // 404 if Search is not enabled
+  if (config.get('search.enabled') === false) {


Should we be more strict and do if (config.get('search.enabled') !== true) {?

eduardoboucas · 2018-08-01T10:38:18Z

dadi/lib/search/index.js

+    return Promise.resolve()
+  }
+
+  if (!Array.isArray(documents)) {


Could we merge this with the first if, perhaps?

jimlambie added 5 commits October 31, 2017 18:31

feat: add search files

59bda75

tests: skip old search methods

1151d1b

tests: add missing helper method

2eddb70

tests: remove old search test file

162a596

tests: complete batchIndex tests

61fce48

jimlambie requested a review from eduardoboucas November 1, 2017 08:00

eduardoboucas added the status: blocked label Apr 5, 2018

eduardoboucas changed the title ~~Actual Search~~ Add search Apr 5, 2018

eduardoboucas mentioned this pull request Apr 5, 2018

Search acceptance tests #337

Closed

abovedave mentioned this pull request Jun 1, 2018

Search dadi/docs#25

Closed

jimlambie added 5 commits June 8, 2018 11:37

refactor: make search ready for future API changes

8cd10a3

refactor: remove search controller

c8fe827

refactor: merge develop intp search

ea98f16

feat: reimplement search

c00efe3

jimlambie added status: needs docs status: needs tests labels Jun 11, 2018

jimlambie added 3 commits June 12, 2018 13:18

test: update search unit tests; add search method to test connector

8a39a3e

Merge branch 'master' of https://github.com/dadi/api into feature/search

a3b2916

eduardoboucas reviewed Jul 17, 2018

View reviewed changes

jimlambie and others added 8 commits July 17, 2018 13:28

test: improve acceptance tests with latest search integration

57588e8

Merge branch 'develop' of https://github.com/dadi/api into feature/se…

c356084

…arch # Conflicts: # CHANGELOG.md

fix: update config schema docs

6257370

refactor: remove unnecessary catch

5338d85

chore(package): update logger dependency

e77a651

fix: fix DELETE roles permissions

ebd5e67

test: fix test

fa99d41

fix: unregister correct routes in documents controller

ebc8e7a

eduardoboucas and others added 6 commits July 17, 2018 17:31

test: add debug code

3b7f31c

test: fix test

ae2aa98

test: add debug code

a78a769

test: remove debug code from test

da52dd3

Merge branch 'develop' of https://github.com/dadi/api into feature/se…

ae17597

…arch # Conflicts: # CHANGELOG.md # config/mongodb.test.json # package.json

refactor: test connector refactor

01c0dcf

eduardoboucas reviewed Jul 25, 2018

View reviewed changes

jimlambie added 7 commits July 25, 2018 16:22

feat: include language parameter in search

b0064f2

refactor: remove promise catch

4786c10

fix: return acl error correctly

90bc3be

Merge branch 'develop' of https://github.com/dadi/api into feature/se…

a828d33

…arch

fix: use regex tokenizer for accented characters

af244b6

Merge branch 'develop' of https://github.com/dadi/api into feature/se…

b7d2a66

…arch

fix: analyse words for current document only

906146e

jimlambie removed status: blocked status: needs tests labels Jul 31, 2018

jimlambie added 5 commits July 31, 2018 16:42

Merge branch 'develop' into feature/search

10824df

feat: add indexing endpoint

49c51a9

Merge branch 'feature/search' of https://github.com/dadi/api into fea…

9c5a6c7

…ture/search

chore: package lock

d229583

chore: update config docs

d74df56

eduardoboucas reviewed Aug 1, 2018

View reviewed changes

jimlambie added 3 commits August 1, 2018 11:42

chore: update data connector requirement

214d495

refactor: slight logic modifications

d5949a0

chore: move metadata dependency

2ae7ac2

jimlambie merged commit 60230f4 into develop Aug 1, 2018

jimlambie deleted the feature/search branch August 1, 2018 11:10


		debug('deleting documents from the %s index', this.searchCollection)

		let deleteQueue = documents.map(document => this.clearDocumentInstances(document._id.toString()))

Add search #352

Add search #352

Uh oh!

Conversation

jimlambie commented Oct 31, 2017

Description

Configuration

minQueryLength

datastore

database

Indexable fields

Running a query

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jul 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1472

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jul 17, 2018 •

edited

Loading