Fix SearchResult.next with sort/size (#508)

## What does this PR do?

:warning: Depends on kuzzleio/kuzzle#1600

Fix few bugs on the `SearchResult` pagination with `sort` and `size`.  

The SDK now support every kind of ES sort:
 - string value
 - array of string
 - array of object

Errors are now thrown when:
 - the sort is invalid
 - the sort combination does not identify one item only

### Other changes

 - the `next` method now return rejected promises instead of throwing (inconsistent behavior that can lead to unhandled rejection)
 - change `_uid` by `_id` because this field does not longer exists in ES 7.x

Author: Aschen

doc/7/core-classes/search-result/next/index.md

@@ -20,9 +20,9 @@ next();
 
 Resolves to a `SearchResult` object, or to `null` if no more pages are available.
 
-## Throw
+## Rejects
 
-This method throws an exception if:
+This method returns a rejected promise with an error if:
 
 - No pagination strategy can be applied (see below)
 - If invoking it would lead to more than 10 000 items being retrieved with the `from/size` strategy
@@ -56,19 +56,26 @@ You can restrict the scroll session maximum duration under the `services.storage
 
 If the initial search contains `sort` and `size` parameters, the `next` method retrieves the next page of results following the sort order, the last item of the current page acting as a live cursor.
 
-To avoid too many duplicates, it is advised to provide a sort combination that will always identify one item only. The recommended way is to use the field `_uid` which is certain to contain one unique value for each document.
+This strategy uses Elasticsearch [search_after](https://www.elastic.co/guide/en/elasticsearch/reference/7.4/search-request-body.html#request-body-search-search-after) parameter.
+
+::: warning
+You have to provide a sort combination that will always identify one item only. The recommended way is to use the field `_id` which is certain to contain one unique value for each document.
+To prevent partial retrieval of results, the SDK will reject with an error if the sort combination can identify multiple items.
+:::
 
 Because this method does not freeze the search results between two calls, there can be missing or duplicated documents between two result pages.
 
 This method efficiently mitigates the costs of scroll searches, but returns less consistent results: it's a middle ground, ideal for real-time search requests.
 
+<<< ./snippets/sortsize.js
+
 ### Strategy: from / size
 
 If the initial search contains `from` and `size` parameters, the `next` method retrieves the next page of result by incrementing the `from` offset.
 
 Because this method does not freeze the search results between two calls, there can be missing or duplicated documents between two result pages.
 
 It's the fastest pagination method available, but also the less consistent, and it is not possible to retrieve more than 10000 items using it.  
-Above that limit, any call to `next` throws an Exception.
+Above that limit, any call to `next` will return a rejected promise with an error.
 
 <<< ./snippets/fromsize.js

doc/7/core-classes/search-result/next/snippets/sortsize.js

@@ -0,0 +1,47 @@
+try {
+  const documents = [];
+
+  for (let i = 0; i < 100; i++) {
+    documents.push({ _id: `suv_no${i}`, body: { category: 'suv' } });
+  }
+
+  await kuzzle.document.mCreate('nyc-open-data', 'yellow-taxi', documents, {
+    refresh: 'wait_for'
+  });
+
+  let results = await kuzzle.document.search(
+    'nyc-open-data',
+    'yellow-taxi',
+    {
+      query: { match: { category: 'suv' } },
+      sort: [
+        { '_kuzzle_info.createdAt': 'desc' },
+        '_id'
+      ]
+    },
+    { size: 5 });
+
+  // Fetch the matched items by advancing through the result pages
+  const matched = [];
+
+  while (results) {
+    matched.push(...results.hits);
+    results = await results.next();
+  }
+
+  console.log(matched[0]);
+  /*
+    { _id: 'suv_no1',
+      _score: 0.03390155,
+      _source:
+       { _kuzzle_info:
+          { author: '-1',
+            updater: null,
+            updatedAt: null,
+            createdAt: 1570093133057 },
+         category: 'suv' } }
+  */
+  console.log(`Successfully retrieved ${matched.length} documents`);
+} catch (error) {
+  console.error(error.message);
+}

doc/7/core-classes/search-result/next/snippets/sortsize.test.yml

@@ -0,0 +1,11 @@
+name: searchresult#sortsize
+description: Next method with sort/size
+hooks:
+  before: |
+    curl -XDELETE kuzzle:7512/nyc-open-data
+    curl -XPOST kuzzle:7512/nyc-open-data/_create
+    curl -XPUT kuzzle:7512/nyc-open-data/yellow-taxi
+  after: |
+    curl -XDELETE kuzzle:7512/nyc-open-data
+template: default
+expected: Successfully retrieved 100 documents

src/core/searchResult/Role.js

@@ -16,7 +16,7 @@ class RoleSearchResult extends SearchResultBase {
     // in Kuzzle API, scrollRoles action is not available, and searchRoles allows only from and size parameters
     // => we deny "scroll" and "sort" parameters.
     if (this._request.scroll || this._request.sort) {
-      throw new Error('only from/size params are allowed for role search');
+      return Promise.reject(new Error('only from/size params are allowed for role search'));
     }
 
     return super.next()

src/core/searchResult/SearchResultBase.js

@@ -38,20 +38,42 @@ class SearchResultBase {
         .then(response => this._buildNextSearchResult(response));
     }
     else if (this._request.size && this._request.body.sort) {
-      const
-        request = Object.assign({}, this._request, {
-          action: this._searchAction
-        }),
-        hit = this._response.hits[this._response.hits.length - 1];
+      const request = { ...this._request, action: this._searchAction };
+      const hit = this._response.hits[this._response.hits.length - 1];
+
+      // When sorting only on a non unique field, the search_after will not iterate
+      // over all documents having the same values but ES will returns the results
+      // directly after.
+      // It resulting in having less fetched documents than the total and thus the SDK
+      // try to fetch the next results page but it's empty
+      if (! hit) {
+        return Promise.reject(new Error('Unable to retrieve all results from search: the sort combination must identify one item only. Add document "_id" to the sort.'));
+      }
 
       request.body.search_after = [];
 
-      for (const sort of this._request.body.sort) {
+      let sorts;
+      if (typeof this._request.body.sort === 'string') {
+        sorts = [this._request.body.sort];
+      }
+      else if (Array.isArray(this._request.body.sort)) {
+        sorts = this._request.body.sort;
+      }
+      else {
+        sorts = Object.keys(this._request.body.sort);
+      }
+
+      if (sorts.length === 0) {
+        return Promise.reject(new Error('Unable to retrieve next results from search: sort param is empty'));
+      }
+
+      for (const sort of sorts) {
         const key = typeof sort === 'string'
           ? sort
           : Object.keys(sort)[0];
-        const value = key === '_uid'
-          ? this._request.collection + '#' + hit._id
+
+        const value = key === '_id'
+          ? hit._id
           : this._get(hit._source, key.split('.'));
 
         request.body.search_after.push(value);
@@ -65,14 +87,15 @@ class SearchResultBase {
         return Promise.resolve(null);
       }
 
-      return this._kuzzle.query(Object.assign({}, this._request, {
+      return this._kuzzle.query({
+        ...this._request,
         action: this._searchAction,
         from: this.fetched
-      }), this._options)
+      }, this._options)
         .then(response => this._buildNextSearchResult(response));
     }
 
-    throw new Error('Unable to retrieve next results from search: missing scrollId, from/sort, or from/size params');
+    return Promise.reject(new Error('Unable to retrieve next results from search: missing scrollId, from/sort, or from/size params'));
   }
 
   _get (object, path) {

test/core/searchResult/document.test.js

@@ -89,9 +89,8 @@ describe('DocumentSearchResult', () => {
 
       searchResult = new DocumentSearchResult(kuzzle, request, options, response);
 
-      should(function () {
-        searchResult.next();
-      }).throw('Unable to retrieve next results from search: missing scrollId, from/sort, or from/size params');
+      return should(searchResult.next())
+        .be.rejectedWith('Unable to retrieve next results from search: missing scrollId, from/sort, or from/size params');
     });
 
     describe('#with scroll option', () => {
@@ -153,18 +152,20 @@ describe('DocumentSearchResult', () => {
     });
 
     describe('#with size and sort option', () => {
-      const nextResponse = {
-        hits: [
-          {_id: 'document3', _score: 0.6543, _source: {foo: 'barbaz', bar: 4567}},
-          {_id: 'document4', _score: 0.6123, _source: {foo: 'bazbar', bar: 6789}}
-        ],
-        aggregations: 'nextAggregations',
-        total: 30
-      };
+      let nextResponse;
 
       beforeEach(() => {
+        nextResponse = {
+          hits: [
+            {_id: 'document3', _score: 0.6543, _source: {foo: 'barbaz', bar: 4567}},
+            {_id: 'document4', _score: 0.6123, _source: {foo: 'bazbar', bar: 6789}}
+          ],
+          aggregations: 'nextAggregations',
+          total: 30
+        };
+
         request.size = 2;
-        request.body.sort = ['foo', {bar: 'asc'}, {_uid: 'desc'}];
+        request.body.sort = ['foo', {bar: 'asc'}, {_id: 'desc'}];
 
         response = {
           hits: [
@@ -191,8 +192,8 @@ describe('DocumentSearchResult', () => {
                   query: {
                     foo: 'bar'
                   },
-                  sort: ['foo', {bar: 'asc'}, {_uid: 'desc'}],
-                  search_after: ['barbar', 2345, 'collection#document2']
+                  sort: ['foo', {bar: 'asc'}, {_id: 'desc'}],
+                  search_after: ['barbar', 2345, 'document2']
                 },
                 controller: 'document',
                 action: 'search',
@@ -215,6 +216,22 @@ describe('DocumentSearchResult', () => {
             should(nextSearchResult.aggregations).equal(nextResponse.aggregations);
           });
       });
+
+      it('should reject with an error if the sort is invalid', () => {
+        request.body.sort = [];
+        searchResult = new DocumentSearchResult(kuzzle, request, options, response);
+
+        return should(searchResult.next())
+          .be.rejected();
+      });
+
+      it('should reject if the sort combination does not allow to retrieve all the documents', () => {
+        response.hits = [];
+        searchResult = new DocumentSearchResult(kuzzle, request, options, response);
+
+        return should(searchResult.next())
+          .be.rejected();
+      });
     });
 
     describe('#with from and size option', () => {

test/core/searchResult/profile.test.js

@@ -82,7 +82,7 @@ describe('ProfileSearchResult', () => {
 
     });
 
-    it('should throw an error if neither scroll, nor size/sort, nor size/from parameters are set', () => {
+    it('should reject with an error if neither scroll, nor size/sort, nor size/from parameters are set', () => {
       response = {
         scrollId: 'scroll-id',
         hits: [
@@ -94,9 +94,8 @@ describe('ProfileSearchResult', () => {
 
       searchResult = new ProfileSearchResult(kuzzle, request, options, response);
 
-      should(function () {
-        searchResult.next();
-      }).throw('Unable to retrieve next results from search: missing scrollId, from/sort, or from/size params');
+      return should(searchResult.next())
+        .be.rejectedWith('Unable to retrieve next results from search: missing scrollId, from/sort, or from/size params');
     });
 
     describe('#with scroll option', () => {

test/core/searchResult/role.test.js

@@ -79,34 +79,30 @@ describe('RoleSearchResult', () => {
           should(kuzzle.query).not.be.called();
           should(result).be.Null();
         });
-
     });
 
 
-    it('should throw an error if scrollId parameters is set', () => {
+    it('should reject with an error if scrollId parameters is set', () => {
       request.scroll = '10s';
       searchResult = new RoleSearchResult(kuzzle, request, options, response);
 
-      should(function () {
-        searchResult.next();
-      }).throw('only from/size params are allowed for role search');
+      return should(searchResult.next())
+        .be.rejectedWith('only from/size params are allowed for role search');
     });
 
-    it('should throw an error if sort parameters is set', () => {
-      request.sort = ['foo', {bar: 'asc'}];
+    it('should reject with an error if sort parameters is set', () => {
+      request.sort = ['foo', { bar: 'asc' }];
       searchResult = new RoleSearchResult(kuzzle, request, options, response);
 
-      should(function () {
-        searchResult.next();
-      }).throw('only from/size params are allowed for role search');
+      return should(searchResult.next())
+        .be.rejectedWith('only from/size params are allowed for role search');
     });
 
-    it('should throw an error if size and from parameters are not set', () => {
+    it('should reject with an error if size and from parameters are not set', () => {
       searchResult = new RoleSearchResult(kuzzle, request, options, response);
 
-      should(function () {
-        searchResult.next();
-      }).throw('Unable to retrieve next results from search: missing scrollId, from/sort, or from/size params');
+      return should(searchResult.next())
+        .be.rejectedWith('Unable to retrieve next results from search: missing scrollId, from/sort, or from/size params');
     });
 
     describe('#with from and size option', () => {

test/core/searchResult/specifications.test.js

@@ -71,7 +71,7 @@ describe('SpecificationsSearchResult', () => {
 
     });
 
-    it('should throw an error if neither scroll, nor size/sort, nor size/from parameters are set', () => {
+    it('should reject with an error if neither scroll, nor size/sort, nor size/from parameters are set', () => {
       response = {
         scrollId: 'scroll-id',
         hits: [
@@ -83,9 +83,8 @@ describe('SpecificationsSearchResult', () => {
 
       searchResult = new SpecificationsSearchResult(kuzzle, request, options, response);
 
-      should(function () {
-        searchResult.next();
-      }).throw('Unable to retrieve next results from search: missing scrollId, from/sort, or from/size params');
+      return should(searchResult.next())
+        .be.rejectedWith('Unable to retrieve next results from search: missing scrollId, from/sort, or from/size params');
     });
 
     describe('#with scroll option', () => {

test/core/searchResult/user.test.js

@@ -84,7 +84,7 @@ describe('UserSearchResult', () => {
 
     });
 
-    it('should throw an error if neither scroll, nor size/sort, nor size/from parameters are set', () => {
+    it('should reject with an error if neither scroll, nor size/sort, nor size/from parameters are set', () => {
       response = {
         scrollId: 'scroll-id',
         hits: [
@@ -96,9 +96,8 @@ describe('UserSearchResult', () => {
 
       searchResult = new UserSearchResult(kuzzle, request, options, response);
 
-      should(function () {
-        searchResult.next();
-      }).throw('Unable to retrieve next results from search: missing scrollId, from/sort, or from/size params');
+      return should(searchResult.next())
+        .be.rejectedWith('Unable to retrieve next results from search: missing scrollId, from/sort, or from/size params');
     });
 
     describe('#with scroll option', () => {