Permalink
Browse files

GRAILS-11585 - improve allowedMethods for command object actions

Functional tests at grails/grails-functional-tests@9f353a0

There are 2 parts to this.

When a command object is a domain object and an error occurs retrieving the instance from the data store, instead of throwing that exception the framework should catch the exception, add an error to the controller and invoke the action with a null value for the command object.

When a controller action accepts a command object argument the compiler creates a no-arg version of the method which creates an instance of the command object, initializes it and passes it as an argument to the method which accepts the command object.  The allowedMethods handling code was being added to the method which accepts the argument.  This means that the allowedMethods check is happening after the command object is initialized.  This commit changes that so the allowedMethods check is added to the no-arg method and is done before the command object is initialialized.  The allowedMethods checking code is still in the method which accepts the argument as well, which allows unit tests which explicitly pass a command object argument to still take advantage of allowedMethods checks.  The code which imposes the allowedMethods check sets a request attribute indicating that the work has been done so when the no-arg method invokes the method which accepts the argument the check is actually only done once.

With this change, the following unit tests will all pass.

A controller which accepts a domain class command object:

    // grails-app/controllers/grails11585/RegisterController.groovy
    package grails11585

    class RegisterController {

        static allowedMethods = [register: 'POST']

        // Person is a domain class
        def register(Person p) {
            if(p) {
                render "Name: ${p.name}"
            } else {
                render 'p is null'
            }
        }
    }

A unit test:

    // test/unit/grails11585/RegisterControllerSpec.groovy
    package grails11585

    import grails.test.mixin.TestFor
    import spock.lang.Specification
    import grails.test.mixin.hibernate.HibernateTestMixin
    import grails.test.mixin.gorm.Domain

    @TestFor(RegisterController)
    @Domain(Person)
    @TestMixin(HibernateTestMixin)
    class RegisterControllerSpec extends Specification {

        void "test register with valid request method"() {
            when: 'the id is of the wrong type and the request method is valid'
            params.id = 'bogus'
            params.name = 'Zack'
            request.method = 'POST'
            controller.register()

            then: 'the response code is 200'
            response.status == 200

            and: 'the command object is null'
            response.contentAsString == 'p is null'

            and: 'an error is added to the controller'
            controller.hasErrors()
            controller.errors.errorCount == 1
            controller.errors.allErrors[0].code == 'grails11585.RegisterController.commandObject.p.error'
            controller.errors.allErrors[0].defaultMessage == 'Provided id of the wrong type for class grails11585.Person. Expected: class java.lang.Long, got class java.lang.String; nested exception is org.hibernate.TypeMismatchException: Provided id of the wrong type for class grails11585.Person. Expected: class java.lang.Long, got class java.lang.String'
        }

        void "test register with invalid request method"() {
            when: 'the id is of the wrong type and the request method is invalid'
            params.id = 'bogus'
            params.name = 'Zack'
            request.method = 'GET'
            controller.register()

            then: 'the response code is 405'
            response.status == 405
        }

        void "test register with explicit command object argument and a valid request method"() {
            when: 'request method is valid'
            request.method = 'POST'
            controller.register(new Person(name: 'Zack'))

            then: 'the response code is 200'
            response.status == 200

            and: 'the response content is correct'
            response.contentAsString == 'Name: Zack'
        }

        void "test register with explicit command object argument and invalid request method"() {
            when: 'request method is invalid'
            request.method = 'GET'
            controller.register(new Person(name: 'Zack'))

            then: 'the response code is 405'
            response.status == 405
        }
    }

Note that this unit test is using @HibernateTestMixin because the default in memory GORM implementation does not throw an exception for type mismatch of the id.
  • Loading branch information...
1 parent 7936805 commit 5dd77fdb393d344718ffabf562edb096b2db0d0d @jeffbrown jeffbrown committed Jul 16, 2014
@@ -359,13 +359,22 @@ private MethodNode convertToMethodAction(ClassNode classNode, MethodNode methodN
MethodNode method = null;
if (methodNode.getParameters().length > 0) {
+ final BlockStatement methodCode = new BlockStatement();
+
+ final BlockStatement codeToHandleAllowedMethods = getCodeToHandleAllowedMethods(classNode, methodNode.getName());
+ final Statement codeToCallOriginalMethod = addOriginalMethodCall(methodNode, initializeActionParameters(
+ classNode, methodNode, methodNode.getName(), parameters, source, context));
+
+ methodCode.addStatement(codeToHandleAllowedMethods);
+ methodCode.addStatement(codeToCallOriginalMethod);
+
method = new MethodNode(
methodNode.getName(),
Modifier.PUBLIC, returnType,
ZERO_PARAMETERS,
EMPTY_CLASS_ARRAY,
- addOriginalMethodCall(methodNode, initializeActionParameters(
- classNode, methodNode, methodNode.getName(), parameters, source, context)));
+ methodCode);
+
GrailsASTUtils.copyAnnotations(methodNode, method);
annotateActionMethod(classNode, parameters, method);
} else {
@@ -469,7 +478,7 @@ protected void annotateActionMethod(ClassNode controllerClassNode, final Paramet
}
}
- protected BlockStatement getCodeToHandleAllowedMethods(ClassNode controllerClass, MethodNode methodNode) {
+ protected BlockStatement getCodeToHandleAllowedMethods(ClassNode controllerClass, String methodName) {
GrailsASTUtils.addEnhancedAnnotation(controllerClass, DefaultGrailsControllerClass.ALLOWED_HTTP_METHODS_PROPERTY);
final BlockStatement checkAllowedMethodsBlock = new BlockStatement();
@@ -483,7 +492,6 @@ protected BlockStatement getCodeToHandleAllowedMethods(ClassNode controllerClass
final List<String> allowedMethodNames = new ArrayList<String>();
final MapExpression allowedMethodsMapExpression = (MapExpression) initialAllowedMethodsExpression;
final List<MapEntryExpression> allowedMethodsMapEntryExpressions = allowedMethodsMapExpression.getMapEntryExpressions();
- final String methodName = methodNode.getName();
for(MapEntryExpression allowedMethodsMapEntryExpression : allowedMethodsMapEntryExpressions) {
final Expression allowedMethodsMapEntryKeyExpression = allowedMethodsMapEntryExpression.getKeyExpression();
if(allowedMethodsMapEntryKeyExpression instanceof ConstantExpression) {
@@ -536,7 +544,7 @@ protected BlockStatement getCodeToHandleAllowedMethods(ClassNode controllerClass
final ArgumentListExpression argumentListExpression = new ArgumentListExpression();
argumentListExpression.addExpression(new ConstantExpression(ALLOWED_METHODS_HANDLED_ATTRIBUTE_NAME));
- argumentListExpression.addExpression(new ConstantExpression(methodNode.getName()));
+ argumentListExpression.addExpression(new ConstantExpression(methodName));
final Expression setAttributeMethodCall = new MethodCallExpression(requestPropertyExpression, "setAttribute", argumentListExpression);
@@ -599,7 +607,7 @@ protected void wrapMethodBodyWithExceptionHandling(final ClassNode controllerCla
final Statement methodBody = methodNode.getCode();
BlockStatement tryBlock = new BlockStatement();
- BlockStatement codeToHandleAllowedMethods = getCodeToHandleAllowedMethods(controllerClassNode, methodNode);
+ BlockStatement codeToHandleAllowedMethods = getCodeToHandleAllowedMethods(controllerClassNode, methodNode.getName());
tryBlock.addStatement(codeToHandleAllowedMethods);
tryBlock.addStatement(methodBody);
@@ -809,7 +817,7 @@ protected void initializeCommandObjectParameter(final BlockStatement wrapper,
final TryCatchStatement tryCatchStatement = new TryCatchStatement(tryBlock, new EmptyStatement());
tryCatchStatement.addCatch(new CatchStatement(new Parameter(new ClassNode(DataBindingSourceCreationException.class), "$dataBindingSourceInitializationException"), catchBlock));
-
+
wrapper.addStatement(tryCatchStatement);
}
@@ -442,7 +442,14 @@ public Object initializeCommandObject(final Object controllerInstance, final Cla
final HttpMethod requestMethod = HttpMethod.valueOf(request.getMethod());
if(entityIdentifierValue != null) {
- commandObjectInstance = InvokerHelper.invokeStaticMethod(type, "get", entityIdentifierValue);
+ try {
+ commandObjectInstance = InvokerHelper.invokeStaticMethod(type, "get", entityIdentifierValue);
+ } catch (Exception e) {
+ final Errors errors = getErrors(controllerInstance);
+ if(errors != null) {
+ errors.reject(controllerInstance.getClass().getName() + ".commandObject." + commandObjectParameterName + ".error", e.getMessage());
+ }
+ }
} else if(requestMethod == HttpMethod.POST || !isDomainClass){
commandObjectInstance = type.newInstance();
}

0 comments on commit 5dd77fd

Please sign in to comment.