Deprecate append method on all filter factories (#1578)

Motivation: All `*FilterFactory` interfaces currently implement default `append` method that helps to chain filters in order of append. Because internally it uses wrapping approach, it can hide `HttpExecutionStrategyInfluencer` implementation of the passed filter factory. To avoid that, users have to use a method on the client/server builder instead. Modifications: - Deprecate `append` method on all filter factories; - Add `FilterFactoryUtils` test fixture as the replacement utility for tests; Result: Users won't hide `HttpExecutionStrategyInfluencer` impl when they chain filters.
apple · May 26, 2021 · 6f0d87f · 6f0d87f
1 parent ce49400
commit 6f0d87f
Show file tree

Hide file tree

Showing 20 changed files with 233 additions and 95 deletions.
diff --git a/servicetalk-client-api/src/main/java/io/servicetalk/client/api/ConnectionFactoryFilter.java b/servicetalk-client-api/src/main/java/io/servicetalk/client/api/ConnectionFactoryFilter.java
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2018 Apple Inc. and the ServiceTalk project authors
+ * Copyright © 2018, 2021 Apple Inc. and the ServiceTalk project authors
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -48,9 +48,12 @@ public interface ConnectionFactoryFilter<ResolvedAddress, C extends ListenableAs
      * <pre>
      *     filter1 =&gt; filter2 =&gt; filter3 =&gt; original connection factory
      * </pre>
+     *
+     * @deprecated Use {@code appendConnectionFactoryFilter(ConnectionFactoryFilter)} at the builder of a client
      * @param before the function to apply before this function is applied
      * @return a composed function that first applies the {@code before} function and then applies this function.
      */
+    @Deprecated
     default ConnectionFactoryFilter<ResolvedAddress, C> append(ConnectionFactoryFilter<ResolvedAddress, C> before) {
         requireNonNull(before);
         return original -> create(before.create(original));

diff --git a/.../src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsServiceDiscovererBuilder.java b/.../src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsServiceDiscovererBuilder.java
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2018 Apple Inc. and the ServiceTalk project authors
+ * Copyright © 2018, 2021 Apple Inc. and the ServiceTalk project authors
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -252,10 +252,17 @@ DefaultDnsServiceDiscovererBuilder srvFilterDuplicateEvents(boolean srvFilterDup
      * @return {@code this}
      */
     DefaultDnsServiceDiscovererBuilder appendFilter(final DnsClientFilterFactory factory) {
-        filterFactory = filterFactory == null ? requireNonNull(factory) : filterFactory.append(factory);
+        requireNonNull(factory);
+        filterFactory = appendFilter(filterFactory, factory);
         return this;
     }
 
+    // Use another method to keep final references and avoid StackOverflowError
+    private static DnsClientFilterFactory appendFilter(@Nullable final DnsClientFilterFactory current,
+                                                       final DnsClientFilterFactory next) {
+        return current == null ? next : dnsClient -> current.create(next.create(dnsClient));
+    }
+
     /**
      * Create a new instance of {@link DnsClient}.
      *

diff --git a/...covery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DnsClientFilterFactory.java b/...covery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DnsClientFilterFactory.java
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2020 Apple Inc. and the ServiceTalk project authors
+ * Copyright © 2020-2021 Apple Inc. and the ServiceTalk project authors
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -15,8 +15,6 @@
  */
 package io.servicetalk.dns.discovery.netty;
 
-import static java.util.Objects.requireNonNull;
-
 /**
  * Factory to apply a {@link DnsClientFilter}s.
  */
@@ -29,27 +27,4 @@ interface DnsClientFilterFactory {
      * @return {@link DnsClientFilter} using the provided {@link DnsClient}.
      */
     DnsClientFilter create(DnsClient dnsClient);
-
-    /**
-     * Returns a composed function that first applies the {@code before} function to its input, and then applies
-     * this function to the result.
-     * <p>
-     * The order of execution of these filters are in order of append. If 3 filters are added as follows:
-     * <pre>
-     *     filter1.append(filter2).append(filter3)
-     * </pre>
-     * making a request to a service discoverer wrapped by this filter chain the order of invocation of these filters
-     * will be:
-     * <pre>
-     *     filter1 =&gt; filter2 =&gt; filter3 =&gt; service discoverer
-     * </pre>
-     *
-     * @param before the function to apply before this function is applied
-     * @return a composed function that first applies the {@code before}
-     * function and then applies this function
-     */
-    default DnsClientFilterFactory append(DnsClientFilterFactory before) {
-        requireNonNull(before);
-        return serviceDiscoverer -> create(before.create(serviceDiscoverer));
-    }
 }
diff --git a/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcServiceFactory.java b/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcServiceFactory.java
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2019 Apple Inc. and the ServiceTalk project authors
+ * Copyright © 2019, 2021 Apple Inc. and the ServiceTalk project authors
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -100,10 +100,11 @@ public final Single<ServerContext> bind(final ServerBinder binder, final Executi
      * @return {@code this}
      */
     public GrpcServiceFactory<Filter, Service, FilterFactory> appendServiceFilter(FilterFactory before) {
+        requireNonNull(before);
         if (filterFactory == null) {
             filterFactory = before;
         } else {
-            this.filterFactory = appendServiceFilterFactory(filterFactory, requireNonNull(before));
+            this.filterFactory = appendServiceFilterFactory(filterFactory, before);
         }
         return this;
     }

diff --git a/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcServiceFilterFactory.java b/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcServiceFilterFactory.java
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2019 Apple Inc. and the ServiceTalk project authors
+ * Copyright © 2019, 2021 Apple Inc. and the ServiceTalk project authors
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -47,9 +47,11 @@ public interface GrpcServiceFilterFactory<Filter extends Service, Service> {
      *     filter1 =&gt; filter2 =&gt; filter3 =&gt; service
      * </pre>
      *
+     * @deprecated Use {@link GrpcServiceFactory#appendServiceFilter(GrpcServiceFilterFactory)}
      * @param before the factory to apply before this factory is applied.
      * @return a composed factory that first applies the {@code before} factory and then applies this factory.
      */
+    @Deprecated
     default GrpcServiceFilterFactory<Filter, Service> append(
             GrpcServiceFilterFactory<Filter, Service> before) {
         requireNonNull(before);

diff --git a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/HttpServerBuilder.java b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/HttpServerBuilder.java
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2018-2020 Apple Inc. and the ServiceTalk project authors
+ * Copyright © 2018-2021 Apple Inc. and the ServiceTalk project authors
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -41,6 +41,7 @@
 import static io.servicetalk.http.api.HttpExecutionStrategyInfluencer.defaultStreamingInfluencer;
 import static io.servicetalk.http.api.StrategyInfluencerAwareConversions.toConditionalServiceFilterFactory;
 import static io.servicetalk.transport.api.ConnectionAcceptor.ACCEPT_ALL;
+import static java.util.Objects.requireNonNull;
 
 /**
  * A builder for building HTTP Servers.
@@ -239,11 +240,8 @@ public final HttpServerBuilder appendConnectionAcceptorFilter(final ConnectionAc
      * @return {@code this}
      */
     public final HttpServerBuilder appendServiceFilter(final StreamingHttpServiceFilterFactory factory) {
-        if (serviceFilter == null) {
-            serviceFilter = factory;
-        } else {
-            serviceFilter = serviceFilter.append(factory);
-        }
+        requireNonNull(factory);
+        serviceFilter = appendFilter(serviceFilter, factory);
         if (!influencerChainBuilder.appendIfInfluencer(factory)) {
             influencerChainBuilder.append(defaultStreamingInfluencer());
         }
@@ -451,11 +449,17 @@ private Single<ServerContext> listenForService(StreamingHttpService rawService,
         StreamingHttpServiceFilterFactory currServiceFilter = serviceFilter;
         if (!AsyncContext.isDisabled()) {
             StreamingHttpServiceFilterFactory asyncContextFilter = new AsyncContextAwareHttpServiceFilter();
-            currServiceFilter = currServiceFilter == null ?
-                    asyncContextFilter : asyncContextFilter.append(currServiceFilter);
+            currServiceFilter = currServiceFilter == null ? asyncContextFilter :
+                    appendFilter(asyncContextFilter, currServiceFilter);
         }
         StreamingHttpService filteredService = currServiceFilter != null ?
                 currServiceFilter.create(rawService) : rawService;
         return doListen(connectionAcceptor, filteredService, strategy, drainRequestPayloadBody);
     }
+
+    private static StreamingHttpServiceFilterFactory appendFilter(
+            @Nullable final StreamingHttpServiceFilterFactory current,
+            final StreamingHttpServiceFilterFactory next) {
+        return current == null ? next : service -> current.create(next.create(service));
+    }
 }
diff --git a/...k-http-api/src/main/java/io/servicetalk/http/api/MultiAddressHttpClientFilterFactory.java b/...k-http-api/src/main/java/io/servicetalk/http/api/MultiAddressHttpClientFilterFactory.java
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2018-2019 Apple Inc. and the ServiceTalk project authors
+ * Copyright © 2018-2019, 2021 Apple Inc. and the ServiceTalk project authors
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -54,9 +54,12 @@ public interface MultiAddressHttpClientFilterFactory<U> {
      * <pre>
      *     filter1 =&gt; filter2 =&gt; filter3 =&gt; client
      * </pre>
+     *
+     * @deprecated Use {@link MultiAddressHttpClientBuilder#appendClientFilter(MultiAddressHttpClientFilterFactory)}
      * @param before the function to apply before this function is applied
      * @return a composed function that first applies the {@code before} function and then applies this function
      */
+    @Deprecated
     default MultiAddressHttpClientFilterFactory<U> append(MultiAddressHttpClientFilterFactory<U> before) {
         requireNonNull(before);
         return (group, client) -> create(group, before.create(group, client));

diff --git a/...talk-http-api/src/main/java/io/servicetalk/http/api/StreamingHttpClientFilterFactory.java b/...talk-http-api/src/main/java/io/servicetalk/http/api/StreamingHttpClientFilterFactory.java
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2018-2019 Apple Inc. and the ServiceTalk project authors
+ * Copyright © 2018-2019, 2021 Apple Inc. and the ServiceTalk project authors
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -44,10 +44,13 @@ public interface StreamingHttpClientFilterFactory {
      * <pre>
      *     filter1 =&gt; filter2 =&gt; filter3 =&gt; client
      * </pre>
+     *
+     * @deprecated Use {@link HttpClientBuilder#appendClientFilter(StreamingHttpClientFilterFactory)}
      * @param before the function to apply before this function is applied
      * @return a composed function that first applies the {@code before}
      * function and then applies this function
      */
+    @Deprecated
     default StreamingHttpClientFilterFactory append(StreamingHttpClientFilterFactory before) {
         requireNonNull(before);
         return client -> create(before.create(client));

diff --git a/...-http-api/src/main/java/io/servicetalk/http/api/StreamingHttpConnectionFilterFactory.java b/...-http-api/src/main/java/io/servicetalk/http/api/StreamingHttpConnectionFilterFactory.java
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2018 Apple Inc. and the ServiceTalk project authors
+ * Copyright © 2018, 2021 Apple Inc. and the ServiceTalk project authors
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -44,10 +44,13 @@ public interface StreamingHttpConnectionFilterFactory {
      * <pre>
      *     filter1 =&gt; filter2 =&gt; filter3 =&gt; connection
      * </pre>
+     *
+     * @deprecated Use {@link HttpClientBuilder#appendConnectionFilter(StreamingHttpConnectionFilterFactory)}
      * @param before the function to apply before this function is applied
      * @return a composed function that first applies the {@code before}
      * function and then applies this function
      */
+    @Deprecated
     default StreamingHttpConnectionFilterFactory append(StreamingHttpConnectionFilterFactory before) {
         requireNonNull(before);
         return connection -> create(before.create(connection));

diff --git a/...alk-http-api/src/main/java/io/servicetalk/http/api/StreamingHttpServiceFilterFactory.java b/...alk-http-api/src/main/java/io/servicetalk/http/api/StreamingHttpServiceFilterFactory.java
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2018 Apple Inc. and the ServiceTalk project authors
+ * Copyright © 2018, 2021 Apple Inc. and the ServiceTalk project authors
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -43,10 +43,13 @@ public interface StreamingHttpServiceFilterFactory {
      * <pre>
      *     filter1 =&gt; filter2 =&gt; filter3 =&gt; service
      * </pre>
+     *
+     * @deprecated Use {@link HttpServerBuilder#appendServiceFilter(StreamingHttpServiceFilterFactory)}
      * @param before the function to apply before this function is applied
      * @return a composed function that first applies the {@code before}
      * function and then applies this function
      */
+    @Deprecated
     default StreamingHttpServiceFilterFactory append(StreamingHttpServiceFilterFactory before) {
         requireNonNull(before);
         return service -> create(before.create(service));

diff --git a/...etalk-http-api/src/test/java/io/servicetalk/http/api/ConditionalHttpClientFilterTest.java b/...etalk-http-api/src/test/java/io/servicetalk/http/api/ConditionalHttpClientFilterTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2018-2019 Apple Inc. and the ServiceTalk project authors
+ * Copyright © 2018-2019, 2021 Apple Inc. and the ServiceTalk project authors
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -21,6 +21,8 @@
 
 import java.util.concurrent.atomic.AtomicBoolean;
 
+import static io.servicetalk.http.api.FilterFactoryUtils.appendClientFilterFactory;
+
 public class ConditionalHttpClientFilterTest extends AbstractConditionalHttpFilterTest {
 
     private static final StreamingHttpClientFilterFactory REQ_FILTER = client -> new StreamingHttpClientFilter(client) {
@@ -64,7 +66,7 @@ public Completable closeAsyncGracefully() {
 
     public static StreamingHttpClient newClient(AtomicBoolean closed) {
         return TestStreamingHttpClient.from(REQ_RES_FACTORY, testHttpExecutionContext(),
-                new TestCondFilterFactory(closed).append(REQ_FILTER));
+                appendClientFilterFactory(new TestCondFilterFactory(closed), REQ_FILTER));
     }
 
     @Override

diff --git a/...k-http-api/src/test/java/io/servicetalk/http/api/ConditionalHttpConnectionFilterTest.java b/...k-http-api/src/test/java/io/servicetalk/http/api/ConditionalHttpConnectionFilterTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2018 Apple Inc. and the ServiceTalk project authors
+ * Copyright © 2018, 2021 Apple Inc. and the ServiceTalk project authors
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -21,6 +21,7 @@
 
 import java.util.concurrent.atomic.AtomicBoolean;
 
+import static io.servicetalk.http.api.FilterFactoryUtils.appendConnectionFilterFactory;
 import static org.mockito.Mockito.mock;
 
 public class ConditionalHttpConnectionFilterTest extends AbstractConditionalHttpFilterTest {
@@ -65,9 +66,10 @@ public Completable closeAsyncGracefully() {
         }
     }
 
-    private StreamingHttpConnection newConnection(AtomicBoolean closed) {
+    private static StreamingHttpConnection newConnection(AtomicBoolean closed) {
         return TestStreamingHttpConnection.from(REQ_RES_FACTORY, testHttpExecutionContext(),
-                mock(HttpConnectionContext.class), new TestCondFilterFactory(closed).append(REQ_FILTER));
+                mock(HttpConnectionContext.class),
+                appendConnectionFilterFactory(new TestCondFilterFactory(closed), REQ_FILTER));
     }
 
     @Override

diff --git a/...tp-api/src/testFixtures/java/io/servicetalk/http/api/AbstractHttpRequesterFilterTest.java b/...tp-api/src/testFixtures/java/io/servicetalk/http/api/AbstractHttpRequesterFilterTest.java
@@ -42,6 +42,8 @@
 import static io.servicetalk.http.api.AbstractHttpRequesterFilterTest.RequesterType.ReservedConnection;
 import static io.servicetalk.http.api.AbstractHttpRequesterFilterTest.SecurityType.Insecure;
 import static io.servicetalk.http.api.AbstractHttpRequesterFilterTest.SecurityType.Secure;
+import static io.servicetalk.http.api.FilterFactoryUtils.appendClientFilterFactory;
+import static io.servicetalk.http.api.FilterFactoryUtils.appendConnectionFilterFactory;
 import static io.servicetalk.http.api.HttpProtocolVersion.HTTP_1_1;
 import static org.mockito.Mockito.lenient;
 import static org.mockito.Mockito.mock;
@@ -332,12 +334,13 @@ private StreamingHttpConnection newConnection(final RequestWithContextHandler rw
             @Override
             public Single<StreamingHttpResponse> request(final HttpExecutionStrategy strategy,
                                                             final StreamingHttpRequest request) {
-                return rwch.request(AbstractHttpRequesterFilterTest.REQ_RES_FACTORY, connectionContext(), request);
+                return rwch.request(REQ_RES_FACTORY, connectionContext(), request);
             }
         };
 
-        return TestStreamingHttpConnection.from(AbstractHttpRequesterFilterTest.REQ_RES_FACTORY, mockExecutionContext,
-                mockConnectionContext, filterFactory == null ? handlerFilter : filterFactory.append(handlerFilter));
+        return TestStreamingHttpConnection.from(REQ_RES_FACTORY, mockExecutionContext,
+                mockConnectionContext, filterFactory == null ? handlerFilter :
+                        appendConnectionFilterFactory(filterFactory, handlerFilter));
     }
 
     private <FF extends StreamingHttpClientFilterFactory & StreamingHttpConnectionFilterFactory> StreamingHttpClient
@@ -348,7 +351,7 @@ public Single<StreamingHttpResponse> request(final HttpExecutionStrategy strateg
                     protected Single<StreamingHttpResponse> request(final StreamingHttpRequester delegate,
                                                                     final HttpExecutionStrategy strategy,
                                                                     final StreamingHttpRequest request) {
-                        return rh.request(AbstractHttpRequesterFilterTest.REQ_RES_FACTORY, request);
+                        return rh.request(REQ_RES_FACTORY, request);
                     }
 
                     @Override
@@ -368,6 +371,7 @@ protected Single<StreamingHttpResponse> request(
                     }
                 };
 
-        return TestStreamingHttpClient.from(REQ_RES_FACTORY, mockExecutionContext, filterFactory.append(handlerFilter));
+        return TestStreamingHttpClient.from(REQ_RES_FACTORY, mockExecutionContext,
+                appendClientFilterFactory(filterFactory, handlerFilter));
     }
 }
diff --git a/...talk-http-api/src/testFixtures/java/io/servicetalk/http/api/ConditionalFilterFactory.java b/...talk-http-api/src/testFixtures/java/io/servicetalk/http/api/ConditionalFilterFactory.java
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2019 Apple Inc. and the ServiceTalk project authors
+ * Copyright © 2019, 2021 Apple Inc. and the ServiceTalk project authors
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -17,6 +17,9 @@
 
 import java.util.function.Predicate;
 
+import static io.servicetalk.http.api.FilterFactoryUtils.appendClientFilterFactory;
+import static io.servicetalk.http.api.FilterFactoryUtils.appendConnectionFilterFactory;
+
 public final class ConditionalFilterFactory
         implements StreamingHttpConnectionFilterFactory, StreamingHttpClientFilterFactory {
     private final Predicate<StreamingHttpRequest> predicate;
@@ -39,8 +42,8 @@ public StreamingHttpConnectionFilter create(final FilterableStreamingHttpConnect
     }
 
     public FilterFactory append(FilterFactory append) {
-        StreamingHttpClientFilterFactory clientFactory = append((StreamingHttpClientFilterFactory) append);
-        StreamingHttpConnectionFilterFactory connectionFactory = append((StreamingHttpConnectionFilterFactory) append);
+        StreamingHttpClientFilterFactory clientFactory = appendClientFilterFactory(this, append);
+        StreamingHttpConnectionFilterFactory connectionFactory = appendConnectionFilterFactory(this, append);
         return new FilterFactory() {
             @Override
             public StreamingHttpClientFilter create(final FilterableStreamingHttpClient client) {