THRIFT-5563: fix deprecation and enable xlint for java library (#2575)

7 files changed, 115 insertions, 114 deletions

diff --git a/lib/java/gradle/sourceConfiguration.gradle b/lib/java/gradle/sourceConfiguration.gradle
index c510a40d5..b45fdc803 100644
--- a/lib/java/gradle/sourceConfiguration.gradle
+++ b/lib/java/gradle/sourceConfiguration.gradle
@@ -60,7 +60,16 @@ tasks.withType(JavaCompile) {
     if (JavaVersion.current() > JavaVersion.VERSION_1_8) {
         options.compilerArgs.addAll(['--release', '8'])
     }
-    // options.compilerArgs.addAll('-Xlint:unchecked')
+    options.compilerArgs.addAll([
+            '-Werror',
+            '-Xlint:deprecation',
+            '-Xlint:cast',
+            '-Xlint:empty',
+            '-Xlint:fallthrough',
+            '-Xlint:finally',
+            '-Xlint:overrides',
+            // we can't enable -Xlint:unchecked just yet
+    ])
 }
 
 // ----------------------------------------------------------------------------
diff --git a/lib/java/src/org/apache/thrift/partial/TFieldData.java b/lib/java/src/org/apache/thrift/partial/TFieldData.java
index 9ba1a17ce..d77302e48 100644
--- a/lib/java/src/org/apache/thrift/partial/TFieldData.java
+++ b/lib/java/src/org/apache/thrift/partial/TFieldData.java
@@ -27,7 +27,7 @@ package org.apache.thrift.partial;
  */
 public class TFieldData {
   public static int encode(byte type) {
-    return (int) (type & 0xff);
+    return type & 0xff;
   }
 
   public static int encode(byte type, short id) {
diff --git a/lib/java/src/org/apache/thrift/partial/ThriftMetadata.java b/lib/java/src/org/apache/thrift/partial/ThriftMetadata.java
index 1146720c2..46a37f2d9 100644
--- a/lib/java/src/org/apache/thrift/partial/ThriftMetadata.java
+++ b/lib/java/src/org/apache/thrift/partial/ThriftMetadata.java
@@ -25,23 +25,20 @@ import org.apache.thrift.TFieldIdEnum;
 import org.apache.thrift.TFieldRequirementType;
 import org.apache.thrift.TUnion;
 import org.apache.thrift.meta_data.FieldMetaData;
-import org.apache.thrift.meta_data.FieldValueMetaData;
 import org.apache.thrift.meta_data.ListMetaData;
 import org.apache.thrift.meta_data.MapMetaData;
 import org.apache.thrift.meta_data.SetMetaData;
 import org.apache.thrift.meta_data.StructMetaData;
-import org.apache.thrift.partial.Validate;
 import org.apache.thrift.protocol.TType;
 
 import java.io.Serializable;
-import java.lang.reflect.Field;
+import java.lang.reflect.Constructor;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.Set;
 
 /**
  * Container for Thrift metadata classes such as {@link ThriftPrimitive},
@@ -59,8 +56,8 @@ public class ThriftMetadata {
     MAP_VALUE((short) 4, "mapValue"),
     SET_ELEMENT((short) 5, "setElement");
 
-    private short id;
-    private String name;
+    private final short id;
+    private final String name;
 
     FieldTypeEnum(short id, String name) {
       this.id = id;
@@ -415,9 +412,9 @@ public class ThriftMetadata {
         Iterable<ThriftField> fieldsData) {
 
       if (isUnion(data)) {
-        return new ThriftUnion(parent, fieldId, data, fieldsData);
+        return new ThriftUnion<>(parent, fieldId, data, fieldsData);
       } else {
-        return new ThriftStruct(parent, fieldId, data, fieldsData);
+        return new ThriftStruct<>(parent, fieldId, data, fieldsData);
       }
     }
   }
@@ -463,18 +460,13 @@ public class ThriftMetadata {
     }
 
     public <T extends TBase> T createNewStruct() {
-      T instance = null;
-
       try {
         Class<T> structClass = getStructClass(this.data);
-        instance = (T) structClass.newInstance();
-      } catch (InstantiationException e) {
-        throw new RuntimeException(e);
-      } catch (IllegalAccessException e) {
+        Constructor<T> declaredConstructor = structClass.getDeclaredConstructor();
+        return declaredConstructor.newInstance();
+      } catch (ReflectiveOperationException e) {
         throw new RuntimeException(e);
       }
-
-      return instance;
     }
 
     public static <T extends TBase> ThriftStruct of(Class<T> clasz) {
@@ -494,7 +486,7 @@ public class ThriftMetadata {
       Validate.checkNotNull(clasz, "clasz");
       Validate.checkNotNull(fields, "fields");
 
-      return new ThriftStruct(
+      return new ThriftStruct<>(
           null,
           FieldTypeEnum.ROOT,
           new FieldMetaData(
@@ -519,7 +511,7 @@ public class ThriftMetadata {
       if (this.fields.size() == 0) {
         this.append(sb, "%s*;", indent2);
       } else {
-        List<Integer> ids = new ArrayList(this.fields.keySet());
+        List<Integer> ids = new ArrayList<>(this.fields.keySet());
         Collections.sort(ids);
         for (Integer id : ids) {
           this.fields.get(id).toPrettyString(sb, level + 1);
@@ -535,7 +527,7 @@ public class ThriftMetadata {
 
       Map<? extends TFieldIdEnum, FieldMetaData> fieldsMetaData =
           FieldMetaData.getStructMetaDataMap(clasz);
-      Map<Integer, ThriftObject> fields = new HashMap();
+      Map<Integer, ThriftObject> fields = new HashMap<>();
       boolean getAllFields = !fieldsData.iterator().hasNext();
 
       if (getAllFields) {
diff --git a/lib/java/src/org/apache/thrift/protocol/TCompactProtocol.java b/lib/java/src/org/apache/thrift/protocol/TCompactProtocol.java
index 832e197dd..3bace8e90 100644
--- a/lib/java/src/org/apache/thrift/protocol/TCompactProtocol.java
+++ b/lib/java/src/org/apache/thrift/protocol/TCompactProtocol.java
@@ -117,7 +117,7 @@ public class TCompactProtocol extends TProtocol {
    * Used to keep track of the last field for the current and previous structs,
    * so we can do the delta stuff.
    */
-  private ShortStack lastField_ = new ShortStack(15);
+  private final ShortStack lastField_ = new ShortStack(15);
 
   private short lastFieldId_ = 0;
 
@@ -617,7 +617,7 @@ public class TCompactProtocol extends TProtocol {
    */
   public boolean readBool() throws TException {
     if (boolValue_ != null) {
-      boolean result = boolValue_.booleanValue();
+      boolean result = boolValue_;
       boolValue_ = null;
       return result;
     }
@@ -750,10 +750,15 @@ public class TCompactProtocol extends TProtocol {
   // These methods are here for the struct to call, but don't have any wire
   // encoding.
   //
+  @Override
   public void readMessageEnd() throws TException {}
+  @Override
   public void readFieldEnd() throws TException {}
+  @Override
   public void readMapEnd() throws TException {}
+  @Override
   public void readListEnd() throws TException {}
+  @Override
   public void readSetEnd() throws TException {}
 
   //
@@ -773,7 +778,7 @@ public class TCompactProtocol extends TProtocol {
       int off = 0;
       while (true) {
         byte b = buf[pos+off];
-        result |= (int) (b & 0x7f) << shift;
+        result |= (b & 0x7f) << shift;
         if ((b & 0x80) != 0x80) break;
         shift += 7;
         off++;
@@ -782,7 +787,7 @@ public class TCompactProtocol extends TProtocol {
     } else {
       while (true) {
         byte b = readByte();
-        result |= (int) (b & 0x7f) << shift;
+        result |= (b & 0x7f) << shift;
         if ((b & 0x80) != 0x80) break;
         shift += 7;
       }
diff --git a/lib/java/src/org/apache/thrift/transport/THttpClient.java b/lib/java/src/org/apache/thrift/transport/THttpClient.java
index 7d61b5c8e..574682248 100644
--- a/lib/java/src/org/apache/thrift/transport/THttpClient.java
+++ b/lib/java/src/org/apache/thrift/transport/THttpClient.java
@@ -21,11 +21,11 @@ package org.apache.thrift.transport;
 
 import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
-import java.io.InputStream;
 import java.io.IOException;
-
-import java.net.URL;
+import java.io.InputStream;
 import java.net.HttpURLConnection;
+import java.net.URL;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
 
@@ -34,9 +34,9 @@ import org.apache.http.HttpHost;
 import org.apache.http.HttpResponse;
 import org.apache.http.HttpStatus;
 import org.apache.http.client.HttpClient;
+import org.apache.http.client.config.RequestConfig;
 import org.apache.http.client.methods.HttpPost;
 import org.apache.http.entity.ByteArrayEntity;
-import org.apache.http.params.CoreConnectionPNames;
 import org.apache.thrift.TConfiguration;
 
 /**
@@ -68,7 +68,7 @@ import org.apache.thrift.TConfiguration;
 
 public class THttpClient extends TEndpointTransport {
 
-  private URL url_ = null;
+  private final URL url_;
 
   private final ByteArrayOutputStream requestBuffer_ = new ByteArrayOutputStream();
 
@@ -84,6 +84,8 @@ public class THttpClient extends TEndpointTransport {
 
   private final HttpClient client;
 
+  private static final Map<String, String> DEFAULT_HEADERS = Collections.unmodifiableMap(getDefaultHeaders());
+
   public static class Factory extends TTransportFactory {
 
     private final String url;
@@ -159,35 +161,27 @@ public class THttpClient extends TEndpointTransport {
 
   public void setConnectTimeout(int timeout) {
     connectTimeout_ = timeout;
-    if (null != this.client) {
-      // WARNING, this modifies the HttpClient params, this might have an impact elsewhere if the
-      // same HttpClient is used for something else.
-      client.getParams().setParameter(CoreConnectionPNames.CONNECTION_TIMEOUT, connectTimeout_);
-    }
   }
 
   public void setReadTimeout(int timeout) {
     readTimeout_ = timeout;
-    if (null != this.client) {
-      // WARNING, this modifies the HttpClient params, this might have an impact elsewhere if the
-      // same HttpClient is used for something else.
-      client.getParams().setParameter(CoreConnectionPNames.SO_TIMEOUT, readTimeout_);
-    }
   }
 
   public void setCustomHeaders(Map<String,String> headers) {
-    customHeaders_ = headers;
+    customHeaders_ = new HashMap<>(headers);
   }
 
   public void setCustomHeader(String key, String value) {
     if (customHeaders_ == null) {
-      customHeaders_ = new HashMap<String, String>();
+      customHeaders_ = new HashMap<>();
     }
     customHeaders_.put(key, value);
   }
 
+  @Override
   public void open() {}
 
+  @Override
   public void close() {
     if (null != inputStream_) {
       try {
@@ -198,10 +192,12 @@ public class THttpClient extends TEndpointTransport {
     }
   }
 
+  @Override
   public boolean isOpen() {
     return true;
   }
 
+  @Override
   public int read(byte[] buf, int off, int len) throws TTransportException {
     if (inputStream_ == null) {
       throw new TTransportException("Response buffer is empty, no request.");
@@ -222,10 +218,30 @@ public class THttpClient extends TEndpointTransport {
     }
   }
 
+  @Override
   public void write(byte[] buf, int off, int len) {
     requestBuffer_.write(buf, off, len);
   }
 
+  private RequestConfig getRequestConfig() {
+    RequestConfig requestConfig = RequestConfig.DEFAULT;
+    if (connectTimeout_ > 0) {
+      requestConfig = RequestConfig.copy(requestConfig).setConnectionRequestTimeout(connectTimeout_).build();
+    }
+    if (readTimeout_ > 0) {
+      requestConfig = RequestConfig.copy(requestConfig).setSocketTimeout(readTimeout_).build();
+    }
+    return requestConfig;
+  }
+
+  private static Map<String, String> getDefaultHeaders() {
+    Map<String, String> headers = new HashMap<>();
+    headers.put("Content-Type", "application/x-thrift");
+    headers.put("Accept", "application/x-thrift");
+    headers.put("User-Agent", "Java/THttpClient/HC");
+    return headers;
+  }
+
   /**
    * copy from org.apache.http.util.EntityUtils#consume. Android has it's own httpcore
    * that doesn't have a consume.
@@ -243,7 +259,6 @@ public class THttpClient extends TEndpointTransport {
   }
 
   private void flushUsingHttpClient() throws TTransportException {
-
     if (null == this.client) {
       throw new TTransportException("Null HttpClient, aborting.");
     }
@@ -252,63 +267,36 @@ public class THttpClient extends TEndpointTransport {
     byte[] data = requestBuffer_.toByteArray();
     requestBuffer_.reset();
 
-    HttpPost post = null;
-
-    InputStream is = null;
-
+    HttpPost post = new HttpPost(this.url_.getFile());
     try {
       // Set request to path + query string
-      post = new HttpPost(this.url_.getFile());
-
-      //
-      // Headers are added to the HttpPost instance, not
-      // to HttpClient.
-      //
-
-      post.setHeader("Content-Type", "application/x-thrift");
-      post.setHeader("Accept", "application/x-thrift");
-      post.setHeader("User-Agent", "Java/THttpClient/HC");
-
+      post.setConfig(getRequestConfig());
+      DEFAULT_HEADERS.forEach(post::addHeader);
       if (null != customHeaders_) {
-        for (Map.Entry<String, String> header : customHeaders_.entrySet()) {
-          post.setHeader(header.getKey(), header.getValue());
-        }
+        customHeaders_.forEach(post::addHeader);
       }
-
       post.setEntity(new ByteArrayEntity(data));
-
       HttpResponse response = this.client.execute(this.host, post);
-      int responseCode = response.getStatusLine().getStatusCode();
-
-      //
-      // Retrieve the inputstream BEFORE checking the status code so
-      // resources get freed in the finally clause.
-      //
-
-      is = response.getEntity().getContent();
+      handleResponse(response);
+    } catch (IOException ioe) {
+      // Abort method so the connection gets released back to the connection manager
+      post.abort();
+      throw new TTransportException(ioe);
+    } finally {
+      resetConsumedMessageSize(-1);
+      post.releaseConnection();
+    }
+  }
 
+  private void handleResponse(HttpResponse response) throws TTransportException {
+    // Retrieve the InputStream BEFORE checking the status code so
+    // resources get freed in the with clause.
+    try (InputStream is = response.getEntity().getContent()) {
+      int responseCode = response.getStatusLine().getStatusCode();
       if (responseCode != HttpStatus.SC_OK) {
         throw new TTransportException("HTTP Response code: " + responseCode);
       }
-
-      // Read the responses into a byte array so we can release the connection
-      // early. This implies that the whole content will have to be read in
-      // memory, and that momentarily we might use up twice the memory (while the
-      // thrift struct is being read up the chain).
-      // Proceeding differently might lead to exhaustion of connections and thus
-      // to app failure.
-
-      byte[] buf = new byte[1024];
-      ByteArrayOutputStream baos = new ByteArrayOutputStream();
-
-      int len = 0;
-      do {
-        len = is.read(buf);
-        if (len > 0) {
-          baos.write(buf, 0, len);
-        }
-      } while (-1 != len);
-
+      byte[] readByteArray = readIntoByteArray(is);
       try {
         // Indicate we're done with the content.
         consume(response.getEntity());
@@ -316,30 +304,37 @@ public class THttpClient extends TEndpointTransport {
         // We ignore this exception, it might only mean the server has no
         // keep-alive capability.
       }
-
-      inputStream_ = new ByteArrayInputStream(baos.toByteArray());
+      inputStream_ = new ByteArrayInputStream(readByteArray);
     } catch (IOException ioe) {
-      // Abort method so the connection gets released back to the connection manager
-      if (null != post) {
-        post.abort();
-      }
       throw new TTransportException(ioe);
-    } finally {
-      resetConsumedMessageSize(-1);
-      if (null != is) {
-        // Close the entity's input stream, this will release the underlying connection
-        try {
-          is.close();
-        } catch (IOException ioe) {
-          throw new TTransportException(ioe);
-        }
-      }
-      if (post != null) {
-        post.releaseConnection();
-      }
     }
   }
 
+  /**
+   * Read the responses into a byte array so we can release the connection
+   * early. This implies that the whole content will have to be read in
+   * memory, and that momentarily we might use up twice the memory (while the
+   * thrift struct is being read up the chain).
+   * Proceeding differently might lead to exhaustion of connections and thus
+   * to app failure.
+   *
+   * @param is input stream
+   * @return read bytes
+   * @throws IOException when exception during read
+   */
+  private static byte[] readIntoByteArray(InputStream is) throws IOException {
+    ByteArrayOutputStream baos = new ByteArrayOutputStream();
+    byte[] buf = new byte[1024];
+    int len;
+    do {
+      len = is.read(buf);
+      if (len > 0) {
+        baos.write(buf, 0, len);
+      }
+    } while (-1 != len);
+    return baos.toByteArray();
+  }
+
   public void flush() throws TTransportException {
 
     if (null != this.client) {
diff --git a/lib/java/test/org/apache/thrift/protocol/ProtocolTestBase.java b/lib/java/test/org/apache/thrift/protocol/ProtocolTestBase.java
index 52b107441..a87327121 100644
--- a/lib/java/test/org/apache/thrift/protocol/ProtocolTestBase.java
+++ b/lib/java/test/org/apache/thrift/protocol/ProtocolTestBase.java
@@ -167,7 +167,7 @@ public abstract class ProtocolTestBase extends TestCase {
       }
 
       public void readMethod(TProtocol proto) throws TException {
-        assertEquals((byte)b, proto.readByte());
+        assertEquals(b, proto.readByte());
       }
     });
   }
diff --git a/lib/java/test/org/apache/thrift/test/JavaBeansTest.java b/lib/java/test/org/apache/thrift/test/JavaBeansTest.java
index 6a2a0ed0c..0bfcefec0 100644
--- a/lib/java/test/org/apache/thrift/test/JavaBeansTest.java
+++ b/lib/java/test/org/apache/thrift/test/JavaBeansTest.java
@@ -61,10 +61,10 @@ public class JavaBeansTest {
     // Everything is set
     ooe.set_a_bite((byte) 1);
     ooe.set_base64(ByteBuffer.wrap("bytes".getBytes()));
-    ooe.set_byte_list(new LinkedList<Byte>());
+    ooe.set_byte_list(new LinkedList<>());
     ooe.set_double_precision(1);
-    ooe.set_i16_list(new LinkedList<Short>());
-    ooe.set_i64_list(new LinkedList<Long>());
+    ooe.set_i16_list(new LinkedList<>());
+    ooe.set_i64_list(new LinkedList<>());
     ooe.set_boolean_field(true);
     ooe.set_integer16((short) 1);
     ooe.set_integer32(1);
@@ -102,7 +102,7 @@ public class JavaBeansTest {
     // Should throw exception when field doesn't exist
     boolean exceptionThrown = false;
     try{
-      if (ooe.isSet(ooe.fieldForId(100)));
+      ooe.isSet(ooe.fieldForId(100));
     } catch (IllegalArgumentException e){
       exceptionThrown = true;
     }