Support simpler annotations for Thrift exceptions

drift-api/src/main/java/io/airlift/drift/annotations/ThriftId.java

@@ -0,0 +1,34 @@
+/*
+ * Copyright (C) 2018 Facebook, Inc.
+ *
+ * Licensed 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.airlift.drift.annotations;
+
+import java.lang.annotation.Documented;
+import java.lang.annotation.Retention;
+import java.lang.annotation.Target;
+
+import static java.lang.annotation.ElementType.TYPE_USE;
+import static java.lang.annotation.RetentionPolicy.RUNTIME;
+
+/**
+ * Provides mapping for Thrift method exceptions IDs
+ */
+@Documented
+@Retention(RUNTIME)
+@Target(TYPE_USE)
+public @interface ThriftId
+{
+    short value();
+}

drift-client/README.md

@@ -58,14 +58,22 @@ As with method parameters, Thrift encodes the response as a struct with field
 zero being a standard return and exceptions be stored in higher number fields.
 If the Java method throws only one exception annotated with `@ThriftStruct`,
 Drift will assume the result struct field id is `1`. Otherwise, you will need to
-add the extremely verbose `@ThriftException` annotations as follows:
+add the `@ThriftId` annotation to the exception declaration:
+
+```java
+@ThriftMethod
+void doSomething() throws @ThriftId(1) MyException, @ThriftId(2) MyOther;
+```
+
+For asynchronous methods, which do not directly throw exceptions, you will need
+to use the ``@ThriftException`` annotation:
 
 ```java
 @ThriftMethod(exception = {
       @ThriftException(type = MyException.class, id = 1),
       @ThriftException(type = MyOther.class, id = 2),
 })
-void doSomething() throws MyException, MyOther;
+ListenableFuture<Void> doSomething();
 ```
 
 ## Using a Client

drift-codec/src/main/java/io/airlift/drift/codec/metadata/ThriftMethodMetadata.java

@@ -24,13 +24,15 @@
 import io.airlift.drift.annotations.ThriftException;
 import io.airlift.drift.annotations.ThriftField;
 import io.airlift.drift.annotations.ThriftHeader;
+import io.airlift.drift.annotations.ThriftId;
 import io.airlift.drift.annotations.ThriftIdlAnnotation;
 import io.airlift.drift.annotations.ThriftMethod;
 import io.airlift.drift.annotations.ThriftStruct;
 
 import javax.annotation.concurrent.Immutable;
 
 import java.lang.annotation.Annotation;
+import java.lang.reflect.AnnotatedType;
 import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
 import java.lang.reflect.Type;
@@ -243,6 +245,10 @@ public List<String> getDocumentation()
 
     private ImmutableMap<Short, ThriftType> buildExceptionMap(ThriftCatalog catalog, ThriftMethod thriftMethod)
     {
+        boolean mixedStyle = (thriftMethod.exception().length > 0) &&
+                stream(method.getAnnotatedExceptionTypes()).anyMatch(type -> type.isAnnotationPresent(ThriftId.class));
+        checkArgument(!mixedStyle, "ThriftMethod [%s] uses a mix of @ThriftException and @ThriftId", methodName(method));
+
         Map<Short, ThriftType> exceptions = new HashMap<>();
         Set<Type> exceptionTypes = new HashSet<>();
 
@@ -253,6 +259,19 @@ private ImmutableMap<Short, ThriftType> buildExceptionMap(ThriftCatalog catalog,
             exceptionTypes.add(thriftException.type());
         }
 
+        Class<?>[] allExceptionClasses = method.getExceptionTypes();
+        AnnotatedType[] exceptionAnnotations = method.getAnnotatedExceptionTypes();
+        for (int i = 0; i < allExceptionClasses.length; i++) {
+            Class<?> exception = allExceptionClasses[i];
+            ThriftId thriftId = exceptionAnnotations[i].getAnnotation(ThriftId.class);
+            if (thriftId != null) {
+                checkArgument(!exceptions.containsKey(thriftId.value()), "ThriftMethod [%s] exception list contains multiple values for field ID [%s]", methodName(method), thriftId.value());
+                checkArgument(!exceptionTypes.contains(exception), "ThriftMethod [%s] exception list contains multiple values for type [%s]", methodName(method), exception.getSimpleName());
+                exceptions.put(thriftId.value(), catalog.getThriftType(exception));
+                exceptionTypes.add(exception);
+            }
+        }
+
         // the built-in exception types don't need special treatment
         List<Class<?>> exceptionClasses = stream(method.getExceptionTypes())
                 .filter(exception -> !exception.isAssignableFrom(TException.class))

drift-codec/src/test/java/io/airlift/drift/codec/metadata/TestThriftMethodMetadata.java

@@ -20,6 +20,7 @@
 import io.airlift.drift.annotations.ThriftException;
 import io.airlift.drift.annotations.ThriftField;
 import io.airlift.drift.annotations.ThriftHeader;
+import io.airlift.drift.annotations.ThriftId;
 import io.airlift.drift.annotations.ThriftMethod;
 import io.airlift.drift.annotations.ThriftStruct;
 import org.testng.annotations.Test;
@@ -112,7 +113,8 @@ public void testNoExceptions()
     @Test
     public void testAnnotatedExceptions()
     {
-        assertExceptions("annotatedExceptions", ExceptionA.class, ExceptionB.class);
+        assertExceptions("annotatedExceptionsMethod", ExceptionA.class, ExceptionB.class);
+        assertExceptions("annotatedExceptionsThrows", ExceptionA.class, ExceptionB.class);
     }
 
     @Test
@@ -151,10 +153,22 @@ public void testInvalidExceptionDuplicateType()
         assertExceptions("testDuplicateExceptionType");
     }
 
-    @Test(expectedExceptions = IllegalArgumentException.class, expectedExceptionsMessageRegExp = "ThriftMethod \\[.*\\.testDuplicateExceptionField] exception list contains multiple values for field ID \\[2]")
-    public void testInvalidExceptionDuplicateField()
+    @Test(expectedExceptions = IllegalArgumentException.class, expectedExceptionsMessageRegExp = "ThriftMethod \\[.*\\.testDuplicateExceptionFieldMethod] exception list contains multiple values for field ID \\[3]")
+    public void testDuplicateExceptionFieldMethod()
     {
-        assertExceptions("testDuplicateExceptionField");
+        assertExceptions("testDuplicateExceptionFieldMethod");
+    }
+
+    @Test(expectedExceptions = IllegalArgumentException.class, expectedExceptionsMessageRegExp = "ThriftMethod \\[.*\\.testDuplicateExceptionFieldThrows] exception list contains multiple values for field ID \\[4]")
+    public void testDuplicateExceptionFieldThrows()
+    {
+        assertExceptions("testDuplicateExceptionFieldThrows");
+    }
+
+    @Test(expectedExceptions = IllegalArgumentException.class, expectedExceptionsMessageRegExp = "ThriftMethod \\[.*\\.testExceptionMixedAnnotationStyle] uses a mix of @ThriftException and @ThriftId")
+    public void testExceptionMixedAnnotationStyle()
+    {
+        assertExceptions("testExceptionMixedAnnotationStyle");
     }
 
     @SafeVarargs
@@ -241,9 +255,13 @@ void validHeaderParameters(
         void noExceptions();
 
         @ThriftMethod(exception = {@ThriftException(id = 1, type = ExceptionA.class), @ThriftException(id = 2, type = ExceptionB.class)})
-        void annotatedExceptions()
+        void annotatedExceptionsMethod()
                 throws ExceptionA, ExceptionB;
 
+        @ThriftMethod
+        void annotatedExceptionsThrows()
+                throws @ThriftId(1) ExceptionA, @ThriftId(2) ExceptionB;
+
         @ThriftMethod
         void inferredException()
                 throws ExceptionA;
@@ -272,11 +290,19 @@ void testDuplicateExceptionType()
                 throws ExceptionA;
 
         @ThriftMethod(exception = {
-                @ThriftException(id = 2, type = ExceptionA.class),
-                @ThriftException(id = 2, type = ExceptionB.class),
+                @ThriftException(id = 3, type = ExceptionA.class),
+                @ThriftException(id = 3, type = ExceptionB.class),
         })
-        void testDuplicateExceptionField()
+        void testDuplicateExceptionFieldMethod()
                 throws ExceptionA, ExceptionB;
+
+        @ThriftMethod
+        void testDuplicateExceptionFieldThrows()
+                throws @ThriftId(4) ExceptionA, @ThriftId(4) ExceptionB;
+
+        @ThriftMethod(exception = @ThriftException(id = 5, type = ExceptionA.class))
+        void testExceptionMixedAnnotationStyle()
+                throws ExceptionA, @ThriftId(6) ExceptionB;
     }
 
     @ThriftStruct

drift-idl-generator/src/test/java/io/airlift/drift/idl/generator/DriftScribe.java

@@ -16,7 +16,7 @@
 package io.airlift.drift.idl.generator;
 
 import io.airlift.drift.annotations.ThriftDocumentation;
-import io.airlift.drift.annotations.ThriftException;
+import io.airlift.drift.annotations.ThriftId;
 import io.airlift.drift.annotations.ThriftMethod;
 import io.airlift.drift.annotations.ThriftOrder;
 import io.airlift.drift.annotations.ThriftService;
@@ -39,22 +39,20 @@ public interface DriftScribe
             "@param messages the list of messages to send",
     })
     @ThriftOrder(1)
-    @ThriftMethod(exception = {
-            @ThriftException(id = 1, type = ScribeDataException.class),
-            @ThriftException(id = 2, type = ScribeTransportException.class),
-    })
+    @ThriftMethod
     DriftResultCode log(List<DriftLogEntry> messages)
-            throws ScribeDataException, ScribeTransportException;
+            throws
+            @ThriftId(1) ScribeDataException,
+            @ThriftId(2) ScribeTransportException;
 
     @ThriftDocumentation("Send a formatted message to Scribe.")
     @ThriftOrder(2)
-    @ThriftMethod(exception = {
-            @ThriftException(id = 1, type = ScribeDataException.class),
-            @ThriftException(id = 2, type = ScribeTransportException.class),
-            @ThriftException(id = 3, type = ScribeMessageException.class),
-    })
+    @ThriftMethod
     DriftResultCode logFormattedMessage(String format, Map<String, DriftLogEntry> messages, int maxSize)
-            throws ScribeDataException, ScribeTransportException;
+            throws
+            @ThriftId(1) ScribeDataException,
+            @ThriftId(2) ScribeTransportException,
+            @ThriftId(3) ScribeMessageException;
 
     @ThriftDocumentation("Check if service is up")
     @ThriftOrder(3)