Skip to content

Prevent mutation of input options in all drivers #612

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jlucaso1
Copy link
Contributor

@jlucaso1 jlucaso1 commented Apr 7, 2025

Ensure that input options are not mutated across all drivers by creating a new options object from the input. This change enhances the integrity of the options passed to each driver.
Resolves #566

@jlucaso1 jlucaso1 requested a review from pi0 as a code owner April 7, 2025 11:59
@43081j
Copy link
Member

43081j commented Apr 7, 2025

can you show us where we were mutating the options?

I wonder if we should treat them as read-only and then only clone them in places where we must mutate

since I suspect most drivers don't mutate, so can avoid having to allocate a new object and could let the TS compiler catch disallowed mutations (it'd need to be read-only deeply I suppose)

@pi0
Copy link
Member

pi0 commented Apr 7, 2025

Typing input as readonly might be good idea I agree. there are also nested mutations possible (like push to array)

@43081j
Copy link
Member

43081j commented Apr 7, 2025

after hacking away at this a little, I think what we should do is just add read-only flags to our options types themselves

I was going to suggest introducing a DeepReadonly<T> and applying it to every opts parameter, but that feels like patching up the problem rather than solving it at the source

we could actually just change the various options types to already be readonly. for example:

export interface AzureKeyVaultOptions {
  readonly vaultName: string;
  readonly serviceVersion?: SecretClientOptions["serviceVersion"];
  readonly pageSize?: number;
}

a mutable object can be assigned to this as well as a read-only one, so it should be backwards compatible from the outside

and any types we expose which are not our own (e.g. SecretClientOptions here), we leave as-is. we don't try transform them into readonly as they're not our concern

any arrays would be ReadonlyArray<T>, and any nested objects would follow these same rules

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.

Make sure drivers don't modify input options object
3 participants