From f26887b084761e79b8b3a6a05a72abaed74d7ca0 Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Fri, 30 Sep 2011 11:05:15 +0100 Subject: [PATCH] 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");