Ensure that primitive values can be used as RequestFactory proxy properties. Make RequestFactoryInterfaceValidator stricter in handling mismatches. Issue 5357. Patch by: bobv Review by: rjrjr, rchandia Review at http://gwt-code-reviews.appspot.com/1216801 git-svn-id: https://google-web-toolkit.googlecode.com/svn/trunk@9414 8db76d5a-ed1c-0410-87a9-c151d255dfc7
diff --git a/user/src/com/google/gwt/requestfactory/server/RequestFactoryInterfaceValidator.java b/user/src/com/google/gwt/requestfactory/server/RequestFactoryInterfaceValidator.java index 6b64aea..fb10d5e 100644 --- a/user/src/com/google/gwt/requestfactory/server/RequestFactoryInterfaceValidator.java +++ b/user/src/com/google/gwt/requestfactory/server/RequestFactoryInterfaceValidator.java
@@ -148,7 +148,7 @@ this.parent = parent; this.validator = parent.validator; } - + public void poison(String msg, Object... args) { poison(); logger.logp(Level.SEVERE, currentType(), currentMethod(), @@ -173,7 +173,7 @@ toReturn.currentType = type; return toReturn; } - + public void spam(String msg, Object... args) { logger.logp(Level.FINEST, currentType(), currentMethod(), String.format(msg, args)); @@ -309,7 +309,7 @@ } else { return; } - + /* * The input is a source name, so we need to convert it to an * internal name. We'll do this by substituting dollar signs for the @@ -555,6 +555,18 @@ System.exit(validator.isPoisoned() ? 1 : 0); } + static String messageCouldNotFindMethod(Type domainType, + List<? extends Method> methods) { + StringBuilder sb = new StringBuilder(); + sb.append(String.format( + "Could not find matching method in %s.\nPossible matches:\n", + print(domainType))); + for (Method domainMethod : methods) { + sb.append(" ").append(print(domainMethod)).append("\n"); + } + return sb.toString(); + } + private static String print(Method method) { StringBuilder sb = new StringBuilder(); sb.append(print(method.getReturnType())).append(" ").append( @@ -632,6 +644,7 @@ * A map of a type to all types that it could be assigned to. */ private final Map<Type, List<Type>> supertypes = new HashMap<Type, List<Type>>(); + /** * The type {@link ValueProxy}. */ @@ -646,7 +659,7 @@ * Contains vaue types (e.g. Integer). */ private final Set<Type> valueTypes = new HashSet<Type>(); - + /** * Maps a domain object to the type returned from its getId method. */ @@ -657,13 +670,13 @@ valueTypes.add(Type.getType(clazz)); } } - + public RequestFactoryInterfaceValidator(Logger logger, Loader loader) { this.parentLogger = new ErrorContext(logger); parentLogger.setValidator(this); this.loader = loader; } - + /** * Visible for testing. */ @@ -790,7 +803,8 @@ for (RFMethod method : getMethodsInHierarchy(logger, requestContextType)) { // Ignore methods in RequestContext itself - if (findCompatibleMethod(logger, requestContextIntf, method, false, true) != null) { + if (findCompatibleMethod(logger, requestContextIntf, method, false, true, + true) != null) { continue; } @@ -961,7 +975,8 @@ Method searchFor = createDomainMethod(logger, new Method(method.getName(), returnType, method.getArgumentTypes())); - RFMethod found = findCompatibleMethod(logger, domainServiceType, searchFor); + RFMethod found = findCompatibleServiceMethod(logger, domainServiceType, + searchFor); if (found != null) { boolean isInstance = isAssignable(logger, instanceRequestIntf, @@ -1048,7 +1063,7 @@ Method clientPropertyMethod, Type domainType) { logger = logger.setMethod(clientPropertyMethod); - findCompatibleMethod(logger, domainType, + findCompatiblePropertyMethod(logger, domainType, createDomainMethod(logger, clientPropertyMethod)); } @@ -1108,20 +1123,14 @@ * hierarchy that is assignment-compatible with the given Method. */ private RFMethod findCompatibleMethod(final ErrorContext logger, - Type domainType, Method searchFor) { - return findCompatibleMethod(logger, domainType, searchFor, true, false); - } - - /** - * Finds a compatible method declaration in <code>domainType</code>'s - * hierarchy that is assignment-compatible with the given Method. - */ - private RFMethod findCompatibleMethod(final ErrorContext logger, Type domainType, Method searchFor, boolean mustFind, - boolean allowOverloads) { + boolean allowOverloads, boolean boxReturnTypes) { String methodName = searchFor.getName(); Type[] clientArgs = searchFor.getArgumentTypes(); Type clientReturnType = searchFor.getReturnType(); + if (boxReturnTypes) { + clientReturnType = maybeBoxType(clientReturnType); + } // Pull all methods out of the domain type Map<String, List<RFMethod>> domainLookup = new LinkedHashMap<String, List<RFMethod>>(); for (RFMethod method : getMethodsInHierarchy(logger, domainType)) { @@ -1155,12 +1164,16 @@ // Check each overloaded name for (RFMethod domainMethod : methods) { - // Box any primitive types to simplify compotibility check Type[] domainArgs = domainMethod.getArgumentTypes(); - for (int i = 0, j = domainArgs.length; i < j; i++) { - domainArgs[i] = maybeBoxType(domainArgs[i]); + Type domainReturnType = domainMethod.getReturnType(); + if (boxReturnTypes) { + /* + * When looking for the implementation of a Request<Integer>, we want to + * match either int or Integer, so we'll box the domain method's return + * type. + */ + domainReturnType = maybeBoxType(domainReturnType); } - Type domainReturnType = maybeBoxType(domainMethod.getReturnType()); /* * Make sure the client args can be passed into the domain args and the @@ -1175,19 +1188,32 @@ } } if (mustFind) { - StringBuilder sb = new StringBuilder(); - sb.append(String.format( - "Could not find matching method in %s:\nPossible matches:\n", - print(domainType))); - for (RFMethod domainMethod : methods) { - sb.append(" ").append(print(domainMethod)).append("\n"); - } - logger.poison(sb.toString()); + logger.poison(messageCouldNotFindMethod(domainType, methods)); } return null; } /** + * Finds a compatible method declaration in <code>domainType</code>'s + * hierarchy that is assignment-compatible with the given Method. + */ + private RFMethod findCompatiblePropertyMethod(final ErrorContext logger, + Type domainType, Method searchFor) { + return findCompatibleMethod(logger, domainType, searchFor, true, false, + false); + } + + /** + * Finds a compatible method declaration in <code>domainType</code>'s + * hierarchy that is assignment-compatible with the given Method. + */ + private RFMethod findCompatibleServiceMethod(final ErrorContext logger, + Type domainType, Method searchFor) { + return findCompatibleMethod(logger, domainType, searchFor, true, false, + true); + } + + /** * This looks like it should be a utility method somewhere else, but I can't * find it. */ @@ -1341,14 +1367,6 @@ return true; } - // Box primitive types to make this simple - if (possibleSupertype.getSort() != Type.OBJECT) { - possibleSupertype = getBoxedType(possibleSupertype); - } - if (possibleSubtype.getSort() != Type.OBJECT) { - possibleSubtype = getBoxedType(possibleSubtype); - } - // Supertype calculation is cached List<Type> allSupertypes = getSupertypes(logger, possibleSubtype); return allSupertypes.contains(possibleSupertype); @@ -1368,7 +1386,8 @@ return true; } - private boolean isCollectionType(@SuppressWarnings("unused") ErrorContext logger, Type type) { + private boolean isCollectionType( + @SuppressWarnings("unused") ErrorContext logger, Type type) { // keeping the logger arg just for internal consistency for our small minds return "java/util/List".equals(type.getInternalName()) || "java/util/Set".equals(type.getInternalName());
diff --git a/user/src/com/google/gwt/requestfactory/server/SimpleRequestProcessor.java b/user/src/com/google/gwt/requestfactory/server/SimpleRequestProcessor.java index 2835056..2fde11d 100644 --- a/user/src/com/google/gwt/requestfactory/server/SimpleRequestProcessor.java +++ b/user/src/com/google/gwt/requestfactory/server/SimpleRequestProcessor.java
@@ -186,7 +186,7 @@ processOperationMessages(state, message); List<Object> decoded = decodeInvocationArguments(state, message.getInvocations().get(0).getParameters(), - new Class<?>[]{proxyType}, new Type[]{domainClass}); + new Class<?>[] {proxyType}, new Type[] {domainClass}); @SuppressWarnings("unchecked") List<T> toReturn = (List<T>) decoded; @@ -234,7 +234,8 @@ private AutoBean<ServerFailureMessage> createFailureMessage( ReportableException e) { - ServerFailure failure = exceptionHandler.createServerFailure(e.getCause()); + ServerFailure failure = exceptionHandler.createServerFailure(e.getCause() == null + ? e : e.getCause()); AutoBean<ServerFailureMessage> bean = FACTORY.failure(); ServerFailureMessage msg = bean.as(); msg.setExceptionType(failure.getExceptionType());
diff --git a/user/test/com/google/gwt/requestfactory/RequestFactoryJreSuite.java b/user/test/com/google/gwt/requestfactory/RequestFactoryJreSuite.java index 61d7071..85d7956 100644 --- a/user/test/com/google/gwt/requestfactory/RequestFactoryJreSuite.java +++ b/user/test/com/google/gwt/requestfactory/RequestFactoryJreSuite.java
@@ -16,6 +16,7 @@ package com.google.gwt.requestfactory; import com.google.gwt.requestfactory.rebind.model.RequestFactoryModelTest; +import com.google.gwt.requestfactory.server.BoxesAndPrimitivesJreTest; import com.google.gwt.requestfactory.server.ComplexKeysJreTest; import com.google.gwt.requestfactory.server.FindServiceJreTest; import com.google.gwt.requestfactory.server.LocatorJreTest; @@ -34,6 +35,7 @@ public static Test suite() { TestSuite suite = new TestSuite( "requestfactory package tests that require the JRE"); + suite.addTestSuite(BoxesAndPrimitivesJreTest.class); suite.addTestSuite(ComplexKeysJreTest.class); suite.addTestSuite(FindServiceJreTest.class); suite.addTestSuite(LocatorJreTest.class);
diff --git a/user/test/com/google/gwt/requestfactory/RequestFactorySuite.java b/user/test/com/google/gwt/requestfactory/RequestFactorySuite.java index 884d005..72cad85 100644 --- a/user/test/com/google/gwt/requestfactory/RequestFactorySuite.java +++ b/user/test/com/google/gwt/requestfactory/RequestFactorySuite.java
@@ -22,6 +22,7 @@ import com.google.gwt.requestfactory.client.RequestFactoryTest; import com.google.gwt.requestfactory.client.RequestFactoryUnicodeEscapingTest; import com.google.gwt.requestfactory.client.ui.EditorTest; +import com.google.gwt.requestfactory.shared.BoxesAndPrimitivesTest; import com.google.gwt.requestfactory.shared.ComplexKeysTest; import com.google.gwt.requestfactory.shared.LocatorTest; @@ -34,6 +35,7 @@ public static Test suite() { GWTTestSuite suite = new GWTTestSuite( "Test suite for requestfactory gwt code."); + suite.addTestSuite(BoxesAndPrimitivesTest.class); suite.addTestSuite(ComplexKeysTest.class); suite.addTestSuite(EditorTest.class); suite.addTestSuite(FindServiceTest.class);
diff --git a/user/test/com/google/gwt/requestfactory/server/BoxesAndPrimitivesJreTest.java b/user/test/com/google/gwt/requestfactory/server/BoxesAndPrimitivesJreTest.java new file mode 100644 index 0000000..c8b178f --- /dev/null +++ b/user/test/com/google/gwt/requestfactory/server/BoxesAndPrimitivesJreTest.java
@@ -0,0 +1,123 @@ +/* + * Copyright 2010 Google 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 com.google.gwt.requestfactory.server; + +import com.google.gwt.dev.asm.Type; +import com.google.gwt.dev.asm.commons.Method; +import com.google.gwt.requestfactory.server.RequestFactoryInterfaceValidatorTest.VisibleErrorContext; +import com.google.gwt.requestfactory.shared.BoxesAndPrimitivesTest; +import com.google.gwt.requestfactory.shared.EntityProxy; +import com.google.gwt.requestfactory.shared.ProxyFor; +import com.google.gwt.requestfactory.shared.Request; +import com.google.gwt.requestfactory.shared.RequestContext; +import com.google.gwt.requestfactory.shared.Service; + +import java.util.Arrays; +import java.util.logging.Logger; + +/** + * A JRE version of {@link BoxesAndPrimitivesTest} with additional validation + * tests. + */ +public class BoxesAndPrimitivesJreTest extends BoxesAndPrimitivesTest { + + @Service(ServiceImpl.class) + interface ContextMismatchedParameterA extends RequestContext { + Request<Void> checkBoxed(int value); + } + + @Service(ServiceImpl.class) + interface ContextMismatchedParameterB extends RequestContext { + Request<Void> checkPrimitive(Integer value); + } + + @ProxyFor(Entity.class) + interface ProxyMismatchedGetterA extends EntityProxy { + int getBoxed(); + } + + @ProxyFor(Entity.class) + interface ProxyMismatchedGetterB extends EntityProxy { + Integer getPrimitive(); + } + + private VisibleErrorContext errors; + private RequestFactoryInterfaceValidator v; + + @Override + public String getModuleName() { + return null; + } + + public void test() { + RequestFactoryInterfaceValidator v = new RequestFactoryInterfaceValidator( + Logger.getAnonymousLogger(), + new RequestFactoryInterfaceValidator.ClassLoaderLoader( + getClass().getClassLoader())); + v.validateRequestFactory(Factory.class.getName()); + assertFalse(v.isPoisoned()); + } + + /** + * Tests that mismatched primitive verses boxed getters are correctly + * reported. + */ + public void testMismatchedGetters() { + v.validateEntityProxy(ProxyMismatchedGetterA.class.getName()); + v.validateEntityProxy(ProxyMismatchedGetterB.class.getName()); + assertTrue(v.isPoisoned()); + + String getBoxedMessage = RequestFactoryInterfaceValidator.messageCouldNotFindMethod( + Type.getType(Entity.class), + Arrays.asList(new Method("getBoxed", "()Ljava/lang/Integer;"))); + String getPrimitiveMessage = RequestFactoryInterfaceValidator.messageCouldNotFindMethod( + Type.getType(Entity.class), + Arrays.asList(new Method("getPrimitive", "()I"))); + assertEquals(Arrays.asList(getBoxedMessage, getPrimitiveMessage), + errors.logs); + } + + /** + * Tests that mismatched parameter types are correctly reported. + */ + public void testMismatchedParameters() { + v.validateRequestContext(ContextMismatchedParameterA.class.getName()); + v.validateRequestContext(ContextMismatchedParameterB.class.getName()); + + String checkBoxedMessage = RequestFactoryInterfaceValidator.messageCouldNotFindMethod( + Type.getType(ServiceImpl.class), + Arrays.asList(new Method("checkBoxed", "(Ljava/lang/Integer;)V"))); + String checkPrimitiveMessage = RequestFactoryInterfaceValidator.messageCouldNotFindMethod( + Type.getType(ServiceImpl.class), + Arrays.asList(new Method("checkPrimitive", "(I)V"))); + assertEquals(Arrays.asList(checkBoxedMessage, checkPrimitiveMessage), + errors.logs); + } + + @Override + protected Factory createFactory() { + return RequestFactoryJreTest.createInProcess(Factory.class); + } + + @Override + protected void gwtSetUp() throws Exception { + super.gwtSetUp(); + errors = new VisibleErrorContext(Logger.getAnonymousLogger()); + v = new RequestFactoryInterfaceValidator(errors, + new RequestFactoryInterfaceValidator.ClassLoaderLoader( + getClass().getClassLoader())); + } +}
diff --git a/user/test/com/google/gwt/requestfactory/server/SimpleFoo.java b/user/test/com/google/gwt/requestfactory/server/SimpleFoo.java index afcd18e..be2632b 100644 --- a/user/test/com/google/gwt/requestfactory/server/SimpleFoo.java +++ b/user/test/com/google/gwt/requestfactory/server/SimpleFoo.java
@@ -783,7 +783,7 @@ this.simpleValueField = simpleValueField.get(0); } - public void setUnpersisted(Boolean unpersisted) { + public void setUnpersisted(boolean unpersisted) { this.unpersisted = unpersisted; }
diff --git a/user/test/com/google/gwt/requestfactory/shared/BaseFooProxy.java b/user/test/com/google/gwt/requestfactory/shared/BaseFooProxy.java index 2e98883..869368e 100644 --- a/user/test/com/google/gwt/requestfactory/shared/BaseFooProxy.java +++ b/user/test/com/google/gwt/requestfactory/shared/BaseFooProxy.java
@@ -75,7 +75,7 @@ List<SimpleValueProxy> getSimpleValues(); - Boolean getUnpersisted(); + boolean getUnpersisted(); String getUserName(); @@ -127,7 +127,7 @@ void setSimpleValues(List<SimpleValueProxy> value); - void setUnpersisted(Boolean unpersisted); + void setUnpersisted(boolean unpersisted); void setUserName(String userName); }
diff --git a/user/test/com/google/gwt/requestfactory/shared/BoxesAndPrimitivesTest.java b/user/test/com/google/gwt/requestfactory/shared/BoxesAndPrimitivesTest.java new file mode 100644 index 0000000..ddcea1d --- /dev/null +++ b/user/test/com/google/gwt/requestfactory/shared/BoxesAndPrimitivesTest.java
@@ -0,0 +1,204 @@ +/* + * Copyright 2010 Google 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 com.google.gwt.requestfactory.shared; + +import com.google.gwt.core.client.GWT; +import com.google.gwt.event.shared.SimpleEventBus; +import com.google.gwt.junit.client.GWTTestCase; + +/** + * Contains a set of checks of how primitive and boxed method declarations + * interact. + */ +public class BoxesAndPrimitivesTest extends GWTTestCase { + + /** + * The domain type. + */ + protected static class Entity { + static final Entity SINGLETON = new Entity(); + + public static Entity findEntity(int id) { + return SINGLETON; + } + + public Integer getBoxed() { + return EXPECTED_BOXED; + } + + public int getId() { + return 0; + } + + public int getPrimitive() { + return EXPECTED; + } + + public int getVersion() { + return 0; + } + + public void setBoxed(Integer value) { + assertEquals(EXPECTED_BOXED, value); + } + + public void setPrimitive(int value) { + assertEquals(EXPECTED, value); + } + } + + /** + * The RequestFactory. + */ + protected interface Factory extends RequestFactory { + Context context(); + } + + /** + * The service method implementations. + */ + protected static class ServiceImpl { + public static void checkBoxed(Integer value) { + assertEquals(EXPECTED_BOXED, value); + } + + public static void checkPrimitive(int value) { + assertEquals(EXPECTED, value); + } + + public static Integer getBoxed() { + return EXPECTED_BOXED; + } + + public static Entity getEntity() { + return Entity.SINGLETON; + } + + public static int getPrimitive() { + return EXPECTED; + } + } + + @Service(ServiceImpl.class) + interface Context extends RequestContext { + Request<Void> checkBoxed(Integer value); + + Request<Void> checkPrimitive(int value); + + Request<Integer> getBoxed(); + + Request<Proxy> getEntity(); + + Request<Integer> getPrimitive(); + } + + @ProxyFor(Entity.class) + interface Proxy extends EntityProxy { + Integer getBoxed(); + + int getPrimitive(); + + void setBoxed(Integer value); + + void setPrimitive(int value); + } + + static abstract class TestReceiver<T> extends Receiver<T> { + @Override + public void onFailure(ServerFailure error) { + fail(error.getMessage()); + } + } + + private static final int EXPECTED = 42; + private static final Integer EXPECTED_BOXED = Integer.valueOf(EXPECTED); + private static final int TEST_DELAY = 5000; + + private Factory factory; + + @Override + public String getModuleName() { + return "com.google.gwt.requestfactory.RequestFactorySuite"; + } + + /** + * Tests that domain service methods that return a primitive type are upcast + * to the boxed type that the generic declaration requires. Also checks that + * primitive and boxed property types can be retrieved and that boxed and + * primitive method arguments work. + */ + public void testReturnAndParamTypes() { + delayTestFinish(TEST_DELAY); + Context ctx = context(); + // Boxed service method + ctx.getBoxed().to(new TestReceiver<Integer>() { + @Override + public void onSuccess(Integer response) { + assertEquals(EXPECTED_BOXED, response); + } + }); + // Primitive service method + ctx.getPrimitive().to(new TestReceiver<Integer>() { + @Override + public void onSuccess(Integer response) { + assertEquals(EXPECTED_BOXED, response); + } + }); + // Boxed and primitive properties + ctx.getEntity().to(new TestReceiver<Proxy>() { + @Override + public void onSuccess(Proxy response) { + assertEquals(EXPECTED_BOXED, response.getBoxed()); + assertEquals(EXPECTED, response.getPrimitive()); + } + }); + // Boxed service argument + ctx.checkBoxed(EXPECTED_BOXED).to(new TestReceiver<Void>() { + @Override + public void onSuccess(Void response) { + // OK + } + }); + // Primitive service argument + ctx.checkPrimitive(EXPECTED).to(new TestReceiver<Void>() { + @Override + public void onSuccess(Void response) { + // OK + } + }); + ctx.fire(new TestReceiver<Void>() { + @Override + public void onSuccess(Void response) { + finishTest(); + } + }); + } + + protected Factory createFactory() { + Factory toReturn = GWT.create(Factory.class); + toReturn.initialize(new SimpleEventBus()); + return toReturn; + } + + @Override + protected void gwtSetUp() throws Exception { + factory = createFactory(); + } + + private Context context() { + return factory.context(); + } +}