From 97243a73eab86b634540756f72be1c500cfeea6c Mon Sep 17 00:00:00 2001 From: Roger Meier Date: Wed, 28 May 2014 14:19:09 +0200 Subject: [PATCH] THRIFT-2239 Address FindBugs errors Client: Java Patch: Liang Xie --- .../src/org/apache/thrift/TBaseHelper.java | 3 +- .../apache/thrift/TMultiplexedProcessor.java | 2 +- .../thrift/async/TAsyncClientManager.java | 3 +- .../org/apache/thrift/protocol/TField.java | 20 ++++++++- .../apache/thrift/protocol/TJSONProtocol.java | 4 +- .../org/apache/thrift/protocol/TMessage.java | 33 +++++++++++---- .../thrift/protocol/TSimpleJSONProtocol.java | 14 +++---- .../server/TThreadedSelectorServer.java | 2 +- .../thrift/transport/TFileTransport.java | 41 ++++++++----------- .../thrift/transport/TMemoryBuffer.java | 6 +-- .../transport/TSSLTransportFactory.java | 15 +++++-- .../transport/TSaslServerTransport.java | 4 +- .../thrift/transport/TSaslTransport.java | 8 ++-- 13 files changed, 101 insertions(+), 54 deletions(-) diff --git a/lib/java/src/org/apache/thrift/TBaseHelper.java b/lib/java/src/org/apache/thrift/TBaseHelper.java index eec648bc..4f02b060 100644 --- a/lib/java/src/org/apache/thrift/TBaseHelper.java +++ b/lib/java/src/org/apache/thrift/TBaseHelper.java @@ -17,6 +17,7 @@ */ package org.apache.thrift; +import java.io.Serializable; import java.nio.ByteBuffer; import java.util.Comparator; import java.util.Iterator; @@ -198,7 +199,7 @@ public final class TBaseHelper { /** * Comparator to compare items inside a structure (e.g. a list, set, or map). */ - private static class NestedStructureComparator implements Comparator { + private static class NestedStructureComparator implements Comparator, Serializable { public int compare(Object oA, Object oB) { if (oA == null && oB == null) { return 0; diff --git a/lib/java/src/org/apache/thrift/TMultiplexedProcessor.java b/lib/java/src/org/apache/thrift/TMultiplexedProcessor.java index dad61553..0bf49199 100644 --- a/lib/java/src/org/apache/thrift/TMultiplexedProcessor.java +++ b/lib/java/src/org/apache/thrift/TMultiplexedProcessor.java @@ -128,7 +128,7 @@ public class TMultiplexedProcessor implements TProcessor { * to allow them to call readMessageBegin() and get a TMessage in exactly * the standard format, without the service name prepended to TMessage.name. */ - private class StoredMessageProtocol extends TProtocolDecorator { + private static class StoredMessageProtocol extends TProtocolDecorator { TMessage messageBegin; public StoredMessageProtocol(TProtocol protocol, TMessage messageBegin) { super(protocol); diff --git a/lib/java/src/org/apache/thrift/async/TAsyncClientManager.java b/lib/java/src/org/apache/thrift/async/TAsyncClientManager.java index 98f71947..6fcf4b41 100644 --- a/lib/java/src/org/apache/thrift/async/TAsyncClientManager.java +++ b/lib/java/src/org/apache/thrift/async/TAsyncClientManager.java @@ -19,6 +19,7 @@ package org.apache.thrift.async; import java.io.IOException; +import java.io.Serializable; import java.nio.channels.ClosedSelectorException; import java.nio.channels.SelectionKey; import java.nio.channels.Selector; @@ -182,7 +183,7 @@ public class TAsyncClientManager { } /** Comparator used in TreeSet */ - private static class TAsyncMethodCallTimeoutComparator implements Comparator { + private static class TAsyncMethodCallTimeoutComparator implements Comparator, Serializable { public int compare(TAsyncMethodCall left, TAsyncMethodCall right) { if (left.getTimeoutTimestamp() == right.getTimeoutTimestamp()) { return (int)(left.getSequenceId() - right.getSequenceId()); diff --git a/lib/java/src/org/apache/thrift/protocol/TField.java b/lib/java/src/org/apache/thrift/protocol/TField.java index 03affdaa..31331bba 100644 --- a/lib/java/src/org/apache/thrift/protocol/TField.java +++ b/lib/java/src/org/apache/thrift/protocol/TField.java @@ -42,7 +42,25 @@ public class TField { return ""; } - public boolean equals(TField otherField) { + @Override + public int hashCode() { + final int prime = 31; + int result = 1; + result = prime * result + id; + result = prime * result + ((name == null) ? 0 : name.hashCode()); + result = prime * result + type; + return result; + } + + @Override + public boolean equals(Object obj) { + if (this == obj) + return true; + if (obj == null) + return false; + if (getClass() != obj.getClass()) + return false; + TField otherField = (TField) obj; return type == otherField.type && id == otherField.id; } } diff --git a/lib/java/src/org/apache/thrift/protocol/TJSONProtocol.java b/lib/java/src/org/apache/thrift/protocol/TJSONProtocol.java index 6ce702e2..f51322c6 100644 --- a/lib/java/src/org/apache/thrift/protocol/TJSONProtocol.java +++ b/lib/java/src/org/apache/thrift/protocol/TJSONProtocol.java @@ -436,7 +436,9 @@ public class TJSONProtocol extends TProtocol { special = true; } break; - } + default: + break; + } boolean escapeNum = special || context_.escapeNum(); if (escapeNum) { diff --git a/lib/java/src/org/apache/thrift/protocol/TMessage.java b/lib/java/src/org/apache/thrift/protocol/TMessage.java index 1438b117..f13b8ca5 100644 --- a/lib/java/src/org/apache/thrift/protocol/TMessage.java +++ b/lib/java/src/org/apache/thrift/protocol/TMessage.java @@ -44,14 +44,33 @@ public final class TMessage { } @Override - public boolean equals(Object other) { - if (other instanceof TMessage) { - return equals((TMessage) other); - } - return false; + public int hashCode() { + final int prime = 31; + int result = 1; + result = prime * result + ((name == null) ? 0 : name.hashCode()); + result = prime * result + seqid; + result = prime * result + type; + return result; } - public boolean equals(TMessage other) { - return name.equals(other.name) && type == other.type && seqid == other.seqid; + @Override + public boolean equals(Object obj) { + if (this == obj) + return true; + if (obj == null) + return false; + if (getClass() != obj.getClass()) + return false; + TMessage other = (TMessage) obj; + if (name == null) { + if (other.name != null) + return false; + } else if (!name.equals(other.name)) + return false; + if (seqid != other.seqid) + return false; + if (type != other.type) + return false; + return true; } } diff --git a/lib/java/src/org/apache/thrift/protocol/TSimpleJSONProtocol.java b/lib/java/src/org/apache/thrift/protocol/TSimpleJSONProtocol.java index e005a4fc..ffa139a1 100644 --- a/lib/java/src/org/apache/thrift/protocol/TSimpleJSONProtocol.java +++ b/lib/java/src/org/apache/thrift/protocol/TSimpleJSONProtocol.java @@ -45,13 +45,13 @@ public class TSimpleJSONProtocol extends TProtocol { } } - public static final byte[] COMMA = new byte[] {','}; - public static final byte[] COLON = new byte[] {':'}; - public static final byte[] LBRACE = new byte[] {'{'}; - public static final byte[] RBRACE = new byte[] {'}'}; - public static final byte[] LBRACKET = new byte[] {'['}; - public static final byte[] RBRACKET = new byte[] {']'}; - public static final char QUOTE = '"'; + private static final byte[] COMMA = new byte[] {','}; + private static final byte[] COLON = new byte[] {':'}; + private static final byte[] LBRACE = new byte[] {'{'}; + private static final byte[] RBRACE = new byte[] {'}'}; + private static final byte[] LBRACKET = new byte[] {'['}; + private static final byte[] RBRACKET = new byte[] {']'}; + private static final char QUOTE = '"'; private static final TStruct ANONYMOUS_STRUCT = new TStruct(); private static final TField ANONYMOUS_FIELD = new TField(); diff --git a/lib/java/src/org/apache/thrift/server/TThreadedSelectorServer.java b/lib/java/src/org/apache/thrift/server/TThreadedSelectorServer.java index 7c5ec4f8..16445e57 100644 --- a/lib/java/src/org/apache/thrift/server/TThreadedSelectorServer.java +++ b/lib/java/src/org/apache/thrift/server/TThreadedSelectorServer.java @@ -643,7 +643,7 @@ public class TThreadedSelectorServer extends AbstractNonblockingServer { * A round robin load balancer for choosing selector threads for new * connections. */ - protected class SelectorThreadLoadBalancer { + protected static class SelectorThreadLoadBalancer { private final Collection threads; private Iterator nextThreadIterator; diff --git a/lib/java/src/org/apache/thrift/transport/TFileTransport.java b/lib/java/src/org/apache/thrift/transport/TFileTransport.java index f5abe536..0044d2a3 100644 --- a/lib/java/src/org/apache/thrift/transport/TFileTransport.java +++ b/lib/java/src/org/apache/thrift/transport/TFileTransport.java @@ -38,14 +38,14 @@ import java.util.Random; */ public class TFileTransport extends TTransport { - public static class truncableBufferedInputStream extends BufferedInputStream { + public static class TruncableBufferedInputStream extends BufferedInputStream { public void trunc() { pos = count = 0; } - public truncableBufferedInputStream(InputStream in) { + public TruncableBufferedInputStream(InputStream in) { super(in); } - public truncableBufferedInputStream(InputStream in, int size) { + public TruncableBufferedInputStream(InputStream in, int size) { super(in, size); } } @@ -87,7 +87,7 @@ public class TFileTransport extends TTransport { } }; - public static class chunkState { + public static class ChunkState { /** * Chunk Size. Must be same across all implementations */ @@ -96,8 +96,8 @@ public class TFileTransport extends TTransport { private int chunk_size_ = DEFAULT_CHUNK_SIZE; private long offset_ = 0; - public chunkState() {} - public chunkState(int chunk_size) { chunk_size_ = chunk_size; } + public ChunkState() {} + public ChunkState(int chunk_size) { chunk_size_ = chunk_size; } public void skip(int size) {offset_ += size; } public void seek(long offset) {offset_ = offset;} @@ -108,7 +108,7 @@ public class TFileTransport extends TTransport { public long getOffset() { return (offset_);} } - public static enum tailPolicy { + public static enum TailPolicy { NOWAIT(0, 0), WAIT_FOREVER(500, -1); @@ -133,7 +133,7 @@ public class TFileTransport extends TTransport { * @param retries number of retries */ - tailPolicy(int timeout, int retries) { + TailPolicy(int timeout, int retries) { timeout_ = timeout; retries_ = retries; } @@ -142,7 +142,7 @@ public class TFileTransport extends TTransport { /** * Current tailing policy */ - tailPolicy currentPolicy_ = tailPolicy.NOWAIT; + TailPolicy currentPolicy_ = TailPolicy.NOWAIT; /** @@ -169,12 +169,7 @@ public class TFileTransport extends TTransport { /** * current Chunk state */ - chunkState cs = null; - - /** - * Read timeout - */ - private int readTimeout_ = 0; + ChunkState cs = null; /** * is read only? @@ -186,7 +181,7 @@ public class TFileTransport extends TTransport { * * @return current read policy */ - public tailPolicy getTailPolicy() { + public TailPolicy getTailPolicy() { return (currentPolicy_); } @@ -196,8 +191,8 @@ public class TFileTransport extends TTransport { * @param policy New policy to set * @return Old policy */ - public tailPolicy setTailPolicy(tailPolicy policy) { - tailPolicy old = currentPolicy_; + public TailPolicy setTailPolicy(TailPolicy policy) { + TailPolicy old = currentPolicy_; currentPolicy_ = policy; return (old); } @@ -212,10 +207,10 @@ public class TFileTransport extends TTransport { InputStream is; try { if(inputStream_ != null) { - ((truncableBufferedInputStream)inputStream_).trunc(); + ((TruncableBufferedInputStream)inputStream_).trunc(); is = inputStream_; } else { - is = new truncableBufferedInputStream(inputFile_.getInputStream()); + is = new TruncableBufferedInputStream(inputFile_.getInputStream()); } } catch (IOException iox) { System.err.println("createInputStream: "+iox.getMessage()); @@ -236,7 +231,7 @@ public class TFileTransport extends TTransport { * @return number of bytes read */ private int tailRead(InputStream is, byte[] buf, - int off, int len, tailPolicy tp) throws TTransportException { + int off, int len, TailPolicy tp) throws TTransportException { int orig_len = len; try { int retries = 0; @@ -369,7 +364,7 @@ public class TFileTransport extends TTransport { try { inputStream_ = createInputStream(); - cs = new chunkState(); + cs = new ChunkState(); currentEvent_ = new Event(new byte [256]); if(!readOnly_) @@ -545,7 +540,7 @@ public class TFileTransport extends TTransport { if(seekToEnd) { // waiting forever here - otherwise we can hit EOF and end up // having consumed partial data from the data stream. - tailPolicy old = setTailPolicy(tailPolicy.WAIT_FOREVER); + TailPolicy old = setTailPolicy(TailPolicy.WAIT_FOREVER); while(cs.getOffset() < eofOffset) { readEvent(); } currentEvent_.setAvailable(0); setTailPolicy(old); diff --git a/lib/java/src/org/apache/thrift/transport/TMemoryBuffer.java b/lib/java/src/org/apache/thrift/transport/TMemoryBuffer.java index 53354afa..ef5f5c28 100644 --- a/lib/java/src/org/apache/thrift/transport/TMemoryBuffer.java +++ b/lib/java/src/org/apache/thrift/transport/TMemoryBuffer.java @@ -77,12 +77,12 @@ public class TMemoryBuffer extends TTransport { } public String inspect() { - String buf = ""; + StringBuilder buf = new StringBuilder(); byte[] bytes = arr_.toByteArray(); for (int i = 0; i < bytes.length; i++) { - buf += (pos_ == i ? "==>" : "" ) + Integer.toHexString(bytes[i] & 0xff) + " "; + buf.append(pos_ == i ? "==>" : "" ).append(Integer.toHexString(bytes[i] & 0xff)).append(" "); } - return buf; + return buf.toString(); } // The contents of the buffer diff --git a/lib/java/src/org/apache/thrift/transport/TSSLTransportFactory.java b/lib/java/src/org/apache/thrift/transport/TSSLTransportFactory.java index ca27729a..16d2fd14 100755 --- a/lib/java/src/org/apache/thrift/transport/TSSLTransportFactory.java +++ b/lib/java/src/org/apache/thrift/transport/TSSLTransportFactory.java @@ -23,6 +23,7 @@ import java.io.FileInputStream; import java.io.IOException; import java.net.InetAddress; import java.security.KeyStore; +import java.util.Arrays; import javax.net.ssl.KeyManagerFactory; import javax.net.ssl.SSLContext; @@ -171,6 +172,7 @@ public class TSSLTransportFactory { private static SSLContext createSSLContext(TSSLTransportParameters params) throws TTransportException { SSLContext ctx; FileInputStream fin = null; + FileInputStream fis = null; try { ctx = SSLContext.getInstance(params.protocol); @@ -188,8 +190,8 @@ public class TSSLTransportFactory { if (params.isKeyStoreSet) { kmf = KeyManagerFactory.getInstance(params.keyManagerType); KeyStore ks = KeyStore.getInstance(params.keyStoreType); - fin = new FileInputStream(params.keyStore); - ks.load(fin, params.keyPass.toCharArray()); + fis = new FileInputStream(params.keyStore); + ks.load(fis, params.keyPass.toCharArray()); kmf.init(ks, params.keyPass.toCharArray()); } @@ -213,6 +215,13 @@ public class TSSLTransportFactory { e.printStackTrace(); } } + if (fis != null) { + try { + fis.close(); + } catch (IOException e) { + e.printStackTrace(); + } + } } return ctx; @@ -271,7 +280,7 @@ public class TSSLTransportFactory { if (protocol != null) { this.protocol = protocol; } - this.cipherSuites = cipherSuites; + this.cipherSuites = Arrays.copyOf(cipherSuites, cipherSuites.length); this.clientAuth = clientAuth; } diff --git a/lib/java/src/org/apache/thrift/transport/TSaslServerTransport.java b/lib/java/src/org/apache/thrift/transport/TSaslServerTransport.java index 1a74d53a..c883c8fd 100644 --- a/lib/java/src/org/apache/thrift/transport/TSaslServerTransport.java +++ b/lib/java/src/org/apache/thrift/transport/TSaslServerTransport.java @@ -126,7 +126,7 @@ public class TSaslServerTransport extends TSaslTransport { LOGGER.debug("Received start message with status {}", message.status); if (message.status != NegotiationStatus.START) { - sendAndThrowMessage(NegotiationStatus.ERROR, "Expecting START status, received " + message.status); + throw sendAndThrowMessage(NegotiationStatus.ERROR, "Expecting START status, received " + message.status); } // Get the mechanism name. @@ -135,7 +135,7 @@ public class TSaslServerTransport extends TSaslTransport { LOGGER.debug("Received mechanism name '{}'", mechanismName); if (serverDefinition == null) { - sendAndThrowMessage(NegotiationStatus.BAD, "Unsupported mechanism type " + mechanismName); + throw sendAndThrowMessage(NegotiationStatus.BAD, "Unsupported mechanism type " + mechanismName); } SaslServer saslServer = Sasl.createSaslServer(serverDefinition.mechanism, serverDefinition.protocol, serverDefinition.serverName, serverDefinition.props, diff --git a/lib/java/src/org/apache/thrift/transport/TSaslTransport.java b/lib/java/src/org/apache/thrift/transport/TSaslTransport.java index cb15e83b..947ee139 100644 --- a/lib/java/src/org/apache/thrift/transport/TSaslTransport.java +++ b/lib/java/src/org/apache/thrift/transport/TSaslTransport.java @@ -183,7 +183,7 @@ abstract class TSaslTransport extends TTransport { NegotiationStatus status = NegotiationStatus.byValue(statusByte); if (status == null) { - sendAndThrowMessage(NegotiationStatus.ERROR, "Invalid status " + statusByte); + throw sendAndThrowMessage(NegotiationStatus.ERROR, "Invalid status " + statusByte); } else if (status == NegotiationStatus.BAD || status == NegotiationStatus.ERROR) { try { String remoteMessage = new String(payload, "UTF-8"); @@ -210,8 +210,10 @@ abstract class TSaslTransport extends TTransport { * The optional message to send to the other side. * @throws TTransportException * Always thrown with the message provided. + * @return always throws TTransportException but declares return type to allow + * throw sendAndThrowMessage(...) to inform compiler control flow */ - protected void sendAndThrowMessage(NegotiationStatus status, String message) throws TTransportException { + protected TTransportException sendAndThrowMessage(NegotiationStatus status, String message) throws TTransportException { try { sendSaslMessage(status, message.getBytes()); } catch (Exception e) { @@ -302,7 +304,7 @@ abstract class TSaslTransport extends TTransport { } catch (SaslException e) { try { LOGGER.error("SASL negotiation failure", e); - sendAndThrowMessage(NegotiationStatus.BAD, e.getMessage()); + throw sendAndThrowMessage(NegotiationStatus.BAD, e.getMessage()); } finally { underlyingTransport.close(); } -- 2.17.1