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

fix: revert parsing username via URI due to potential breaking changes #1134

Merged
merged 1 commit into from
May 16, 2020

Conversation

luin
Copy link
Collaborator

@luin luin commented May 16, 2020

Closes #1132.

Previously, users can specify the username in Redis connect URI unintentionally so parsing username leads potential breaking changes in this case. We still support username parameter so users can opt-in ACL explicitly.

@luin luin merged commit 225ef45 into master May 16, 2020
ioredis-robot pushed a commit that referenced this pull request May 16, 2020
## [4.17.1](v4.17.0...v4.17.1) (2020-05-16)

### Bug Fixes

* revert parsing username via URI due to potential breaking changes ([#1134](#1134)) ([225ef45](225ef45))
@ioredis-robot
Copy link
Collaborator

🎉 This PR is included in version 4.17.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@billinghamj
Copy link

Could this be brought back with a flag to allow proper URI parsing? Or in a v5 release?

@luin
Copy link
Collaborator Author

luin commented May 20, 2020

@billinghamj I thought we can launch this in v5 but a flag sounds good to me.

@bpo
Copy link

bpo commented May 30, 2020

@luin what do you think about just skipping the username assignment if the username is default?

    if (parsedAuth[0] && parsedAuth[0] != "default") {

I think that would preserve backward compatibility for older servers without requiring a flag or a major release.

@luin
Copy link
Collaborator Author

luin commented May 30, 2020

@bpo Actually people may put arbitrary usernames ex "user" for whatever reasons in the URL as they thought it doesn't matter and will be ignored by ioredis. So your approach works in most normal cases but still introduces risks of breaking their assumption.

It's debatable what kind of changes should be considered as breaking changes, but I think we should be cautious anyway so I'd prefer adding a flag or next major release.

@luin luin deleted the url-breaking-change branch May 30, 2020 02:48
@gkorland
Copy link
Contributor

gkorland commented Feb 3, 2021

@luin do you still plan to put it back?

@luin
Copy link
Collaborator Author

luin commented Feb 5, 2021

@luin do you still plan to put it back?

Added back via #1284 😄

janus-dev87 added a commit to janus-dev87/ioredis-work that referenced this pull request Mar 1, 2024
## [4.17.1](redis/ioredis@v4.17.0...v4.17.1) (2020-05-16)

### Bug Fixes

* revert parsing username via URI due to potential breaking changes ([#1134](redis/ioredis#1134)) ([225ef45](redis/ioredis@225ef45))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v4.17.0 breaking change for Redis parser
5 participants