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

Add support for configuring the Redis database using spring.data.redis.url #43813

Closed
wants to merge 2 commits into from

Conversation

quaff
Copy link
Contributor

@quaff quaff commented Jan 14, 2025

Fix GH-43810

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 14, 2025
@wilkinsona wilkinsona changed the title Parse redis database from url if present Add support for configuring the Redis database using spring.data.redis.url Jan 15, 2025
@wilkinsona wilkinsona added this to the 3.5.x milestone Jan 15, 2025
@wilkinsona wilkinsona added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 15, 2025
@wilkinsona wilkinsona self-assigned this Jan 15, 2025
@wilkinsona
Copy link
Member

Thanks for the PR, @quaff. A couple of things:

  1. Could you please rebase this on top of the head of main to address the conflicts?
  2. Did you consider making getDatabase() of the Sentinel instance returned by PropertiesRedisConnectionDetails use the database from the URL? I am in two minds as to whether that's the right thing to do.

@wilkinsona wilkinsona added the status: waiting-for-feedback We need additional information before we can continue label Jan 15, 2025
@quaff
Copy link
Contributor Author

quaff commented Jan 16, 2025

  1. Could you please rebase this on top of the head of main to address the conflicts?

done.

  1. Did you consider making getDatabase() of the Sentinel instance returned by PropertiesRedisConnectionDetails use the database from the URL? I am in two minds as to whether that's the right thing to do.

I think sentinel should respect database in url also, I appended another commit, feel free to squash or drop it.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jan 16, 2025
@wilkinsona wilkinsona removed the status: feedback-provided Feedback has been provided label Jan 21, 2025
@wilkinsona wilkinsona modified the milestones: 3.5.x, 3.5.0-M1 Jan 21, 2025
wilkinsona pushed a commit that referenced this pull request Jan 21, 2025
See gh-43813

Signed-off-by: Yanming Zhou <zhouyanming@gmail.com>
@wilkinsona
Copy link
Member

Thanks very much, @quaff.

wilkinsona added a commit that referenced this pull request Jan 21, 2025
This reverts commit 4478bd5, reversing
changes made to c032e1f.
@wilkinsona
Copy link
Member

wilkinsona commented Jan 21, 2025

Unfortunately, I've just reverted this as, while updating the release notes, I realised that the implementation's incorrect.

The property's documentation states that the URL overrides the database (as it should do) but the implementation doesn't do so. Unlike username, password, etc that are completely overridden by the URL, database has been implemented such that the property is used as a fallback when the URL doesn't have a database. I don't think this is right. Instead, it should use the default database which is 0.

@quaff, can you please update your proposal so that ConnectionInfo defaults to 0 for its database value when the URL does not specify the database. The database value from ConnectionInfo should then always be used when the URL has been configured. This will align the behaviour of the database property with that of the hostname, port, username, and password properties when the URL property has been set.

@wilkinsona wilkinsona reopened this Jan 21, 2025
@wilkinsona wilkinsona modified the milestones: 3.5.0-M1, 3.5.x Jan 21, 2025
@wilkinsona wilkinsona added the status: waiting-for-feedback We need additional information before we can continue label Jan 21, 2025
Fix spring-projectsGH-43810

Signed-off-by: Yanming Zhou <zhouyanming@gmail.com>
@quaff
Copy link
Contributor Author

quaff commented Jan 23, 2025

@wilkinsona Good point, I updated the PR.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jan 23, 2025
Database property should be ignored if url is used.

Signed-off-by: Yanming Zhou <zhouyanming@gmail.com>
@wilkinsona wilkinsona removed the status: feedback-provided Feedback has been provided label Jan 23, 2025
@wilkinsona wilkinsona modified the milestones: 3.5.x, 3.5.0-M1 Jan 23, 2025
wilkinsona pushed a commit that referenced this pull request Jan 23, 2025
See gh-43813

Signed-off-by: Yanming Zhou <zhouyanming@gmail.com>
wilkinsona added a commit that referenced this pull request Jan 23, 2025
@wilkinsona
Copy link
Member

Thanks, @quaff.

arefbehboudi pushed a commit to arefbehboudi/spring-boot that referenced this pull request Jan 29, 2025
See spring-projectsgh-43813

Signed-off-by: Yanming Zhou <zhouyanming@gmail.com>
Signed-off-by: arefbehboudi <behboodiaref@gmail.com>
arefbehboudi pushed a commit to arefbehboudi/spring-boot that referenced this pull request Jan 29, 2025
This reverts commit 4478bd5, reversing
changes made to c032e1f.

Signed-off-by: arefbehboudi <behboodiaref@gmail.com>
arefbehboudi pushed a commit to arefbehboudi/spring-boot that referenced this pull request Jan 29, 2025
See spring-projectsgh-43813

Signed-off-by: Yanming Zhou <zhouyanming@gmail.com>
Signed-off-by: arefbehboudi <behboodiaref@gmail.com>
arefbehboudi pushed a commit to arefbehboudi/spring-boot that referenced this pull request Jan 29, 2025
See spring-projectsgh-43813

Signed-off-by: arefbehboudi <behboodiaref@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for configuring the Redis database using spring.data.redis.url
3 participants