Add redis database support

[petergallagher]

A redis connection string can have a database number after the port, separated by a slash.

Currently if you try and this in the redis server strings it is not passed in to the client so you can only use database 0.

This gets the path and removes the / and casts it to an int as all redis dbs are integers. If the path is not a valid numeric it will set it to 0 which is the current behaviour.

[acelaya]

Thanks!

I didn't even consider this situation. Have you managed to test your changes with Shlink and verify it works?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (7d8e6c0) to head (892a56b).
Report is 5 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##                main      #150   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
- Complexity       191       192    +1     
===========================================
  Files             49        49           
  Lines            580       585    +5     
===========================================
+ Hits             580       585    +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[petergallagher]

I've changed the format of the error message and added the todo. I also applied it to my local shlink and confirmed I can write keys to different databases, and it continues to write to db0 if no database is provided.

[acelaya]

I've changed the format of the error message and added the todo. I also applied it to my local shlink and confirmed I can write keys to different databases, and it continues to write to db0 if no database is provided.

Thanks!

Could you also point me to the redis documentation that explains this "indexed" database system?

Also, is using the URI path for that the "standard" approach, or was that your idea? (EDIT: I haven't found any official mention to this, but I've found search results for other tools using this same approach)

[petergallagher]

There isn't a great deal of information about the databases on the official redis website, the command you use to choose one is https://redis.io/docs/latest/commands/select/, this has a bit more information https://www.digitalocean.com/community/cheatsheets/how-to-manage-redis-databases-and-keys. It's mostly just like using a key prefix but it's more baked in to the connection so you don't have to worry about libraries you don't control using any namespace you want to limit them to which is what we were trying to do here.

Using the data from path I did because that's where parse_url puts everything after the port so I could just fetch it out without adding too much complication for a minor change. I looked at how the Symfony redis cache creates it's connection (https://github.com/symfony/symfony/blob/7.2/src/Symfony/Component/Cache/Traits/RedisTrait.php#L87) and they use parse_url to validate the connection string too but the rest of it is far more complex, but maybe it could be a good idea to use that to create a connection instead of parsing the string in this project?

[acelaya]

There isn't a great deal of information about the databases on the official redis website, the command you use to choose one is https://redis.io/docs/latest/commands/select/, this has a bit more information https://www.digitalocean.com/community/cheatsheets/how-to-manage-redis-databases-and-keys

Thanks! This is more than enough.

Using the data from path I did because that's where parse_url puts everything after the port so I could just fetch it out without adding too much complication for a minor change

Yeah, I know you have to read it from the path key. I meant if you used the path in the connection URI for a reason, or just because it felt right.

I guess it's probably a bit of both, but seems to be a sensible decision as far as I can see.

Originally, the decision to require redis servers to be provided in the form of connection URIs is because they used to be passed verbatim to Predis, and later on I started to parse them beforehand to support other stuff.

So one good reason to go with server-index-in-path would be if Predis would work also with that format, which I'm almost sure it would.

I looked at how the Symfony redis cache creates it's connection (https://github.com/symfony/symfony/blob/7.2/src/Symfony/Component/Cache/Traits/RedisTrait.php#L87) and they use parse_url to validate the connection string too but the rest of it is far more complex, but maybe it could be a good idea to use that to create a connection instead of parsing the string in this project?

Symfony's logic is way too complex, and tries to outsmart you in many ways (too many for my taste), so I prefer to keep a very limited but straightforward and easy to debug custom piece of logic here.

Among other things, Symfony sports many other connection "drivers" that Shlink doesn't need to support.

[petergallagher]

Oh I see what you mean about the path, this is how redis does it, so if you wanted to connect on a specific database in the cli you would do redis-cli -u redis://shlink_redis:6379/5.

[acelaya]

Oh I see what you mean about the path, this is how redis does it, so if you wanted to connect on a specific database in the cli you would do redis-cli -u redis://shlink_redis:6379/5.

That's an even better point 🙂

[acelaya]

Just added two small requests. Looking good otherwise.

[acelaya]

Thanks!

[petergallagher]

No problem.