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

MongoDB Insert deprecation (updated to insertOne/insertMany) #280

Merged
merged 11 commits into from
Sep 24, 2019
Merged

MongoDB Insert deprecation (updated to insertOne/insertMany) #280

merged 11 commits into from
Sep 24, 2019

Conversation

RobertoMachorro
Copy link
Contributor

Added bindings and updated tests for insertOne/insertMany. Monk already supports bulkWrite.
This addresses issue #276 .

(node:30558) DeprecationWarning: collection.insert is deprecated. Use insertOne, insertMany or bulkWrite instead.

@codecov-io
Copy link

codecov-io commented Sep 23, 2019

Codecov Report

Merging #280 into master will decrease coverage by 0.24%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #280      +/-   ##
==========================================
- Coverage   94.81%   94.57%   -0.25%     
==========================================
  Files           6        6              
  Lines         386      387       +1     
==========================================
  Hits          366      366              
- Misses         20       21       +1
Impacted Files Coverage Δ
lib/collection.js 94.31% <100%> (+0.02%) ⬆️
lib/manager.js 94.73% <0%> (-0.76%) ⬇️

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 bd1db07...109bcc0. Read the comment docs.

@faradaytrs
Copy link

Could you please do the same for count()

@mathieudutour
Copy link
Collaborator

How about updating the insert method to either use insertOne or insertMany` depending on whether an array is passed instead of adding 2 new methods?

@RobertoMachorro
Copy link
Contributor Author

Could you please do the same for count()

@faradaytrs most probably will, also group is going to be deprecated in favor of aggregation, i saw some of the tests spitting that out

@RobertoMachorro
Copy link
Contributor Author

RobertoMachorro commented Sep 23, 2019

How about updating the insert method to either use insertOne or insertMany` depending on whether an array is passed instead of adding 2 new methods?

@mathieudutour that's a great idea! Will do that for backwards compatibility, however, will still keep insertOne/Many because of upwards compatibility with MongoDB itself. The final say in all this - of course - is the Monk maintainers.

@osban
Copy link

osban commented Sep 23, 2019

@RobertoMachorro apparently the fields option is also deprecated, so if you could fix that too, that'd be great 🙂

@RobertoMachorro
Copy link
Contributor Author

@RobertoMachorro apparently the fields option is also deprecated, so if you could fix that too, that'd be great 🙂

@osban will do, but didn't see the warning? could you post more info about it?

@osban
Copy link

osban commented Sep 23, 2019

@RobertoMachorro here's the warning message: (node:5151) DeprecationWarning: collection.find option [fields] is deprecated and will be removed in a later version.. It's when you only want the query to return certain fields. Do you need more info?

@RobertoMachorro
Copy link
Contributor Author

@RobertoMachorro here's the warning message: (node:5151) DeprecationWarning: collection.find option [fields] is deprecated and will be removed in a later version.. It's when you only want the query to return certain fields. Do you need more info?

That works, thanks - just needed some context.

Copy link
Collaborator

@mathieudutour mathieudutour left a comment

Choose a reason for hiding this comment

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

let's even remove the other 2 methods, they are superfluous

@RobertoMachorro
Copy link
Contributor Author

let's even remove the other 2 methods, they are superfluous

@mathieudutour that makes sense, i'll remove the insertOne/inserMany calls. Some of the mongodb-cli intuitiveness will be out, though.

@mathieudutour
Copy link
Collaborator

I'd say a single insert is a more intuitive than 2 different calls? The API surface is smaller -> easier to remember

Implementation was requested in a way better approached from scratch.
@RobertoMachorro
Copy link
Contributor Author

@mathieudutour i'm making sure to match your code style, but I don't see a ternary operator anywhere on the source, do you prefer not to use them and go with regular if blocks?

It internally maps to mongodb's insertOne or insertMany.
@mathieudutour
Copy link
Collaborator

mathieudutour commented Sep 23, 2019

I do use ternaries, but maybe I’d write const colcall = arrayInsert ? args.col.insertMany : args.col.insertOne (eg. remove the unnecessary parentheses and add a space)

Or

const colcall = arrayInsert
  ? args.col.insertMany
  : args.col.insertOne

On multiple lines

It might be worth adding prettier to not have to care about it hahaha

@RobertoMachorro
Copy link
Contributor Author

RobertoMachorro commented Sep 23, 2019

I do use ternaries, but maybe I’d write const colcall = arrayInsert ? args.col.insertMany : args.col.insertOne (eg. remove the unnecessary parentheses and add a space)

I had it like that originally (not including the parameters in the ternary), however, Node doesn't like it. I've seen that happen with functions that return a promise as opposed to a value:

stack: >
  Collection.insertMany
  (/Projects/monk/node_modules/mongodb/lib/collection.js:504:32)

  insert (/Projects/monk/lib/collection.js:322:12)

  /Projects/monk/node_modules/monk-middleware-wait-for-connection/index.js:8:16
at:
  line: 504
  column: 32
  file: /Projects/monk/node_modules/mongodb/lib/collection.js
  function: Collection.insertMany
type: TypeError
tapCaught: returnedPromiseRejection
test: collection find limit/skip data
source: |2

    return executeOperation(this.s.topology, insertManyOperation, callback);
  -------------------------------^
  };
...

@RobertoMachorro
Copy link
Contributor Author

Thanks to all that can test, I'll start a count and find/fields update on a separate branch.

@mathieudutour mathieudutour merged commit d0dad4b into Automattic:master Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants