From 2e0424be615d7ce79687007cedba7375da1be7a8 Mon Sep 17 00:00:00 2001 From: "FOLIO3PK\\muhammadnoman" Date: Thu, 27 Sep 2018 17:38:09 +0500 Subject: [PATCH 1/3] Fix: Allowing Empty UserID as valid field --- .../main/java/com/optimizely/ab/Optimizely.java | 4 ---- .../com/optimizely/ab/config/ProjectConfig.java | 2 +- .../java/com/optimizely/ab/OptimizelyTest.java | 14 +++++--------- 3 files changed, 6 insertions(+), 14 deletions(-) diff --git a/core-api/src/main/java/com/optimizely/ab/Optimizely.java b/core-api/src/main/java/com/optimizely/ab/Optimizely.java index e4070e681..197601947 100644 --- a/core-api/src/main/java/com/optimizely/ab/Optimizely.java +++ b/core-api/src/main/java/com/optimizely/ab/Optimizely.java @@ -872,10 +872,6 @@ private boolean validateUserId(String userId) { logger.error("The user ID parameter must be nonnull."); return false; } - if (userId.trim().isEmpty()) { - logger.error("Non-empty user ID required"); - return false; - } return true; } diff --git a/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java b/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java index 25a84f823..37958af96 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java +++ b/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java @@ -537,7 +537,7 @@ public boolean setForcedVariation(@Nonnull String experimentKey, @Nonnull String userId) { // if the user id is invalid, return false. - if (userId == null || userId.trim().isEmpty()) { + if (userId == null) { logger.error("User ID is invalid"); return null; } diff --git a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java index 1add5241e..21abaf28c 100644 --- a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java +++ b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java @@ -991,7 +991,7 @@ public void activateUserNoAttributesWithAudiences() throws Exception { } /** - * Verify that {@link Optimizely#activate(String, String)} doesn't return a variation when provided an empty string. + * Verify that {@link Optimizely#activate(String, String)} return a variation when provided an empty string. */ @Test public void activateWithEmptyUserId() throws Exception { @@ -1003,9 +1003,7 @@ public void activateWithEmptyUserId() throws Exception { .withErrorHandler(new RaiseExceptionErrorHandler()) .build(); - logbackVerifier.expectMessage(Level.ERROR, "Non-empty user ID required"); - logbackVerifier.expectMessage(Level.INFO, "Not activating user for experiment \"" + experimentKey + "\"."); - assertNull(optimizely.activate(experimentKey, "")); + assertNotNull(optimizely.activate(experimentKey, "")); } /** @@ -2297,7 +2295,7 @@ public void getVariationWithUnknownExperimentKeyAndRaiseExceptionErrorHandler() } /** - * Verify that {@link Optimizely#getVariation(String, String)} doesn't return a variation when provided an + * Verify that {@link Optimizely#getVariation(String, String)} return a variation when provided an * empty string. */ @Test @@ -2309,8 +2307,7 @@ public void getVariationWithEmptyUserId() throws Exception { .withErrorHandler(new RaiseExceptionErrorHandler()) .build(); - logbackVerifier.expectMessage(Level.ERROR, "Non-empty user ID required"); - assertNull(optimizely.getVariation(experiment.getKey(), "")); + assertNotNull(optimizely.getVariation(experiment.getKey(), "")); } /** @@ -3653,8 +3650,7 @@ public void getEnabledFeatureWithEmptyUserId() throws ConfigParseException{ .build()); ArrayList featureFlags = (ArrayList) spyOptimizely.getEnabledFeatures("", new HashMap()); - logbackVerifier.expectMessage(Level.ERROR, "Non-empty user ID required"); - assertTrue(featureFlags.isEmpty()); + assertFalse(featureFlags.isEmpty()); } From 35ea56081338ea7a8bddfeb24264d3a0331d099b Mon Sep 17 00:00:00 2001 From: "FOLIO3PK\\muhammadnoman" Date: Thu, 27 Sep 2018 18:02:53 +0500 Subject: [PATCH 2/3] Removed userid.isempty check --- .../java/com/optimizely/ab/config/ProjectConfig.java | 2 +- .../com/optimizely/ab/config/ProjectConfigTest.java | 10 +++------- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java b/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java index 37958af96..95fa5f7c6 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java +++ b/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java @@ -479,7 +479,7 @@ public boolean setForcedVariation(@Nonnull String experimentKey, } // if the user id is invalid, return false. - if (userId == null || userId.trim().isEmpty()) { + if (userId == null) { logger.error("User ID is invalid"); return false; } diff --git a/core-api/src/test/java/com/optimizely/ab/config/ProjectConfigTest.java b/core-api/src/test/java/com/optimizely/ab/config/ProjectConfigTest.java index 5fd28dcab..c19e2fdfb 100644 --- a/core-api/src/test/java/com/optimizely/ab/config/ProjectConfigTest.java +++ b/core-api/src/test/java/com/optimizely/ab/config/ProjectConfigTest.java @@ -31,11 +31,7 @@ import static java.util.Arrays.asList; import static org.hamcrest.CoreMatchers.is; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertThat; -import static org.junit.Assert.assertTrue; -import static org.junit.Assert.assertEquals; +import static org.junit.Assert.*; import com.optimizely.ab.internal.LogbackVerifier; @@ -244,12 +240,12 @@ public void getForcedVariationNullUserId() { @Test public void setForcedVariationEmptyUserId() { - assertFalse(projectConfig.setForcedVariation("etag1", "", "vtag1")); + assertTrue(projectConfig.setForcedVariation("etag1", "", "vtag1")); } @Test public void getForcedVariationEmptyUserId() { - assertNull(projectConfig.getForcedVariation("etag1", "")); + assertNotNull(projectConfig.getForcedVariation("etag1", "")); } /* Invalid Experiement */ From afda184204b6607ad7810b1c86ea24db4b0ff18f Mon Sep 17 00:00:00 2001 From: "FOLIO3PK\\muhammadnoman" Date: Mon, 8 Oct 2018 17:36:15 +0500 Subject: [PATCH 3/3] made function of validateUserID in projectConfig to check if userID is valid --- .../optimizely/ab/config/ProjectConfig.java | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java b/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java index 301c02fbc..3b57cf75c 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java +++ b/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java @@ -501,8 +501,7 @@ public boolean setForcedVariation(@Nonnull String experimentKey, } // if the user id is invalid, return false. - if (userId == null) { - logger.error("User ID is invalid"); + if (!validateUserId(userId)) { return false; } @@ -559,8 +558,7 @@ public boolean setForcedVariation(@Nonnull String experimentKey, @Nonnull String userId) { // if the user id is invalid, return false. - if (userId == null) { - logger.error("User ID is invalid"); + if (!validateUserId(userId)) { return null; } @@ -592,6 +590,21 @@ public boolean setForcedVariation(@Nonnull String experimentKey, return null; } + /** + * Helper function to check that the provided userId is valid + * + * @param userId the userId being validated + * @return whether the user ID is valid + */ + private boolean validateUserId(String userId) { + if (userId == null) { + logger.error("User ID is invalid"); + return false; + } + + return true; + } + @Override public String toString() { return "ProjectConfig{" +