From f26887b084761e79b8b3a6a05a72abaed74d7ca0 Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Fri, 30 Sep 2011 11:05:15 +0100 Subject: [PATCH 1/4] fix for 698, and test (better error messages on illegal null params); also spotted some tests that don't actually test what's expected --- .../internal/RestAnnotationProcessor.java | 9 ++- .../internal/RestAnnotationProcessorTest.java | 59 ++++++++++++++++++- 2 files changed, 65 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/org/jclouds/rest/internal/RestAnnotationProcessor.java b/core/src/main/java/org/jclouds/rest/internal/RestAnnotationProcessor.java index b6f7b95403..75f96b24c9 100644 --- a/core/src/main/java/org/jclouds/rest/internal/RestAnnotationProcessor.java +++ b/core/src/main/java/org/jclouds/rest/internal/RestAnnotationProcessor.java @@ -135,6 +135,7 @@ import org.jclouds.util.Strings2; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Function; import com.google.common.base.Functions; +import com.google.common.base.Preconditions; import com.google.common.base.Predicate; import com.google.common.base.Predicates; import com.google.common.base.Throwables; @@ -1093,7 +1094,9 @@ public class RestAnnotationProcessor { options.contentType(param.contentType()); if (!PartParam.NO_FILENAME.equals(param.filename())) options.filename(Strings2.replaceTokens(param.filename(), iterable)); - Part part = Part.create(param.name(), newPayload(args[entry.getKey()]), options); + Object arg = args[entry.getKey()]; + Preconditions.checkNotNull(arg, param.name()); + Part part = Part.create(param.name(), newPayload(arg), options); parts.add(part); } } @@ -1200,7 +1203,9 @@ public class RestAnnotationProcessor { ParamParser extractor = (ParamParser) extractors.iterator().next(); paramValue = injector.getInstance(extractor.value()).apply(args[entry.getKey()]); } else { - paramValue = args[entry.getKey()].toString(); + Object pvo = args[entry.getKey()]; + Preconditions.checkNotNull(pvo, paramKey); + paramValue = pvo.toString(); } formParamValues.put(paramKey, paramValue); } diff --git a/core/src/test/java/org/jclouds/rest/internal/RestAnnotationProcessorTest.java b/core/src/test/java/org/jclouds/rest/internal/RestAnnotationProcessorTest.java index f98fb7ab18..f943e2ee98 100644 --- a/core/src/test/java/org/jclouds/rest/internal/RestAnnotationProcessorTest.java +++ b/core/src/test/java/org/jclouds/rest/internal/RestAnnotationProcessorTest.java @@ -143,6 +143,7 @@ import org.jclouds.rest.binders.BindToJsonPayload; import org.jclouds.rest.binders.BindToStringPayload; import org.jclouds.rest.config.RestClientModule; import org.jclouds.util.Strings2; +import org.testng.Assert; import org.testng.annotations.BeforeClass; import org.testng.annotations.DataProvider; import org.testng.annotations.Test; @@ -552,6 +553,9 @@ public class RestAnnotationProcessorTest extends BaseRestClientTest { @POST void post(@Nullable @BinderParam(BindToStringPayload.class) String content); + @POST + void postNonnull(@BinderParam(BindToStringPayload.class) String content); + @POST public void postAsJson(@BinderParam(BindToJsonPayload.class) String content); @@ -587,7 +591,7 @@ public class RestAnnotationProcessorTest extends BaseRestClientTest { assertPayloadEquals(request, "data", "application/unknown", false); } - public void testCreatePostRequestNullOk() throws SecurityException, NoSuchMethodException, IOException { + public void testCreatePostRequestNullOk1() throws SecurityException, NoSuchMethodException, IOException { Method method = TestPost.class.getMethod("post", String.class); HttpRequest request = factory(TestPost.class).createRequest(method); @@ -596,6 +600,37 @@ public class RestAnnotationProcessorTest extends BaseRestClientTest { assertPayloadEquals(request, null, "application/unknown", false); } + public void testCreatePostRequestNullOk2() throws SecurityException, NoSuchMethodException, IOException { + Method method = TestPost.class.getMethod("post", String.class); + HttpRequest request = factory(TestPost.class).createRequest(method, (String)null); + + assertRequestLineEquals(request, "POST http://localhost:9999 HTTP/1.1"); + assertNonPayloadHeadersEqual(request, ""); + assertPayloadEquals(request, null, "application/unknown", false); + } + + //FIXME the following two tests fail, suggesting that the tests above aren't actually testing Nullable + //but rather indicates that RestAnnotationProcessor.decorateRequest silently ignores nulls +// public void testCreatePostRequestNullNotOk1() throws SecurityException, NoSuchMethodException, IOException { +// Method method = TestPost.class.getMethod("postNonnull", String.class); +// try { +// HttpRequest request = factory(TestPost.class).createRequest(method); +// Assert.fail("call should have failed with illegal null parameter, not permitted "+request+" to be created"); +// } catch (NullPointerException e) { +//// Assert.assertTrue(e.toString().indexOf("???")>=0, "Error message should have referred to parameter 'fooble': "+e); +// } +// } +// +// public void testCreatePostRequestNullNotOk2() throws SecurityException, NoSuchMethodException, IOException { +// Method method = TestPost.class.getMethod("postNonnull", String.class); +// try { +// HttpRequest request = factory(TestPost.class).createRequest(method, (String)null); +// Assert.fail("call should have failed with illegal null parameter, not permitted "+request+" to be created"); +// } catch (NullPointerException e) { +// // Assert.assertTrue(e.toString().indexOf("???")>=0, "Error message should have referred to parameter 'fooble': "+e); +// } +// } + public void testCreatePostJsonRequest() throws SecurityException, NoSuchMethodException, IOException { Method method = TestPost.class.getMethod("postAsJson", String.class); HttpRequest request = factory(TestPost.class).createRequest(method, "data"); @@ -684,6 +719,17 @@ public class RestAnnotationProcessorTest extends BaseRestClientTest { "----JCLOUDS----\r\n", "multipart/form-data; boundary=--JCLOUDS--", false); } + public void testMultipartWithStringPartNullNotOkay() throws SecurityException, NoSuchMethodException, IOException { + Method method = TestMultipartForm.class.getMethod("withStringPart", String.class); + try { + GeneratedHttpRequest httpRequest = factory(TestMultipartForm.class).createRequest(method, + (String)null); + Assert.fail("call should have failed with illegal null parameter, not permitted "+httpRequest+" to be created"); + } catch (NullPointerException e) { + Assert.assertTrue(e.toString().indexOf("fooble")>=0, "Error message should have referred to parameter 'fooble': "+e); + } + } + public void testMultipartWithParamStringPart() throws SecurityException, NoSuchMethodException, IOException { Method method = TestMultipartForm.class.getMethod("withParamStringPart", String.class, String.class); GeneratedHttpRequest httpRequest = factory(TestMultipartForm.class).createRequest(method, @@ -702,6 +748,17 @@ public class RestAnnotationProcessorTest extends BaseRestClientTest { "----JCLOUDS----\r\n", "multipart/form-data; boundary=--JCLOUDS--", false); } + public void testMultipartWithParamStringPartNullNotOk() throws SecurityException, NoSuchMethodException, IOException { + Method method = TestMultipartForm.class.getMethod("withParamStringPart", String.class, String.class); + try { + GeneratedHttpRequest httpRequest = factory(TestMultipartForm.class).createRequest(method, + null, "foobledata"); + Assert.fail("call should have failed with illegal null parameter, not permitted "+httpRequest+" to be created"); + } catch (NullPointerException e) { + Assert.assertTrue(e.toString().indexOf("name")>=0, "Error message should have referred to parameter 'name': "+e); + } + } + public void testMultipartWithParamFilePart() throws SecurityException, NoSuchMethodException, IOException { Method method = TestMultipartForm.class.getMethod("withParamFilePart", String.class, File.class); File file = File.createTempFile("foo", "bar"); From 70d58180e078a34fd8c82efdfa3a9b8efada07b3 Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Fri, 30 Sep 2011 11:26:53 +0100 Subject: [PATCH 2/4] fix to check for nullable in those places where we test that nullable is supported, and to test that leaving out nullable disallows null parameters --- .../internal/RestAnnotationProcessor.java | 15 +++++++ .../internal/RestAnnotationProcessorTest.java | 40 +++++++++---------- 2 files changed, 34 insertions(+), 21 deletions(-) diff --git a/core/src/main/java/org/jclouds/rest/internal/RestAnnotationProcessor.java b/core/src/main/java/org/jclouds/rest/internal/RestAnnotationProcessor.java index 75f96b24c9..19321a8228 100644 --- a/core/src/main/java/org/jclouds/rest/internal/RestAnnotationProcessor.java +++ b/core/src/main/java/org/jclouds/rest/internal/RestAnnotationProcessor.java @@ -960,6 +960,21 @@ public class RestAnnotationProcessor { } if (shouldBreak) break OUTER; + } else { + // either arg is null, or request.getArgs().size() < entry.getKey() + 1 + // in either case, we require that null be allowed + // (first, however, let's make sure we have enough args on the actual method) + if (entry.getKey() >= request.getJavaMethod().getParameterAnnotations().length) { + // not known whether this happens + throw new IllegalArgumentException("Argument index "+(entry.getKey()+1)+" is out of bounds for method "+request.getJavaMethod()); + } + + Annotation[] annotations = request.getJavaMethod().getParameterAnnotations()[entry.getKey()]; + for (Annotation a: annotations) { + if (Nullable.class.isAssignableFrom(a.annotationType())) + continue OUTER; + } + Preconditions.checkNotNull(null, request.getJavaMethod().getName()+" parameter "+(entry.getKey()+1)); } } diff --git a/core/src/test/java/org/jclouds/rest/internal/RestAnnotationProcessorTest.java b/core/src/test/java/org/jclouds/rest/internal/RestAnnotationProcessorTest.java index f943e2ee98..ad225d1884 100644 --- a/core/src/test/java/org/jclouds/rest/internal/RestAnnotationProcessorTest.java +++ b/core/src/test/java/org/jclouds/rest/internal/RestAnnotationProcessorTest.java @@ -609,27 +609,25 @@ public class RestAnnotationProcessorTest extends BaseRestClientTest { assertPayloadEquals(request, null, "application/unknown", false); } - //FIXME the following two tests fail, suggesting that the tests above aren't actually testing Nullable - //but rather indicates that RestAnnotationProcessor.decorateRequest silently ignores nulls -// public void testCreatePostRequestNullNotOk1() throws SecurityException, NoSuchMethodException, IOException { -// Method method = TestPost.class.getMethod("postNonnull", String.class); -// try { -// HttpRequest request = factory(TestPost.class).createRequest(method); -// Assert.fail("call should have failed with illegal null parameter, not permitted "+request+" to be created"); -// } catch (NullPointerException e) { -//// Assert.assertTrue(e.toString().indexOf("???")>=0, "Error message should have referred to parameter 'fooble': "+e); -// } -// } -// -// public void testCreatePostRequestNullNotOk2() throws SecurityException, NoSuchMethodException, IOException { -// Method method = TestPost.class.getMethod("postNonnull", String.class); -// try { -// HttpRequest request = factory(TestPost.class).createRequest(method, (String)null); -// Assert.fail("call should have failed with illegal null parameter, not permitted "+request+" to be created"); -// } catch (NullPointerException e) { -// // Assert.assertTrue(e.toString().indexOf("???")>=0, "Error message should have referred to parameter 'fooble': "+e); -// } -// } + public void testCreatePostRequestNullNotOk1() throws SecurityException, NoSuchMethodException, IOException { + Method method = TestPost.class.getMethod("postNonnull", String.class); + try { + HttpRequest request = factory(TestPost.class).createRequest(method); + Assert.fail("call should have failed with illegal null parameter, not permitted "+request+" to be created"); + } catch (NullPointerException e) { + Assert.assertTrue(e.toString().indexOf("postNonnull parameter 1")>=0, "Error message should have referred to 'parameter 1': "+e); + } + } + + public void testCreatePostRequestNullNotOk2() throws SecurityException, NoSuchMethodException, IOException { + Method method = TestPost.class.getMethod("postNonnull", String.class); + try { + HttpRequest request = factory(TestPost.class).createRequest(method, (String)null); + Assert.fail("call should have failed with illegal null parameter, not permitted "+request+" to be created"); + } catch (NullPointerException e) { + Assert.assertTrue(e.toString().indexOf("postNonnull parameter 1")>=0, "Error message should have referred to parameter 'parameter 1': "+e); + } + } public void testCreatePostJsonRequest() throws SecurityException, NoSuchMethodException, IOException { Method method = TestPost.class.getMethod("postAsJson", String.class); From f59d1fec0bb956948e4044405b40a1a0687f64ea Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Fri, 30 Sep 2011 11:49:50 +0100 Subject: [PATCH 3/4] allow null/missing for varargs params (needed e.g. for EC2 ElasticBlockStoreAsyncClient.describeVolumesInRegion) --- .../org/jclouds/rest/internal/RestAnnotationProcessor.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/core/src/main/java/org/jclouds/rest/internal/RestAnnotationProcessor.java b/core/src/main/java/org/jclouds/rest/internal/RestAnnotationProcessor.java index 19321a8228..460d866482 100644 --- a/core/src/main/java/org/jclouds/rest/internal/RestAnnotationProcessor.java +++ b/core/src/main/java/org/jclouds/rest/internal/RestAnnotationProcessor.java @@ -969,6 +969,10 @@ public class RestAnnotationProcessor { throw new IllegalArgumentException("Argument index "+(entry.getKey()+1)+" is out of bounds for method "+request.getJavaMethod()); } + if (request.getJavaMethod().isVarArgs() && entry.getKey() + 1 == request.getJavaMethod().getParameterTypes().length) + //allow null/missing for var args + continue OUTER; + Annotation[] annotations = request.getJavaMethod().getParameterAnnotations()[entry.getKey()]; for (Annotation a: annotations) { if (Nullable.class.isAssignableFrom(a.annotationType())) From 1fdd46c1640693ffa518268178f3247ebd4ad934 Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Fri, 30 Sep 2011 09:47:52 -0700 Subject: [PATCH 4/4] formatting --- .../internal/RestAnnotationProcessor.java | 34 ++++++++++--------- .../internal/RestAnnotationProcessorTest.java | 18 ++++++---- 2 files changed, 30 insertions(+), 22 deletions(-) diff --git a/core/src/main/java/org/jclouds/rest/internal/RestAnnotationProcessor.java b/core/src/main/java/org/jclouds/rest/internal/RestAnnotationProcessor.java index 460d866482..6ea7e2a4a7 100644 --- a/core/src/main/java/org/jclouds/rest/internal/RestAnnotationProcessor.java +++ b/core/src/main/java/org/jclouds/rest/internal/RestAnnotationProcessor.java @@ -46,9 +46,9 @@ import java.util.Collections; import java.util.Comparator; import java.util.List; import java.util.Map; -import java.util.Map.Entry; import java.util.Set; import java.util.SortedSet; +import java.util.Map.Entry; import java.util.concurrent.ExecutionException; import javax.annotation.Resource; @@ -77,7 +77,6 @@ import org.jclouds.http.HttpUtils; import org.jclouds.http.functions.ParseFirstJsonValueNamed; import org.jclouds.http.functions.ParseJson; import org.jclouds.http.functions.ParseSax; -import org.jclouds.http.functions.ParseSax.HandlerWithResult; import org.jclouds.http.functions.ParseURIFromListOrLocationHeaderIf20x; import org.jclouds.http.functions.ReleasePayloadAndReturn; import org.jclouds.http.functions.ReturnInputStream; @@ -87,6 +86,7 @@ import org.jclouds.http.functions.UnwrapOnlyJsonValue; import org.jclouds.http.functions.UnwrapOnlyJsonValueInSet; import org.jclouds.http.functions.UnwrapOnlyNestedJsonValue; import org.jclouds.http.functions.UnwrapOnlyNestedJsonValueInSet; +import org.jclouds.http.functions.ParseSax.HandlerWithResult; import org.jclouds.http.options.HttpRequestOptions; import org.jclouds.http.utils.ModifyRequest; import org.jclouds.internal.ClassMethodArgs; @@ -145,12 +145,12 @@ import com.google.common.cache.CacheLoader; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMultimap; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.ImmutableSet.Builder; import com.google.common.collect.Iterables; import com.google.common.collect.LinkedHashMultimap; import com.google.common.collect.LinkedListMultimap; import com.google.common.collect.Lists; import com.google.common.collect.Multimap; +import com.google.common.collect.ImmutableSet.Builder; import com.google.common.util.concurrent.ListenableFuture; import com.google.inject.Inject; import com.google.inject.Injector; @@ -921,12 +921,12 @@ public class RestAnnotationProcessor { }; public GeneratedHttpRequest decorateRequest(GeneratedHttpRequest request) throws NegativeArraySizeException, - ExecutionException { + ExecutionException { OUTER: for (Entry> entry : concat(// - filterValues(methodToIndexOfParamToBinderParamAnnotation.get(request.getJavaMethod()).asMap(), notEmpty) - .entrySet(), // - filterValues(methodToIndexOfParamToWrapWithAnnotation.get(request.getJavaMethod()).asMap(), notEmpty) - .entrySet())) { + filterValues(methodToIndexOfParamToBinderParamAnnotation.get(request.getJavaMethod()).asMap(), notEmpty) + .entrySet(), // + filterValues(methodToIndexOfParamToWrapWithAnnotation.get(request.getJavaMethod()).asMap(), notEmpty) + .entrySet())) { boolean shouldBreak = false; Annotation annotation = Iterables.get(entry.getValue(), 0); Binder binder; @@ -934,7 +934,7 @@ public class RestAnnotationProcessor { binder = injector.getInstance(BinderParam.class.cast(annotation).value()); else binder = injector.getInstance(BindToJsonPayloadWrappedWith.Factory.class).create( - WrapWith.class.cast(annotation).value()); + WrapWith.class.cast(annotation).value()); if (request.getArgs().size() >= entry.getKey() + 1 && request.getArgs().get(entry.getKey()) != null) { Object input; Class parameterType = request.getJavaMethod().getParameterTypes()[entry.getKey()]; @@ -966,19 +966,21 @@ public class RestAnnotationProcessor { // (first, however, let's make sure we have enough args on the actual method) if (entry.getKey() >= request.getJavaMethod().getParameterAnnotations().length) { // not known whether this happens - throw new IllegalArgumentException("Argument index "+(entry.getKey()+1)+" is out of bounds for method "+request.getJavaMethod()); + throw new IllegalArgumentException("Argument index " + (entry.getKey() + 1) + + " is out of bounds for method " + request.getJavaMethod()); } - - if (request.getJavaMethod().isVarArgs() && entry.getKey() + 1 == request.getJavaMethod().getParameterTypes().length) - //allow null/missing for var args + + if (request.getJavaMethod().isVarArgs() + && entry.getKey() + 1 == request.getJavaMethod().getParameterTypes().length) + // allow null/missing for var args continue OUTER; - + Annotation[] annotations = request.getJavaMethod().getParameterAnnotations()[entry.getKey()]; - for (Annotation a: annotations) { + for (Annotation a : annotations) { if (Nullable.class.isAssignableFrom(a.annotationType())) continue OUTER; } - Preconditions.checkNotNull(null, request.getJavaMethod().getName()+" parameter "+(entry.getKey()+1)); + Preconditions.checkNotNull(null, request.getJavaMethod().getName() + " parameter " + (entry.getKey() + 1)); } } diff --git a/core/src/test/java/org/jclouds/rest/internal/RestAnnotationProcessorTest.java b/core/src/test/java/org/jclouds/rest/internal/RestAnnotationProcessorTest.java index ad225d1884..ced959e11b 100644 --- a/core/src/test/java/org/jclouds/rest/internal/RestAnnotationProcessorTest.java +++ b/core/src/test/java/org/jclouds/rest/internal/RestAnnotationProcessorTest.java @@ -602,7 +602,7 @@ public class RestAnnotationProcessorTest extends BaseRestClientTest { public void testCreatePostRequestNullOk2() throws SecurityException, NoSuchMethodException, IOException { Method method = TestPost.class.getMethod("post", String.class); - HttpRequest request = factory(TestPost.class).createRequest(method, (String)null); + HttpRequest request = factory(TestPost.class).createRequest(method, (String) null); assertRequestLineEquals(request, "POST http://localhost:9999 HTTP/1.1"); assertNonPayloadHeadersEqual(request, ""); @@ -613,19 +613,25 @@ public class RestAnnotationProcessorTest extends BaseRestClientTest { Method method = TestPost.class.getMethod("postNonnull", String.class); try { HttpRequest request = factory(TestPost.class).createRequest(method); - Assert.fail("call should have failed with illegal null parameter, not permitted "+request+" to be created"); + Assert + .fail("call should have failed with illegal null parameter, not permitted " + request + + " to be created"); } catch (NullPointerException e) { - Assert.assertTrue(e.toString().indexOf("postNonnull parameter 1")>=0, "Error message should have referred to 'parameter 1': "+e); + Assert.assertTrue(e.toString().indexOf("postNonnull parameter 1") >= 0, + "Error message should have referred to 'parameter 1': " + e); } } public void testCreatePostRequestNullNotOk2() throws SecurityException, NoSuchMethodException, IOException { Method method = TestPost.class.getMethod("postNonnull", String.class); try { - HttpRequest request = factory(TestPost.class).createRequest(method, (String)null); - Assert.fail("call should have failed with illegal null parameter, not permitted "+request+" to be created"); + HttpRequest request = factory(TestPost.class).createRequest(method, (String) null); + Assert + .fail("call should have failed with illegal null parameter, not permitted " + request + + " to be created"); } catch (NullPointerException e) { - Assert.assertTrue(e.toString().indexOf("postNonnull parameter 1")>=0, "Error message should have referred to parameter 'parameter 1': "+e); + Assert.assertTrue(e.toString().indexOf("postNonnull parameter 1") >= 0, + "Error message should have referred to parameter 'parameter 1': " + e); } }