Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove _id from bulk:write 2e route #1314

Merged
merged 8 commits into from
Jun 5, 2019
Merged

Conversation

Aschen
Copy link
Contributor

@Aschen Aschen commented Jun 3, 2019

What does this PR do ?

Remove the second route for bulk:write. The _id is now a query string parameter and not part of the url

How should this be manually tested?

  • Step 1 :
  • Step 2 :
  • Step 3 :
    ...

Other changes

Boyscout

@Aschen Aschen added the changelog:exclude Internal changes only label Jun 3, 2019
@Aschen Aschen self-assigned this Jun 3, 2019
@codecov
Copy link

codecov bot commented Jun 4, 2019

Codecov Report

Merging #1314 into 1-dev will decrease coverage by 87.01%.
The diff coverage is 25%.

Impacted file tree graph

@@            Coverage Diff             @@
##            1-dev   #1314       +/-   ##
==========================================
- Coverage   93.79%   6.78%   -87.02%     
==========================================
  Files          98      98               
  Lines        6967    6971        +4     
==========================================
- Hits         6535     473     -6062     
- Misses        432    6498     +6066
Impacted Files Coverage Δ
lib/config/httpRoutes.js 100% <ø> (ø) ⬆️
lib/api/core/plugins/pluginContext.js 10.21% <0%> (-85.37%) ⬇️
lib/api/kuzzle.js 20% <33.33%> (-66.37%) ⬇️
lib/api/core/validation/types/integer.js 0% <0%> (-100%) ⬇️
lib/api/core/validation/types/object.js 0% <0%> (-100%) ⬇️
lib/api/core/validation/types/date.js 0% <0%> (-100%) ⬇️
lib/api/core/validation/types/string.js 0% <0%> (-100%) ⬇️
lib/api/core/validation/types/numeric.js 0% <0%> (-100%) ⬇️
lib/api/core/validation/types/enum.js 0% <0%> (-100%) ⬇️
lib/api/core/validation/types/anything.js 0% <0%> (-100%) ⬇️
... and 86 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 cd0b88b...4679e43. Read the comment docs.

@codecov
Copy link

codecov bot commented Jun 4, 2019

Codecov Report

Merging #1314 into 1-dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           1-dev   #1314   +/-   ##
=====================================
  Coverage   93.8%   93.8%           
=====================================
  Files         98      98           
  Lines       6991    6991           
=====================================
  Hits        6558    6558           
  Misses       433     433
Impacted Files Coverage Δ
lib/api/kuzzle.js 86.36% <ø> (ø) ⬆️
lib/config/httpRoutes.js 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 430806b...1761b66. Read the comment docs.

Copy link
Contributor

@benoitvidis benoitvidis left a comment

Choose a reason for hiding this comment

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

I don't understand why this route should be removed.
To me, it is quite consistent with the document:create|replace routes.

@scottinet
Copy link
Contributor

scottinet commented Jun 5, 2019

@benoitvidis > this started from this comment in the corresponding documentation PR: kuzzleio/documentation#300 (comment)

The routes you mention are old, and created prior to our own HTTP router. Passing a document ID in the querystring is now free, and having multiple URLs because of an optional argument doesn't seem clean to me.

Thinking of that, I wonder if perhaps we should deprecate the duplicated routes for old API actions 🤔

@Aschen
Copy link
Contributor Author

Aschen commented Jun 5, 2019

@scottinet Yes I think we should deprecate other duplicated routes

@benoitvidis benoitvidis merged commit ea9205d into 1-dev Jun 5, 2019
@benoitvidis benoitvidis deleted the change-write-http-route branch June 5, 2019 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:exclude Internal changes only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants