fix(NODE-5044): Write Concern 0 Must Not Affect Read Operations (#3541)

[baileympearson]

Description

What is changing?

Cherry-picked changes from the 4.x branch with the same fix.

Is there new documentation needed for these changes?

no.

Double check the following

• Ran npm run check:lint script
• Self-review completed using the steps outlined here
• PR title follows the correct format: type(NODE-xxxx)[!]: description
  • Example: feat(NODE-1234)!: rewriting everything in coffeescript
• Changes are covered by tests
• New TODOs have a related JIRA ticket

[nbbeeken]

Interesting that we need ts-expect-error in src code, is there another way we could organize this? (can be in a follow up)

[baileympearson]

Not with strong typing. I mean, ultimately the real fix is to pass the full operation all the way down to the command layer, so that when we apply the session, we have access to the operation class and can check if it has a read aspect.

[nbbeeken]

If we're creating a new object, then can their be a new type that allows deleting the writeConcern without a ts-expect-error?

[baileympearson]

Do you mean like an internal options type? Does that not seem like overkill?

[baileympearson]

Also, how / why would we type something such that it allows the existence of a property that we will always delete?

[baileympearson]

What do you mean by types not being true? The type is correct. The argument could be made that we should omit this logic entirely because if someone passes a write concern to read operation and our types no longer support it, they are operating outside the bounds of our API.

[nbbeeken]

I thought the issue was with the option being inherited rather than being directly passed, right?
I think if it was a correct type then we wouldn't need ts-expect-error

[baileympearson]

Good point about options being inherited.

I don't think the value of modifying the type to include writeConcern?: never is worthwhile. I don't think that's the correct fix (I mention what I believe the correct fix to be above), so I'm going to leave this as-is.

[baileympearson]

Alternative option, again outside the scope of this work, is to consider a generic approach to constructing the options objects we use internally. Each command would construct an options object, only assigning the options that are actually supported for that command, instead of spreading all the existing options (maybe add an abstract parseOptions(userOptions, inheritedOptions) : OptionInterface method that each command implements). But that is a larger change that impacts the driver as a whole, so I won't address it here.

[nbbeeken]

I seem to recall a ticket related to that idea was a part of the Operation Layer Unification epic we used to have but I can't seem to find it, seems worth filing something for esp if we've considered it before.

I'd prefer if we made the types accurate rather than putting in compiler overrides

[nbbeeken]

requested changes already ^ bump

comments addressed

[nbbeeken]

Lgtm