-
Notifications
You must be signed in to change notification settings - Fork 32
Fix: Added notificationRegistry to make sure that odpSettings updates #501
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 10 commits
a006cf0
ed2bcbb
51357cf
f1c3d7a
5d791f2
01fd113
9c4f729
92004cf
01313a4
ecb12b5
38d735a
0deec47
e6bbc30
43da46a
0fe4fb3
dd6a823
680a5bf
65c150d
5bc8ffd
26d50ae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
/** | ||
* | ||
* Copyright 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 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
package com.optimizely.ab.internal; | ||
|
||
import com.optimizely.ab.notification.NotificationCenter; | ||
|
||
import javax.annotation.Nonnull; | ||
import java.util.Map; | ||
import java.util.concurrent.ConcurrentHashMap; | ||
|
||
public class NotificationRegistry { | ||
private final static Map<String, NotificationCenter> _notificationCenters = new ConcurrentHashMap<>(); | ||
|
||
private NotificationRegistry() | ||
{ | ||
} | ||
|
||
public static NotificationCenter getInternalNotificationCenter(@Nonnull String sdkKey) | ||
{ | ||
NotificationCenter notificationCenter = null; | ||
if (sdkKey != null) { | ||
if (_notificationCenters.containsKey(sdkKey)) { | ||
notificationCenter = _notificationCenters.get(sdkKey); | ||
} else { | ||
notificationCenter = new NotificationCenter(); | ||
_notificationCenters.put(sdkKey, notificationCenter); | ||
} | ||
} | ||
return notificationCenter; | ||
} | ||
|
||
public static void clearNotificationCenterRegistry(@Nonnull String sdkKey) { | ||
if (sdkKey != null) { | ||
_notificationCenters.remove(sdkKey); | ||
} | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,84 @@ | ||
/** | ||
* | ||
* Copyright 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 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
package com.optimizely.ab.internal; | ||
|
||
import com.optimizely.ab.notification.NotificationCenter; | ||
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; | ||
import org.junit.Assert; | ||
import org.junit.Test; | ||
|
||
import static org.junit.Assert.assertEquals; | ||
import static org.junit.Assert.assertNull; | ||
|
||
|
||
public class NotificationRegistryTest { | ||
|
||
@SuppressFBWarnings("NP_NONNULL_PARAM_VIOLATION") | ||
@Test | ||
public void getNullNotificationCenterWhenSDKeyIsNull() { | ||
String sdkKey = null; | ||
NotificationCenter notificationCenter = NotificationRegistry.getInternalNotificationCenter(sdkKey); | ||
assertNull(notificationCenter); | ||
} | ||
|
||
@Test | ||
public void getSameNotificationCenterWhenSDKKeyIsSameButNotNull() { | ||
String sdkKey = "testSDkKey"; | ||
NotificationCenter notificationCenter1 = NotificationRegistry.getInternalNotificationCenter(sdkKey); | ||
NotificationCenter notificationCenter2 = NotificationRegistry.getInternalNotificationCenter(sdkKey); | ||
assertEquals(notificationCenter1, notificationCenter2); | ||
} | ||
|
||
@Test | ||
public void getSameNotificationCenterWhenSDKKeyIsEmpty() { | ||
String sdkKey1 = ""; | ||
String sdkKey2 = ""; | ||
NotificationCenter notificationCenter1 = NotificationRegistry.getInternalNotificationCenter(sdkKey1); | ||
NotificationCenter notificationCenter2 = NotificationRegistry.getInternalNotificationCenter(sdkKey2); | ||
assertEquals(notificationCenter1, notificationCenter2); | ||
} | ||
|
||
@Test | ||
public void getDifferentNotificationCenterWhenSDKKeyIsNotSame() { | ||
String sdkKey1 = "testSDkKey1"; | ||
String sdkKey2 = "testSDkKey2"; | ||
NotificationCenter notificationCenter1 = NotificationRegistry.getInternalNotificationCenter(sdkKey1); | ||
NotificationCenter notificationCenter2 = NotificationRegistry.getInternalNotificationCenter(sdkKey2); | ||
Assert.assertNotEquals(notificationCenter1, notificationCenter2); | ||
} | ||
|
||
@Test | ||
public void clearRegistryNotificationCenterClearsOldNotificationCenter() { | ||
String sdkKey1 = "testSDkKey1"; | ||
NotificationCenter notificationCenter1 = NotificationRegistry.getInternalNotificationCenter(sdkKey1); | ||
NotificationRegistry.clearNotificationCenterRegistry(sdkKey1); | ||
NotificationCenter notificationCenter2 = NotificationRegistry.getInternalNotificationCenter(sdkKey1); | ||
|
||
Assert.assertNotEquals(notificationCenter1, notificationCenter2); | ||
} | ||
|
||
@SuppressFBWarnings("NP_NONNULL_PARAM_VIOLATION") | ||
@Test | ||
public void clearRegistryNotificationCenterWillNotCauseExceptionIfPassedNullSDkKey() { | ||
String sdkKey1 = "testSDkKey1"; | ||
NotificationCenter notificationCenter1 = NotificationRegistry.getInternalNotificationCenter(sdkKey1); | ||
NotificationRegistry.clearNotificationCenterRegistry(null); | ||
NotificationCenter notificationCenter2 = NotificationRegistry.getInternalNotificationCenter(sdkKey1); | ||
|
||
Assert.assertEquals(notificationCenter1, notificationCenter2); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -65,6 +65,7 @@ public class HttpProjectConfigManager extends PollingProjectConfigManager { | |
private final URI uri; | ||
private final String datafileAccessToken; | ||
private String datafileLastModified; | ||
private String sdkKey; | ||
|
||
private HttpProjectConfigManager(long period, | ||
TimeUnit timeUnit, | ||
|
@@ -73,11 +74,13 @@ private HttpProjectConfigManager(long period, | |
String datafileAccessToken, | ||
long blockingTimeoutPeriod, | ||
TimeUnit blockingTimeoutUnit, | ||
NotificationCenter notificationCenter) { | ||
NotificationCenter notificationCenter, | ||
String sdkKey) { | ||
|
||
super(period, timeUnit, blockingTimeoutPeriod, blockingTimeoutUnit, notificationCenter); | ||
this.httpClient = httpClient; | ||
this.uri = URI.create(url); | ||
this.datafileAccessToken = datafileAccessToken; | ||
this.sdkKey = sdkKey; | ||
} | ||
|
||
public URI getUri() { | ||
|
@@ -167,6 +170,11 @@ public static Builder builder() { | |
return new Builder(); | ||
} | ||
|
||
@Override | ||
public String getSDKKey() { | ||
return this.sdkKey; | ||
} | ||
|
||
public static class Builder { | ||
private String datafile; | ||
private String url; | ||
|
@@ -357,7 +365,8 @@ public HttpProjectConfigManager build(boolean defer) { | |
datafileAccessToken, | ||
blockingTimeoutPeriod, | ||
blockingTimeoutUnit, | ||
notificationCenter); | ||
notificationCenter, | ||
sdkKey); | ||
mnoman09 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
||
if (datafile != null) { | ||
try { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PollingProjectConfigManager can also have a preset projectConfig from a fallback datafile. In that case, we have to call
updateODPSettings
for the projectConfig immediately (we may not get notification later if the fallback is updated already).I think we can keep the
getProjectConfig() != null
condition as before, with the change that `getProjectConfig() returns null immediately if config is not available.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getProjectConfig() != null
condition is not possible without wait, with our current implementation. However, we can use the similar approach as we are using in Csharp, adding additional function ofgetCachedConfig()
inProjectConfigManager
. Now the only concern I have, related to this approach, is that do we want to enforce its implementation or return null by default as we are doing forgetSDKKey()
function?In my opinion we should enforce it because if anyone uses custom
ProjectConfigManager
without givinggetCachedConfig
implementation then they won't be able use ODP as it only gets config using this new function.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mnoman09 Agreed. Let's make them (
getSdkKey
andgetCachedConfig
) mandatory changes and keep them in the list of breaking changes for release.