Xabaril / Esquio

Esquio is a Feature Toggle Library for .NET Developers.
Apache License 2.0
428 stars 49 forks source link

Toggle parameters missing for non default delpoyments in DetailsConfigurationRequestHandler.cs #144

Closed gkfischer closed 4 years ago

gkfischer commented 4 years ago

I added a ClaimValueToggle to a feature 'TechnicalAdministration' for a deployment named 'Dev' in product 'RCM' with parameter key 'Role' and value 'Admin'. But although the specified claim is present in the users tokens, the evalation of the feature always returned false and an exception like this was logged:

[13:43:55 ERR Esquio] DefaultFeatureService threw an unhandled exception checking TechnicalAdministration-RCM-Dev.
System.Collections.Generic.KeyNotFoundException: The given key 'ClaimType' was not present in the dictionary.
   at System.Collections.Generic.Dictionary`2.get_Item(TKey key)
   at Esquio.AspNetCore.Toggles.ClaimValueToggle.IsActiveAsync(ToggleExecutionContext context, CancellationToken cancellationToken) in D:\repos\Esquiov3\src\Esquio.AspNetCore\Toggles\ClaimValueToggle.cs:line 36
   at Esquio.DefaultFeatureService.GetRuntimeEvaluationResult(String featureName, String productName, String deploymentName, Guid correlationId, CancellationToken cancellationToken) in D:\repos\Esquiov3\src\Esquio\DefaultFeatureService.cs:line 116
   at Esquio.DefaultFeatureService.IsEnabledAsync(String featureName, CancellationToken cancellationToken) in D:\repos\Esquiov3\src\Esquio\DefaultFeatureService.cs:line 54

After quite a bit of investigating things I discovered the following code block in DetailsConfigurationRequestHandler.cs - starting line 104

               if (deploymentName != EsquioConstants.DEFAULT_DEPLOYMENT_NAME)
                {
                    var selectedRingParameters = groupingParameters
                        .Where(g => g.Key == deploymentName)
                        .SingleOrDefault();

                    if (selectedRingParameters != null
                        &&
                        selectedRingParameters.Any())
                    {
                        foreach (var item in selectedRingParameters)
                        {
                            if (parameters.ContainsKey(item.Name))
                            {
                                parameters[item.Name] = item.Value;
                            }
                        }
                    }
                }

The line if (parameters.ContainsKey(item.Name)) actually results in no parameters being added, as the parameters dicitionary is initialy empty and the preceeding code only handles the default deployment name. I think the line should read if (!parameters.ContainsKey(item.Name)) or the clause could be omitted altogether.

unaizorrilla commented 4 years ago

Hi @gkfischer

Let me check this! But some questions!!

Are you sure the claim type is correctly configured?

gkfischer commented 4 years ago

Yes the claim is configured properly, and was working with the previous version of Esquio. And it does work with my suggested change/fix. So it has to be there :-)

unaizorrilla commented 4 years ago

Ok let me check and fix

unaizorrilla commented 4 years ago

Hi @gkfischer

I can't reproduce the issue, i follow next steps and all is working fine:

image

                .services
                .AddEsquio(setup=>
                {
                    setup.UseScopedEvaluation(useScopedEvaluation: true);
                    setup.ConfigureDefaultDeploymentName("production");
                    setup.ConfigureDefaultProductName("myproduct");
                })

And all is working fine!! I try to investigate more but if you can send the steps to reproduce it will be apreceited!

unaizorrilla commented 4 years ago

The line if (parameters.ContainsKey(item.Name)) actually results in no parameters being added, as the parameters dicitionary is initialy empty and the preceeding code only handles the default deployment name. I think the line should read if (!parameters.ContainsKey(item.Name)) or the clause could be omitted altogether.

This is not true, the behavior is , by default all configured parameters on default deployment ( Tests ) are included and if this environment ( production ) override some value modify it!

Are you modifying the database content directly, ie, without Esquio UI?

gkfischer commented 4 years ago

I am using the Http.Store to connect to a running instance of the API. Maybe that's the difference. I included the source code on the API side instead of the nuget and debugged it. When the API is repsonding with the ConfigurationController to a Get request the processing ends up in class DetailsConfigurationRequestHandler -> CreateResponse. In line 95 the parameters dictionary is initialized. The next code block is handling the default deployment name and after that the dictionary is still empty. When it comes to the block for non default names, the selectedRingParameters dictionary is build correctly, as the data is returned from the database. But because of the if clause in line 126 the parameters are not added as the parameters dictionary is empty.

Probably that's because I modifed the default deployment name, or better said changed it in the database if I remember correctly. We dont have a deployment 'Tests' at all, ours is called 'Test' and the default implementation doesn't know about that.

image

gkfischer commented 4 years ago

So probably the references to EsquioConstants.DEFAULT_DEPLOYMENT_NAME should be replaced by a reference to the default deployment from the database.

unaizorrilla commented 4 years ago

Hi @gkfischer

I am using the Http.Store to connect to a running instance of the API. Maybe that's the difference. I included the source code on the API side instead of the nuget and debugged it.

I use the same a web app using http store in the scenario mentioned!

Probably that's because I modifed the default deployment name, or better said changed it in the database if I remember correctly. We dont have a deployment 'Tests' at all, ours is called 'Test' and the default implementation doesn't know about that.

This is de problem, default deployment name is Test you can't change it ( on V3.X this is a feature on V4.X ), this is not allowed on the UI and you shouldn't modify databse directly because the api contains some logic when parameters are added, modified etc

unaizorrilla commented 4 years ago

So probably the references to EsquioConstants.DEFAULT_DEPLOYMENT_NAME should be replaced by a reference to the default deployment from the database.

Yeap, we are working on it for V4 ( probably to be released on August )

gkfischer commented 4 years ago

Hm, having a deployment that is unnecessary doesn't realy solve the problem and is pretty confusing. We are using deployments dev, test, int, demo and prod. To throw in tests as well is not gonna work. How about exposing the default names through config?

unaizorrilla commented 4 years ago

Yeap, agree. I will work on this soon and could be happy your feedback on this. Meanwhile on V3 you need to maintain Tests and modify the configuration using the API ( Esquio UI or http request ) but not modifying directly the db!

Thanks

gkfischer commented 4 years ago

Ok, if I can be of any help in testing or whatever I'm glad to help. Cool stuff you build. thanks for the quick help.