Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

GRAILS-1984 - improve id binding"

Domain class identifiers should be assignable via data binding if the identifier is marked with "bindable: true".

A relevant functional test is at grails/grails-functional-tests-v2@47ce4b5
  • Loading branch information...
commit 4684e460c07fd21b30fd3b61c28d4145baae92d9 1 parent f07c9ce
@jeffbrown jeffbrown authored
View
23 ...core/src/main/groovy/org/codehaus/groovy/grails/compiler/injection/DefaultGrailsDomainClassInjector.java
@@ -17,7 +17,6 @@
import grails.build.logging.GrailsConsole;
-import java.io.File;
import java.lang.reflect.Modifier;
import java.net.URL;
import java.util.ArrayList;
@@ -56,14 +55,10 @@
@AstTransformer
public class DefaultGrailsDomainClassInjector implements GrailsDomainClassInjector {
- private static final String DOMAIN_DIR = "domain";
-
- private static final String GRAILS_APP_DIR = "grails-app";
-
private List<ClassNode> classesWithInjectedToString = new ArrayList<ClassNode>();
public void performInjection(SourceUnit source, GeneratorContext context, ClassNode classNode) {
- if (isDomainClass(classNode, source) && shouldInjectClass(classNode)) {
+ if (GrailsASTUtils.isDomainClass(classNode, source) && shouldInjectClass(classNode)) {
performInjectionOnAnnotatedEntity(classNode);
}
}
@@ -79,22 +74,6 @@ public boolean shouldInject(URL url) {
return GrailsResourceUtils.isDomainClass(url);
}
- protected boolean isDomainClass(@SuppressWarnings("unused") ClassNode classNode, SourceUnit sourceNode) {
- String sourcePath = sourceNode.getName();
- File sourceFile = new File(sourcePath);
- File parent = sourceFile.getParentFile();
- while (parent != null) {
- File parentParent = parent.getParentFile();
- if (parent.getName().equals(DOMAIN_DIR) && parentParent != null &&
- parentParent.getName().equals(GRAILS_APP_DIR)) {
- return true;
- }
- parent = parentParent;
- }
-
- return false;
- }
-
protected boolean shouldInjectClass(ClassNode classNode) {
String fullName = GrailsASTUtils.getFullName(classNode);
String mappingFile = getMappingFileName(fullName);
View
51 grails-core/src/main/groovy/org/codehaus/groovy/grails/compiler/injection/GrailsASTUtils.java
@@ -20,13 +20,19 @@
import grails.util.GrailsNameUtils;
import groovy.lang.MissingMethodException;
+import java.io.File;
import java.lang.annotation.Annotation;
import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;
import java.lang.reflect.Modifier;
-import java.util.*;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
import org.apache.commons.collections.CollectionUtils;
import org.apache.commons.collections.Predicate;
@@ -39,7 +45,21 @@
import org.codehaus.groovy.ast.MethodNode;
import org.codehaus.groovy.ast.Parameter;
import org.codehaus.groovy.ast.PropertyNode;
-import org.codehaus.groovy.ast.expr.*;
+import org.codehaus.groovy.ast.expr.ArgumentListExpression;
+import org.codehaus.groovy.ast.expr.BinaryExpression;
+import org.codehaus.groovy.ast.expr.BooleanExpression;
+import org.codehaus.groovy.ast.expr.ClassExpression;
+import org.codehaus.groovy.ast.expr.ClosureExpression;
+import org.codehaus.groovy.ast.expr.ConstantExpression;
+import org.codehaus.groovy.ast.expr.ConstructorCallExpression;
+import org.codehaus.groovy.ast.expr.DeclarationExpression;
+import org.codehaus.groovy.ast.expr.Expression;
+import org.codehaus.groovy.ast.expr.MapEntryExpression;
+import org.codehaus.groovy.ast.expr.MapExpression;
+import org.codehaus.groovy.ast.expr.MethodCallExpression;
+import org.codehaus.groovy.ast.expr.NamedArgumentListExpression;
+import org.codehaus.groovy.ast.expr.TupleExpression;
+import org.codehaus.groovy.ast.expr.VariableExpression;
import org.codehaus.groovy.ast.stmt.BlockStatement;
import org.codehaus.groovy.ast.stmt.CatchStatement;
import org.codehaus.groovy.ast.stmt.ExpressionStatement;
@@ -65,6 +85,8 @@
*/
public class GrailsASTUtils {
+ public static final String DOMAIN_DIR = "domain";
+ public static final String GRAILS_APP_DIR = "grails-app";
public static final String METHOD_MISSING_METHOD_NAME = "methodMissing";
public static final String STATIC_METHOD_MISSING_METHOD_NAME = "$static_methodMissing";
public static final Token EQUALS_OPERATOR = Token.newSymbol("==", 0, 0);
@@ -570,6 +592,31 @@ public static boolean isConstructorMethod(MethodNode declaredMethod) {
declaredMethod.getParameters().length >= 1 &&
declaredMethod.getParameters()[0].getType().equals(AbstractGrailsArtefactTransformer.OBJECT_CLASS);
}
+
+ public static boolean isDomainClass(@SuppressWarnings("unused") final ClassNode classNode, final SourceUnit sourceNode) {
+ @SuppressWarnings("unchecked")
+ boolean isDomainClass = GrailsASTUtils.hasAnyAnnotations(classNode,
+ grails.persistence.Entity.class,
+ javax.persistence.Entity.class);
+
+ if(!isDomainClass) {
+ final String sourcePath = sourceNode.getName();
+ final File sourceFile = new File(sourcePath);
+ File parent = sourceFile.getParentFile();
+ while (parent != null && !isDomainClass) {
+ final File parentParent = parent.getParentFile();
+ if (parent.getName().equals(DOMAIN_DIR) &&
+ parentParent != null &&
+ parentParent.getName().equals(GRAILS_APP_DIR)) {
+ isDomainClass = true;
+ }
+ parent = parentParent;
+ }
+ }
+
+ return isDomainClass;
+ }
+
public static void addDelegateInstanceMethods(ClassNode classNode, ClassNode delegateNode, Expression delegateInstance) {
addDelegateInstanceMethods(classNode, classNode,delegateNode, delegateInstance);
View
76 ...test-suite-persistence/src/test/groovy/org/codehaus/groovy/grails/web/binding/DomainIdBindingSpec.groovy
@@ -0,0 +1,76 @@
+package org.codehaus.groovy.grails.web.binding
+
+import grails.persistence.Entity
+
+import org.codehaus.groovy.grails.commons.GrailsApplication
+import org.codehaus.groovy.grails.orm.hibernate.GormSpec
+import org.codehaus.groovy.grails.plugins.testing.GrailsMockHttpServletRequest
+import org.codehaus.groovy.grails.plugins.testing.GrailsMockHttpServletResponse
+import org.codehaus.groovy.grails.web.servlet.GrailsApplicationAttributes
+import org.codehaus.groovy.grails.web.servlet.mvc.GrailsParameterMap
+import org.codehaus.groovy.grails.web.servlet.mvc.GrailsWebRequest
+import org.springframework.mock.web.MockServletContext
+
+import spock.lang.Shared
+
+class DomainIdBindingSpec extends GormSpec{
+
+ @Shared
+ private mockRequest
+
+ def setupSpec() {
+ def servletContext = new MockServletContext()
+ def grailsApplication = [isArtefactOfType: {String type, Class clzz -> true },
+ getArtefact: {String type, String name -> }] as GrailsApplication
+
+ def applicationAttributes = [getApplicationContext: { -> }, getServletContext: { -> servletContext },
+ getGrailsApplication: { -> grailsApplication}] as GrailsApplicationAttributes
+ mockRequest = new GrailsMockHttpServletRequest()
+ def grailsWebRequest = new GrailsWebRequest(mockRequest, new GrailsMockHttpServletResponse(), servletContext)
+ grailsWebRequest.@attributes = applicationAttributes
+ mockRequest.setAttribute(GrailsApplicationAttributes.WEB_REQUEST, grailsWebRequest)
+ }
+
+ void 'Test non bindable id'() {
+ given:
+ def map = new GrailsParameterMap(name: 'Joe', id: 42, mockRequest)
+
+ when:
+ def widget = new WidgetWithNonBindableId(map)
+
+ then:
+ 'Joe' == widget.name
+ null == widget.id
+ }
+
+ void 'Test bindable id'() {
+ given:
+ def map = new GrailsParameterMap(name: 'Joe', id: 42, mockRequest)
+
+ when:
+ def widget = new WidgetWithBindableId(map)
+
+ then:
+ 'Joe' == widget.name
+ 42 == widget.id
+ }
+
+ @Override
+ public List getDomainClasses() {
+ [WidgetWithBindableId, WidgetWithNonBindableId]
+ }
+
+}
+
+@Entity
+class WidgetWithBindableId {
+ String name
+ static constraints = {
+ id bindable: true
+ }
+}
+
+@Entity
+class WidgetWithNonBindableId {
+ String name
+}
View
38 ...suite-web/src/test/groovy/org/codehaus/groovy/grails/web/converters/AutoParamsXmlMarshallingTests.groovy
@@ -77,6 +77,30 @@ import grails.persistence.*
// "id" should not bind because we are binding to a domain class.
assertNull model.book.id
}
+ void testXmlMarshallingIntoParamsObjectWithBindableId() {
+ def controller = ga.getControllerClass(TestConverterController.name).newInstance()
+
+ controller.request.contentType = "text/xml"
+ controller.request.content = '''<?xml version="1.0" encoding="ISO-8859-1"?>
+<book id="1">
+ <author id="1">
+ <name>Stephen King</name>
+ </author>
+ <releaseDate>2007-11-27 11:52:53.447</releaseDate>
+ <title>The Stand</title>
+</book>
+'''.bytes
+
+ webRequest.informParameterCreationListeners()
+ def model = controller.createWithBindableId()
+
+ assert model
+ assert model.book
+ assertEquals "The Stand", model.book.title
+ assertEquals 1, model.book.author.id
+ assertEquals 'Stephen King', model.book.author.name
+ assertEquals 1, model.book.id
+ }
}
@@ -84,6 +108,9 @@ class TestConverterController {
def create = {
[book:new AutoParamsXmlMarshallingBook(params['book'])]
}
+ def createWithBindableId = {
+ [book:new AutoParamsXmlMarshallingBookWithBindableId(params['book'])]
+ }
}
@Entity
@@ -95,6 +122,17 @@ class AutoParamsXmlMarshallingBook {
}
@Entity
+class AutoParamsXmlMarshallingBookWithBindableId {
+ String title
+ Date releaseDate
+
+ static belongsTo = [author:AutoParamsXmlMarshallingAuthor]
+ static constraints = {
+ id bindable: true
+ }
+}
+
+@Entity
class AutoParamsXmlMarshallingAuthor {
String name
View
14 grails-web/src/main/groovy/org/codehaus/groovy/grails/web/binding/DataBindingUtils.java
@@ -294,7 +294,7 @@ private static Object unwrapGString(Object value) {
private static void includeExcludeFields(GrailsDataBinder dataBinder, List allowed, List disallowed) {
updateAllowed(dataBinder, allowed);
- updateDisallowed(dataBinder, disallowed);
+ updateDisallowed(dataBinder, allowed, disallowed);
}
@SuppressWarnings("unchecked")
@@ -312,14 +312,22 @@ private static void updateAllowed(GrailsDataBinder binder, List allowed) {
}
@SuppressWarnings("unchecked")
- private static void updateDisallowed(GrailsDataBinder binder, List disallowed) {
+ private static void updateDisallowed(GrailsDataBinder binder, List allowed, List disallowed) {
if (disallowed == null) {
return;
}
String[] currentDisallowed = binder.getDisallowedFields();
List newDisallowed = new ArrayList(disallowed);
- CollectionUtils.addAll(newDisallowed, currentDisallowed);
+ if(allowed == null) {
+ CollectionUtils.addAll(newDisallowed, currentDisallowed);
+ } else {
+ for(String s : currentDisallowed) {
+ if(!allowed.contains(s)) {
+ newDisallowed.add(s);
+ }
+ }
+ }
String[] value = new String[newDisallowed.size()];
newDisallowed.toArray(value);
binder.setDisallowedFields(value);
View
14 grails-web/src/main/groovy/org/codehaus/groovy/grails/web/binding/DefaultASTDatabindingHelper.java
@@ -189,8 +189,9 @@ private FieldNode getDeclaredFieldInInheritanceHierarchy(
final List<FieldNode> fields = classNode.getFields();
for(FieldNode fieldNode : fields) {
final String fieldName = fieldNode.getName();
+ final boolean isDomainClass = GrailsASTUtils.isDomainClass(classNode, sourceUnit);
if((!unbindablePropertyNames.contains(fieldName)) &&
- (bindablePropertyNames.contains(fieldName) || shouldFieldBeInWhiteList(fieldNode, fieldsInTransientsList))) {
+ (bindablePropertyNames.contains(fieldName) || shouldFieldBeInWhiteList(fieldNode, fieldsInTransientsList, isDomainClass))) {
propertyNamesToIncludeInWhiteList.add(fieldName);
}
}
@@ -226,14 +227,19 @@ private FieldNode getDeclaredFieldInInheritanceHierarchy(
return propertyNamesToIncludeInWhiteList;
}
- private boolean shouldFieldBeInWhiteList(final FieldNode fieldNode, final Set<String> fieldsInTransientsList) {
+ private boolean shouldFieldBeInWhiteList(final FieldNode fieldNode, final Set<String> fieldsInTransientsList, final boolean isDomainClass) {
boolean shouldInclude = true;
final int modifiers = fieldNode.getModifiers();
- if((modifiers & Modifier.STATIC) != 0 ||
+ final String fieldName = fieldNode.getName();
+ if((modifiers & Modifier.STATIC) != 0 ||
(modifiers & Modifier.TRANSIENT) != 0 ||
- fieldsInTransientsList.contains(fieldNode.getName()) ||
+ fieldsInTransientsList.contains(fieldName) ||
fieldNode.getType().equals(new ClassNode(Object.class))) {
shouldInclude = false;
+ } else if(isDomainClass) {
+ if("id".equals(fieldName) || "version".equals(fieldName)) {
+ shouldInclude = false;
+ }
}
return shouldInclude;
}
Please sign in to comment.
Something went wrong with that request. Please try again.