From 0716b7e9815a4783a503cf59adbfb3a5b18ffc9c Mon Sep 17 00:00:00 2001 From: Louis Chan <91093020+louis-launchdarkly@users.noreply.github.com> Date: Wed, 31 May 2023 13:39:19 -0700 Subject: [PATCH 1/3] fix: Fix the README link to LaunchDarkly docs page (#294) Just a simple link fix before I get distracted by something else. --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index f2099fc4..33f60035 100644 --- a/README.md +++ b/README.md @@ -18,7 +18,7 @@ Refer to the [SDK documentation](https://docs.launchdarkly.com/sdk/client-side/a ## Learn more -Check out our [documentation](https://docs.launchdarkly.com) for in-depth instructions on configuring and using LaunchDarkly. You can also head straight to the [complete reference guide for this SDK](https://docs.launchdarkly.com/docs/android-sdk-reference) or our [Javadocs](http://launchdarkly.github.io/android-client-sdk/). +Check out our [documentation](https://docs.launchdarkly.com) for in-depth instructions on configuring and using LaunchDarkly. You can also head straight to the [complete reference guide for this SDK](https://docs.launchdarkly.com/sdk/client-side/android) or our [Javadocs](http://launchdarkly.github.io/android-client-sdk/). ## Testing From 9b5e80d4e4371742c19f31008ebc6ef30294ef11 Mon Sep 17 00:00:00 2001 From: Todd Anderson Date: Fri, 2 Jun 2023 09:57:47 -0500 Subject: [PATCH 2/3] Fixing identify listener bug. Now checking value instead of versionf or listener callback --- .../sdk/android/ContextDataManager.java | 14 ++++++- .../ContextDataManagerListenersTest.java | 40 +++++++++++++++++++ 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/ContextDataManager.java b/launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/ContextDataManager.java index a1f97805..d004d85c 100644 --- a/launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/ContextDataManager.java +++ b/launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/ContextDataManager.java @@ -182,11 +182,18 @@ private void initDataInternal( Set updatedFlagKeys = new HashSet<>(); for (Flag newFlag: newData.values()) { Flag oldFlag = oldData.getFlag(newFlag.getKey()); - if (oldFlag == null || oldFlag.getVersion() != newFlag.getVersion()) { + if (oldFlag == null || !oldFlag.getValue().equals(newFlag.getValue())) { + // if the flag is new or the value has changed, notify. This logic can be run if + // the context changes, which can result in an evaluation change even if the version + // of the flag stays the same. You will notice this logic slightly differs from + // upsert. Upsert should only be calling to listeners if the value has changed, + // but we left upsert alone out of fear of that we'd uncover bugs in customer code + // if we added conditionals in upsert updatedFlagKeys.add(newFlag.getKey()); } } for (Flag oldFlag: oldData.values()) { + // if old flag is no longer appearing, notify if (newData.getFlag(oldFlag.getKey()) == null) { updatedFlagKeys.add(oldFlag.getKey()); } @@ -283,6 +290,10 @@ public boolean upsert(@NonNull Flag flag) { environmentStore.setContextData(contextId, updatedFlags); Collection updatedFlag = Collections.singletonList(flag.getKey()); + + // We really should only be calling to listeners if the value has changed, but we left this + // unconditional out of fear that we'd uncover bugs in customer code as a result of + // conditionally notifying listeners notifyAllFlagsListeners(updatedFlag); notifyFlagListeners(updatedFlag); @@ -343,6 +354,7 @@ private void notifyFlagListeners(Collection updatedFlagKeys) { if (listenersToCall.isEmpty()) { return; } + // We make sure to call listener callbacks on the main thread, as we consistently did so in // the past by virtue of using SharedPreferences to implement the callbacks. taskExecutor.executeOnMainThread(() -> { diff --git a/launchdarkly-android-client-sdk/src/test/java/com/launchdarkly/sdk/android/ContextDataManagerListenersTest.java b/launchdarkly-android-client-sdk/src/test/java/com/launchdarkly/sdk/android/ContextDataManagerListenersTest.java index 33f3373e..e6550290 100644 --- a/launchdarkly-android-client-sdk/src/test/java/com/launchdarkly/sdk/android/ContextDataManagerListenersTest.java +++ b/launchdarkly-android-client-sdk/src/test/java/com/launchdarkly/sdk/android/ContextDataManagerListenersTest.java @@ -4,6 +4,8 @@ import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import com.launchdarkly.sdk.LDContext; +import com.launchdarkly.sdk.LDValue; import com.launchdarkly.sdk.android.DataModel.Flag; import org.junit.Test; @@ -86,6 +88,44 @@ public void listenerIsNotCalledAfterUnregistering() throws InterruptedException verifyAll(); } + @Test + public void listenerIsCalledAfterInitData() { + + LDContext context = LDContext.create("user"); + final String FLAG_KEY = "key"; + Flag flagState1 = new FlagBuilder(FLAG_KEY).value(LDValue.of(1)).build(); + + final ContextDataManager manager = createDataManager(); + + // register some listeners + AwaitableFlagListener specific1 = new AwaitableFlagListener(); + AwaitableFlagListener all1 = new AwaitableFlagListener(); + manager.registerListener(FLAG_KEY, specific1); + manager.registerAllFlagsListener(all1); + + // change the data + manager.upsert(flagState1); + + // verify callbacks + assertEquals(FLAG_KEY, specific1.expectUpdate(5, TimeUnit.SECONDS)); + assertEquals(FLAG_KEY, all1.expectUpdate(5, TimeUnit.SECONDS)); + + // register some more listeners + AwaitableFlagListener specific2 = new AwaitableFlagListener(); + AwaitableFlagListener all2 = new AwaitableFlagListener(); + manager.registerListener(FLAG_KEY, specific2); + manager.registerAllFlagsListener(all2); + + // simulate switching context + Flag flagState2 = new FlagBuilder(FLAG_KEY).value(LDValue.of(2)).build(); + EnvironmentData envData = new EnvironmentData().withFlagUpdatedOrAdded(flagState2); + manager.initData(context, envData); + + // verify callbacks + assertEquals(FLAG_KEY, specific2.expectUpdate(5, TimeUnit.SECONDS)); + assertEquals(FLAG_KEY, all2.expectUpdate(5, TimeUnit.SECONDS)); + } + @Test public void listenerIsCalledOnMainThread() throws InterruptedException { Flag flag = Flag.deletedItemPlaceholder("flag", 1); From 37a1cd2c6363492cb6a74b55f48bb0dd2ff585bf Mon Sep 17 00:00:00 2001 From: Louis Chan <91093020+louis-launchdarkly@users.noreply.github.com> Date: Tue, 6 Jun 2023 15:13:46 -0700 Subject: [PATCH 3/3] fix: add the result callback for ping stream mode (#296) **Requirements** - [x] I have added test coverage for new or changed functionality - [x] I have followed the repository's [pull request submission guidelines](../blob/main/CONTRIBUTING.md#submitting-pull-requests) - [x] I have validated my changes against all supported platform versions **Related issues** https://app.shortcut.com/launchdarkly/story/205006/escalation-customer-found-that-after-upgrading-the-rn-sdk-from-6-2-4-to-7-1-4-our-android-app-s-ld-integration-started-failing **Describe the solution you've provided** Because of the rework of the SDK streaming data source module, we missed a change to signal to the SDK that initialization is complete, for the ping and fetch case (which uses by Relay). This callback happens for both Android 3.x and iOS currently, so we are adding this back to Android 4.x. Issue: The method used are private and hard to test individually. **Describe alternatives you've considered** Potentially making some method public, or making a public testing wrapper of a method that we don't expose. But that is a janky pattern so I want to discuss. **Additional context** N/A --- .../sdk/android/StreamingDataSource.java | 6 +- .../sdk/android/StreamingDataSourceTest.java | 59 +++++++++++++++++++ 2 files changed, 63 insertions(+), 2 deletions(-) diff --git a/launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/StreamingDataSource.java b/launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/StreamingDataSource.java index a1d3ce8e..96b7dcf2 100644 --- a/launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/StreamingDataSource.java +++ b/launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/StreamingDataSource.java @@ -2,6 +2,7 @@ import androidx.annotation.NonNull; import androidx.annotation.Nullable; +import androidx.annotation.VisibleForTesting; import com.launchdarkly.eventsource.EventHandler; import com.launchdarkly.eventsource.EventSource; @@ -202,7 +203,8 @@ private URI getUri(@Nullable LDContext context) { return uri; } - private void handle(final String name, final String eventData, + @VisibleForTesting + void handle(final String name, final String eventData, @NonNull final Callback resultCallback) { switch (name.toLowerCase()) { case PUT: @@ -226,7 +228,7 @@ private void handle(final String name, final String eventData, break; case PING: ConnectivityManager.fetchAndSetData(fetcher, currentContext, dataSourceUpdateSink, - LDUtil.noOpCallback(), logger); + resultCallback, logger); break; default: logger.debug("Found an unknown stream protocol: {}", name); diff --git a/launchdarkly-android-client-sdk/src/test/java/com/launchdarkly/sdk/android/StreamingDataSourceTest.java b/launchdarkly-android-client-sdk/src/test/java/com/launchdarkly/sdk/android/StreamingDataSourceTest.java index 90339a4e..2c6a086c 100644 --- a/launchdarkly-android-client-sdk/src/test/java/com/launchdarkly/sdk/android/StreamingDataSourceTest.java +++ b/launchdarkly-android-client-sdk/src/test/java/com/launchdarkly/sdk/android/StreamingDataSourceTest.java @@ -1,14 +1,18 @@ package com.launchdarkly.sdk.android; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; import com.launchdarkly.sdk.LDContext; +import com.launchdarkly.sdk.android.subsystems.Callback; import com.launchdarkly.sdk.android.subsystems.ClientContext; import com.launchdarkly.sdk.android.subsystems.DataSource; import org.junit.Rule; import org.junit.Test; +import java.io.IOException; + public class StreamingDataSourceTest { // This class doesn't currently include any tests done with real HTTP, because we don't yet have // a good solution for setting up an embedded HTTP server with chunked streaming data. When we @@ -37,6 +41,39 @@ private ClientContext makeClientContext(boolean inBackground, Boolean previously ); } + // When react to a PING message from the stream, + // the StreamingDataSource will try to use the fetcher + // to get flag data, so we need to create a ClientContext (Android runtime state) + // that has a fetcher + private ClientContext makeClientContextWithFetcher() { + ClientContext baseClientContext = ClientContextImpl.fromConfig( + new LDConfig.Builder().build(), "", "", makeFeatureFetcher(), CONTEXT, + logging.logger, platformState, taskExecutor); + return ClientContextImpl.forDataSource( + baseClientContext, + dataSourceUpdateSink, + CONTEXT, + false, //Not in background + false //Not Previously in background + ); + } + private FeatureFetcher makeFeatureFetcher() { + return new FeatureFetcher() { + @Override + public void close() throws IOException { + // Do nothing + } + + @Override + public void fetch(LDContext context, Callback callback) { + String json = "{" + + "\"flag1\":{\"key\":\"flag1\",\"version\":200,\"value\":false}" + + "}"; + callback.onSuccess(json); + } + }; + } + @Test public void builderCreatesStreamingDataSourceWhenStartingInForeground() { ClientContext clientContext = makeClientContext(false, null); @@ -86,4 +123,26 @@ public void builderCreatesStreamingDataSourceWhenStartingInBackgroundWithOverrid assertEquals(StreamingDataSource.class, ds.getClass()); } + @Test + public void handlePingMessageBehavior() { + ClientContext clientContext = makeClientContextWithFetcher(); + StreamingDataSource sds = (StreamingDataSource) Components.streamingDataSource() + .streamEvenInBackground(true) + .build(clientContext); + + final Boolean[] callbackInvoked = {false}; + Callback callback = new Callback() { + @Override + public void onSuccess(Boolean result) { + callbackInvoked[0] = true; + } + + @Override + public void onError(Throwable error) { + // We are testing the callback is getting call, not which callback is called + } + }; + sds.handle("ping", null, callback); + assertTrue(callbackInvoked[0]); + } }