RUBY-3087 reinstate ability to call insert_many with Enumerable inputs which are not Array instances #2594

midnight-wonderer · 2022-08-10T15:00:44Z

Hello,
As I mentioned on #2430, we use insert_many with an enumerable instead of an array; we want to make sure it will continue to work into the future.

It would be great if you guys could include this test.
I am happy to adjust the pull request as you see fit.

alexbevi · 2022-08-10T16:03:54Z

Hi @midnight-wonderer, and thanks for submitting this PR! We will review this and get back to you with any feedback.

midnight-wonderer · 2022-08-11T04:13:34Z

It already breaks D: (test failed)
Let me see how I can help with the situation; I'll get back to you shortly.

midnight-wonderer · 2022-08-11T04:43:01Z

Updated!
The breaking change was introduced in commit#5931b9 by @neilshweky (pinging to request a code review).
The updated commit should still comply with RUBY-2976.

The user should be responsible for side effects that may occur because of multiple each iterations from the driver. It is the power of duck-typing, and also, it is undocumented, after all.

I would like to suggest subclassing ArgumentError so people can confidently rescue from the error; submitting an empty operation to bulk_write has a valid use case, IMO, but probably another time (not in this PR).

neilshweky · 2022-08-18T22:07:45Z

Hey @midnight-wonderer thanks for the ping! I just added another test, and some docs stating that we now support enumerables in the respective methods. Would you mind taking a look at that?

lib/mongo/collection/view/writable.rb

midnight-wonderer · 2022-08-19T06:27:21Z

LGTM (Neil's commit)

p-mongo · 2022-08-24T21:57:41Z

docs/release-notes.txt

 - When 0 is given as the max connection pool size, it is now interpreted to
  mean no limit.
 - The default maximum connection pool size has been increased to 20 from 5.
+- Added support for enumerables in bulk write operations.


If we are fixing this let's backport the fix to 2.18-stable and then it won't need a release note here.

Why won't it need a release not even if we're backporting to 2.18?

Because the break was in 2.18, thus after the fix there wouldn't be any stable branch where the issue exists so it might be confusing to people to try to work out which versions are broken.

But if you wish to have the note, that's fine, I would just phrase it as "Reinstated support for enumerables" or similar since it was already present and unintentionally broken.

oh okay I will remove it

lib/mongo/bulk_write.rb

p-mongo · 2022-08-24T21:59:51Z

lib/mongo/collection.rb

    #   collection.insert_many([{ name: 'test' }])
    #
-    # @param [ Array<Hash> ] documents The documents to insert.
+    # @param [ Array<Hash>, Enumerable ] documents The documents to insert.


Suggested change

# @param [ Array<Hash>, Enumerable ] documents The documents to insert.

# @param [ Array<Hash>, Enumerable<Hash> ] documents The documents to insert.

That would be correct yes?

I don't use type in Ruby, so I don't know which one can be generic.
However, if Enumerable can be generic too and Array is also an Enumerable, we can also just do Enumerable<Hash>.

There are three possibilities forward:

Array<Hash>, Enumerable as Neil suggested
probably maybe because Enumerable is not a generic type? And also not to confuse developers that are new to the concept of Ruby's module.

Array<Hash>, Enumerable<Hash> as you suggested
because Enumerable is actually a generic type; leaving the Array type as is for the same reason as above.

Enumerable<Hash>
if Enumerable is a generic type, and we prioritize conciseness.

I don't have a strong opinion on this, I had just looked for examples elsewhere in the code base and this is what we did. I don't mind switching it...

I think Array<Hash> | Enumerable<Hash> is redundant, my vote is for Enumerable<Hash> only.

@midnight-wonderer This notation simply signifies the expected type of values in the container, we haven't magically come up with a way to add generics to Ruby :)

That's fine with me

lib/mongo/collection.rb

spec/mongo/collection_crud_spec.rb

midnight-wonderer · 2022-08-26T13:45:27Z

I resolved the spec description issue mentioned by p-mongo, hope you don't mind; could you take a look at the other comments? @neilshweky

docs/release-notes.txt

neilshweky · 2022-08-26T19:27:17Z

@p-mongo this is rfal

p-mongo · 2022-08-28T16:54:43Z

@midnight-wonderer Thank you again for the PR and the fix.

…s which are not Array instances (#2594) * RUBY-3087 add a test for insert_many with enumerable inputs * RUBY-3087 raise exception on bulk_write without using an Array method * RUBY-3087 add test, docs * add docs, release notes * Fix the description of empty bulk_write spec to match the behavior * Update docs/release-notes.txt * change docstrings to use enumerables Co-authored-by: Neil Shweky <neilshweky@gmail.com>

RUBY-3087 add a test for insert_many with enumerable inputs

6e99af9

RUBY-3087 raise exception on bulk_write without using an Array method

4c67160

RUBY-3087 add test, docs

de213bb

neilshweky requested review from comandeo, neilshweky and p-mongo and removed request for neilshweky August 18, 2022 22:07

neilshweky approved these changes Aug 18, 2022

View reviewed changes

neilshweky reviewed Aug 18, 2022

View reviewed changes

lib/mongo/collection/view/writable.rb Show resolved Hide resolved

add docs, release notes

f173a36

p-mongo reviewed Aug 24, 2022

View reviewed changes

Fix the description of empty bulk_write spec to match the behavior

3bd3243

neilshweky reviewed Aug 26, 2022

View reviewed changes

docs/release-notes.txt Outdated Show resolved Hide resolved

neilshweky added 2 commits August 26, 2022 15:23

Update docs/release-notes.txt

6cfcfc2

change docstrings to use enumerables

b260cb4

neilshweky approved these changes Aug 26, 2022

View reviewed changes

neilshweky requested a review from p-mongo August 26, 2022 19:27

p-mongo approved these changes Aug 26, 2022

View reviewed changes

p-mongo changed the title ~~RUBY-3087 add a test for insert_many with enumerable inputs~~ RUBY-3087 reinstate ability to call insert_many with Enumerable inputs which are not Array instances Aug 28, 2022

p-mongo merged commit 6bf442e into mongodb:master Aug 28, 2022

	# @param [ Array<Hash>, Enumerable ] documents The documents to insert.
	# @param [ Array<Hash>, Enumerable<Hash> ] documents The documents to insert.

RUBY-3087 reinstate ability to call insert_many with Enumerable inputs which are not Array instances #2594

RUBY-3087 reinstate ability to call insert_many with Enumerable inputs which are not Array instances #2594

Uh oh!

Conversation

midnight-wonderer commented Aug 10, 2022

Uh oh!

alexbevi commented Aug 10, 2022

Uh oh!

midnight-wonderer commented Aug 11, 2022

Uh oh!

midnight-wonderer commented Aug 11, 2022

Uh oh!

neilshweky commented Aug 18, 2022

Uh oh!

Uh oh!

midnight-wonderer commented Aug 19, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

midnight-wonderer commented Aug 26, 2022

Uh oh!

Uh oh!

neilshweky commented Aug 26, 2022

Uh oh!

p-mongo commented Aug 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants