fix(data-service): set readPreference to primary unconditionally in updateCollection COMPASS-10872

[nbbeeken]

Description

Apologies, a closer look at the web e2e before merging #8250 would've revealed this.

Checklist

• New tests and/or benchmarks are included
• Documentation is changed or added
• If this change updates the UI, screenshots/videos are added and a design review is requested
• If this change could impact the load on the MongoDB cluster, please describe the expected and worst case impact
• I have signed the MongoDB Contributor License Agreement (https://www.mongodb.com/legal/contributor-agreement)

Motivation and Context

• Bugfix
• New feature
• Dependency update
• Misc

Open Questions

Dependents

Types of changes

• Backport Needed
• Patch (non-breaking change which fixes an issue)
• Minor (non-breaking change which adds functionality)
• Major (fix or feature that would cause existing functionality to change)

[Copilot]

Pull request overview

This PR addresses a replica set routing issue in data-service where updateCollection (via collMod) can inherit a non-primary read preference from the client and fail on secondaries. Overall direction is good; the implementation is small and targeted, but the new test should be made more deterministic to reliably prevent regressions.

Changes:

• Forces collMod in updateCollection to execute with readPreference: primary.
• Adds a regression test covering updateCollection behavior when the connection string specifies a non-primary read preference.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
packages/data-service/src/data-service.ts	Forces primary read preference for collMod invoked by updateCollection.
packages/data-service/src/data-service.spec.ts	Adds an integration test for updateCollection under a non-primary read preference.

[gribnoysup]

Is it expected that when the user explicitly provided us with a readPreference in the connection string, we will force-override it without telling them? In other places we use _getOptionsWithFallbackReadPreference for that to make sure we only apply a default if something wasn't set via connection string

Same q for the already merged #8250

[nbbeeken]

Is it expected that when the user explicitly provided us with a readPreference in the connection string, we will force-override it without telling them?

I would argue in these scenarios yes it makes sense to, open to alternative thought though. For the case in the other PR, txn cannot be run if you've set a secondary readpref as the client option the reason is from the driver spec: https://github.com/mongodb/specifications/blob/master/source/transactions/transactions.md#why-is-readpreference-part-of-transactionoptions a 4.0-4.2 era "prevents a breaking change" decision to not just pin to primary inside the txn API itself.

In this one, colMod is not allowed against other nodes, and that's enforced by the server rather than the driver. Anyone who uses a diff readPreference than primary on their client will never be able to use the validation UI on that connection.

In both cases we're doing some special operation to make the GUI functional and they both are write operations which, normal writes (insert etc.) always go to the primary anyway regardless of your readpref setting.

[gribnoysup]

I think there are a couple of things to consider before we decide to make this change:

The critical one is that this breaks the contract of forceConnectionOptions preference. This is a feature requested by enterprise users and so at the very least we should account for it and not override the settings against their will if some overrides were force applied to the connections.

Generally speaking I think the point about applying these in some cases to make UI better is valid, but I do think that we should be very careful with how exactly we use it. Downgrading the read pref is one thing, but going from secondary to primary seems to change the requested connection significantly. From my perspective, I think we should always assume that if user provided connection string contains a special, non-default preference it is done on purpose. This can have unexpected consequences for the UI, yes, but we have ways of accounting for that: a bunch of places in Compass UI already account for current topology and disable certain operations when they are not allowed, it doesn't make sense to break away from this pattern in some cases, but not in others.

EDIT: Looking at the ticket that you opened, if that's related to us forcing a custom readPref on the DE users, I think I have maybe two extra thoughts here:

• We might want to pass these overrides somehow very explicitly and not as part of the connection string so that we can clearly understand what is out internal forced override and what is user connection string (similar to the preference I mentioned above)
• As in this case secondaryPreferred and primary do have an overlap, doing the override sounds okay, but I think we should then very explicitly code it in a way that only allows this in cases like this one where preferences overlap, not do a blanket override for everything completely dismissing user provided one

[nbbeeken]

I don't understand the consideration we are giving to logic inside the application that breaks write operations. 🤔 We are the ones that have decided to take the client readPreference and feed it to runCommand, breaking colMod, and we are the ones that are ignoring the driver's documentation about setting primary on a txn. Yes the user has set an alternative readPreference but if they use the same connection string in their application they would not face the errors that Compass does.

(sort of, the txn one is historical driver quirkiness, but there's an essay over there about how the driver would if it could just force primary internally like it does for many other operations)

Let me familiarize myself with the history of forceConnectionOptions and the systems Compass uses to disable certain UIs, maybe there's a way to combine certain conditions to determine what should work. 👀

We might want to pass these overrides somehow very explicitly and not as part of the connection string

Perhaps, but sounds like a heavy lift to make a new endpoint for connectionInfo, or change the schema it returns, but maybe we have to

[gribnoysup]

RE the history of this, I think the expectations for Compass are definitely different compared to direct usage of the driver and so people do expect the readPref in the connection string to apply to everything. Initial fix for this was an outcome of this particular HELP ticket. With driver you have more control over every place where it will be used, with Compass you don't and so even if counterintuitive from the "writing an app using driver" perspective I think it does make sense that this one point where you can set it will apply everywhere inside a GUI.

For UI adjustments, it's a bit of a messy thing to track through the app, but you'd need to look for topology changed even listeners and isReadonly / isWritable flags usage in the app. For example if topology changes to the one that doesn't support writes, we will disable editing in the CRUD view and show the tooltip saying that topology doesn't support that (which is helpful when you're in Compass and control the connection string, but obviously less so in mms where we control it)