DnsNameResolver empty/null hostname missed by a416b79

Motivation:
a416b79 introduced a check for null or empty host name to be compatible with the JDK resolution. However the doResolve(String, Promise) method, and if the doResolve(String, DnsRecord[], Promise, DnsCache) method was overridden the empty/null hostname would not be correctly resolved.

Modifications:
- Move the empty/null host name check into the lowest level doResolve method in DnsNameResolver
- Remove the duplicate logic in InetNameResolver.java which can be bypassed anyways

Result:
By default (unless behavior is overridden) DnsNameResolver resolves null/empty host names to local host just like the JDK.

resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolver.java

@@ -31,10 +31,10 @@ import io.netty.channel.socket.DatagramChannel;
 import io.netty.channel.socket.InternetProtocolFamily;
 import io.netty.handler.codec.dns.DatagramDnsQueryEncoder;
 import io.netty.handler.codec.dns.DatagramDnsResponse;
-import io.netty.handler.codec.dns.DnsRawRecord;
-import io.netty.handler.codec.dns.DnsRecord;
 import io.netty.handler.codec.dns.DatagramDnsResponseDecoder;
 import io.netty.handler.codec.dns.DnsQuestion;
+import io.netty.handler.codec.dns.DnsRawRecord;
+import io.netty.handler.codec.dns.DnsRecord;
 import io.netty.handler.codec.dns.DnsRecordType;
 import io.netty.handler.codec.dns.DnsResponse;
 import io.netty.resolver.HostsFileEntriesResolver;
@@ -64,7 +64,10 @@ import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Set;
 
-import static io.netty.util.internal.ObjectUtil.*;
+import static io.netty.util.internal.ObjectUtil.checkNonEmpty;
+import static io.netty.util.internal.ObjectUtil.checkNotNull;
+import static io.netty.util.internal.ObjectUtil.checkPositive;
+import static io.netty.util.internal.ObjectUtil.checkPositiveOrZero;
 
 /**
  * A DNS-based {@link InetNameResolver}.
@@ -479,10 +482,6 @@ public class DnsNameResolver extends InetNameResolver {
     public final Future<InetAddress> resolve(String inetHost, Iterable<DnsRecord> additionals,
                                              Promise<InetAddress> promise) {
         checkNotNull(promise, "promise");
-        if (inetHost == null || inetHost.isEmpty()) {
-            // If an empty hostname is used we should use "localhost", just like InetAddress.getByName(...) does.
-            return promise.setSuccess(loopbackAddress());
-        }
         DnsRecord[] additionalsArray = toArray(additionals, true);
         try {
             doResolve(inetHost, additionalsArray, promise, resolveCache);
@@ -516,12 +515,6 @@ public class DnsNameResolver extends InetNameResolver {
     public final Future<List<InetAddress>> resolveAll(String inetHost, Iterable<DnsRecord> additionals,
                                                 Promise<List<InetAddress>> promise) {
         checkNotNull(promise, "promise");
-
-        if (inetHost == null || inetHost.isEmpty()) {
-            // If an empty hostname is used we should use "localhost", just like InetAddress.getAllByName(...) does.
-            return promise.setSuccess(Collections.singletonList(loopbackAddress()));
-        }
-
         DnsRecord[] additionalsArray = toArray(additionals, true);
         try {
             doResolveAll(inetHost, additionalsArray, promise, resolveCache);
@@ -567,8 +560,7 @@ public class DnsNameResolver extends InetNameResolver {
         }
     }
 
-    @Override
+    private InetAddress loopbackAddress() {
-    protected final InetAddress loopbackAddress() {
         return preferredAddressType() == InternetProtocolFamily.IPv4 ?
                     NetUtil.LOCALHOST4 : NetUtil.LOCALHOST6;
     }
@@ -581,6 +573,11 @@ public class DnsNameResolver extends InetNameResolver {
                              DnsRecord[] additionals,
                              Promise<InetAddress> promise,
                              DnsCache resolveCache) throws Exception {
+        if (inetHost == null || inetHost.isEmpty()) {
+            // If an empty hostname is used we should use "localhost", just like InetAddress.getByName(...) does.
+            promise.setSuccess(loopbackAddress());
+            return;
+        }
         final byte[] bytes = NetUtil.createByteArrayFromIpAddressString(inetHost);
         if (bytes != null) {
             // The inetHost is actually an ipaddress.
@@ -706,6 +703,11 @@ public class DnsNameResolver extends InetNameResolver {
                                 DnsRecord[] additionals,
                                 Promise<List<InetAddress>> promise,
                                 DnsCache resolveCache) throws Exception {
+        if (inetHost == null || inetHost.isEmpty()) {
+            // If an empty hostname is used we should use "localhost", just like InetAddress.getAllByName(...) does.
+            promise.setSuccess(Collections.singletonList(loopbackAddress()));
+            return;
+        }
         final byte[] bytes = NetUtil.createByteArrayFromIpAddressString(inetHost);
         if (bytes != null) {
             // The unresolvedAddress was created via a String that contains an ipaddress.

resolver-dns/src/test/java/io/netty/resolver/dns/DnsNameResolverTest.java

@@ -34,6 +34,7 @@ import io.netty.handler.codec.dns.DnsSection;
 import io.netty.util.NetUtil;
 import io.netty.resolver.HostsFileEntriesResolver;
 import io.netty.util.concurrent.Future;
+import io.netty.util.internal.SocketUtils;
 import io.netty.util.internal.StringUtil;
 import io.netty.util.internal.logging.InternalLogger;
 import io.netty.util.internal.logging.InternalLoggerFactory;
@@ -374,6 +375,47 @@ public class DnsNameResolverTest {
         }
     }
 
+    @Test(timeout = 5000)
+    public void testNonCachedResolveEmptyHostName() throws Exception {
+        testNonCachedResolveEmptyHostName("");
+    }
+
+    @Test(timeout = 5000)
+    public void testNonCachedResolveNullHostName() throws Exception {
+        testNonCachedResolveEmptyHostName(null);
+    }
+
+    public void testNonCachedResolveEmptyHostName(String inetHost) throws Exception {
+        DnsNameResolver resolver = newNonCachedResolver(InternetProtocolFamily.IPv4).build();
+        try {
+            InetAddress addr = resolver.resolve(inetHost).syncUninterruptibly().getNow();
+            assertEquals(SocketUtils.addressByName(inetHost), addr);
+        } finally {
+            resolver.close();
+        }
+    }
+
+    @Test(timeout = 5000)
+    public void testNonCachedResolveAllEmptyHostName() throws Exception {
+        testNonCachedResolveAllEmptyHostName("");
+    }
+
+    @Test(timeout = 5000)
+    public void testNonCachedResolveAllNullHostName() throws Exception {
+        testNonCachedResolveAllEmptyHostName(null);
+    }
+
+    private static void testNonCachedResolveAllEmptyHostName(String inetHost) throws UnknownHostException {
+        DnsNameResolver resolver = newNonCachedResolver(InternetProtocolFamily.IPv4).build();
+        try {
+            List<InetAddress> addrs = resolver.resolveAll(inetHost).syncUninterruptibly().getNow();
+            assertEquals(Arrays.asList(
+                    SocketUtils.allAddressesByName(inetHost)), addrs);
+        } finally {
+            resolver.close();
+        }
+    }
+
     private static Map<String, InetAddress> testResolve0(DnsNameResolver resolver, Set<String> excludedDomains)
             throws InterruptedException {
 

resolver/src/main/java/io/netty/resolver/InetNameResolver.java

@@ -17,21 +17,16 @@ package io.netty.resolver;
 
 import io.netty.util.concurrent.EventExecutor;
 import io.netty.util.concurrent.Future;
-import io.netty.util.concurrent.Promise;
+import io.netty.util.internal.UnstableApi;
-import io.netty.util.internal.SocketUtils;
 
 import java.net.InetAddress;
 import java.net.InetSocketAddress;
-import java.util.Collections;
-import java.util.List;
 
 /**
  * A skeletal {@link NameResolver} implementation that resolves {@link InetAddress}.
  */
+@UnstableApi
 public abstract class InetNameResolver extends SimpleNameResolver<InetAddress> {
-
-    private final InetAddress loopbackAddress;
-
     private volatile AddressResolver<InetSocketAddress> addressResolver;
 
     /**
@@ -40,32 +35,6 @@ public abstract class InetNameResolver extends SimpleNameResolver<InetAddress> {
      */
     protected InetNameResolver(EventExecutor executor) {
         super(executor);
-        loopbackAddress = SocketUtils.loopbackAddress();
-    }
-
-    /**
-     * Returns the {@link InetAddress} for loopback.
-     */
-    protected InetAddress loopbackAddress() {
-        return loopbackAddress;
-    }
-
-    @Override
-    public Future<InetAddress> resolve(String inetHost, Promise<InetAddress> promise) {
-        if (inetHost == null || inetHost.isEmpty()) {
-            // If an empty hostname is used we should use "localhost", just like InetAddress.getByName(...) does.
-            return promise.setSuccess(loopbackAddress());
-        }
-        return super.resolve(inetHost, promise);
-    }
-
-    @Override
-    public Future<List<InetAddress>> resolveAll(String inetHost, Promise<List<InetAddress>> promise) {
-        if (inetHost == null || inetHost.isEmpty()) {
-            // If an empty hostname is used we should use "localhost", just like InetAddress.getByName(...) does.
-            return promise.setSuccess(Collections.singletonList(loopbackAddress()));
-        }
-        return super.resolveAll(inetHost, promise);
     }
 
     /**

resolver/src/main/java/io/netty/resolver/SimpleNameResolver.java

@@ -55,7 +55,6 @@ public abstract class SimpleNameResolver<T> implements NameResolver<T> {
 
     @Override
     public Future<T> resolve(String inetHost, Promise<T> promise) {
-        checkNotNull(inetHost, "inetHost");
         checkNotNull(promise, "promise");
 
         try {
@@ -74,7 +73,6 @@ public abstract class SimpleNameResolver<T> implements NameResolver<T> {
 
     @Override
     public Future<List<T>> resolveAll(String inetHost, Promise<List<T>> promise) {
-        checkNotNull(inetHost, "inetHost");
         checkNotNull(promise, "promise");
 
         try {

resolver/src/test/java/io/netty/resolver/InetNameResolverTest.java

@@ -1,90 +0,0 @@
-/*@
- * Copyright 2017 The Netty Project
- *
- * The Netty Project licenses this file to you 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 io.netty.resolver;
-
-import io.netty.util.concurrent.ImmediateEventExecutor;
-import io.netty.util.concurrent.Promise;
-import io.netty.util.internal.SocketUtils;
-import io.netty.util.internal.StringUtil;
-import org.junit.Test;
-
-import java.net.InetAddress;
-import java.net.UnknownHostException;
-import java.util.Arrays;
-import java.util.List;
-
-import static org.junit.Assert.assertEquals;
-
-public class InetNameResolverTest {
-
-    @Test
-    public void testResolveEmpty() throws UnknownHostException {
-        testResolve0(SocketUtils.addressByName(StringUtil.EMPTY_STRING), StringUtil.EMPTY_STRING);
-    }
-
-    @Test
-    public void testResolveNull() throws UnknownHostException {
-        testResolve0(SocketUtils.addressByName(null), null);
-    }
-
-    @Test
-    public void testResolveAllEmpty() throws UnknownHostException {
-        testResolveAll0(Arrays.asList(
-                SocketUtils.allAddressesByName(StringUtil.EMPTY_STRING)), StringUtil.EMPTY_STRING);
-    }
-
-    @Test
-    public void testResolveAllNull() throws UnknownHostException {
-        testResolveAll0(Arrays.asList(
-                SocketUtils.allAddressesByName(null)), null);
-    }
-
-    private static void testResolve0(InetAddress expectedAddr, String name) {
-        InetNameResolver resolver = new TestInetNameResolver();
-        try {
-            InetAddress address = resolver.resolve(name).syncUninterruptibly().getNow();
-            assertEquals(expectedAddr, address);
-        } finally {
-            resolver.close();
-        }
-    }
-
-    private static void testResolveAll0(List<InetAddress> expectedAddrs, String name) {
-        InetNameResolver resolver = new TestInetNameResolver();
-        try {
-            List<InetAddress> addresses = resolver.resolveAll(name).syncUninterruptibly().getNow();
-            assertEquals(expectedAddrs, addresses);
-        } finally {
-            resolver.close();
-        }
-    }
-
-    private static final class TestInetNameResolver extends InetNameResolver {
-        public TestInetNameResolver() {
-            super(ImmediateEventExecutor.INSTANCE);
-        }
-
-        @Override
-        protected void doResolve(String inetHost, Promise<InetAddress> promise) throws Exception {
-            promise.setFailure(new UnsupportedOperationException());
-        }
-
-        @Override
-        protected void doResolveAll(String inetHost, Promise<List<InetAddress>> promise) throws Exception {
-            promise.setFailure(new UnsupportedOperationException());
-        }
-    }
-}