Skip to content

Conversation

Yoann-Abbes
Copy link
Contributor

@Yoann-Abbes Yoann-Abbes commented Nov 15, 2019

What does this PR do?

This PR changes the way we get the correct route if there is several of them.

Before this PR:
The SDK checked the size of the url, and chose the shortest.
If they had the same size, it chose the POST route.
And you could not force the verb in your request.

Now:
Same thing, except it chooses the GET route.
Plus, you can force the verb with the option {verb: 'POST'}

How should this be manually tested?

Install this version of the sdk npm install git://github.com/kuzzleio/sdk-javascript#handle-double-routes
Run Kuzzle in debug mod.
Run this script

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

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

async function main () {
const doc1 = { capacity: 4 };
const doc2 = { capacity: 7 };

    try {
	await kuzzle.connect();
	await kuzzle.document.createOrReplace('nyc-open-data', 'yellow-taxi', 'some-id', doc1);
	await kuzzle.document.createOrReplace('nyc-open-data', 'yellow-taxi', 'some-other-id', doc2);

  const response = await kuzzle.document.mGet(
    'nyc-open-data',
    'yellow-taxi',
      ['some-id', 'some-other-id']
//   ,{verb: 'POST'}
  );

  console.log(response);

} catch (error) {
  console.error(error.message);
}
}

main()

You will see in the logs that the route verb was GET

Now uncomment the line // ,{verb: 'POST'}
The route will be in POST.

You can do the same with mGetUsers

Other changes

Adapt doc snippet test for getAllStats and getLastStats, because of the new properties order of the response object recently defined

Boyscout

@Yoann-Abbes
Copy link
Contributor Author

I've tested it by running Kuzzle with this branch kuzzleio/kuzzle#1519
It works and tests are ok

@Yoann-Abbes Yoann-Abbes removed the wip label Nov 21, 2019
@codecov
Copy link

codecov bot commented Nov 21, 2019

Codecov Report

Merging #465 into 7-dev will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##            7-dev     #465      +/-   ##
==========================================
+ Coverage   95.56%   95.57%   +0.01%     
==========================================
  Files          32       32              
  Lines        1307     1312       +5     
==========================================
+ Hits         1249     1254       +5     
  Misses         58       58
Impacted Files Coverage Δ
src/protocols/http.js 90.78% <100%> (+0.31%) ⬆️
src/controllers/document.js 95.45% <100%> (ø) ⬆️

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 391c5c1...ada96e4. 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.

just a few documentation problems to fix and this PR will be gtg

Comment on lines 17 to 19
The route exists in `GET` and `POST`.
By default, the SDK hits the `GET` one.
You can force it to be `POST` in the `options`.
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be part of the route description, but should instead be a dedicated sub-chapter for the verb option below.

Something such as:

#### verb

When instantiated with a HTTP protocol object, the SDK uses the GET API by default for this API route.
You can set the `verb` option to `POST` to force the SDK to use the POST API instead.

options.verb = 'POST';
for (const opt of ['from', 'size', 'scroll']) {
request[opt] = options[opt];
delete options[opt];
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it's not directly related to this PR but here the options object should not be mutated because the user may want to re-use it across subsequent calls

@Yoann-Abbes
Copy link
Contributor Author

@Aschen despite our discussions, everything is OK

Yoann-Abbes added a commit to kuzzleio/kuzzle that referenced this pull request Feb 13, 2020
What does this PR do ?
This PR removes unintended changes on functional tests that will make them fail when
kuzzleio/sdk-javascript#465 will be merged

How should this be manually tested?
Launch test with the actual released SDK
Launch them with this version of the SDK kuzzleio/sdk-javascript#465
@Aschen Aschen merged commit 84a59f0 into 7-dev Feb 13, 2020
@Aschen Aschen deleted the handle-double-routes branch February 13, 2020 16:05
@Yoann-Abbes Yoann-Abbes mentioned this pull request Feb 19, 2020
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.

4 participants