From e6fdf0ccdb1ec340fc4f7d683be2eabed464b6be Mon Sep 17 00:00:00 2001 From: NomanShoaib Date: Thu, 20 Apr 2023 08:32:56 +0500 Subject: [PATCH 1/2] [FSSDK-9076] fix: send odp event empty values --- .../java/com/optimizely/ab/Optimizely.java | 6 + .../java/com/optimizely/ab/odp/ODPEvent.java | 2 +- .../optimizely/ab/odp/ODPEventManager.java | 28 +++- .../com/optimizely/ab/odp/ODPUserKey.java | 6 +- .../com/optimizely/ab/OptimizelyTest.java | 122 ++++++++++++++++++ .../ab/odp/ODPEventManagerTest.java | 67 ++++++++++ 6 files changed, 227 insertions(+), 4 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 fe28a8bb4..e4a4b0343 100644 --- a/core-api/src/main/java/com/optimizely/ab/Optimizely.java +++ b/core-api/src/main/java/com/optimizely/ab/Optimizely.java @@ -1479,6 +1479,12 @@ public ODPManager getODPManager() { */ public void sendODPEvent(@Nullable String type, @Nonnull String action, @Nullable Map identifiers, @Nullable Map data) { if (odpManager != null) { + if (action == null || action.trim().isEmpty()) + { + logger.error("ODP action is not valid (cannot be empty)."); + return; + } + ODPEvent event = new ODPEvent(type, action, identifiers, data); odpManager.getEventManager().sendEvent(event); } else { diff --git a/core-api/src/main/java/com/optimizely/ab/odp/ODPEvent.java b/core-api/src/main/java/com/optimizely/ab/odp/ODPEvent.java index 71e054df0..a505bf6d1 100644 --- a/core-api/src/main/java/com/optimizely/ab/odp/ODPEvent.java +++ b/core-api/src/main/java/com/optimizely/ab/odp/ODPEvent.java @@ -30,7 +30,7 @@ public class ODPEvent { @Nonnull private Map data; public ODPEvent(@Nullable String type, @Nonnull String action, @Nullable Map identifiers, @Nullable Map data) { - this.type = type == null ? EVENT_TYPE_FULLSTACK : type; + this.type = type == null || type.trim().isEmpty() ? EVENT_TYPE_FULLSTACK : type; this.action = action; this.identifiers = identifiers != null ? identifiers : Collections.emptyMap(); this.data = data != null ? data : Collections.emptyMap(); diff --git a/core-api/src/main/java/com/optimizely/ab/odp/ODPEventManager.java b/core-api/src/main/java/com/optimizely/ab/odp/ODPEventManager.java index 352c47eac..8e5f975b2 100644 --- a/core-api/src/main/java/com/optimizely/ab/odp/ODPEventManager.java +++ b/core-api/src/main/java/com/optimizely/ab/odp/ODPEventManager.java @@ -35,6 +35,10 @@ public class ODPEventManager { private static final int DEFAULT_FLUSH_INTERVAL = 1000; private static final int MAX_RETRIES = 3; private static final String EVENT_URL_PATH = "/v3/events"; + private static final List FS_USER_ID_MATCHES = new ArrayList<>(Arrays.asList( + ODPUserKey.FS_USER_ID.getKeyString(), + ODPUserKey.FS_USER_ID_ALIAS.getKeyString() + )); private final int queueSize; private final int batchSize; @@ -120,7 +124,7 @@ public void identifyUser(@Nullable String vuid, @Nullable String userId) { public void sendEvent(ODPEvent event) { event.setData(augmentCommonData(event.getData())); - event.setIdentifiers(augmentCommonIdentifiers(event.getIdentifiers())); + event.setIdentifiers(convertCriticalIdentifiers(augmentCommonIdentifiers(event.getIdentifiers()))); if (!event.isIdentifiersValid()) { logger.error("ODP event send failed (event identifiers must have at least one key-value pair)"); @@ -132,6 +136,7 @@ public void sendEvent(ODPEvent event) { return; } + processEvent(event); } @@ -157,6 +162,27 @@ protected Map augmentCommonIdentifiers(Map sourc Map identifiers = new HashMap<>(); identifiers.putAll(userCommonIdentifiers); identifiers.putAll(sourceIdentifiers); + + return identifiers; + } + + private static Map convertCriticalIdentifiers(Map identifiers) { + + if (identifiers.containsKey(ODPUserKey.FS_USER_ID.getKeyString())) { + return identifiers; + } + + List> identifiersList = new ArrayList<>(identifiers.entrySet()); + + for (Map.Entry kvp : identifiersList) { + + if (FS_USER_ID_MATCHES.contains(kvp.getKey().toLowerCase())) { + identifiers.remove(kvp.getKey()); + identifiers.put(ODPUserKey.FS_USER_ID.getKeyString(), kvp.getValue()); + break; + } + } + return identifiers; } diff --git a/core-api/src/main/java/com/optimizely/ab/odp/ODPUserKey.java b/core-api/src/main/java/com/optimizely/ab/odp/ODPUserKey.java index d7cdbb641..ef0bce3ff 100644 --- a/core-api/src/main/java/com/optimizely/ab/odp/ODPUserKey.java +++ b/core-api/src/main/java/com/optimizely/ab/odp/ODPUserKey.java @@ -1,6 +1,6 @@ /** * - * Copyright 2022, Optimizely + * Copyright 2022-2023, Optimizely * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -20,7 +20,9 @@ public enum ODPUserKey { VUID("vuid"), - FS_USER_ID("fs_user_id"); + FS_USER_ID("fs_user_id"), + + FS_USER_ID_ALIAS("fs-user-id"); private final String keyString; 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 705ce1cb6..700780f75 100644 --- a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java +++ b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java @@ -4762,6 +4762,128 @@ public void sendODPEvent() { assertEquals(data, eventArgument.getValue().getData()); } + @Test + @SuppressFBWarnings(value = "NP_NONNULL_PARAM_VIOLATION", justification = "Testing nullness contract violation") + public void sendODPEventErrorNullAction() { + ProjectConfigManager mockProjectConfigManager = mock(ProjectConfigManager.class); + Mockito.when(mockProjectConfigManager.getConfig()).thenReturn(validProjectConfig); + ODPEventManager mockODPEventManager = mock(ODPEventManager.class); + ODPManager mockODPManager = mock(ODPManager.class); + + Mockito.when(mockODPManager.getEventManager()).thenReturn(mockODPEventManager); + Optimizely optimizely = Optimizely.builder() + .withConfigManager(mockProjectConfigManager) + .withODPManager(mockODPManager) + .build(); + + verify(mockODPEventManager).start(); + + Map identifiers = new HashMap<>(); + identifiers.put("id1", "value1"); + identifiers.put("id2", "value2"); + + Map data = new HashMap<>(); + data.put("sdk", "java"); + data.put("revision", 52); + + optimizely.sendODPEvent("fullstack", null, identifiers, data); + logbackVerifier.expectMessage(Level.ERROR, "ODP action is not valid (cannot be empty)."); + } + + @Test + public void sendODPEventErrorEmptyAction() { + ProjectConfigManager mockProjectConfigManager = mock(ProjectConfigManager.class); + Mockito.when(mockProjectConfigManager.getConfig()).thenReturn(validProjectConfig); + ODPEventManager mockODPEventManager = mock(ODPEventManager.class); + ODPManager mockODPManager = mock(ODPManager.class); + + Mockito.when(mockODPManager.getEventManager()).thenReturn(mockODPEventManager); + Optimizely optimizely = Optimizely.builder() + .withConfigManager(mockProjectConfigManager) + .withODPManager(mockODPManager) + .build(); + + verify(mockODPEventManager).start(); + + Map identifiers = new HashMap<>(); + identifiers.put("id1", "value1"); + identifiers.put("id2", "value2"); + + Map data = new HashMap<>(); + data.put("sdk", "java"); + data.put("revision", 52); + + optimizely.sendODPEvent("fullstack", "", identifiers, data); + logbackVerifier.expectMessage(Level.ERROR, "ODP action is not valid (cannot be empty)."); + } + + @Test + @SuppressFBWarnings(value = "NP_NONNULL_PARAM_VIOLATION", justification = "Testing nullness contract violation") + public void sendODPEventNullType() { + ProjectConfigManager mockProjectConfigManager = mock(ProjectConfigManager.class); + Mockito.when(mockProjectConfigManager.getConfig()).thenReturn(validProjectConfig); + ODPEventManager mockODPEventManager = mock(ODPEventManager.class); + ODPManager mockODPManager = mock(ODPManager.class); + + Mockito.when(mockODPManager.getEventManager()).thenReturn(mockODPEventManager); + Optimizely optimizely = Optimizely.builder() + .withConfigManager(mockProjectConfigManager) + .withODPManager(mockODPManager) + .build(); + + verify(mockODPEventManager).start(); + + Map identifiers = new HashMap<>(); + identifiers.put("id1", "value1"); + identifiers.put("id2", "value2"); + + Map data = new HashMap<>(); + data.put("sdk", "java"); + data.put("revision", 52); + + optimizely.sendODPEvent(null, "identify", identifiers, data); + ArgumentCaptor eventArgument = ArgumentCaptor.forClass(ODPEvent.class); + verify(mockODPEventManager).sendEvent(eventArgument.capture()); + + assertEquals("fullstack", eventArgument.getValue().getType()); + assertEquals("identify", eventArgument.getValue().getAction()); + assertEquals(identifiers, eventArgument.getValue().getIdentifiers()); + assertEquals(data, eventArgument.getValue().getData()); + } + + @Test + public void sendODPEventEmptyType() { + ProjectConfigManager mockProjectConfigManager = mock(ProjectConfigManager.class); + Mockito.when(mockProjectConfigManager.getConfig()).thenReturn(validProjectConfig); + ODPEventManager mockODPEventManager = mock(ODPEventManager.class); + ODPManager mockODPManager = mock(ODPManager.class); + + Mockito.when(mockODPManager.getEventManager()).thenReturn(mockODPEventManager); + Optimizely optimizely = Optimizely.builder() + .withConfigManager(mockProjectConfigManager) + .withODPManager(mockODPManager) + .build(); + + verify(mockODPEventManager).start(); + + Map identifiers = new HashMap<>(); + identifiers.put("id1", "value1"); + identifiers.put("id2", "value2"); + + Map data = new HashMap<>(); + data.put("sdk", "java"); + data.put("revision", 52); + + optimizely.sendODPEvent("", "identify", identifiers, data); + ArgumentCaptor eventArgument = ArgumentCaptor.forClass(ODPEvent.class); + verify(mockODPEventManager).sendEvent(eventArgument.capture()); + + assertEquals("fullstack", eventArgument.getValue().getType()); + assertEquals("identify", eventArgument.getValue().getAction()); + assertEquals(identifiers, eventArgument.getValue().getIdentifiers()); + assertEquals(data, eventArgument.getValue().getData()); + } + @Test public void sendODPEventError() { ProjectConfigManager mockProjectConfigManager = mock(ProjectConfigManager.class); diff --git a/core-api/src/test/java/com/optimizely/ab/odp/ODPEventManagerTest.java b/core-api/src/test/java/com/optimizely/ab/odp/ODPEventManagerTest.java index d2163a6b6..8f47ec973 100644 --- a/core-api/src/test/java/com/optimizely/ab/odp/ODPEventManagerTest.java +++ b/core-api/src/test/java/com/optimizely/ab/odp/ODPEventManagerTest.java @@ -226,6 +226,73 @@ public void prepareCorrectPayloadForIdentifyUser() throws InterruptedException { } } + @Test + public void preparePayloadForIdentifyUserWithVariationsOfFsUserId() throws InterruptedException { + Mockito.reset(mockApiManager); + Mockito.when(mockApiManager.sendEvents(any(), any(), any())).thenReturn(202); + int flushInterval = 1; + ODPConfig odpConfig = new ODPConfig("key", "http://www.odp-host.com", null); + ODPEventManager eventManager = new ODPEventManager(mockApiManager, null, flushInterval); + eventManager.updateSettings(odpConfig); + eventManager.start(); + ODPEvent event1 = new ODPEvent("fullstack", + "identified", + new HashMap() {{ + put("fs-user-id", "123"); + }}, null); + + ODPEvent event2 = new ODPEvent("fullstack", + "identified", + new HashMap() {{ + put("FS-user-ID", "123"); + }}, null); + + ODPEvent event3 = new ODPEvent("fullstack", + "identified", + new HashMap() {{ + put("FS_USER_ID", "123"); + put("fs.user.id", "456"); + }}, null); + + ODPEvent event4 = new ODPEvent("fullstack", + "identified", + new HashMap() {{ + put("fs_user_id", "123"); + put("fsuserid", "456"); + }}, null); + List> expectedIdentifiers = new ArrayList>() {{ + add(new HashMap() {{ + put("fs_user_id", "123"); + }}); + add(new HashMap() {{ + put("fs_user_id", "123"); + }}); + add(new HashMap() {{ + put("fs_user_id", "123"); + put("fs.user.id", "456"); + }}); + add(new HashMap() {{ + put("fs_user_id", "123"); + put("fsuserid", "456"); + }}); + }}; + eventManager.sendEvent(event1); + eventManager.sendEvent(event2); + eventManager.sendEvent(event3); + eventManager.sendEvent(event4); + + Thread.sleep(1500); + Mockito.verify(mockApiManager, times(1)).sendEvents(eq("key"), eq("http://www.odp-host.com/v3/events"), payloadCaptor.capture()); + + String payload = payloadCaptor.getValue(); + JSONArray events = new JSONArray(payload); + assertEquals(4, events.length()); + for (int i = 0; i < events.length(); i++) { + JSONObject event = events.getJSONObject(i); + assertEquals(event.getJSONObject("identifiers").toMap(), expectedIdentifiers.get(i)); + } + } + @Test public void identifyUserWithVuidAndUserId() throws InterruptedException { ODPEventManager eventManager = spy(new ODPEventManager(mockApiManager)); From e966e21f776bef1be7724d194da7193530ba47df Mon Sep 17 00:00:00 2001 From: NomanShoaib Date: Thu, 20 Apr 2023 08:39:27 +0500 Subject: [PATCH 2/2] nit fix --- core-api/src/main/java/com/optimizely/ab/Optimizely.java | 3 +-- 1 file changed, 1 insertion(+), 2 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 e4a4b0343..c8ca3d55b 100644 --- a/core-api/src/main/java/com/optimizely/ab/Optimizely.java +++ b/core-api/src/main/java/com/optimizely/ab/Optimizely.java @@ -1479,8 +1479,7 @@ public ODPManager getODPManager() { */ public void sendODPEvent(@Nullable String type, @Nonnull String action, @Nullable Map identifiers, @Nullable Map data) { if (odpManager != null) { - if (action == null || action.trim().isEmpty()) - { + if (action == null || action.trim().isEmpty()) { logger.error("ODP action is not valid (cannot be empty)."); return; }