Made user info not required for RedisDSN. This will fix the issue #1275 #1302
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1302 +/- ##
===========================================
- Coverage 100.00% 99.94% -0.06%
===========================================
Files 21 21
Lines 3770 3772 +2
Branches 749 750 +1
===========================================
Hits 3770 3770
- Misses 0 1 +1
- Partials 0 1 +1
Continue to review full report at Codecov.
|
|
@kesavkolla any chance of this getting fixed? |
|
sorry I didn't understand the comment. I thought it's waiting on you to merge |
|
no, I've made suggestions, and asked for some more changes. |
Co-authored-by: Samuel Colvin <samcolvin@gmail.com>
I think that is a good idea of chanding the behaviour of |
…hen both user and password are empty
…hen both user and password are empty
merge changes
|
@samuelcolvin anything else to do for this PR? |
There was a problem hiding this comment.
I'm so sorry to have changed my mind on this - I know it's very annoying.
I think we need to either add password_required and credentials_required (which is either user or password provided), or remove the change in logic and just set user_required = False on RedisDsn.
Having looked again neither username or password are required for redis, so fixing the original error on this should be very simple.
| @@ -189,7 +190,8 @@ def validate(cls, value: Any, field: 'ModelField', config: 'BaseConfig') -> 'Any | |||
| raise errors.UrlSchemePermittedError(cls.allowed_schemes) | |||
|
|
|||
| user = parts['user'] | |||
| if cls.user_required and user is None: | |||
| password = parts['password'] | |||
| if cls.user_required and user is None and password is None: | |||
There was a problem hiding this comment.
Humm, this is a breaking change (and arguably a bug) since now user is no longer required when user_required = True - just password will suffice .
To fix this, either we need to:
- add
password_required(and then probablyuser_or_password_required, maybecredentials_requiredwould be a better name) - or, leave this logic unchanged and just change
user_required = FalseonRedisDsn
I think I favour option 1., but on RedisDsn it looks like neither user or password are required, so it doesn't make much difference in this case.
| @@ -53,7 +53,7 @@ def url_regex() -> Pattern[str]: | |||
| if _url_regex_cache is None: | |||
| _url_regex_cache = re.compile( | |||
| r'(?:(?P<scheme>[a-z][a-z0-9+\-.]+)://)?' # scheme https://tools.ietf.org/html/rfc3986#appendix-A | |||
| r'(?:(?P<user>[^\s:/]+)(?::(?P<password>[^\s/]*))?@)?' # user info | |||
| r'(?:(?P<user>[^\s:/]+)?(?::(?P<password>[^\s/]*))?@)?' # user info | |||
There was a problem hiding this comment.
| r'(?:(?P<user>[^\s:/]+)?(?::(?P<password>[^\s/]*))?@)?' # user info | |
| r'(?:(?P<user>[^\s:/]+)?' # user | |
| r'(?::(?P<password>[^\s/]*))?@)?' # password |
as i understand it user and password are now independent, so should go on separate lines.
| @@ -0,0 +1 @@ | |||
| Made user info optional for `RedisDSN` | |||
There was a problem hiding this comment.
If we're adding password_required and credentials_required we should put that in the change description.
|
Thanks so much for working on this. In the end to get something out, I've implemented a simple fix in #1658. |
Change Summary
Making user info optional for RedisDSN. Most of the redis instances will have either no authentication or only password is needed.
Related issue number
Fix #1275
Checklist
changes/1275-kesavkolla.mdfile added describing change(see changes/README.md for details)