fix(store): merge WithURL fields with WithAuth/WithPassword/WithDB, only error on real conflict - #103
Open
destinyoooo wants to merge 1 commit into
Open
fix(store): merge WithURL fields with WithAuth/WithPassword/WithDB, only error on real conflict#103destinyoooo wants to merge 1 commit into
destinyoooo wants to merge 1 commit into
Conversation
…or on real conflict
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background
WithURLinteracts silently withWithAuth/WithPassword/WithDB: the URL branch ofbuildPoolonly callsredis.DialURL(cfg.url), so any values set viaWithAuth("u", "p")orWithDB("5")are completely ignored whenWithURLis also set. There is no error, no warning — the user just silently connects with the URL's credentials (or none) instead of the ones they thought they were configuring. In production this is a footgun for multi-tenant Redis setups and ACL-based auth.Changes
Rather than a blanket "URL and options are mutually exclusive" rule, the fix is per-field conflict detection + field-level merge:
parseRedisURLand structredisURLParts, supporting theredis://,rediss://, andunix://schemes. Forunix://the database component is forced to""— the socket path is not a db.validate(), whencfg.url != "", checks theusername/password/dbfields one by one. It only returns an error when both the URL and an option set the same field, and the error names the conflicting field.buildPool()no longer callsredis.DialURLfor the URL branch. It parses the URL and merges it withcfg.username/cfg.password/cfg.dbusing "URL first, options fill the gaps", then routes throughdialClientso every value is actually applied to the dial.