feat: Filter invalid values in getIds

[duncanbeevers]

🏋️ getIds robustness

getIds is a useful utility function, but it accepts a limited subset of the valid ObjectId constructor argument types, and throws when presented with invalid input.

It's not straight-forward to filter inputs to be valid constructor arguments; in some codebases we maintain bespoke utilities for performing these checks (eg; isValidObjectId).

Unfortunately, the canonical validation for whether one of these arguments is a valid argument to the ObjectId constructor is cooked-in to the constructor itself.

Therefore, rather than re-implementing this logic (or trying to get it extracted and exposed upstream), I updated the getIds implementation to rely on the constructor logic through try/catch, and to discard invalid elements.

🐤 Backwards-Compatibility

Per discussion, this is going to be a breaking change.

In order to keep the change backwards-compatible, I gated the filtering functionality behind a filterInvalid option, passed to getIds. If we were okay with making a breaking release, I think this would be much nicer as the default behavior.

🔧 Changes

I updated getAll (🟦) and its specs (🟡) in a few ways.

• 🟦 make getAll accept an Iterable<ObjectIdConstructorParameter>, widening acceptable input types
• 🟦 add optional GetIdsOptions argument to getIds, with optional filterInvalid boolean member
• 🟦 change getIds inner map to flatMap, to allow single-pass filtering
• 🟦 add try/catch to getIds inner flatMap, to rely on ObjectId constructor validation behavior
• 🟦 add error re-throw to getIds inner flatMap, gated on filterInvalid option
• 🟡 make expected result part of each test case
• 🟡 introduce variety between test case values / fixtures
• 🟡 add invariant check that expected result length must be less-than-or-equal-to input length
• 🟡 add test cases for Uint8Array arguments
• 🟡 add test cases for invalid input
• 🟡 add test case for invalid input with no filterInvalid option

[ejmartin504]

If we were okay with making a breaking release, I think this would be much nicer as the default behavior.

I would 100% prefer this.

I know I'm always the one who says "is this really a breaking change" -- and I must say this is an interesting example! After all, this should fix existing uses, rather than break them... right? It's hard to imagine code depending on invalid input to this function throwing 🤷

[avaly]

I agree with making this a breaking change. Let's remove the options argument.

[duncanbeevers]

Is it worth it to extract the validation functionality from bson to avoid the try/catch, or ship with this implementation?

[avaly]

We can ship this as is, but let's mark the commit as a breaking change.

[duncanbeevers]

I removed the filterInvalid option and made the filtering the default behavior. Let me know if there's more I need to do in order to mark this as a breaking change.

I also squashed this all down to a single commit because it was terrible fixing each of the conflicts that arose in the test file; easier to just fix everything all at once.

[avaly]

I think this PR should be fine as is. I don't think we need to mark it as breaking change. The only thing to change though is the commit prefix from feat: to fix: since I think this can be viewed as a bug fix.

[nunomarks]

CLGTM! 🎉