From 00295b23c8ac47d4e39198695aa3b6e508a2ecf7 Mon Sep 17 00:00:00 2001 From: Trustin Lee Date: Wed, 23 Jul 2014 14:33:35 -0700 Subject: [PATCH] Overall cleanup of codec-dns Related issue: #2688 - DnsClass and DnsType - Make DnsClass and DnsType implement Comparable - Optimize the message generation of IllegalArgumentException, by pre-populating the list of the expected parameters - Move the static methods up - Relax the validation rule of DnsClass so that a user can define a CLASS which is not listed in the RFC 1035 - valueOf(int) does not throw IllegalArgumentException anymore as long as the specified value is an unsigned short. - Rename create() and forName() to valueOf() so that they look like a real enum - Rename type() and clazz() to intValue() so that they conform to our naming convention - Add missing null checks in DnsEntry --- .../io/netty/handler/codec/dns/DnsClass.java | 154 ++++++++++-------- .../io/netty/handler/codec/dns/DnsEntry.java | 28 +++- .../handler/codec/dns/DnsQueryEncoder.java | 4 +- .../codec/dns/DnsResponseException.java | 40 ----- .../io/netty/handler/codec/dns/DnsType.java | 130 ++++++++------- .../netty/handler/codec/dns/DnsClassTest.java | 21 ++- .../netty/handler/codec/dns/DnsTypeTest.java | 16 +- 7 files changed, 187 insertions(+), 206 deletions(-) delete mode 100644 codec-dns/src/main/java/io/netty/handler/codec/dns/DnsResponseException.java diff --git a/codec-dns/src/main/java/io/netty/handler/codec/dns/DnsClass.java b/codec-dns/src/main/java/io/netty/handler/codec/dns/DnsClass.java index 2059762a3a..296c9ee1f6 100644 --- a/codec-dns/src/main/java/io/netty/handler/codec/dns/DnsClass.java +++ b/codec-dns/src/main/java/io/netty/handler/codec/dns/DnsClass.java @@ -15,12 +15,10 @@ */ package io.netty.handler.codec.dns; -import java.util.Arrays; - /** * Represents a class field in DNS protocol */ -public class DnsClass { +public final class DnsClass implements Comparable { /** * Default class for DNS entries. @@ -32,32 +30,55 @@ public class DnsClass { public static final DnsClass NONE = new DnsClass(0x00fe, "NONE"); public static final DnsClass ANY = new DnsClass(0x00ff, "ANY"); - /** - * The protocol value of this DNS class - */ - private final int clazz; - /** - * The name of this DNS class - */ - private final String name; + private static final String EXPECTED = + " (expected: " + + IN + '(' + IN.intValue() + "), " + + CSNET + '(' + CSNET.intValue() + "), " + + CHAOS + '(' + CHAOS.intValue() + "), " + + HESIOD + '(' + HESIOD.intValue() + "), " + + NONE + '(' + NONE.intValue() + "), " + + ANY + '(' + ANY.intValue() + "))"; - DnsClass(int clazz, String name) { - this.clazz = clazz; - this.name = name; + public static DnsClass valueOf(String name) { + if (IN.name().equals(name)) { + return IN; + } + if (NONE.name().equals(name)) { + return NONE; + } + if (ANY.name().equals(name)) { + return ANY; + } + if (CSNET.name().equals(name)) { + return CSNET; + } + if (CHAOS.name().equals(name)) { + return CHAOS; + } + if (HESIOD.name().equals(name)) { + return HESIOD; + } + + throw new IllegalArgumentException("name: " + name + EXPECTED); } - /** - * Returns the name of this class as used in bind config files - */ - public final String name() { - return name; - } - - /** - * Returns the protocol value represented by this class - */ - public final int clazz() { - return clazz; + public static DnsClass valueOf(int intValue) { + switch (intValue) { + case 0x0001: + return IN; + case 0x0002: + return CSNET; + case 0x0003: + return CHAOS; + case 0x0004: + return HESIOD; + case 0x00fe: + return NONE; + case 0x00ff: + return ANY; + default: + return new DnsClass(intValue, "UNKNOWN"); + } } /** @@ -66,71 +87,60 @@ public class DnsClass { * @param clazz The class * @param name The name */ - public static DnsClass create(int clazz, String name) { + public static DnsClass valueOf(int clazz, String name) { return new DnsClass(clazz, name); } /** - * Returns true if this class is valid with respect to DNS protocol + * The protocol value of this DNS class */ - public boolean isValid() { - if (clazz < 1 || clazz > 4 && clazz != NONE.clazz && clazz != ANY.clazz) { - return false; + private final int intValue; + + /** + * The name of this DNS class + */ + private final String name; + + private DnsClass(int intValue, String name) { + if ((intValue & 0xffff) != intValue) { + throw new IllegalArgumentException("intValue: " + intValue + " (expected: 0 ~ 65535)"); } - return true; + + this.intValue = intValue; + this.name = name; } - public static DnsClass forName(String name) { - if (IN.name.equals(name)) { - return IN; - } else if (NONE.name().equals(name)) { - return NONE; - } else if (ANY.name().equals(name)) { - return ANY; - } else if (CSNET.name().equals(name)) { - return CSNET; - } else if (CHAOS.name().equals(name)) { - return CHAOS; - } else if (HESIOD.name().equals(name)) { - return HESIOD; - } - throw new IllegalArgumentException("name: " + name + " (expected: " - + "IN, ANY, CSNET, CHAOS, HESIOD)"); + /** + * Returns the name of this class as used in bind config files + */ + public String name() { + return name; } - public static DnsClass valueOf(int clazz) { - switch (clazz) { - case 0x0001: - return IN; - case 0x0002: - return CSNET; - case 0x0003: - return CHAOS; - case 0x0004: - return HESIOD; - case 0x00fe: - return NONE; - case 0x00ff: - return ANY; - default: - throw new IllegalArgumentException("clazz: " + clazz + " (expected: " - + Arrays.asList(new Integer[] {IN.clazz, CSNET.clazz, - CHAOS.clazz, HESIOD.clazz, NONE.clazz, ANY.clazz}) + ")"); - } + /** + * Returns the protocol value represented by this class + */ + public int intValue() { + return intValue; } @Override - public final int hashCode() { - return clazz; + public int hashCode() { + return intValue; } @Override - public final boolean equals(Object o) { - return o instanceof DnsClass && ((DnsClass) o).clazz == clazz; + public boolean equals(Object o) { + return o instanceof DnsClass && ((DnsClass) o).intValue == intValue; } @Override - public final String toString() { + public int compareTo(DnsClass o) { + return intValue() - o.intValue(); + } + + @Override + public String toString() { return name; } } diff --git a/codec-dns/src/main/java/io/netty/handler/codec/dns/DnsEntry.java b/codec-dns/src/main/java/io/netty/handler/codec/dns/DnsEntry.java index 1308e37ff5..7be10af1d4 100644 --- a/codec-dns/src/main/java/io/netty/handler/codec/dns/DnsEntry.java +++ b/codec-dns/src/main/java/io/netty/handler/codec/dns/DnsEntry.java @@ -15,6 +15,8 @@ */ package io.netty.handler.codec.dns; +import io.netty.util.internal.StringUtil; + /** * A class representing entries in a DNS packet (questions, and all resource * records). Contains data shared by entries such as name, type, and class. @@ -30,9 +32,13 @@ public class DnsEntry { if (name == null) { throw new NullPointerException("name"); } - if (!dnsClass.isValid()) { - throw new IllegalArgumentException("an invalid class has been supplied."); + if (type == null) { + throw new NullPointerException("type"); } + if (dnsClass == null) { + throw new NullPointerException("dnsClass"); + } + this.name = name; this.type = type; this.dnsClass = dnsClass; @@ -66,8 +72,11 @@ public class DnsEntry { @Override public String toString() { - return new StringBuilder(32).append(getClass().getSimpleName()).append("(domain name: ").append(name) - .append(", type: ").append(type).append(", class: ").append(dnsClass).append(')').toString(); + return new StringBuilder(128).append(StringUtil.simpleClassName(this)) + .append("(name: ").append(name) + .append(", type: ").append(type) + .append(", class: ").append(dnsClass) + .append(')').toString(); } @Override @@ -75,10 +84,13 @@ public class DnsEntry { if (this == o) { return true; } - if (o instanceof DnsEntry) { - DnsEntry other = (DnsEntry) o; - return other.type() == type && other.dnsClass() == dnsClass && other.name().equals(name); + if (!(o instanceof DnsEntry)) { + return false; } - return false; + + DnsEntry that = (DnsEntry) o; + return type().intValue() == that.type().intValue() && + dnsClass().intValue() == that.dnsClass().intValue() && + name().equals(that.name()); } } diff --git a/codec-dns/src/main/java/io/netty/handler/codec/dns/DnsQueryEncoder.java b/codec-dns/src/main/java/io/netty/handler/codec/dns/DnsQueryEncoder.java index 638ab85f2c..b38a0f056d 100644 --- a/codec-dns/src/main/java/io/netty/handler/codec/dns/DnsQueryEncoder.java +++ b/codec-dns/src/main/java/io/netty/handler/codec/dns/DnsQueryEncoder.java @@ -85,7 +85,7 @@ public class DnsQueryEncoder extends MessageToMessageEncoder { buf.writeBytes(part.getBytes(charset)); } buf.writeByte(0); // marks end of name field - buf.writeShort(question.type().type()); - buf.writeShort(question.dnsClass().clazz()); + buf.writeShort(question.type().intValue()); + buf.writeShort(question.dnsClass().intValue()); } } diff --git a/codec-dns/src/main/java/io/netty/handler/codec/dns/DnsResponseException.java b/codec-dns/src/main/java/io/netty/handler/codec/dns/DnsResponseException.java deleted file mode 100644 index 4497463fe5..0000000000 --- a/codec-dns/src/main/java/io/netty/handler/codec/dns/DnsResponseException.java +++ /dev/null @@ -1,40 +0,0 @@ -/* - * Copyright 2014 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.handler.codec.dns; - - -import io.netty.handler.codec.DecoderException; - -public final class DnsResponseException extends DecoderException { - - private static final long serialVersionUID = -8519053051363525286L; - - private final DnsResponseCode code; - - public DnsResponseException(DnsResponseCode code) { - if (code == null) { - throw new NullPointerException("code"); - } - this.code = code; - } - - /** - * The {@link DnsResponseCode} which caused this {@link DnsResponseException} to be created. - */ - public DnsResponseCode responseCode() { - return code; - } -} diff --git a/codec-dns/src/main/java/io/netty/handler/codec/dns/DnsType.java b/codec-dns/src/main/java/io/netty/handler/codec/dns/DnsType.java index eb2d166cbc..a0059fff67 100644 --- a/codec-dns/src/main/java/io/netty/handler/codec/dns/DnsType.java +++ b/codec-dns/src/main/java/io/netty/handler/codec/dns/DnsType.java @@ -22,7 +22,7 @@ import java.util.Map; /** * Represents a DNS record type. */ -public class DnsType { +public final class DnsType implements Comparable { /** * Address record RFC 1035 Returns a 32-bit IPv4 address, most commonly used @@ -300,101 +300,99 @@ public class DnsType { */ public static final DnsType DLV = new DnsType(0x8001, "DLV"); - private static final Map BY_NAME - = new HashMap(); - - private static final IntObjectHashMap BY_TYPE - = new IntObjectHashMap(); + private static final Map BY_NAME = new HashMap(); + private static final IntObjectHashMap BY_TYPE = new IntObjectHashMap(); + private static final String EXPECTED; static { - DnsType[] all = {A, NS, CNAME, SOA, PTR, MX, - TXT, RP, AFSDB, SIG, KEY, AAAA, LOC, - SRV, NAPTR, KX, CERT, DNAME, OPT, - APL, DS, SSHFP, IPSECKEY, RRSIG, NSEC, - DNSKEY, DHCID, NSEC3, NSEC3PARAM, TLSA, - HIP, SPF, TKEY, TSIG, IXFR, AXFR, - ANY, CAA, TA, DLV}; - for (DnsType type : all) { + DnsType[] all = { + A, NS, CNAME, SOA, PTR, MX, TXT, RP, AFSDB, SIG, KEY, AAAA, LOC, SRV, NAPTR, KX, CERT, DNAME, OPT, APL, + DS, SSHFP, IPSECKEY, RRSIG, NSEC, DNSKEY, DHCID, NSEC3, NSEC3PARAM, TLSA, HIP, SPF, TKEY, TSIG, IXFR, + AXFR, ANY, CAA, TA, DLV + }; + + StringBuilder expected = new StringBuilder(512); + expected.append(" (expected: "); + + for (DnsType type: all) { BY_NAME.put(type.name(), type); - BY_TYPE.put(type.type(), type); + BY_TYPE.put(type.intValue(), type); + expected.append(type.name()); + expected.append('('); + expected.append(type.intValue()); + expected.append("), "); } + + expected.setLength(expected.length() - 2); + expected.append(')'); + EXPECTED = expected.toString(); } - private final int type; + public static DnsType valueOf(int intValue) { + DnsType result = BY_TYPE.get(intValue); + if (result == null) { + return new DnsType(intValue, "UNKNOWN"); + } + return result; + } + + public static DnsType valueOf(String name) { + DnsType result = BY_NAME.get(name); + if (result == null) { + throw new IllegalArgumentException("name: " + name + EXPECTED); + } + return result; + } + + /** + * Returns a new instance. + */ + public static DnsType valueOf(int intValue, String name) { + return new DnsType(intValue, name); + } + + private final int intValue; private final String name; - DnsType(int type, String name) { - if ((type & 0xffff) != type) { - throw new IllegalArgumentException("type: " + type - + " (expected: 0 ~ 65535)"); + private DnsType(int intValue, String name) { + if ((intValue & 0xffff) != intValue) { + throw new IllegalArgumentException("intValue: " + intValue + " (expected: 0 ~ 65535)"); } - this.type = type; + this.intValue = intValue; this.name = name; } /** * Returns the name of this type, as seen in bind config files */ - public final String name() { + public String name() { return name; } /** * Returns the value of this DnsType as it appears in DNS protocol */ - public final int type() { - return type; - } - - /** - * Returns a new DnsType instance - */ - public static DnsType create(int type, String name) { - return new DnsType(type, name); + public int intValue() { + return intValue; } @Override - public final int hashCode() { - return type; + public int hashCode() { + return intValue; } @Override - public final boolean equals(Object o) { - return o instanceof DnsType && ((DnsType) o).type == type; + public boolean equals(Object o) { + return o instanceof DnsType && ((DnsType) o).intValue == intValue; } @Override - public final String toString() { + public int compareTo(DnsType o) { + return intValue() - o.intValue(); + } + + @Override + public String toString() { return name; } - - public static DnsType valueOf(int type) { - DnsType result = BY_TYPE.get(type); - if (result == null) { - throw new IllegalArgumentException("type: " + type - + "(expected: " + keysString() + ")"); - } - return result; - } - - private static CharSequence keysString() { - // Replace with Objects.toString(BY_TYPE.keys()) when JDK 7 supported - StringBuilder sb = new StringBuilder(); - for (int key : BY_TYPE.keys()) { - if (sb.length() > 0) { - sb.append(", "); - } - sb.append(key); - } - return sb; - } - - public static DnsType forName(String name) { - DnsType result = BY_NAME.get(name); - if (result == null) { - throw new IllegalArgumentException("name: " + name - + " (expected: " + BY_NAME.keySet() + ")"); - } - return result; - } } diff --git a/codec-dns/src/test/java/io/netty/handler/codec/dns/DnsClassTest.java b/codec-dns/src/test/java/io/netty/handler/codec/dns/DnsClassTest.java index 2fa8344b0b..fc03b8a6d1 100644 --- a/codec-dns/src/test/java/io/netty/handler/codec/dns/DnsClassTest.java +++ b/codec-dns/src/test/java/io/netty/handler/codec/dns/DnsClassTest.java @@ -15,23 +15,22 @@ */ package io.netty.handler.codec.dns; +import org.junit.Test; + import java.lang.reflect.Field; import java.lang.reflect.Modifier; import java.util.ArrayList; import java.util.HashSet; import java.util.List; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNotEquals; -import static org.junit.Assert.assertSame; -import org.junit.Test; + +import static org.junit.Assert.*; public class DnsClassTest { - private List allTypes() throws Exception { + private static List allTypes() throws Exception { List result = new ArrayList(); for (Field field : DnsClass.class.getDeclaredFields()) { - if ((field.getModifiers() & Modifier.STATIC) != 0) { + if ((field.getModifiers() & Modifier.STATIC) != 0 && field.getType() == DnsClass.class) { result.add((DnsClass) field.get(null)); } } @@ -42,7 +41,7 @@ public class DnsClassTest { @Test public void testSanity() throws Exception { assertEquals("More than one type has the same int value", - allTypes().size(), new HashSet(allTypes()).size()); + allTypes().size(), new HashSet(allTypes()).size()); } /** @@ -51,7 +50,7 @@ public class DnsClassTest { @Test public void testHashCode() throws Exception { for (DnsClass t : allTypes()) { - assertEquals(t.clazz(), t.hashCode()); + assertEquals(t.intValue(), t.hashCode()); } } @@ -75,9 +74,9 @@ public class DnsClassTest { @Test public void testFind() throws Exception { for (DnsClass t : allTypes()) { - DnsClass found = DnsClass.valueOf(t.clazz()); + DnsClass found = DnsClass.valueOf(t.intValue()); assertSame(t, found); - found = DnsClass.forName(t.toString()); + found = DnsClass.valueOf(t.toString()); assertSame(t.toString(), t, found); } } diff --git a/codec-dns/src/test/java/io/netty/handler/codec/dns/DnsTypeTest.java b/codec-dns/src/test/java/io/netty/handler/codec/dns/DnsTypeTest.java index aac8035cde..7cce75b7f2 100644 --- a/codec-dns/src/test/java/io/netty/handler/codec/dns/DnsTypeTest.java +++ b/codec-dns/src/test/java/io/netty/handler/codec/dns/DnsTypeTest.java @@ -15,20 +15,22 @@ */ package io.netty.handler.codec.dns; +import org.junit.Test; + import java.lang.reflect.Field; import java.lang.reflect.Modifier; import java.util.ArrayList; import java.util.HashSet; import java.util.List; -import org.junit.Test; + import static org.junit.Assert.*; public class DnsTypeTest { - private List allTypes() throws Exception { + private static List allTypes() throws Exception { List result = new ArrayList(); for (Field field : DnsType.class.getFields()) { - if ((field.getModifiers() & Modifier.STATIC) != 0 && DnsType.class == field.getType()) { + if ((field.getModifiers() & Modifier.STATIC) != 0 && field.getType() == DnsType.class) { result.add((DnsType) field.get(null)); } } @@ -39,7 +41,7 @@ public class DnsTypeTest { @Test public void testSanity() throws Exception { assertEquals("More than one type has the same int value", - allTypes().size(), new HashSet(allTypes()).size()); + allTypes().size(), new HashSet(allTypes()).size()); } /** @@ -48,7 +50,7 @@ public class DnsTypeTest { @Test public void testHashCode() throws Exception { for (DnsType t : allTypes()) { - assertEquals(t.type(), t.hashCode()); + assertEquals(t.intValue(), t.hashCode()); } } @@ -72,9 +74,9 @@ public class DnsTypeTest { @Test public void testFind() throws Exception { for (DnsType t : allTypes()) { - DnsType found = DnsType.valueOf(t.type()); + DnsType found = DnsType.valueOf(t.intValue()); assertSame(t, found); - found = DnsType.forName(t.toString()); + found = DnsType.valueOf(t.toString()); assertSame(t.toString(), t, found); } }