Unleash / unleash-client-dotnet

Unleash client SDK for .NET
Apache License 2.0
77 stars 39 forks source link

Empty userId breaks gradual rollout 100% expected behavior. #215

Closed karimkod closed 2 months ago

karimkod commented 3 months ago

Describe the bug When passing an empty userId (string.Empty) the toggle is not enabled. Although the toggle have a gradual rollout strategy of 100%. When passing a null or a non-empty value, the same toogle is enabled as expected.

To Reproduce Steps to reproduce the behavior: Create a toggle with a gradual rollout strategy. Pass the userId as string.empty ("") :

  var unleashEnabled = unleash.IsEnabled("togglename", new UnleashContext()
  {

      UserId = string.Empty

  });

Expected behavior The value of unleashedEnabled should be always true, since the gradual rollout is 100% and there is not constraints. Screenshots If applicable, add screenshots to help explain your problem.

Version

    <PackageReference Include="Unleash.Client" Version="4.1.8" />

Additional context Did my own digging and I think that the source is the calculation of stickinessId https://github.com/Unleash/unleash-client-dotnet/blob/main/src/Unleash/Strategies/FlexibleRolloutStrategy.cs#L43 When userId is empty, it is returned as the first choice of stickinessId but then we do the if (!string.IsNullOrEmpty(stickinessId)) which will be false, always.

karimkod commented 3 months ago

If that's a real issue, I'm ready to open a PR, thanks :) Need to be iso with other sdks behavior. Checked node sdk, it doesn't exclude empty userId: https://github.com/Unleash/unleash-client-node/blob/main/src/strategy/flexible-rollout-strategy.ts

sighphyre commented 2 months ago

Hey @karimkod,

Thanks for reporting this! Yes, this appears to be a bug. Empty string should not be doing this!

Oh if you're willing to submit a PR that would be amazing!

karimkod commented 2 months ago

@sighphyre here it is #216