Skip to content
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

createClient url+tls invariant violation check #2835

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

Conversation

dearlordylord
Copy link

Description

on url rediss:.. + tls: false passed or on url: redis: + rls: true passed, the lib blows up during connection with a non-descriptive error

I explicitly check for this invariant violation during initialisation

  • parseOptions was added to make it testable and to avoid the pesky this.#selectedDB = options.database; in #initiateOptions I dare not to touch
  • parseOptions is supposed to check cross-field invariants that parseUrl alone won't be able to do
  • parseUrl explicitly states on type-level that it affects tls (it did this anyways, implicitly, by blowing up the connection on wrong tls passed)
  • parseOptions mutates its arguments as well as returns the mutated value in accord with how #initiateOptions was implemented (let me know if you'd like me to not-mutate the argument; I'd actually prefer it this way)
  • the technical change is that initialization will now set tls explicitly to boolean if it wasn't set but an url was passed: reasons are both technical (I needed to test without changing your interfaces much) and logical: tls undefined with URL: redis[s]: seems to be an incorrect invariant anyways

Options:

  • not throw any errors, silently overriding user-given value - I prefer to fail-fast, because the instance will fail anyways on connection

Checklist

  • Does npm test pass with this change (including linting)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

}

static parseURL(url: string): RedisClientOptions & {
socket: Exclude<RedisClientOptions['socket'], undefined> & {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

union is a bit wordy, please guide me where you'd like to define an interface if you think it's not ok to inline that

throw new TypeError('Invalid protocol');
}

parsed.socket.tls = protocol === 'rediss:';
Copy link
Author

@dearlordylord dearlordylord Sep 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"DRY enough": at this point protocol is a literal union and won't accept anything except those two literals

if (options?.database) {
this.#selectedDB = options.database;
}

if (options) {
return RedisClient.parseOptions(options);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe better to put before this.#selectedDB = options.database but it's a private method use in a constructor anyways?

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.

1 participant