Skip to content

Conversation

Yoann-Abbes
Copy link
Contributor

@Yoann-Abbes Yoann-Abbes commented May 15, 2020

What does this PR do?

Jira ticket

This PR provides the possibility to execute a document:search with the GET http verb

How should this be manually tested?

Create a nyc-open-data' index and yellow-taxi collection.
Create some files with 2 containing field {name:"ok"}
Execute this code

Remove the verb option and/or change it to POST and re-execute the code

By default, document:search is still POST

const { Kuzzle, Http } = require('kuzzle-sdk');

const kuzzle = new Kuzzle(
  new Http(`localhost`, { port: 7512})
);

async function main () {

  try {
  	await kuzzle.connect();

        const response = await kuzzle.document.search(
      'nyc-open-data',
      'yellow-taxi',
      {
        query:{
            term: {
              name: 'ok'
            }
          }
        },{verb: 'GET'}
    );

    console.log(`Successfully get ${response.total} documents`);
  } catch (error) {
    console.error(error);
  }
}

main()

Other changes

update Mocha to 7.2.0
update codecov to 3.7.0

Boyscout

Removes an unused url who was in conflict with collection:update url
Fix eslint version for the documentation CI

@codecov
Copy link

codecov bot commented May 15, 2020

Codecov Report

❗ No coverage uploaded for pull request base (7-dev@d877d6f). Click here to learn what that means.
The diff coverage is 87.50%.

Impacted file tree graph

@@           Coverage Diff            @@
##             7-dev     #514   +/-   ##
========================================
  Coverage         ?   95.57%           
========================================
  Files            ?       32           
  Lines            ?     1310           
  Branches         ?        0           
========================================
  Hits             ?     1252           
  Misses           ?       58           
  Partials         ?        0           
Impacted Files Coverage Δ
src/protocols/abstract/Base.js 92.30% <50.00%> (ø)
src/controllers/Document.js 95.16% <100.00%> (ø)
src/protocols/Http.js 91.08% <100.00%> (ø)
src/protocols/WebSocket.js 100.00% <100.00%> (ø)
src/core/searchResult/Document.js 100.00% <0.00%> (ø)
src/core/security/Role.js 80.00% <0.00%> (ø)
src/controllers/Security.js 97.05% <0.00%> (ø)
src/controllers/Bulk.js 100.00% <0.00%> (ø)
src/core/searchResult/User.js 100.00% <0.00%> (ø)
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d877d6f...4fb7d3b. Read the comment docs.

Copy link
Contributor

@scottinet scottinet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Circular dependency: this PR is required by kuzzleio/kuzzle#1629, but kuzzleio/kuzzle#1629 requires it for the bugfix.

This is impossible to release properly.

You should split this PR in two parts: first the bugfix, and then, once a new version of kuzzle containing the fixed SDK version is released, the other part with the new ability to use document:search using the GET verb.

};

for (const opt of ['from', 'size', 'scroll']) {
for (const opt of ['from', 'size', 'scroll', 'verb']) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to pass the verb as an argument since the verb option on query will change the http verb

Suggested change
for (const opt of ['from', 'size', 'scroll', 'verb']) {
for (const opt of ['from', 'size', 'scroll']) {


_search (index, collection, body = {}, options = {}) {
if (options.verb && options.verb.toLowerCase() === 'get') {
if ( this._kuzzle.protocol instanceof HttpProtocol
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to have a name property on the protocol objects

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes thanks, I was wondering

@Yoann-Abbes Yoann-Abbes requested review from Aschen and scottinet May 22, 2020 12:33
@Yoann-Abbes Yoann-Abbes force-pushed the KZL-1493-document-search-get branch from a74e09c to 6b4bade Compare May 28, 2020 03:22

this._pendingRequests = new Map();
this._host = host;
this._name = name;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We prefix properties with an underscore as a convention for private properties. You should add a getter, like what's done for other properties.

@Yoann-Abbes Yoann-Abbes requested a review from scottinet June 4, 2020 06:08
@scottinet
Copy link
Contributor

scottinet commented Jun 4, 2020

Seriously?
You did add the getter, but you still use the class "private" property directly...

@Yoann-Abbes
Copy link
Contributor Author

Seriously?
You did add the getter, but you still use the class "private" property directly...

sorry, went too fast

@scottinet scottinet merged commit df6d4d9 into 7-dev Jun 8, 2020
@scottinet scottinet deleted the KZL-1493-document-search-get branch June 8, 2020 08:10
@scottinet scottinet mentioned this pull request Jul 23, 2020
scottinet added a commit that referenced this pull request Jul 23, 2020
# [7.3.0](https://github.com/kuzzleio/sdk-javascript/releases/tag/7.3.0) (2020-07-23)


#### Bug fixes

- [ [#532](#532) ] Encode URI parameters when using the HTTTP protocol   ([scottinet](https://github.com/scottinet))
- [ [#516](#516) ] Fix query string construction   ([Yoann-Abbes](https://github.com/Yoann-Abbes))

#### New features

- [ [#526](#526) ] Add typescript definitions for Auth controller   ([Aschen](https://github.com/Aschen))
- [ [#524](#524) ] Support ms:mexecute   ([Leodau](https://github.com/Leodau))
- [ [#522](#522) ] Add bulk:deleteByQuery   ([Yoann-Abbes](https://github.com/Yoann-Abbes))
- [ [#519](#519) ] Add document:updateByQuery   ([Yoann-Abbes](https://github.com/Yoann-Abbes))

#### Enhancements

- [ [#517](#517) ] Add collection:update and deprecate collection:updateMapping   ([Yoann-Abbes](https://github.com/Yoann-Abbes))
- [ [#514](#514) ] Able to execute document search as GET http method   ([Yoann-Abbes](https://github.com/Yoann-Abbes))
---
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants