search

Flagsmith / flagsmith-dotnet-client

.NET Standard Client for Flagsmith. Ship features with confidence using feature flags and remote config. Host yourself or use our hosted version at https://www.flagsmith.com/
https://www.flagsmith.com/
BSD 3-Clause "New" or "Revised" License
19 stars 12 forks source link

Providing an OfflineHandler causes the SDK to bypass live flag lookups (even if OfflineMode is false) #103

Open brendenk-cdata opened 5 days ago

brendenk-cdata commented 5 days ago

Bug Report

.NET SDK Version: 5.3.0

Description of problem

We're trying to make use of the offline handler feature to provide default flags if the SDK cannot contact our Flagsmith server for some reason. I.e., we're trying to use it for scenario 2 listed in this section of your documentation: https://docs.flagsmith.com/clients/server-side#using-an-offline-handler.

We have OfflineMode disabled, and have an instance of your LocalFileHandler class set as the offline handler. This is how we're instantiating the FlagsmithClient class when we register it with DI: image

Expected Behavior

The SDK should still attempt live API calls to our Flagsmith server to obtain flag values, only using the offline handler as a fallback if the live API calls fail.

Actual Behavior

The SDK never attempts any live API calls, it just uses the offline file directly.

Analysis

The bug is easily reproducible because we can change the value of a flag on the Flagsmith server, but if we debug our code, the results of the FlagsmithClient.GetEnvironmentFlags() call shows us that the flag always has its offline file value.

Looking at the source code for FlagsmithClient, the cause becomes clear:

Requested Changes

Please change the logic of the FlagsmithClient such that the Environment model is not automatically registered from the offline handler during construction if offline mode is false. I am not sure what-all considerations will need to be made in order to make this change, but that's the basic idea.

As it stands, this bug completely invalidates the scenario of using an offline handler for default flag handling, so we would appreciate it if it could be fixed with some priority.

brendenk-cdata commented 5 days ago

Just to follow up on this, I've been looking at the code a bit more. It seems to me that the initial changes to add support for the offline handler basically tried to shoe-horn in support by riding on the existing Environment variable, which is really more for the use of the local evaluation feature (and seems like it would work in that scenario).

In the offlineMode = true case, this would be fine because you're setting Environment immediately, which (with the understanding of what I've described in the original issue) works because the currently code always uses Environment if it's available.

But for the offlineMode = false case, the fact that it's setting Environment from the offline handler at all is fundamentally wrong. It shouldn't be asking the offline handler for its environment model at all until it's already failed to do a live lookup.

Anyway—that's about as far as I can understand it, and unfortunately my company is unlikely to let me pursue making a fix myself here, so I'm hoping someone on the Flagsmith team (or another contributor) has some bandwidth for this.

rolodato commented 2 days ago

@brendenk-cdata Offline mode intentionally will never make any Flagsmith API calls - this is the expected behaviour across all SDKs. An example use case for offline mode is to run automated tests that should not require Flagsmith access.

Have you looked into using defaultFlagHandler instead? https://docs.flagsmith.com/clients/server-side?language=dotnet#managing-default-flags