feat: add HookDecorator so a hook wrapper forwards the stages it does not override

launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/integrations/DedupingHook.java

@@ -28,12 +28,11 @@
  *         .addHook(new MetricsHook())                                  // observes every evaluation
  *         .addHook(new DedupingHook(new ObservabilityHook()))           // default window
  *         .addHook(new DedupingHook(new TelemetryHook(), 60_000))
- *         .addHook(new DedupingHook(new ExperimentHook(), myCustomDeduper))
+ *         .addHook(new DedupingHook(new ExperimentHook(), sharedDeduper))
  * </code></pre>
  * <p>
  * Two evaluations resolve to the same result when they agree on everything
- * {@link EvaluationExposureKey} describes. Pass your own {@link EvaluationExposureDeduper} subclass to
- * decide that differently.
+ * {@link EvaluationExposureKey} describes.
  * <p>
  * An evaluation the SDK has no flag data for resolves to the default value, and is the same result as
  * another that does. Evaluations made before the client has flags are of that kind, so the wrapped
@@ -58,7 +57,7 @@
  * it stored in its own before stage. A decorator inside this one is unaffected, since a suppressed
  * evaluation never reaches it.
  */
-public final class DedupingHook extends Hook {
+public final class DedupingHook extends HookDecorator {
 
     /**
      * Reads the clock a window is measured against. Exists so that tests can control it; the SDK has
@@ -80,7 +79,6 @@ interface Clock {
     // Namespaced because it travels in series data that the wrapped hook may also write to.
     private static final String SUPPRESSED = "com.launchdarkly.sdk.android.DedupingHook.suppressed";
 
-    private final Hook delegate;
     private final EvaluationExposureDeduper deduper;
     private final Clock clock;
 
@@ -117,20 +115,11 @@ public DedupingHook(Hook delegate, EvaluationExposureDeduper deduper) {
 
     @VisibleForTesting
     DedupingHook(Hook delegate, EvaluationExposureDeduper deduper, Clock clock) {
-        super(nameOf(delegate));
-        this.delegate = Objects.requireNonNull(delegate, "a deduping hook must wrap a hook");
+        super(delegate);
         this.deduper = Objects.requireNonNull(deduper, "a deduping hook must have a deduper");
         this.clock = clock;
     }
 
-    /**
-     * @return the wrapped hook's metadata, so that the SDK names the hook a stage belongs to
-     */
-    @Override
-    public HookMetadata getMetadata() {
-        return delegate.getMetadata();
-    }
-
     /**
      * Forwards the evaluation unless the wrapped hook has just been told about the same result.
      * <p>
@@ -148,7 +137,7 @@ public Map<String, Object> beforeEvaluation(EvaluationSeriesContext seriesContex
         if (key != null && !deduper.shouldRecord(key, clock.elapsedMillis())) {
             return suppressedSeriesData;
         }
-        return delegate.beforeEvaluation(seriesContext, seriesData);
+        return super.beforeEvaluation(seriesContext, seriesData);
     }
 
     /**
@@ -165,7 +154,7 @@ public Map<String, Object> afterEvaluation(EvaluationSeriesContext seriesContext
         if (seriesData != null && seriesData.get(SUPPRESSED) == this) {
             return seriesData;
         }
-        return delegate.afterEvaluation(seriesContext, seriesData, evaluationDetail);
+        return super.afterEvaluation(seriesContext, seriesData, evaluationDetail);
     }
 
     /**
@@ -183,27 +172,6 @@ public Map<String, Object> afterEvaluation(EvaluationSeriesContext seriesContext
     @Override
     public Map<String, Object> beforeIdentify(IdentifySeriesContext seriesContext, Map<String, Object> seriesData) {
         deduper.reset();
-        return delegate.beforeIdentify(seriesContext, seriesData);
-    }
-
-    @Override
-    public Map<String, Object> afterIdentify(IdentifySeriesContext seriesContext, Map<String, Object> seriesData,
-                                             IdentifySeriesResult result) {
-        return delegate.afterIdentify(seriesContext, seriesData, result);
-    }
-
-    @Override
-    public void afterTrack(TrackSeriesContext seriesContext) {
-        delegate.afterTrack(seriesContext);
-    }
-
-    // Static because it runs in the super() call, before this instance exists. Tolerates a hook whose
-    // metadata throws, which the SDK reports rather than propagates.
-    private static String nameOf(Hook delegate) {
-        try {
-            return delegate == null ? null : delegate.getMetadata().getName();
-        } catch (Exception e) {
-            return null;
-        }
+        return super.beforeIdentify(seriesContext, seriesData);
     }
 }

launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/integrations/EvaluationExposureDeduper.java

@@ -16,16 +16,14 @@
  *         .addHook(new MetricsHook())                        // told about every evaluation
  *         .addHook(new DedupingHook(new ObservabilityHook())) // default window
  *         .addHook(new DedupingHook(new TelemetryHook(), 30_000))
- *         .addHook(new DedupingHook(new ExperimentHook(), myCustomDeduper))
+ *         .addHook(new DedupingHook(new ExperimentHook(), sharedDeduper))
  * </code></pre>
  * <p>
  * This class is the SDK's implementation: it remembers the result each flag last reported, and tells
  * the hook about the flag again as soon as that result changes, or once the window elapses while it
  * stays the same. Tracking one result per flag rather than every result seen keeps a flag that flips
  * back and forth from hiding the flips, and holds one record per flag the application evaluates, so
- * the window is the only thing there is to configure. Subclass this to implement a different policy;
- * only {@link #shouldRecord(EvaluationExposureKey, long)} and {@link #reset()} are called by
- * {@link DedupingHook}.
+ * the window is the only thing there is to configure.
  * <p>
  * A deduper is consulted once per evaluation, before the series opens, so a suppressed evaluation
  * invokes neither {@code beforeEvaluation} nor {@code afterEvaluation}. Implementations must be

launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/integrations/Hook.java

@@ -13,8 +13,10 @@
  * stages in the order they were configured, and each hook's after stages in reverse order. (i.e.
  * myHook1.beforeEvaluation, myHook2.beforeEvaluation, myHook2.afterEvaluation, myHook1.afterEvaluation)
  * <p>
- * To deduplicate the repeated evaluations observed by one hook, wrap it in a
- * {@link DedupingHook} and register the wrapper.
+ * Note that each stage below has a default implementation that does nothing, so a hook implements only
+ * the stages it cares about. A {@link HookDecorator} wraps a hook and forwards every stage to it, which
+ * is what a hook that adds behavior to another hook is built on. To deduplicate the repeated
+ * evaluations observed by one hook, wrap it in a {@link DedupingHook} and register the wrapper.
  */
 public abstract class Hook {
 

launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/integrations/HookDecorator.java

@@ -0,0 +1,172 @@
+package com.launchdarkly.sdk.android.integrations;
+
+import androidx.annotation.CallSuper;
+
+import com.launchdarkly.sdk.EvaluationDetail;
+import com.launchdarkly.sdk.LDValue;
+
+import java.util.Map;
+import java.util.Objects;
+
+/**
+ * A hook that wraps another hook, forwarding every stage to it. A subclass adds behavior to a hook
+ * without changing it, and is registered in place of the hook it wraps.
+ * <p>
+ * Every stage forwards to the wrapped hook, so a subclass overrides only the stages it changes, and
+ * calls {@code super} to forward the ones it does. The stages it leaves alone still reach the wrapped
+ * hook.
+ * <p>
+ * An override that never calls {@code super} stops forwarding that stage, which for the identify and
+ * track stages is a decorator swallowing something it has no reason to: a {@link DedupingHook} inside
+ * such an override would stop being told to forget what it has reported, and would go on suppressing
+ * across an identify. Those three stages are {@link CallSuper}, so Android Lint reports an override of
+ * one that does not forward. The evaluation stages are not, because suppressing an evaluation series is
+ * what a decorator is for.
+ *
+ * <pre><code>
+ *     public final class FlagFilteringHook extends HookDecorator {
+ *         private final Set&lt;String&gt; flagKeys;
+ *
+ *         public FlagFilteringHook(Hook delegate, Set&lt;String&gt; flagKeys) {
+ *             super(delegate);
+ *             this.flagKeys = flagKeys;
+ *         }
+ *
+ *         &#64;Override
+ *         public Map&lt;String, Object&gt; beforeEvaluation(EvaluationSeriesContext seriesContext,
+ *                                                     Map&lt;String, Object&gt; seriesData) {
+ *             return flagKeys.contains(seriesContext.flagKey)
+ *                     ? super.beforeEvaluation(seriesContext, seriesData)
+ *                     : seriesData;
+ *         }
+ *     }
+ * </code></pre>
+ * <p>
+ * That hook filters evaluations and still forwards identify and track, which it never mentions.
+ * <p>
+ * {@link DedupingHook} is the decorator the SDK ships: it forwards an evaluation series only when the
+ * flag's result is one its hook has not just been told about.
+ * <p>
+ * Decorators stack, so a hook may be wrapped in as many as it needs, each wrapping the one inside it:
+ *
+ * <pre><code>
+ *     Components.hooks()
+ *         .addHook(new DedupingHook(new FlagFilteringHook(new ObservabilityHook(), myFlagKeys)))
+ * </code></pre>
+ * <p>
+ * A decorator reports the wrapped hook's metadata as its own, so the SDK names the hook that a stage
+ * belongs to rather than the wrappers around it.
+ * <p>
+ * A decorator that suppresses a stage must suppress the whole evaluation series, because hooks pair
+ * their stages: an observability hook opens a span in
+ * {@link Hook#beforeEvaluation(EvaluationSeriesContext, Map)} and closes it in
+ * {@link Hook#afterEvaluation(EvaluationSeriesContext, Map, EvaluationDetail)}, so suppressing only
+ * the after stage leaves that span open. To carry the decision from one stage to the other, return
+ * series data the after stage recognizes, the way {@link DedupingHook} does.
+ * <p>
+ * A decorator that does that belongs outermost, because the series data it returns replaces what it
+ * was given: a decorator outside it does not get back what it stored in its own before stage.
+ * <p>
+ * This class is not stable, and not subject to any backwards compatibility guarantees or semantic versioning.
+ * It is experimental.
+ */
+public abstract class HookDecorator extends Hook {
+
+    // Reachable only through super calls. Handing the wrapped hook back out invites code to unwrap a
+    // decorator and decide what to do with what it finds, when a decorator is meant to stand in for the
+    // hook it wraps.
+    private final Hook delegate;
+
+    /**
+     * Wraps a hook, taking its name as this decorator's own.
+     *
+     * @param delegate the hook to forward each stage to
+     */
+    protected HookDecorator(Hook delegate) {
+        super(nameOf(delegate));
+        this.delegate = Objects.requireNonNull(delegate, "a decorator must wrap a hook");
+    }
+
+    /**
+     * @return the wrapped hook's metadata, so that the SDK names the hook a stage belongs to
+     */
+    @Override
+    public HookMetadata getMetadata() {
+        return delegate.getMetadata();
+    }
+
+    /**
+     * Forwards the stage to the wrapped hook.
+     *
+     * @param seriesContext container of parameters associated with this evaluation
+     * @param seriesData    immutable data from the previous stage in the evaluation series
+     * @return the wrapped hook's series data
+     */
+    @Override
+    public Map<String, Object> beforeEvaluation(EvaluationSeriesContext seriesContext, Map<String, Object> seriesData) {
+        return delegate.beforeEvaluation(seriesContext, seriesData);
+    }
+
+    /**
+     * Forwards the stage to the wrapped hook.
+     *
+     * @param seriesContext    container of parameters associated with this evaluation
+     * @param seriesData       data from the previous stage in the evaluation series
+     * @param evaluationDetail the result of the evaluation
+     * @return the wrapped hook's series data
+     */
+    @Override
+    public Map<String, Object> afterEvaluation(EvaluationSeriesContext seriesContext, Map<String, Object> seriesData,
+                                               EvaluationDetail<LDValue> evaluationDetail) {
+        return delegate.afterEvaluation(seriesContext, seriesData, evaluationDetail);
+    }
+
+    /**
+     * Forwards the stage to the wrapped hook.
+     *
+     * @param seriesContext container of parameters associated with this identify
+     * @param seriesData    immutable data from the previous stage in the identify series
+     * @return the wrapped hook's series data
+     */
+    @CallSuper
+    @Override
+    public Map<String, Object> beforeIdentify(IdentifySeriesContext seriesContext, Map<String, Object> seriesData) {
+        return delegate.beforeIdentify(seriesContext, seriesData);
+    }
+
+    /**
+     * Forwards the stage to the wrapped hook.
+     *
+     * @param seriesContext container of parameters associated with this identify
+     * @param seriesData    data from the previous stage in the identify series
+     * @param result        the result of the identify operation
+     * @return the wrapped hook's series data
+     */
+    @CallSuper
+    @Override
+    public Map<String, Object> afterIdentify(IdentifySeriesContext seriesContext, Map<String, Object> seriesData,
+                                             IdentifySeriesResult result) {
+        return delegate.afterIdentify(seriesContext, seriesData, result);
+    }
+
+    /**
+     * Forwards the stage to the wrapped hook.
+     *
+     * @param seriesContext container of parameters associated with this track
+     */
+    @CallSuper
+    @Override
+    public void afterTrack(TrackSeriesContext seriesContext) {
+        delegate.afterTrack(seriesContext);
+    }
+
+    // Static because it runs in the super() call, before this instance exists. Tolerates a hook whose
+    // metadata throws, which the SDK reports rather than propagates.
+    private static String nameOf(Hook delegate) {
+        try {
+            return delegate == null ? null : delegate.getMetadata().getName();
+        } catch (Exception e) {
+            return null;
+        }
+    }
+}

launchdarkly-android-client-sdk/src/test/java/com/launchdarkly/sdk/android/integrations/DedupingHookTest.java

@@ -87,33 +87,30 @@ public void afterTrack(TrackSeriesContext seriesContext) {
         }
     }
 
-    /** A wrapper with its own behavior, to check that hook wrappers compose. */
-    private static class CountingDecorator extends Hook {
-        private final Hook delegate;
+    /**
+     * A wrapper with its own behavior, to check that hook wrappers compose.
+     * <p>
+     * Overrides only the evaluation stages, as a wrapper a customer writes would: the rest are inherited.
+     */
+    private static class CountingDecorator extends HookDecorator {
         int evaluationsForwarded = 0;
         int resultsForwarded = 0;
 
         CountingDecorator(Hook delegate) {
-            super(delegate.getMetadata().getName());
-            this.delegate = delegate;
-        }
-
-        @Override
-        public HookMetadata getMetadata() {
-            return delegate.getMetadata();
+            super(delegate);
         }
 
         @Override
         public Map<String, Object> beforeEvaluation(EvaluationSeriesContext seriesContext, Map<String, Object> seriesData) {
             evaluationsForwarded++;
-            return delegate.beforeEvaluation(seriesContext, seriesData);
+            return super.beforeEvaluation(seriesContext, seriesData);
         }
 
         @Override
         public Map<String, Object> afterEvaluation(EvaluationSeriesContext seriesContext, Map<String, Object> seriesData,
                                                    EvaluationDetail<LDValue> evaluationDetail) {
             resultsForwarded++;
-            return delegate.afterEvaluation(seriesContext, seriesData, evaluationDetail);
+            return super.afterEvaluation(seriesContext, seriesData, evaluationDetail);
         }
     }
 
@@ -266,6 +263,19 @@ public void forwardsTheStagesItDoesNotDeduplicate() {
         assertEquals(List.of("beforeIdentify", "afterIdentify", "afterTrack"), hook.stages);
     }
 
+    @Test
+    public void aDecoratorForwardsTheStagesItDoesNotOverride() {
+        RecordingHook hook = new RecordingHook("decorated");
+        HookRunner runner = runner(EXPOSURE_KEY, new CountingDecorator(hook));
+
+        identify(runner);
+        runner.afterTrack("event-key", LDContext.create("user-123"), LDValue.ofNull(), null);
+
+        // The decorator mentions neither identify nor track, and the hook it wraps is still told about
+        // both: a stage a decorator leaves alone is forwarded rather than dropped.
+        assertEquals(List.of("beforeIdentify", "afterIdentify", "afterTrack"), hook.stages);
+    }
+
     @Test
     public void forwardsAnEvaluationWhoseResultTheSdkDidNotDescribe() {
         RecordingHook hook = new RecordingHook("deduping");