From 64630bdf7e91dddd8399946cc27f7b7a47f2fc2f Mon Sep 17 00:00:00 2001 From: Mike Chu Date: Thu, 30 Nov 2023 15:07:26 -0500 Subject: [PATCH 1/3] feat: log duplicate experiment keys Also refactored private fields to readonly & standard names --- .../OptlyConfig/OptimizelyConfigService.cs | 30 ++++++++++++------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/OptimizelySDK/OptlyConfig/OptimizelyConfigService.cs b/OptimizelySDK/OptlyConfig/OptimizelyConfigService.cs index 29e47441..ff9e8c0c 100644 --- a/OptimizelySDK/OptlyConfig/OptimizelyConfigService.cs +++ b/OptimizelySDK/OptlyConfig/OptimizelyConfigService.cs @@ -1,11 +1,11 @@ /* - * Copyright 2019-2021, Optimizely + * Copyright 2019-2021, 2023, Optimizely * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at * - * http://www.apache.org/licenses/LICENSE-2.0 + * https://www.apache.org/licenses/LICENSE-2.0 * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, @@ -15,30 +15,34 @@ */ using System; -using System.Collections; using System.Collections.Generic; using System.Linq; using System.Text; using Newtonsoft.Json; using Newtonsoft.Json.Linq; using OptimizelySDK.Entity; +using OptimizelySDK.Logger; namespace OptimizelySDK.OptlyConfig { public class OptimizelyConfigService { - private OptimizelyConfig OptimizelyConfig; + private readonly OptimizelyConfig _optimizelyConfig; - private IDictionary> featureIdVariablesMap; + private readonly IDictionary> _featureIdVariablesMap; - public OptimizelyConfigService(ProjectConfig projectConfig) + private readonly ILogger _logger; + + public OptimizelyConfigService(ProjectConfig projectConfig, ILogger logger = null) { if (projectConfig == null) { return; } - featureIdVariablesMap = GetFeatureVariablesByIdMap(projectConfig); + _logger = logger ?? new DefaultLogger(); + + _featureIdVariablesMap = GetFeatureVariablesByIdMap(projectConfig); var attributes = GetAttributes(projectConfig); var audiences = GetAudiences(projectConfig); var experimentsMapById = GetExperimentsMapById(projectConfig); @@ -47,7 +51,7 @@ public OptimizelyConfigService(ProjectConfig projectConfig) var featureMap = GetFeaturesMap(projectConfig, experimentsMapById); var events = GetEvents(projectConfig); - OptimizelyConfig = new OptimizelyConfig(projectConfig.Revision, + _optimizelyConfig = new OptimizelyConfig(projectConfig.Revision, projectConfig.SDKKey, projectConfig.EnvironmentKey, attributes, @@ -118,6 +122,12 @@ IDictionary experimentsMapById foreach (var experiment in experimentsMapById.Values) { + if (experimentKeyMaps.ContainsKey(experiment.Key)) + { + _logger.Log(LogLevel.WARN, + $"Duplicate experiment keys found in datafile: {experiment.Key}"); + } + experimentKeyMaps[experiment.Key] = experiment; } @@ -210,7 +220,7 @@ bool isFeatureEnabled if (!string.IsNullOrEmpty(featureId)) { - variablesMap = featureIdVariablesMap[featureId]?. + variablesMap = _featureIdVariablesMap[featureId]?. Select(f => new OptimizelyVariable(f.Id, f.Key, f.Type.ToString().ToLower(), @@ -451,7 +461,7 @@ private IDictionary GetVariableIdMap(ProjectConfig proj public OptimizelyConfig GetOptimizelyConfig() { - return OptimizelyConfig; + return _optimizelyConfig; } } } From e45067d0bafd4ee64fa411fc217af48f441ec091 Mon Sep 17 00:00:00 2001 From: Mike Chu Date: Thu, 30 Nov 2023 15:09:10 -0500 Subject: [PATCH 2/3] test: updated existing dupe exp keys test Also refactored private fields to readonly & names --- .../OptimizelyConfigTest.cs | 38 +++++++++++-------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/OptimizelySDK.Tests/OptimizelyConfigTests/OptimizelyConfigTest.cs b/OptimizelySDK.Tests/OptimizelyConfigTests/OptimizelyConfigTest.cs index 10883416..d49d6245 100644 --- a/OptimizelySDK.Tests/OptimizelyConfigTests/OptimizelyConfigTest.cs +++ b/OptimizelySDK.Tests/OptimizelyConfigTests/OptimizelyConfigTest.cs @@ -1,5 +1,5 @@ /* - * Copyright 2020-2022, Optimizely + * Copyright 2020-2023, Optimizely * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -29,28 +29,30 @@ namespace OptimizelySDK.Tests.OptimizelyConfigTests [TestFixture] public class OptimizelyConfigTest { - private Mock LoggerMock; + private Mock _loggerMock; [SetUp] public void Setup() { - LoggerMock = new Mock(); - LoggerMock.Setup(l => l.Log(It.IsAny(), It.IsAny())); + _loggerMock = new Mock(); + _loggerMock.Setup(l => l.Log(It.IsAny(), It.IsAny())); } #region Test OptimizelyConfigService - private static Type[] ParameterTypes = + private static readonly Type[] parameterTypes = { typeof(ProjectConfig), + typeof(ILogger), }; private PrivateObject CreatePrivateOptimizelyConfigService(ProjectConfig projectConfig) { - return new PrivateObject(typeof(OptimizelyConfigService), ParameterTypes, + return new PrivateObject(typeof(OptimizelyConfigService), parameterTypes, new object[] { projectConfig, + new NoOpLogger(), }); } @@ -144,22 +146,26 @@ public void TestGetOptimizelyConfigServiceNullConfig() } [Test] - [Obsolete] public void TestGetOptimizelyConfigWithDuplicateExperimentKeys() { + // OptimizelySDK.Tests/similar_exp_keys.json has DUPLICATED_EXPERIMENT_KEY + const string DUPLICATED_EXPERIMENT_KEY = "targeted_delivery"; + const string SECOND_DUPLICATED_EXPERIMENT_ID = "9300000007573"; var datafileProjectConfig = DatafileProjectConfig.Create( - TestData.DuplicateExpKeysDatafile, new NoOpLogger(), + TestData.DuplicateExpKeysDatafile, + new NoOpLogger(), new ErrorHandler.NoOpErrorHandler()); - var optimizelyConfigService = new OptimizelyConfigService(datafileProjectConfig); + var optimizelyConfigService = + new OptimizelyConfigService(datafileProjectConfig, _loggerMock.Object); + var optimizelyConfig = optimizelyConfigService.GetOptimizelyConfig(); - Assert.AreEqual(optimizelyConfig.ExperimentsMap.Count, 1); - var experimentMapFlag1 = - optimizelyConfig.FeaturesMap["flag1"].ExperimentsMap; //9300000007569 - var experimentMapFlag2 = - optimizelyConfig.FeaturesMap["flag2"].ExperimentsMap; // 9300000007573 - Assert.AreEqual(experimentMapFlag1["targeted_delivery"].Id, "9300000007569"); - Assert.AreEqual(experimentMapFlag2["targeted_delivery"].Id, "9300000007573"); + Assert.AreEqual(optimizelyConfig.ExperimentsMap.Count, 1); + Assert.AreEqual(optimizelyConfig.ExperimentsMap[DUPLICATED_EXPERIMENT_KEY].Id, + SECOND_DUPLICATED_EXPERIMENT_ID); + _loggerMock.Verify( + l => l.Log(LogLevel.WARN, + "Duplicate experiment keys found in datafile: targeted_delivery"), Times.Once); } [Test] From 3af8396403f57c7f44ca7ffe1ca92c70ea6d7cc0 Mon Sep 17 00:00:00 2001 From: Mike Chu Date: Fri, 1 Dec 2023 13:30:29 -0500 Subject: [PATCH 3/3] fix: remove `readonly` from fields that aren't mine --- OptimizelySDK/OptlyConfig/OptimizelyConfigService.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/OptimizelySDK/OptlyConfig/OptimizelyConfigService.cs b/OptimizelySDK/OptlyConfig/OptimizelyConfigService.cs index ff9e8c0c..bf2973c6 100644 --- a/OptimizelySDK/OptlyConfig/OptimizelyConfigService.cs +++ b/OptimizelySDK/OptlyConfig/OptimizelyConfigService.cs @@ -27,9 +27,9 @@ namespace OptimizelySDK.OptlyConfig { public class OptimizelyConfigService { - private readonly OptimizelyConfig _optimizelyConfig; + private OptimizelyConfig _optimizelyConfig; - private readonly IDictionary> _featureIdVariablesMap; + private IDictionary> _featureIdVariablesMap; private readonly ILogger _logger;