Make AddHealthieRedis one method, because two could not be told apart - #41
Merged
Conversation
AddHealthieRedis("localhost:6379") did not register a Redis connection. It
registered "localhost:6379" as a key prefix.
There were two overloads: one taking a configuration string and an optional
prefix, one taking just an optional prefix. Given a single string argument C#
prefers the second -- no optional parameter left unfilled -- so the connection
string silently became the prefix, no IConnectionMultiplexer was registered,
and the failure surfaced later as a missing service. That is the exact call in
the package README, so the documented usage was broken.
Nothing caught it because every test built RedisStateProvider directly. The
provider was well covered and its registration was not covered at all.
One method now, with configuration optional: pass it to have the connection
opened, leave it out to use the one the application already registers. There
is nothing left to resolve between. Three tests go through AddHealthieRedis
rather than around it -- with a configuration string, with an existing
connection, and with only a named prefix -- and the first of those fails
against the two-overload version.
Found by building the packages into a local feed and consuming them from a
project outside the repository, which is the only thing that exercises what
dotnet add package actually hands someone.
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.
AddHealthieRedis("localhost:6379")did not register a Redis connection. It registered"localhost:6379"as a key prefix.There were two overloads:
Given a single string argument, C# prefers the second — no optional parameter left unfilled. So the connection string silently became the prefix, no
IConnectionMultiplexerwas ever registered, and the failure surfaced later as:That is the exact call the package README documents, so the documented usage was broken.
Why nothing caught it
Every Redis test constructed
RedisStateProviderdirectly. The provider had 15 tests and a 12-writer race; its registration had none. Well-covered code behind an unreachable front door.The fix
One method,
configurationoptional. Pass it to have the connection opened; leave it out to use the one the application already registers. There is nothing left for overload resolution to get wrong, andkeyPrefixhas to be named to be passed alone.Three new tests go through
AddHealthieRedisrather than around it — with a configuration string, with an existing connection, and with only a named prefix. The first fails against the two-overload version:How it was found
By packing all 20 packages into a local feed and consuming them from a console project outside the repository —
dotnet add package Healthie.NET.Redis, then actually running it against a real Redis. That is the only thing that exercises what a consumer is handed, and it is a step I had not taken for any package until now.Verification
466 tests green on net8.0 and net10.0, 0 errors and 0 warnings on a cleaned build.