From c7fd03be6923e572fc50dca880e40201b2ad1cf8 Mon Sep 17 00:00:00 2001 From: Chris Beams Date: Wed, 1 Feb 2012 17:19:42 +0100 Subject: [PATCH 01/61] Polish ReflectiveMethodResolver and unit tests - Update Javadoc - Fix whitespace errors (tabs v. spaces, trailing spaces) - Break at 90 chars where sensible - Remove dead test code - Fix generics warnings, remove @SuppressWarnings --- .../support/ReflectiveMethodResolver.java | 27 +- .../expression/spel/SpringEL300Tests.java | 575 ++++++++---------- 2 files changed, 275 insertions(+), 327 deletions(-) diff --git a/org.springframework.expression/src/main/java/org/springframework/expression/spel/support/ReflectiveMethodResolver.java b/org.springframework.expression/src/main/java/org/springframework/expression/spel/support/ReflectiveMethodResolver.java index d40daa3cbae..019faf2836b 100644 --- a/org.springframework.expression/src/main/java/org/springframework/expression/spel/support/ReflectiveMethodResolver.java +++ b/org.springframework.expression/src/main/java/org/springframework/expression/spel/support/ReflectiveMethodResolver.java @@ -38,27 +38,30 @@ import org.springframework.expression.spel.SpelMessage; import org.springframework.util.CollectionUtils; /** - * A method resolver that uses reflection to locate the method that should be invoked. + * Reflection-based {@link MethodResolver} used by default in + * {@link StandardEvaluationContext} unless explicit method resolvers have been specified. * * @author Andy Clement * @author Juergen Hoeller + * @author Chris Beams * @since 3.0 + * @see StandardEvaluationContext#addMethodResolver(MethodResolver) */ public class ReflectiveMethodResolver implements MethodResolver { private static Method[] NO_METHODS = new Method[0]; private Map, MethodFilter> filters = null; - + // Using distance will ensure a more accurate match is discovered, // more closely following the Java rules. private boolean useDistance = false; - + public ReflectiveMethodResolver() { - + } - + /** * This constructors allows the ReflectiveMethodResolver to be configured such that it will * use a distance computation to check which is the better of two close matches (when there @@ -71,7 +74,7 @@ public class ReflectiveMethodResolver implements MethodResolver { public ReflectiveMethodResolver(boolean useDistance) { this.useDistance = useDistance; } - + /** * Locate a method on a type. There are three kinds of match that might occur: *
    @@ -88,14 +91,14 @@ public class ReflectiveMethodResolver implements MethodResolver { TypeConverter typeConverter = context.getTypeConverter(); Class type = (targetObject instanceof Class ? (Class) targetObject : targetObject.getClass()); Method[] methods = type.getMethods(); - + // If a filter is registered for this type, call it MethodFilter filter = (this.filters != null ? this.filters.get(type) : null); if (filter != null) { - List methodsForFiltering = new ArrayList(); - for (Method method: methods) { - methodsForFiltering.add(method); - } + List methodsForFiltering = new ArrayList(); + for (Method method: methods) { + methodsForFiltering.add(method); + } List methodsFiltered = filter.filter(methodsForFiltering); if (CollectionUtils.isEmpty(methodsFiltered)) { methods = NO_METHODS; @@ -124,7 +127,7 @@ public class ReflectiveMethodResolver implements MethodResolver { continue; } if (method.getName().equals(name)) { - Class[] paramTypes = method.getParameterTypes(); + Class[] paramTypes = method.getParameterTypes(); List paramDescriptors = new ArrayList(paramTypes.length); for (int i = 0; i < paramTypes.length; i++) { paramDescriptors.add(new TypeDescriptor(new MethodParameter(method, i))); diff --git a/org.springframework.expression/src/test/java/org/springframework/expression/spel/SpringEL300Tests.java b/org.springframework.expression/src/test/java/org/springframework/expression/spel/SpringEL300Tests.java index c947e27e025..e28f9dd3a15 100644 --- a/org.springframework.expression/src/test/java/org/springframework/expression/spel/SpringEL300Tests.java +++ b/org.springframework.expression/src/test/java/org/springframework/expression/spel/SpringEL300Tests.java @@ -50,12 +50,12 @@ import org.springframework.expression.spel.support.StandardTypeLocator; /** * Tests based on Jiras up to the release of Spring 3.0.0 - * + * * @author Andy Clement * @author Clark Duplichien */ public class SpringEL300Tests extends ExpressionTestCase { - + @Test public void testNPE_SPR5661() { evaluate("joinThreeStrings('a',null,'c')", "anullc", String.class); @@ -66,7 +66,7 @@ public class SpringEL300Tests extends ExpressionTestCase { public void testSWF1086() { evaluate("printDouble(T(java.math.BigDecimal).valueOf(14.35))", "anullc", String.class); } - + @Test public void testDoubleCoercion() { evaluate("printDouble(14.35)", "14.35", String.class); @@ -92,15 +92,15 @@ public class SpringEL300Tests extends ExpressionTestCase { // success } eContext.setTypeLocator(new MyTypeLocator()); - + // varargs expr = new SpelExpressionParser().parseRaw("tryToInvokeWithNull3(null,'a','b')"); Assert.assertEquals("ab",expr.getValue(eContext)); - + // varargs 2 - null is packed into the varargs expr = new SpelExpressionParser().parseRaw("tryToInvokeWithNull3(12,'a',null,'c')"); Assert.assertEquals("anullc",expr.getValue(eContext)); - + // check we can find the ctor ok expr = new SpelExpressionParser().parseRaw("new Spr5899Class().toString()"); Assert.assertEquals("instance",expr.getValue(eContext)); @@ -116,7 +116,7 @@ public class SpringEL300Tests extends ExpressionTestCase { expr = new SpelExpressionParser().parseRaw("new Spr5899Class(null,'a', null, 'b').toString()"); Assert.assertEquals("instance",expr.getValue(eContext)); } - + static class MyTypeLocator extends StandardTypeLocator { public Class findType(String typename) throws EvaluationException { @@ -132,12 +132,12 @@ public class SpringEL300Tests extends ExpressionTestCase { static class Spr5899Class { public Spr5899Class() {} - public Spr5899Class(Integer i) { } - public Spr5899Class(Integer i, String... s) { } - + public Spr5899Class(Integer i) { } + public Spr5899Class(Integer i, String... s) { } + public Integer tryToInvokeWithNull(Integer value) { return value; } public Integer tryToInvokeWithNull2(int i) { return new Integer(i); } - public String tryToInvokeWithNull3(Integer value,String... strings) { + public String tryToInvokeWithNull3(Integer value,String... strings) { StringBuilder sb = new StringBuilder(); for (int i=0;i m = new HashMap(); m.put("foo", "bar"); StandardEvaluationContext eContext = new StandardEvaluationContext(m); // root is a map instance eContext.addPropertyAccessor(new MapAccessor()); Expression expr = new SpelExpressionParser().parseRaw("['foo']"); Assert.assertEquals("bar", expr.getValue(eContext)); } - + @Test public void testSPR5847() throws Exception { StandardEvaluationContext eContext = new StandardEvaluationContext(new TestProperties()); String name = null; Expression expr = null; - + expr = new SpelExpressionParser().parseRaw("jdbcProperties['username']"); name = expr.getValue(eContext,String.class); Assert.assertEquals("Dave",name); - + expr = new SpelExpressionParser().parseRaw("jdbcProperties[username]"); name = expr.getValue(eContext,String.class); Assert.assertEquals("Dave",name); @@ -209,7 +208,7 @@ public class SpringEL300Tests extends ExpressionTestCase { eContext.addPropertyAccessor(new MapAccessor()); name = expr.getValue(eContext,String.class); Assert.assertEquals("Dave",name); - + // --- dotted property names // lookup foo on the root, then bar on that, then use that as the key into jdbcProperties @@ -222,9 +221,9 @@ public class SpringEL300Tests extends ExpressionTestCase { expr = new SpelExpressionParser().parseRaw("jdbcProperties['foo.bar']"); eContext.addPropertyAccessor(new MapAccessor()); name = expr.getValue(eContext,String.class); - Assert.assertEquals("Elephant",name); + Assert.assertEquals("Elephant",name); } - + static class TestProperties { public Properties jdbcProperties = new Properties(); public Properties foo = new Properties(); @@ -239,11 +238,11 @@ public class SpringEL300Tests extends ExpressionTestCase { static class MapAccessor implements PropertyAccessor { public boolean canRead(EvaluationContext context, Object target, String name) throws AccessException { - return (((Map) target).containsKey(name)); + return (((Map) target).containsKey(name)); } public TypedValue read(EvaluationContext context, Object target, String name) throws AccessException { - return new TypedValue(((Map) target).get(name)); + return new TypedValue(((Map) target).get(name)); } public boolean canWrite(EvaluationContext context, Object target, String name) throws AccessException { @@ -252,22 +251,22 @@ public class SpringEL300Tests extends ExpressionTestCase { @SuppressWarnings("unchecked") public void write(EvaluationContext context, Object target, String name, Object newValue) throws AccessException { - ((Map) target).put(name, newValue); + ((Map) target).put(name, newValue); } - public Class[] getSpecificTargetClasses() { + public Class[] getSpecificTargetClasses() { return new Class[] {Map.class}; } - + } - + @Test public void testNPE_SPR5673() throws Exception { ParserContext hashes = TemplateExpressionParsingTests.HASH_DELIMITED_PARSER_CONTEXT; ParserContext dollars = TemplateExpressionParsingTests.DEFAULT_TEMPLATE_PARSER_CONTEXT; - + checkTemplateParsing("abc${'def'} ghi","abcdef ghi"); - + checkTemplateParsingError("abc${ {}( 'abc'","Missing closing ')' for '(' at position 8"); checkTemplateParsingError("abc${ {}[ 'abc'","Missing closing ']' for '[' at position 8"); checkTemplateParsingError("abc${ {}{ 'abc'","Missing closing '}' for '{' at position 8"); @@ -278,7 +277,7 @@ public class SpringEL300Tests extends ExpressionTestCase { checkTemplateParsingError("abc${ ] }","Found closing ']' at position 6 without an opening '['"); checkTemplateParsingError("abc${ } }","No expression defined within delimiter '${}' at character 3"); checkTemplateParsingError("abc$[ } ]",DOLLARSQUARE_TEMPLATE_PARSER_CONTEXT,"Found closing '}' at position 6 without an opening '{'"); - + checkTemplateParsing("abc ${\"def''g}hi\"} jkl","abc def'g}hi jkl"); checkTemplateParsing("abc ${'def''g}hi'} jkl","abc def'g}hi jkl"); checkTemplateParsing("}","}"); @@ -295,7 +294,7 @@ public class SpringEL300Tests extends ExpressionTestCase { checkTemplateParsing("Hello ${'inner literal that''s got {[(])]}an escaped quote in it'} World","Hello inner literal that's got {[(])]}an escaped quote in it World"); checkTemplateParsingError("Hello ${","No ending suffix '}' for expression starting at character 6: ${"); } - + @Test public void testAccessingNullPropertyViaReflection_SPR5663() throws AccessException { PropertyAccessor propertyAccessor = new ReflectivePropertyAccessor(); @@ -315,33 +314,33 @@ public class SpringEL300Tests extends ExpressionTestCase { // success } } - + @Test public void testNestedProperties_SPR6923() { StandardEvaluationContext eContext = new StandardEvaluationContext(new Foo()); String name = null; Expression expr = null; - + expr = new SpelExpressionParser().parseRaw("resource.resource.server"); name = expr.getValue(eContext,String.class); Assert.assertEquals("abc",name); } - + static class Foo { public ResourceSummary resource = new ResourceSummary(); } - + static class ResourceSummary { ResourceSummary() { this.resource = new Resource(); } private final Resource resource; public Resource getResource() { - return resource; - } + return resource; + } } - + static class Resource { public String getServer() { return "abc"; @@ -374,9 +373,8 @@ public class SpringEL300Tests extends ExpressionTestCase { name = expr.getValue(eContext,String.class); // will be using the cached accessor this time Assert.assertEquals("hello",name); } - + /** $ related identifiers */ - @SuppressWarnings("unchecked") @Test public void testDollarPrefixedIdentifier_SPR7100() { Holder h = new Holder(); @@ -390,7 +388,7 @@ public class SpringEL300Tests extends ExpressionTestCase { h.map.put("$_$","tribble"); String name = null; Expression expr = null; - + expr = new SpelExpressionParser().parseRaw("map.$foo"); name = expr.getValue(eContext,String.class); Assert.assertEquals("wibble",name); @@ -430,8 +428,11 @@ public class SpringEL300Tests extends ExpressionTestCase { name = expr.getValue(eContext,String.class); // will be using the cached accessor this time Assert.assertEquals("wobble",name); } - - /** Should be accessing (setting) Goo.wibble field because 'bar' variable evaluates to "wibble" */ + + /** + * Should be accessing (setting) Goo.wibble field because 'bar' variable evaluates to + * "wibble" + */ @Test public void testIndexingAsAPropertyAccess_SPR6968_4() { Goo g = Goo.instance; @@ -458,7 +459,7 @@ public class SpringEL300Tests extends ExpressionTestCase { expr.getValue(eContext,String.class); // will be using the cached accessor this time Assert.assertEquals("world",g.value); } - + @Test public void testDollars() { StandardEvaluationContext eContext = new StandardEvaluationContext(new XX()); @@ -467,7 +468,7 @@ public class SpringEL300Tests extends ExpressionTestCase { eContext.setVariable("file_name","$foo"); Assert.assertEquals("wibble",expr.getValue(eContext,String.class)); } - + @Test public void testDollars2() { StandardEvaluationContext eContext = new StandardEvaluationContext(new XX()); @@ -476,12 +477,12 @@ public class SpringEL300Tests extends ExpressionTestCase { eContext.setVariable("file_name","$foo"); Assert.assertEquals("wibble",expr.getValue(eContext,String.class)); } - + static class XX { public Map m; - + public String floo ="bar"; - + public XX() { m = new HashMap(); m.put("$foo","wibble"); @@ -490,34 +491,34 @@ public class SpringEL300Tests extends ExpressionTestCase { } static class Goo { - + public static Goo instance = new Goo(); public String bar = "key"; public String value = null; public String wibble = "wobble"; - + public String getKey() { return "hello"; } - + public void setKey(String s) { value = s; } - + } - + static class Holder { - - public Map map = new HashMap(); + + public Map map = new HashMap(); } - + // --- private void checkTemplateParsing(String expression, String expectedValue) throws Exception { checkTemplateParsing(expression,TemplateExpressionParsingTests.DEFAULT_TEMPLATE_PARSER_CONTEXT, expectedValue); } - + private void checkTemplateParsing(String expression, ParserContext context, String expectedValue) throws Exception { SpelExpressionParser parser = new SpelExpressionParser(); Expression expr = parser.parseExpression(expression,context); @@ -527,7 +528,7 @@ public class SpringEL300Tests extends ExpressionTestCase { private void checkTemplateParsingError(String expression,String expectedMessage) throws Exception { checkTemplateParsingError(expression, TemplateExpressionParsingTests.DEFAULT_TEMPLATE_PARSER_CONTEXT,expectedMessage); } - + private void checkTemplateParsingError(String expression,ParserContext context, String expectedMessage) throws Exception { SpelExpressionParser parser = new SpelExpressionParser(); try { @@ -540,7 +541,7 @@ public class SpringEL300Tests extends ExpressionTestCase { Assert.assertEquals(expectedMessage,e.getMessage()); } } - + private static final ParserContext DOLLARSQUARE_TEMPLATE_PARSER_CONTEXT = new ParserContext() { public String getExpressionPrefix() { return "$["; @@ -552,28 +553,13 @@ public class SpringEL300Tests extends ExpressionTestCase { return true; } }; - -// @Test -// public void testFails() { -// -// StandardEvaluationContext evaluationContext = new StandardEvaluationContext(); -// evaluationContext.setVariable("target", new Foo2()); -// for (int i = 0; i < 300000; i++) { -// evaluationContext.addPropertyAccessor(new MapAccessor()); -// ExpressionParser parser = new SpelExpressionParser(); -// Expression expression = parser.parseExpression("#target.execute(payload)"); -// Message message = new Message(); -// message.setPayload(i+""); -// expression.getValue(evaluationContext, message); -// } -// } - + static class Foo2 { public void execute(String str){ System.out.println("Value: " + str); } } - + static class Message{ private String payload; @@ -587,7 +573,7 @@ public class SpringEL300Tests extends ExpressionTestCase { } // bean resolver tests - + @Test public void beanResolution() { StandardEvaluationContext eContext = new StandardEvaluationContext(new XX()); @@ -601,7 +587,7 @@ public class SpringEL300Tests extends ExpressionTestCase { Assert.assertEquals(SpelMessage.NO_BEAN_RESOLVER_REGISTERED,see.getMessageCode()); Assert.assertEquals("foo",see.getInserts()[0]); } - + eContext.setBeanResolver(new MyBeanResolver()); // bean exists @@ -622,11 +608,11 @@ public class SpringEL300Tests extends ExpressionTestCase { Assert.assertTrue(see.getCause() instanceof AccessException); Assert.assertTrue(((AccessException)see.getCause()).getMessage().startsWith("DONT")); } - + // bean exists expr = new SpelExpressionParser().parseRaw("@'foo.bar'"); Assert.assertEquals("trouble",expr.getValue(eContext,String.class)); - + // bean exists try { expr = new SpelExpressionParser().parseRaw("@378"); @@ -635,7 +621,7 @@ public class SpringEL300Tests extends ExpressionTestCase { Assert.assertEquals(SpelMessage.INVALID_BEAN_REFERENCE,spe.getMessageCode()); } } - + static class MyBeanResolver implements BeanResolver { public Object resolve(EvaluationContext context, String beanname) throws AccessException { if (beanname.equals("foo")) { @@ -648,14 +634,14 @@ public class SpringEL300Tests extends ExpressionTestCase { return null; } } - + // end bean resolver tests @Test public void elvis_SPR7209_1() { StandardEvaluationContext eContext = new StandardEvaluationContext(new XX()); Expression expr = null; - + // Different parts of elvis expression are null expr = new SpelExpressionParser().parseRaw("(?:'default')"); Assert.assertEquals("default", expr.getValue()); @@ -696,68 +682,67 @@ public class SpringEL300Tests extends ExpressionTestCase { expr = new SpelExpressionParser().parseRaw("''?:'default'"); Assert.assertEquals("default", expr.getValue()); } - - @Test - @SuppressWarnings("unchecked") - public void testMapOfMap_SPR7244() throws Exception { - Map map = new LinkedHashMap(); - map.put("uri", "http:"); - Map nameMap = new LinkedHashMap(); - nameMap.put("givenName", "Arthur"); - map.put("value", nameMap); - - StandardEvaluationContext ctx = new StandardEvaluationContext(map); - ExpressionParser parser = new SpelExpressionParser(); - String el1 = "#root['value'].get('givenName')"; - Expression exp = parser.parseExpression(el1); - Object evaluated = exp.getValue(ctx); - Assert.assertEquals("Arthur", evaluated); - String el2 = "#root['value']['givenName']"; - exp = parser.parseExpression(el2); - evaluated = exp.getValue(ctx); - Assert.assertEquals("Arthur",evaluated); - } - + @Test + public void testMapOfMap_SPR7244() throws Exception { + Map map = new LinkedHashMap(); + map.put("uri", "http:"); + Map nameMap = new LinkedHashMap(); + nameMap.put("givenName", "Arthur"); + map.put("value", nameMap); + + StandardEvaluationContext ctx = new StandardEvaluationContext(map); + ExpressionParser parser = new SpelExpressionParser(); + String el1 = "#root['value'].get('givenName')"; + Expression exp = parser.parseExpression(el1); + Object evaluated = exp.getValue(ctx); + Assert.assertEquals("Arthur", evaluated); + + String el2 = "#root['value']['givenName']"; + exp = parser.parseExpression(el2); + evaluated = exp.getValue(ctx); + Assert.assertEquals("Arthur",evaluated); + } + @Test public void testProjectionTypeDescriptors_1() throws Exception { - StandardEvaluationContext ctx = new StandardEvaluationContext(new C()); - SpelExpressionParser parser = new SpelExpressionParser(); - String el1 = "ls.![#this.equals('abc')]"; - SpelExpression exp = parser.parseRaw(el1); - List value = (List)exp.getValue(ctx); - // value is list containing [true,false] - Assert.assertEquals(Boolean.class,value.get(0).getClass()); - TypeDescriptor evaluated = exp.getValueTypeDescriptor(ctx); - Assert.assertEquals(null, evaluated.getElementTypeDescriptor()); + StandardEvaluationContext ctx = new StandardEvaluationContext(new C()); + SpelExpressionParser parser = new SpelExpressionParser(); + String el1 = "ls.![#this.equals('abc')]"; + SpelExpression exp = parser.parseRaw(el1); + List value = (List)exp.getValue(ctx); + // value is list containing [true,false] + Assert.assertEquals(Boolean.class,value.get(0).getClass()); + TypeDescriptor evaluated = exp.getValueTypeDescriptor(ctx); + Assert.assertEquals(null, evaluated.getElementTypeDescriptor()); } - + @Test public void testProjectionTypeDescriptors_2() throws Exception { - StandardEvaluationContext ctx = new StandardEvaluationContext(new C()); - SpelExpressionParser parser = new SpelExpressionParser(); - String el1 = "as.![#this.equals('abc')]"; - SpelExpression exp = parser.parseRaw(el1); - Object[] value = (Object[])exp.getValue(ctx); - // value is array containing [true,false] - Assert.assertEquals(Boolean.class,value[0].getClass()); - TypeDescriptor evaluated = exp.getValueTypeDescriptor(ctx); - Assert.assertEquals(Boolean.class, evaluated.getElementTypeDescriptor().getType()); + StandardEvaluationContext ctx = new StandardEvaluationContext(new C()); + SpelExpressionParser parser = new SpelExpressionParser(); + String el1 = "as.![#this.equals('abc')]"; + SpelExpression exp = parser.parseRaw(el1); + Object[] value = (Object[])exp.getValue(ctx); + // value is array containing [true,false] + Assert.assertEquals(Boolean.class,value[0].getClass()); + TypeDescriptor evaluated = exp.getValueTypeDescriptor(ctx); + Assert.assertEquals(Boolean.class, evaluated.getElementTypeDescriptor().getType()); } - + @Test public void testProjectionTypeDescriptors_3() throws Exception { - StandardEvaluationContext ctx = new StandardEvaluationContext(new C()); - SpelExpressionParser parser = new SpelExpressionParser(); - String el1 = "ms.![key.equals('abc')]"; - SpelExpression exp = parser.parseRaw(el1); - List value = (List)exp.getValue(ctx); - // value is list containing [true,false] - Assert.assertEquals(Boolean.class,value.get(0).getClass()); - TypeDescriptor evaluated = exp.getValueTypeDescriptor(ctx); - Assert.assertEquals(null, evaluated.getElementTypeDescriptor()); + StandardEvaluationContext ctx = new StandardEvaluationContext(new C()); + SpelExpressionParser parser = new SpelExpressionParser(); + String el1 = "ms.![key.equals('abc')]"; + SpelExpression exp = parser.parseRaw(el1); + List value = (List)exp.getValue(ctx); + // value is list containing [true,false] + Assert.assertEquals(Boolean.class,value.get(0).getClass()); + TypeDescriptor evaluated = exp.getValueTypeDescriptor(ctx); + Assert.assertEquals(null, evaluated.getElementTypeDescriptor()); } - + static class C { public List ls; public String[] as; @@ -772,19 +757,19 @@ public class SpringEL300Tests extends ExpressionTestCase { ms.put("def","pqr"); } } - + static class D { public String a; - + private D(String s) { a=s; } - + public String toString() { return "D("+a+")"; } } - + @Test public void testGreaterThanWithNulls_SPR7840() throws Exception { List list = new ArrayList(); @@ -794,30 +779,31 @@ public class SpringEL300Tests extends ExpressionTestCase { list.add(new D("ccc")); list.add(new D(null)); list.add(new D("zzz")); - - StandardEvaluationContext ctx = new StandardEvaluationContext(list); - SpelExpressionParser parser = new SpelExpressionParser(); - String el1 = "#root.?[a < 'hhh']"; - SpelExpression exp = parser.parseRaw(el1); - Object value = exp.getValue(ctx); - assertEquals("[D(aaa), D(bbb), D(null), D(ccc), D(null)]",value.toString()); + StandardEvaluationContext ctx = new StandardEvaluationContext(list); + SpelExpressionParser parser = new SpelExpressionParser(); - String el2 = "#root.?[a > 'hhh']"; - SpelExpression exp2 = parser.parseRaw(el2); - Object value2 = exp2.getValue(ctx); - assertEquals("[D(zzz)]",value2.toString()); - - // trim out the nulls first - String el3 = "#root.?[a!=null].?[a < 'hhh']"; - SpelExpression exp3 = parser.parseRaw(el3); - Object value3 = exp3.getValue(ctx); - assertEquals("[D(aaa), D(bbb), D(ccc)]",value3.toString()); + String el1 = "#root.?[a < 'hhh']"; + SpelExpression exp = parser.parseRaw(el1); + Object value = exp.getValue(ctx); + assertEquals("[D(aaa), D(bbb), D(null), D(ccc), D(null)]",value.toString()); + + String el2 = "#root.?[a > 'hhh']"; + SpelExpression exp2 = parser.parseRaw(el2); + Object value2 = exp2.getValue(ctx); + assertEquals("[D(zzz)]",value2.toString()); + + // trim out the nulls first + String el3 = "#root.?[a!=null].?[a < 'hhh']"; + SpelExpression exp3 = parser.parseRaw(el3); + Object value3 = exp3.getValue(ctx); + assertEquals("[D(aaa), D(bbb), D(ccc)]",value3.toString()); } /** - * Test whether {@link ReflectiveMethodResolver} follows Java Method Invocation Conversion order. And more precisely - * that widening reference conversion is 'higher' than a unboxing conversion. + * Test whether {@link ReflectiveMethodResolver} follows Java Method Invocation + * Conversion order. And more precisely that widening reference conversion is 'higher' + * than a unboxing conversion. */ @Test public void testConversionPriority_8224() throws Exception { @@ -904,16 +890,16 @@ public class SpringEL300Tests extends ExpressionTestCase { } - + @Test public void varargsAndPrimitives_SPR8174() throws Exception { EvaluationContext emptyEvalContext = new StandardEvaluationContext(); List args = new ArrayList(); - + args.add(TypeDescriptor.forObject(34L)); ReflectionUtil ru = new ReflectionUtil(); MethodExecutor me = new ReflectiveMethodResolver().resolve(emptyEvalContext,ru,"methodToCall",args); - + args.set(0,TypeDescriptor.forObject(23)); me = new ReflectiveMethodResolver().resolve(emptyEvalContext,ru,"foo",args); me.execute(emptyEvalContext, ru, 45); @@ -941,127 +927,126 @@ public class SpringEL300Tests extends ExpressionTestCase { args.set(0,TypeDescriptor.forObject((byte)23)); me = new ReflectiveMethodResolver().resolve(emptyEvalContext,ru,"foo",args); me.execute(emptyEvalContext, ru, (byte)23); - + args.set(0,TypeDescriptor.forObject(true)); me = new ReflectiveMethodResolver().resolve(emptyEvalContext,ru,"foo",args); me.execute(emptyEvalContext, ru, true); - + // trickier: args.set(0,TypeDescriptor.forObject(12)); args.add(TypeDescriptor.forObject(23f)); me = new ReflectiveMethodResolver().resolve(emptyEvalContext,ru,"bar",args); me.execute(emptyEvalContext, ru, 12,23f); - + } - - - + + + public class ReflectionUtil { - public Object methodToCall(T param) { - System.out.println(param+" "+param.getClass()); - return "Object methodToCall(T param)"; - } - - public void foo(int... array) { - if (array.length==0) { - throw new RuntimeException(); - } - } - public void foo(float...array) { - if (array.length==0) { - throw new RuntimeException(); - } - } - public void foo(double...array) { - if (array.length==0) { - throw new RuntimeException(); - } - } - public void foo(short...array) { - if (array.length==0) { - throw new RuntimeException(); - } - } - public void foo(long...array) { - if (array.length==0) { - throw new RuntimeException(); - } - } - public void foo(boolean...array) { - if (array.length==0) { - throw new RuntimeException(); - } - } - public void foo(char...array) { - if (array.length==0) { - throw new RuntimeException(); - } - } - public void foo(byte...array) { - if (array.length==0) { - throw new RuntimeException(); - } - } - - public void bar(int... array) { - if (array.length==0) { - throw new RuntimeException(); - } - } - } - + public Object methodToCall(T param) { + System.out.println(param+" "+param.getClass()); + return "Object methodToCall(T param)"; + } + + public void foo(int... array) { + if (array.length==0) { + throw new RuntimeException(); + } + } + public void foo(float...array) { + if (array.length==0) { + throw new RuntimeException(); + } + } + public void foo(double...array) { + if (array.length==0) { + throw new RuntimeException(); + } + } + public void foo(short...array) { + if (array.length==0) { + throw new RuntimeException(); + } + } + public void foo(long...array) { + if (array.length==0) { + throw new RuntimeException(); + } + } + public void foo(boolean...array) { + if (array.length==0) { + throw new RuntimeException(); + } + } + public void foo(char...array) { + if (array.length==0) { + throw new RuntimeException(); + } + } + public void foo(byte...array) { + if (array.length==0) { + throw new RuntimeException(); + } + } + + public void bar(int... array) { + if (array.length==0) { + throw new RuntimeException(); + } + } + } + @Test public void testReservedWords_8228() throws Exception { // "DIV","EQ","GE","GT","LE","LT","MOD","NE","NOT" - @SuppressWarnings("unused") - class Reserver { - public Reserver getReserver() { - return this; - } - public String NE = "abc"; - public String ne = "def"; - - public int DIV = 1; - public int div = 3; - - public Map m = new HashMap(); - - @SuppressWarnings("unchecked") + @SuppressWarnings("unused") + class Reserver { + public Reserver getReserver() { + return this; + } + public String NE = "abc"; + public String ne = "def"; + + public int DIV = 1; + public int div = 3; + + public Map m = new HashMap(); + Reserver() { - m.put("NE","xyz"); - } - } - StandardEvaluationContext ctx = new StandardEvaluationContext(new Reserver()); - SpelExpressionParser parser = new SpelExpressionParser(); - String ex = "getReserver().NE"; - SpelExpression exp = null; - exp = parser.parseRaw(ex); - String value = (String)exp.getValue(ctx); - Assert.assertEquals("abc",value); + m.put("NE","xyz"); + } + } + StandardEvaluationContext ctx = new StandardEvaluationContext(new Reserver()); + SpelExpressionParser parser = new SpelExpressionParser(); + String ex = "getReserver().NE"; + SpelExpression exp = null; + exp = parser.parseRaw(ex); + String value = (String)exp.getValue(ctx); + Assert.assertEquals("abc",value); - ex = "getReserver().ne"; - exp = parser.parseRaw(ex); - value = (String)exp.getValue(ctx); - Assert.assertEquals("def",value); + ex = "getReserver().ne"; + exp = parser.parseRaw(ex); + value = (String)exp.getValue(ctx); + Assert.assertEquals("def",value); - ex = "getReserver().m[NE]"; - exp = parser.parseRaw(ex); - value = (String)exp.getValue(ctx); - Assert.assertEquals("xyz",value); - - ex = "getReserver().DIV"; - exp = parser.parseRaw(ex); - Assert.assertEquals(1,exp.getValue(ctx)); + ex = "getReserver().m[NE]"; + exp = parser.parseRaw(ex); + value = (String)exp.getValue(ctx); + Assert.assertEquals("xyz",value); - ex = "getReserver().div"; - exp = parser.parseRaw(ex); - Assert.assertEquals(3,exp.getValue(ctx)); - - exp = parser.parseRaw("NE"); - Assert.assertEquals("abc",exp.getValue(ctx)); + ex = "getReserver().DIV"; + exp = parser.parseRaw(ex); + Assert.assertEquals(1,exp.getValue(ctx)); + + ex = "getReserver().div"; + exp = parser.parseRaw(ex); + Assert.assertEquals(3,exp.getValue(ctx)); + + exp = parser.parseRaw("NE"); + Assert.assertEquals("abc",exp.getValue(ctx)); } - + /** * We add property accessors in the order: * First, Second, Third, Fourth. @@ -1071,39 +1056,24 @@ public class SpringEL300Tests extends ExpressionTestCase { @Test public void testPropertyAccessorOrder_8211() { ExpressionParser expressionParser = new SpelExpressionParser(); - StandardEvaluationContext evaluationContext = + StandardEvaluationContext evaluationContext = new StandardEvaluationContext(new ContextObject()); - + evaluationContext.addPropertyAccessor(new TestPropertyAccessor("firstContext")); evaluationContext.addPropertyAccessor(new TestPropertyAccessor("secondContext")); evaluationContext.addPropertyAccessor(new TestPropertyAccessor("thirdContext")); evaluationContext.addPropertyAccessor(new TestPropertyAccessor("fourthContext")); - - assertEquals("first", + + assertEquals("first", expressionParser.parseExpression("shouldBeFirst").getValue(evaluationContext)); - assertEquals("second", + assertEquals("second", expressionParser.parseExpression("shouldBeSecond").getValue(evaluationContext)); - assertEquals("third", + assertEquals("third", expressionParser.parseExpression("shouldBeThird").getValue(evaluationContext)); - assertEquals("fourth", + assertEquals("fourth", expressionParser.parseExpression("shouldBeFourth").getValue(evaluationContext)); } - - // test not quite complete, it doesn't check that the list of resolvers at the end of - // PropertyOrFieldReference.getPropertyAccessorsToTry() doesn't contain duplicates, which - // is what it is trying to test by having a property accessor that returns specific - // classes Integer and Number -// @Test -// public void testPropertyAccessorOrder_8211_2() { -// ExpressionParser expressionParser = new SpelExpressionParser(); -// StandardEvaluationContext evaluationContext = -// new StandardEvaluationContext(new Integer(42)); -// -// evaluationContext.addPropertyAccessor(new TestPropertyAccessor2()); -// -// assertEquals("42", expressionParser.parseExpression("x").getValue(evaluationContext)); -// } - + class TestPropertyAccessor implements PropertyAccessor { private String mapName; public TestPropertyAccessor(String mapName) { @@ -1139,38 +1109,13 @@ public class SpringEL300Tests extends ExpressionTestCase { } } - -// class TestPropertyAccessor2 implements PropertyAccessor { -// -// public Class[] getSpecificTargetClasses() { -// return new Class[]{Integer.class,Number.class}; -// } -// -// public boolean canRead(EvaluationContext context, Object target, String name) throws AccessException { -// return true; -// } -// -// public TypedValue read(EvaluationContext context, Object target, String name) throws AccessException { -// return new TypedValue(target.toString(),TypeDescriptor.valueOf(String.class)); -// } -// -// public boolean canWrite(EvaluationContext context, Object target, String name) throws AccessException { -// return false; -// } -// -// public void write(EvaluationContext context, Object target, String name, Object newValue) -// throws AccessException { -// throw new UnsupportedOperationException(); -// } -// -// } class ContextObject { public Map firstContext = new HashMap(); public Map secondContext = new HashMap(); public Map thirdContext = new HashMap(); public Map fourthContext = new HashMap(); - + public ContextObject() { firstContext.put("shouldBeFirst", "first"); secondContext.put("shouldBeFirst", "second"); @@ -1180,10 +1125,10 @@ public class SpringEL300Tests extends ExpressionTestCase { secondContext.put("shouldBeSecond", "second"); thirdContext.put("shouldBeSecond", "third"); fourthContext.put("shouldBeSecond", "fourth"); - + thirdContext.put("shouldBeThird", "third"); fourthContext.put("shouldBeThird", "fourth"); - + fourthContext.put("shouldBeFourth", "fourth"); } public Map getFirstContext() {return firstContext;} From 90bed9718f17bfd8f3d9dbe6cd2b3cf7ed2e6573 Mon Sep 17 00:00:00 2001 From: Andy Clement Date: Wed, 25 Jan 2012 17:32:15 -0800 Subject: [PATCH 02/61] Allow customization of SpEL method resolution This change introduces a protected ReflectiveMethodResolver#getMethods, allowing subclasses to specify additional static methods not declared directly on the type being evaluated. These methods then become candidates for filtering by any registered MethodFilters and ultimately become available within for use within SpEL expressions. Issue: SPR-9038 --- .../support/ReflectiveMethodResolver.java | 16 +++++++- .../expression/spel/SpringEL300Tests.java | 38 +++++++++++++++++++ 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/org.springframework.expression/src/main/java/org/springframework/expression/spel/support/ReflectiveMethodResolver.java b/org.springframework.expression/src/main/java/org/springframework/expression/spel/support/ReflectiveMethodResolver.java index 019faf2836b..33d1f510b17 100644 --- a/org.springframework.expression/src/main/java/org/springframework/expression/spel/support/ReflectiveMethodResolver.java +++ b/org.springframework.expression/src/main/java/org/springframework/expression/spel/support/ReflectiveMethodResolver.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2010 the original author or authors. + * Copyright 2002-2011 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -90,7 +90,7 @@ public class ReflectiveMethodResolver implements MethodResolver { try { TypeConverter typeConverter = context.getTypeConverter(); Class type = (targetObject instanceof Class ? (Class) targetObject : targetObject.getClass()); - Method[] methods = type.getMethods(); + Method[] methods = getMethods(type); // If a filter is registered for this type, call it MethodFilter filter = (this.filters != null ? this.filters.get(type) : null); @@ -197,4 +197,16 @@ public class ReflectiveMethodResolver implements MethodResolver { } } + /** + * Return the set of methods for this type. The default implementation returns the + * result of Class#getMethods for the given {@code type}, but subclasses may override + * in order to alter the results, e.g. specifying static methods declared elsewhere. + * + * @param type the class for which to return the methods + * @since 3.1.1 + */ + protected Method[] getMethods(Class type) { + return type.getMethods(); + } + } diff --git a/org.springframework.expression/src/test/java/org/springframework/expression/spel/SpringEL300Tests.java b/org.springframework.expression/src/test/java/org/springframework/expression/spel/SpringEL300Tests.java index e28f9dd3a15..0fa4d1fbd1d 100644 --- a/org.springframework.expression/src/test/java/org/springframework/expression/spel/SpringEL300Tests.java +++ b/org.springframework.expression/src/test/java/org/springframework/expression/spel/SpringEL300Tests.java @@ -17,8 +17,10 @@ package org.springframework.expression.spel; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; import java.lang.reflect.Field; +import java.lang.reflect.Method; import java.util.ArrayList; import java.util.HashMap; import java.util.LinkedHashMap; @@ -38,6 +40,7 @@ import org.springframework.expression.EvaluationException; import org.springframework.expression.Expression; import org.springframework.expression.ExpressionParser; import org.springframework.expression.MethodExecutor; +import org.springframework.expression.MethodResolver; import org.springframework.expression.ParserContext; import org.springframework.expression.PropertyAccessor; import org.springframework.expression.TypedValue; @@ -1137,6 +1140,41 @@ public class SpringEL300Tests extends ExpressionTestCase { public Map getFourthContext() {return fourthContext;} } + /** + * Test the ability to subclass the ReflectiveMethodResolver and change how it + * determines the set of methods for a type. + */ + @Test + public void testCustomStaticFunctions_SPR9038() { + try { + ExpressionParser parser = new SpelExpressionParser(); + StandardEvaluationContext context = new StandardEvaluationContext(); + List methodResolvers = new ArrayList(); + methodResolvers.add(new ReflectiveMethodResolver() { + @Override + protected Method[] getMethods(Class type) { + try { + return new Method[] { + Integer.class.getDeclaredMethod("parseInt", new Class[] { + String.class, Integer.TYPE }) }; + } catch (NoSuchMethodException e1) { + return new Method[0]; + } + } + }); + + context.setMethodResolvers(methodResolvers); + org.springframework.expression.Expression expression = + parser.parseExpression("parseInt('-FF', 16)"); + + Integer result = expression.getValue(context, "", Integer.class); + assertEquals("Equal assertion failed: ", -255, result.intValue()); + } catch (Exception e) { + e.printStackTrace(); + fail("Unexpected exception: "+e.toString()); + } + } + } From 64a69f7cf826c39c0b72da4f910b8445c4627815 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Wed, 1 Feb 2012 13:22:12 -0500 Subject: [PATCH 03/61] SPR-9079 Don't check for "POST" multipart request method arg resolvers --- .../resources/changelog.txt | 1 + .../RequestPartMethodArgumentResolver.java | 13 +++++------- ...equestPartMethodArgumentResolverTests.java | 14 +++++++++++-- .../RequestParamMethodArgumentResolver.java | 13 +++--------- ...questParamMethodArgumentResolverTests.java | 21 ++++++++++++++++--- 5 files changed, 39 insertions(+), 23 deletions(-) diff --git a/build-spring-framework/resources/changelog.txt b/build-spring-framework/resources/changelog.txt index a5ddc80ba75..0597b8d2357 100644 --- a/build-spring-framework/resources/changelog.txt +++ b/build-spring-framework/resources/changelog.txt @@ -30,6 +30,7 @@ Changes in version 3.1.1 (2012-02-06) * add property to RedirectView to disable expanding URI variables in redirect URL * fix request mapping bug involving direct vs pattern path matches with HTTP methods * revise the FlashMapManager contract and implemenation to address a flaw in its design +* Remove check for HTTP "POST" when resolving multipart request controller method arguments Changes in version 3.1 GA (2011-12-12) -------------------------------------- diff --git a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/method/annotation/RequestPartMethodArgumentResolver.java b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/method/annotation/RequestPartMethodArgumentResolver.java index 6b1c1125c0a..f517cc03501 100644 --- a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/method/annotation/RequestPartMethodArgumentResolver.java +++ b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/method/annotation/RequestPartMethodArgumentResolver.java @@ -110,9 +110,7 @@ public class RequestPartMethodArgumentResolver extends AbstractMessageConverterM NativeWebRequest request, WebDataBinderFactory binderFactory) throws Exception { HttpServletRequest servletRequest = request.getNativeRequest(HttpServletRequest.class); - if (!isMultipartRequest(servletRequest)) { - throw new MultipartException("The current request is not a multipart request"); - } + assertIsMultipartRequest(servletRequest); MultipartHttpServletRequest multipartRequest = WebUtils.getNativeRequest(servletRequest, MultipartHttpServletRequest.class); @@ -166,12 +164,11 @@ public class RequestPartMethodArgumentResolver extends AbstractMessageConverterM return arg; } - private boolean isMultipartRequest(HttpServletRequest request) { - if (!"post".equals(request.getMethod().toLowerCase())) { - return false; - } + private static void assertIsMultipartRequest(HttpServletRequest request) { String contentType = request.getContentType(); - return (contentType != null && contentType.toLowerCase().startsWith("multipart/")); + if (contentType == null || !contentType.toLowerCase().startsWith("multipart/")) { + throw new MultipartException("The current request is not a multipart request"); + } } private String getPartName(MethodParameter parameter) { diff --git a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestPartMethodArgumentResolverTests.java b/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestPartMethodArgumentResolverTests.java index afbaf311d3c..94b18935fa2 100644 --- a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestPartMethodArgumentResolverTests.java +++ b/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestPartMethodArgumentResolverTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2011 the original author or authors. + * Copyright 2002-2012 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -227,12 +227,22 @@ public class RequestPartMethodArgumentResolverTests { } @Test(expected=MultipartException.class) - public void notMultipartRequest() throws Exception { + public void isMultipartRequest() throws Exception { MockHttpServletRequest request = new MockHttpServletRequest(); resolver.resolveArgument(paramMultipartFile, new ModelAndViewContainer(), new ServletWebRequest(request), null); fail("Expected exception"); } + // SPR-9079 + + @Test + public void isMultipartRequestPut() throws Exception { + this.multipartRequest.setMethod("PUT"); + Object actual = resolver.resolveArgument(paramMultipartFile, null, webRequest, null); + assertNotNull(actual); + assertSame(multipartFile1, actual); + } + private void testResolveArgument(SimpleBean argValue, MethodParameter parameter) throws IOException, Exception { MediaType contentType = MediaType.TEXT_PLAIN; diff --git a/org.springframework.web/src/main/java/org/springframework/web/method/annotation/RequestParamMethodArgumentResolver.java b/org.springframework.web/src/main/java/org/springframework/web/method/annotation/RequestParamMethodArgumentResolver.java index c659d699d29..0fb76c9bfb4 100644 --- a/org.springframework.web/src/main/java/org/springframework/web/method/annotation/RequestParamMethodArgumentResolver.java +++ b/org.springframework.web/src/main/java/org/springframework/web/method/annotation/RequestParamMethodArgumentResolver.java @@ -179,17 +179,10 @@ public class RequestParamMethodArgumentResolver extends AbstractNamedValueMethod } private void assertIsMultipartRequest(HttpServletRequest request) { - if (!isMultipartRequest(request)) { - throw new MultipartException("The current request is not a multipart request."); - } - } - - private boolean isMultipartRequest(HttpServletRequest request) { - if (!"post".equals(request.getMethod().toLowerCase())) { - return false; - } String contentType = request.getContentType(); - return (contentType != null && contentType.toLowerCase().startsWith("multipart/")); + if (contentType == null || !contentType.toLowerCase().startsWith("multipart/")) { + throw new MultipartException("The current request is not a multipart request"); + } } private boolean isMultipartFileCollection(MethodParameter parameter) { diff --git a/org.springframework.web/src/test/java/org/springframework/web/method/annotation/RequestParamMethodArgumentResolverTests.java b/org.springframework.web/src/test/java/org/springframework/web/method/annotation/RequestParamMethodArgumentResolverTests.java index 38af0e20a12..fe4099c6811 100644 --- a/org.springframework.web/src/test/java/org/springframework/web/method/annotation/RequestParamMethodArgumentResolverTests.java +++ b/org.springframework.web/src/test/java/org/springframework/web/method/annotation/RequestParamMethodArgumentResolverTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2011 the original author or authors. + * Copyright 2002-2012 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -45,7 +45,6 @@ import org.springframework.web.bind.annotation.RequestParam; import org.springframework.web.bind.annotation.RequestPart; import org.springframework.web.context.request.NativeWebRequest; import org.springframework.web.context.request.ServletWebRequest; -import org.springframework.web.method.annotation.RequestParamMethodArgumentResolver; import org.springframework.web.multipart.MultipartException; import org.springframework.web.multipart.MultipartFile; @@ -183,11 +182,27 @@ public class RequestParamMethodArgumentResolverTests { } @Test(expected = MultipartException.class) - public void notMultipartRequest() throws Exception { + public void isMultipartRequest() throws Exception { resolver.resolveArgument(paramMultiPartFile, null, webRequest, null); fail("Expected exception: request is not a multipart request"); } + // SPR-9079 + + @Test + public void isMultipartRequestHttpPut() throws Exception { + MockMultipartHttpServletRequest request = new MockMultipartHttpServletRequest(); + MultipartFile expected = new MockMultipartFile("multipartFileList", "Hello World".getBytes()); + request.addFile(expected); + request.setMethod("PUT"); + webRequest = new ServletWebRequest(request); + + Object actual = resolver.resolveArgument(paramMultipartFileList, null, webRequest, null); + + assertTrue(actual instanceof List); + assertEquals(expected, ((List) actual).get(0)); + } + @Test(expected = IllegalArgumentException.class) public void missingMultipartFile() throws Exception { request.setMethod("POST"); From 95683f5137f22d432135be3cf04e78a0bfeee8b4 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Wed, 1 Feb 2012 18:54:08 -0500 Subject: [PATCH 04/61] SPR-9075 Add fromRequestUri() methods to ServletUriComponentsBuilder --- .../support/ServletUriComponentsBuilder.java | 66 ++++++++++++------- .../ServletUriComponentsBuilderTests.java | 21 +++--- 2 files changed, 54 insertions(+), 33 deletions(-) diff --git a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/support/ServletUriComponentsBuilder.java b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/support/ServletUriComponentsBuilder.java index 818653ec7d5..8aa58643900 100644 --- a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/support/ServletUriComponentsBuilder.java +++ b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/support/ServletUriComponentsBuilder.java @@ -23,13 +23,11 @@ import org.springframework.util.StringUtils; import org.springframework.web.context.request.RequestAttributes; import org.springframework.web.context.request.RequestContextHolder; import org.springframework.web.context.request.ServletRequestAttributes; -import org.springframework.web.util.UriComponents; import org.springframework.web.util.UriComponentsBuilder; import org.springframework.web.util.UrlPathHelper; /** - * A builder for {@link UriComponents} that offers static factory methods to - * extract information from an {@code HttpServletRequest}. + * A UriComponentsBuilder that extracts information from an HttpServletRequest. * * @author Rossen Stoyanchev * @since 3.1 @@ -50,24 +48,25 @@ public class ServletUriComponentsBuilder extends UriComponentsBuilder { } /** - * Return a builder initialized with the host, port, scheme, and the - * context path of the given request. + * Prepare a builder from the host, port, scheme, and context path of + * an HttpServletRequest. */ public static ServletUriComponentsBuilder fromContextPath(HttpServletRequest request) { ServletUriComponentsBuilder builder = fromRequest(request); - builder.replacePath(new UrlPathHelper().getContextPath(request)); + builder.replacePath(request.getContextPath()); builder.replaceQuery(null); return builder; } /** - * Return a builder initialized with the host, port, scheme, context path, - * and the servlet mapping of the given request. - * - *

    For example if the servlet is mapped by name, i.e. {@code "/main/*"}, - * then the resulting path will be {@code /appContext/main}. If the servlet - * path is not mapped by name, i.e. {@code "/"} or {@code "*.html"}, then - * the resulting path will contain the context path only. + * Prepare a builder from the host, port, scheme, context path, and + * servlet mapping of an HttpServletRequest. The results may vary depending + * on the type of servlet mapping used. + * + *

    If the servlet is mapped by name, e.g. {@code "/main/*"}, the path + * will end with "/main". If the servlet is mapped otherwise, e.g. + * {@code "/"} or {@code "*.do"}, the result will be the same as + * if calling {@link #fromContextPath(HttpServletRequest)}. */ public static ServletUriComponentsBuilder fromServletMapping(HttpServletRequest request) { ServletUriComponentsBuilder builder = fromContextPath(request); @@ -78,8 +77,19 @@ public class ServletUriComponentsBuilder extends UriComponentsBuilder { } /** - * Return a builder initialized with all available information in the given - * request including scheme, host, port, path, and query string. + * Prepare a builder from the host, port, scheme, and path of + * an HttpSevletRequest. + */ + public static ServletUriComponentsBuilder fromRequestUri(HttpServletRequest request) { + ServletUriComponentsBuilder builder = fromRequest(request); + builder.replacePath(request.getRequestURI()); + builder.replaceQuery(null); + return builder; + } + + /** + * Prepare a builder by copying the scheme, host, port, path, and + * query string of an HttpServletRequest. */ public static ServletUriComponentsBuilder fromRequest(HttpServletRequest request) { String scheme = request.getScheme(); @@ -91,30 +101,38 @@ public class ServletUriComponentsBuilder extends UriComponentsBuilder { if ((scheme.equals("http") && port != 80) || (scheme.equals("https") && port != 443)) { builder.port(port); } - builder.path(new UrlPathHelper().getRequestUri(request)); + builder.path(request.getRequestURI()); builder.query(request.getQueryString()); return builder; } - + /** - * Equivalent to {@link #fromContextPath(HttpServletRequest)} except the - * request is obtained via {@link RequestContextHolder}. + * Same as {@link #fromContextPath(HttpServletRequest)} except the + * request is obtained through {@link RequestContextHolder}. */ public static ServletUriComponentsBuilder fromCurrentContextPath() { return fromContextPath(getCurrentRequest()); } /** - * Equivalent to {@link #fromServletMapping(HttpServletRequest)} except the - * request is obtained via {@link RequestContextHolder}. + * Same as {@link #fromServletMapping(HttpServletRequest)} except the + * request is obtained through {@link RequestContextHolder}. */ public static ServletUriComponentsBuilder fromCurrentServletMapping() { return fromServletMapping(getCurrentRequest()); } /** - * Equivalent to {@link #fromRequest(HttpServletRequest)} except the - * request is obtained via {@link RequestContextHolder}. + * Same as {@link #fromRequestUri(HttpServletRequest)} except the + * request is obtained through {@link RequestContextHolder}. + */ + public static ServletUriComponentsBuilder fromCurrentRequestUri() { + return fromRequestUri(getCurrentRequest()); + } + + /** + * Same as {@link #fromRequest(HttpServletRequest)} except the + * request is obtained through {@link RequestContextHolder}. */ public static ServletUriComponentsBuilder fromCurrentRequest() { return fromRequest(getCurrentRequest()); @@ -128,5 +146,5 @@ public class ServletUriComponentsBuilder extends UriComponentsBuilder { Assert.state(servletRequest != null, "Could not find current HttpServletRequest"); return servletRequest; } - + } diff --git a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/support/ServletUriComponentsBuilderTests.java b/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/support/ServletUriComponentsBuilderTests.java index cd1798c242b..8b11249e882 100644 --- a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/support/ServletUriComponentsBuilderTests.java +++ b/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/support/ServletUriComponentsBuilderTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2011 the original author or authors. + * Copyright 2002-2012 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -44,7 +44,6 @@ public class ServletUriComponentsBuilderTests { public void fromRequest() { request.setRequestURI("/mvc-showcase/data/param"); request.setQueryString("foo=123"); - String result = ServletUriComponentsBuilder.fromRequest(request).build().toUriString(); assertEquals("http://localhost/mvc-showcase/data/param?foo=123", result); @@ -52,11 +51,10 @@ public class ServletUriComponentsBuilderTests { @Test public void fromRequestEncodedPath() { - request.setRequestURI("/mvc-showcase/data/foo%20bar;jsessionid=123"); - + request.setRequestURI("/mvc-showcase/data/foo%20bar"); String result = ServletUriComponentsBuilder.fromRequest(request).build().toUriString(); - assertEquals("http://localhost/mvc-showcase/data/foo bar", result); + assertEquals("http://localhost/mvc-showcase/data/foo%20bar", result); } @Test @@ -71,17 +69,24 @@ public class ServletUriComponentsBuilderTests { public void fromRequestAtypicalHttpsPort() { request.setScheme("https"); request.setServerPort(9043); - String result = ServletUriComponentsBuilder.fromRequest(request).build().toUriString(); assertEquals("https://localhost:9043", result); } + @Test + public void fromRequestUri() { + request.setRequestURI("/mvc-showcase/data/param"); + request.setQueryString("foo=123"); + String result = ServletUriComponentsBuilder.fromRequestUri(request).build().toUriString(); + + assertEquals("http://localhost/mvc-showcase/data/param", result); + } + @Test public void fromContextPath() { request.setRequestURI("/mvc-showcase/data/param"); request.setQueryString("foo=123"); - String result = ServletUriComponentsBuilder.fromContextPath(request).build().toUriString(); assertEquals("http://localhost/mvc-showcase", result); @@ -92,7 +97,6 @@ public class ServletUriComponentsBuilderTests { request.setRequestURI("/mvc-showcase/app/simple"); request.setServletPath("/app"); request.setQueryString("foo=123"); - String result = ServletUriComponentsBuilder.fromServletMapping(request).build().toUriString(); assertEquals("http://localhost/mvc-showcase/app", result); @@ -103,7 +107,6 @@ public class ServletUriComponentsBuilderTests { request.setRequestURI("/mvc-showcase/data/param"); request.setQueryString("foo=123"); RequestContextHolder.setRequestAttributes(new ServletRequestAttributes(this.request)); - try { String result = ServletUriComponentsBuilder.fromCurrentRequest().build().toUriString(); From 8530828eb474702673094e719d72d834cb45080f Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Wed, 1 Feb 2012 19:12:52 -0500 Subject: [PATCH 05/61] SPR-9076 Add normalize() method to UriComponents. --- .../web/util/UriComponents.java | 18 ++++++++++++++---- .../web/util/UriComponentsTests.java | 8 +++++++- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/org.springframework.web/src/main/java/org/springframework/web/util/UriComponents.java b/org.springframework.web/src/main/java/org/springframework/web/util/UriComponents.java index a5b4ec74b9c..6123223e792 100644 --- a/org.springframework.web/src/main/java/org/springframework/web/util/UriComponents.java +++ b/org.springframework.web/src/main/java/org/springframework/web/util/UriComponents.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2011 the original author or authors. + * Copyright 2002-2012 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -286,7 +286,7 @@ public final class UriComponents { if (source == null) { return null; } - + Assert.hasLength(encoding, "'encoding' must not be empty"); byte[] bytes = encodeBytes(source.getBytes(encoding), type); @@ -406,7 +406,7 @@ public final class UriComponents { private UriComponents expandInternal(UriTemplateVariables uriVariables) { Assert.state(!encoded, "Cannot expand an already encoded UriComponents object"); - + String expandedScheme = expandUriComponent(this.scheme, uriVariables); String expandedUserInfo = expandUriComponent(this.userInfo, uriVariables); String expandedHost = expandUriComponent(this.host, uriVariables); @@ -458,6 +458,16 @@ public final class UriComponents { return variableValue != null ? variableValue.toString() : ""; } + /** + * Normalize the path removing sequences like "path/..". + * @see StringUtils#cleanPath(String) + */ + public UriComponents normalize() { + String normalizedPath = StringUtils.cleanPath(getPath()); + return new UriComponents(scheme, userInfo, host, this.port, new FullPathComponent(normalizedPath), + queryParams, fragment, encoded, false); + } + // other functionality /** @@ -930,7 +940,7 @@ public final class UriComponents { } } - + /** * Represents an empty path. diff --git a/org.springframework.web/src/test/java/org/springframework/web/util/UriComponentsTests.java b/org.springframework.web/src/test/java/org/springframework/web/util/UriComponentsTests.java index 00049b00c8b..234a21c0904 100644 --- a/org.springframework.web/src/test/java/org/springframework/web/util/UriComponentsTests.java +++ b/org.springframework.web/src/test/java/org/springframework/web/util/UriComponentsTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2011 the original author or authors. + * Copyright 2002-2012 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -69,4 +69,10 @@ public class UriComponentsTests { UriComponentsBuilder.fromPath("/fo%2o").build(true); } + @Test + public void normalize() { + UriComponents uriComponents = UriComponentsBuilder.fromUriString("http://example.com/foo/../bar").build(); + assertEquals("http://example.com/bar", uriComponents.normalize().toString()); + } + } From 010abd06e351686dc035fcb4f8fdd802990b8c9d Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Wed, 1 Feb 2012 19:51:00 -0500 Subject: [PATCH 06/61] SPR-9077 Remove empty path segments from input to UriComponentsBuilder. --- .../resources/changelog.txt | 3 ++ .../web/util/UriComponentsBuilder.java | 48 +++++++++++-------- .../web/util/UriComponentsBuilderTests.java | 25 ++++++---- 3 files changed, 49 insertions(+), 27 deletions(-) diff --git a/build-spring-framework/resources/changelog.txt b/build-spring-framework/resources/changelog.txt index 0597b8d2357..ae57f45e774 100644 --- a/build-spring-framework/resources/changelog.txt +++ b/build-spring-framework/resources/changelog.txt @@ -31,6 +31,9 @@ Changes in version 3.1.1 (2012-02-06) * fix request mapping bug involving direct vs pattern path matches with HTTP methods * revise the FlashMapManager contract and implemenation to address a flaw in its design * Remove check for HTTP "POST" when resolving multipart request controller method arguments +* add normalize() method to UriComponents +* remove empty path segments from input to UriComponentsBuilder +* add fromRequestUri(request) and fromCurrentRequestUri() methods to ServletUriCommonentsBuilder Changes in version 3.1 GA (2011-12-12) -------------------------------------- diff --git a/org.springframework.web/src/main/java/org/springframework/web/util/UriComponentsBuilder.java b/org.springframework.web/src/main/java/org/springframework/web/util/UriComponentsBuilder.java index f4ed1f252e1..c95d620256e 100644 --- a/org.springframework.web/src/main/java/org/springframework/web/util/UriComponentsBuilder.java +++ b/org.springframework.web/src/main/java/org/springframework/web/util/UriComponentsBuilder.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2011 the original author or authors. + * Copyright 2002-2012 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -18,7 +18,7 @@ package org.springframework.web.util; import java.net.URI; import java.util.ArrayList; -import java.util.Collections; +import java.util.Collection; import java.util.List; import java.util.Map; import java.util.regex.Matcher; @@ -80,7 +80,7 @@ public class UriComponentsBuilder { "^" + HTTP_PATTERN + "(//(" + USERINFO_PATTERN + "@)?" + HOST_PATTERN + "(:" + PORT_PATTERN + ")?" + ")?" + PATH_PATTERN + "(\\?" + LAST_PATTERN + ")?"); - + private String scheme; private String userInfo; @@ -223,10 +223,10 @@ public class UriComponentsBuilder { } /** - * Builds a {@code UriComponents} instance and replaces URI template variables - * with the values from a map. This is a shortcut method, which combines - * calls to {@link #build()} and then {@link UriComponents#expand(Map)}. - * + * Builds a {@code UriComponents} instance and replaces URI template variables + * with the values from a map. This is a shortcut method, which combines + * calls to {@link #build()} and then {@link UriComponents#expand(Map)}. + * * @param uriVariables the map of URI variables * @return the URI components with expanded values */ @@ -235,17 +235,17 @@ public class UriComponentsBuilder { } /** - * Builds a {@code UriComponents} instance and replaces URI template variables - * with the values from an array. This is a shortcut method, which combines - * calls to {@link #build()} and then {@link UriComponents#expand(Object...)}. - * + * Builds a {@code UriComponents} instance and replaces URI template variables + * with the values from an array. This is a shortcut method, which combines + * calls to {@link #build()} and then {@link UriComponents#expand(Object...)}. + * * @param uriVariableValues URI variable values * @return the URI components with expanded values */ public UriComponents buildAndExpand(Object... uriVariableValues) { return build(false).expand(uriVariableValues); } - + // URI components methods /** @@ -347,8 +347,8 @@ public class UriComponentsBuilder { } /** - * Sets the path of this builder overriding all existing path and path segment values. - * + * Sets the path of this builder overriding all existing path and path segment values. + * * @param path the URI path; a {@code null} value results in an empty path. * @return this UriComponentsBuilder */ @@ -394,7 +394,7 @@ public class UriComponentsBuilder { /** * Sets the query of this builder overriding all existing query parameters. - * + * * @param query the query string; a {@code null} value removes all query parameters. * @return this UriComponentsBuilder */ @@ -430,7 +430,7 @@ public class UriComponentsBuilder { /** * Sets the query parameter values overriding all existing query values for the same parameter. * If no values are given, the query parameter is removed. - * + * * @param name the query parameter name * @param values the query parameter values * @return this UriComponentsBuilder @@ -443,7 +443,7 @@ public class UriComponentsBuilder { } return this; } - + /** * Sets the URI fragment. The given fragment may contain URI template variables, and may also be {@code null} to clear * the fragment of this builder. @@ -509,7 +509,17 @@ public class UriComponentsBuilder { private final List pathSegments = new ArrayList(); private PathSegmentComponentBuilder(String... pathSegments) { - Collections.addAll(this.pathSegments, pathSegments); + this.pathSegments.addAll(removeEmptyPathSegments(pathSegments)); + } + + private Collection removeEmptyPathSegments(String... pathSegments) { + List result = new ArrayList(); + for (String segment : pathSegments) { + if (StringUtils.hasText(segment)) { + result.add(segment); + } + } + return result; } public UriComponents.PathComponent build() { @@ -523,7 +533,7 @@ public class UriComponentsBuilder { } public PathComponentBuilder appendPathSegments(String... pathSegments) { - Collections.addAll(this.pathSegments, pathSegments); + this.pathSegments.addAll(removeEmptyPathSegments(pathSegments)); return this; } } diff --git a/org.springframework.web/src/test/java/org/springframework/web/util/UriComponentsBuilderTests.java b/org.springframework.web/src/test/java/org/springframework/web/util/UriComponentsBuilderTests.java index 303bcd2d157..2d9323ca8d6 100644 --- a/org.springframework.web/src/test/java/org/springframework/web/util/UriComponentsBuilderTests.java +++ b/org.springframework.web/src/test/java/org/springframework/web/util/UriComponentsBuilderTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2011 the original author or authors. + * Copyright 2002-2012 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -45,7 +45,7 @@ public class UriComponentsBuilderTests { URI expected = new URI("http://example.com/foo?bar#baz"); assertEquals("Invalid result URI", expected, result.toUri()); } - + @Test public void fromPath() throws URISyntaxException { UriComponents result = UriComponentsBuilder.fromPath("foo").queryParam("bar").fragment("baz").build(); @@ -176,6 +176,15 @@ public class UriComponentsBuilderTests { assertEquals(Arrays.asList("foo"), result.getPathSegments()); } + @Test + public void pathSegmentsSomeEmpty() { + UriComponentsBuilder builder = UriComponentsBuilder.newInstance().pathSegment("", "foo", "", "bar"); + UriComponents result = builder.build(); + + assertEquals("/foo/bar", result.getPath()); + assertEquals(Arrays.asList("foo", "bar"), result.getPathSegments()); + } + @Test public void replacePath() { UriComponentsBuilder builder = UriComponentsBuilder.fromUriString("http://www.ietf.org/rfc/rfc2396.txt"); @@ -183,26 +192,26 @@ public class UriComponentsBuilderTests { UriComponents result = builder.build(); assertEquals("http://www.ietf.org/rfc/rfc3986.txt", result.toUriString()); - + builder = UriComponentsBuilder.fromUriString("http://www.ietf.org/rfc/rfc2396.txt"); builder.replacePath(null); result = builder.build(); assertEquals("http://www.ietf.org", result.toUriString()); } - + @Test public void replaceQuery() { UriComponentsBuilder builder = UriComponentsBuilder.fromUriString("http://example.com/foo?foo=bar&baz=qux"); builder.replaceQuery("baz=42"); UriComponents result = builder.build(); - + assertEquals("http://example.com/foo?baz=42", result.toUriString()); builder = UriComponentsBuilder.fromUriString("http://example.com/foo?foo=bar&baz=qux"); builder.replaceQuery(null); result = builder.build(); - + assertEquals("http://example.com/foo", result.toUriString()); } @@ -234,13 +243,13 @@ public class UriComponentsBuilderTests { UriComponentsBuilder builder = UriComponentsBuilder.newInstance().queryParam("baz", "qux", 42); builder.replaceQueryParam("baz", "xuq", 24); UriComponents result = builder.build(); - + assertEquals("baz=xuq&baz=24", result.getQuery()); builder = UriComponentsBuilder.newInstance().queryParam("baz", "qux", 42); builder.replaceQueryParam("baz"); result = builder.build(); - + assertNull("Query param should have been deleted", result.getQuery()); } From f3f73f0e32dc54c463831668d4700f022bc8461f Mon Sep 17 00:00:00 2001 From: Chris Beams Date: Tue, 10 Jan 2012 14:30:16 +0100 Subject: [PATCH 07/61] Check original beanClass in #isFactoryBean calls This issue originates from a need in Spring Data JPA, wherein a custom InstantiationAwareBeanPostProcessor may alter the predicted type of FactoryBean objects, effectively preventing retrieval of those beans via calls to #getBeansOfType(FactoryBean.class). The reason for this "masking effect" is that prior to this change, the implementation of AbstractBeanFactory#isFactoryBean considered only the "predicted type" returned from #predictBeanType when evaluating assignability to FactoryBean.class The implementation of #isFactoryBean now ensures that not only the predicted bean type is considered, but also the original bean definition's beanClass (if one is available). Issue: SPR-8954 --- .../factory/support/AbstractBeanFactory.java | 7 +- .../beans/factory/support/Spr8954Tests.java | 96 +++++++++++++++++++ 2 files changed, 100 insertions(+), 3 deletions(-) create mode 100644 org.springframework.beans/src/test/java/org/springframework/beans/factory/support/Spr8954Tests.java diff --git a/org.springframework.beans/src/main/java/org/springframework/beans/factory/support/AbstractBeanFactory.java b/org.springframework.beans/src/main/java/org/springframework/beans/factory/support/AbstractBeanFactory.java index 783945fe49a..296577b9304 100644 --- a/org.springframework.beans/src/main/java/org/springframework/beans/factory/support/AbstractBeanFactory.java +++ b/org.springframework.beans/src/main/java/org/springframework/beans/factory/support/AbstractBeanFactory.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2011 the original author or authors. + * Copyright 2002-2012 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -1328,8 +1328,9 @@ public abstract class AbstractBeanFactory extends FactoryBeanRegistrySupport imp * @param mbd the corresponding bean definition */ protected boolean isFactoryBean(String beanName, RootBeanDefinition mbd) { - Class beanClass = predictBeanType(beanName, mbd, FactoryBean.class); - return (beanClass != null && FactoryBean.class.isAssignableFrom(beanClass)); + Class predictedType = predictBeanType(beanName, mbd, FactoryBean.class); + return (predictedType != null && FactoryBean.class.isAssignableFrom(predictedType)) || + (mbd.hasBeanClass() && FactoryBean.class.isAssignableFrom(mbd.getBeanClass())); } /** diff --git a/org.springframework.beans/src/test/java/org/springframework/beans/factory/support/Spr8954Tests.java b/org.springframework.beans/src/test/java/org/springframework/beans/factory/support/Spr8954Tests.java new file mode 100644 index 00000000000..0cd040b4695 --- /dev/null +++ b/org.springframework.beans/src/test/java/org/springframework/beans/factory/support/Spr8954Tests.java @@ -0,0 +1,96 @@ +/* + * Copyright 2002-2012 the original author or authors. + * + * 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 org.springframework.beans.factory.support; + +import static org.hamcrest.CoreMatchers.*; +import static org.junit.Assert.*; + +import java.util.Map; + +import org.junit.Test; +import org.springframework.beans.factory.FactoryBean; +import org.springframework.beans.factory.config.InstantiationAwareBeanPostProcessorAdapter; +import org.springframework.beans.factory.support.DefaultListableBeanFactory; +import org.springframework.beans.factory.support.RootBeanDefinition; + +/** + * Unit tests for SPR-8954, in which a custom {@link InstantiationAwareBeanPostProcessor} + * forces the predicted type of a FactoryBean, effectively preventing retrieval of the + * bean from calls to #getBeansOfType(FactoryBean.class). The implementation of + * {@link AbstractBeanFactory#isFactoryBean(String, RootBeanDefinition)} now ensures + * that not only the predicted bean type is considered, but also the original bean + * definition's beanClass. + * + * @author Chris Beams + * @author Oliver Gierke + */ +public class Spr8954Tests { + + @Test + public void repro() { + DefaultListableBeanFactory bf = new DefaultListableBeanFactory(); + bf.registerBeanDefinition("foo", new RootBeanDefinition(FooFactoryBean.class)); + bf.addBeanPostProcessor(new PredictingBPP()); + + assertThat(bf.getBean("foo"), instanceOf(Foo.class)); + assertThat(bf.getBean("&foo"), instanceOf(FooFactoryBean.class)); + + @SuppressWarnings("rawtypes") + Map fbBeans = bf.getBeansOfType(FactoryBean.class); + assertThat(1, equalTo(fbBeans.size())); + assertThat("&foo", equalTo(fbBeans.keySet().iterator().next())); + + Map aiBeans = bf.getBeansOfType(AnInterface.class); + assertThat(1, equalTo(aiBeans.size())); + assertThat("&foo", equalTo(aiBeans.keySet().iterator().next())); + } + + static class FooFactoryBean implements FactoryBean, AnInterface { + + public Foo getObject() throws Exception { + return new Foo(); + } + + public Class getObjectType() { + return Foo.class; + } + + public boolean isSingleton() { + return true; + } + } + + interface AnInterface { + } + + static class Foo { + } + + interface PredictedType { + + } + + static class PredictingBPP extends InstantiationAwareBeanPostProcessorAdapter { + + @Override + public Class predictBeanType(Class beanClass, String beanName) { + return FactoryBean.class.isAssignableFrom(beanClass) ? + PredictedType.class : + super.predictBeanType(beanClass, beanName); + } + } +} From 17cf465d239605632fee99d8c869ea5fd0bee14c Mon Sep 17 00:00:00 2001 From: Chris Beams Date: Mon, 23 Jan 2012 11:15:13 +0100 Subject: [PATCH 08/61] Fix method equality bug in ExtendedBeanInfo A number of users reported issues with comparing method identity vs equivalence when discovering JavaBeans property methods in ExtendedBeanInfo. This commit updates the implementation to consistently use '.equals()' instead of '=='. Issue: SPR-8079, SPR-8347 --- .../main/java/org/springframework/beans/ExtendedBeanInfo.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/org.springframework.beans/src/main/java/org/springframework/beans/ExtendedBeanInfo.java b/org.springframework.beans/src/main/java/org/springframework/beans/ExtendedBeanInfo.java index 1b63f37a3d2..205cae1c179 100644 --- a/org.springframework.beans/src/main/java/org/springframework/beans/ExtendedBeanInfo.java +++ b/org.springframework.beans/src/main/java/org/springframework/beans/ExtendedBeanInfo.java @@ -170,8 +170,8 @@ class ExtendedBeanInfo implements BeanInfo { continue ALL_METHODS; } } - if (method == pd.getReadMethod() - || (pd instanceof IndexedPropertyDescriptor && method == ((IndexedPropertyDescriptor) pd).getIndexedReadMethod())) { + if (method.equals(pd.getReadMethod()) + || (pd instanceof IndexedPropertyDescriptor && method.equals(((IndexedPropertyDescriptor) pd).getIndexedReadMethod()))) { // yes -> copy it, including corresponding setter method (if any -- may be null) if (pd instanceof IndexedPropertyDescriptor) { this.addOrUpdatePropertyDescriptor(pd, pd.getName(), pd.getReadMethod(), pd.getWriteMethod(), ((IndexedPropertyDescriptor)pd).getIndexedReadMethod(), ((IndexedPropertyDescriptor)pd).getIndexedWriteMethod()); From 6e5cc53fc9dc48daeadbda1d2056c791dc83c245 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Thu, 2 Feb 2012 09:56:02 -0500 Subject: [PATCH 09/61] SPR-9085 Correct typo. --- spring-framework-reference/src/remoting.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spring-framework-reference/src/remoting.xml b/spring-framework-reference/src/remoting.xml index d26c1b61540..b4896b241c9 100644 --- a/spring-framework-reference/src/remoting.xml +++ b/spring-framework-reference/src/remoting.xml @@ -1579,7 +1579,7 @@ String body = response.getBody();

    - ByteArrayMessageConverter + ByteArrayHttpMessageConverter An HttpMessageConverter implementation that can read and write byte arrays from the HTTP From 81e25b91c20a6a6d28c3935912bbe76e93ba103c Mon Sep 17 00:00:00 2001 From: Chris Beams Date: Fri, 3 Feb 2012 17:20:10 +0100 Subject: [PATCH 10/61] Respect @Configuration(value) for @Imported classes Prior to this commit, @Configuration classes included via @Import (or via automatic registration of nested configuration classes) would always be registered with a generated bean name, regardless of whether the user had specified a 'value' indicating a customized bean name, e.g. @Configuration("myConfig") public class AppConfig { ... } Now this bean name is propagated as intended in all cases, meaning that in the example above, the resulting bean definition of type AppConfig will be named "myConfig" regardless how it was registered with the container -- directly against the application context, via component scanning, via @Import, or via automatic registration of nested configuration classes. Issue: SPR-9023 --- .../annotation/ConfigurationClass.java | 58 ++++++++++++++++++- ...onfigurationClassBeanDefinitionReader.java | 37 +++++++----- .../annotation/ConfigurationClassParser.java | 7 +-- .../AbstractCircularImportDetectionTests.java | 4 +- .../annotation/configuration/ImportTests.java | 40 ++++++++++--- 5 files changed, 119 insertions(+), 27 deletions(-) diff --git a/org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClass.java b/org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClass.java index e7a8baa24e1..7374bc4b342 100644 --- a/org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClass.java +++ b/org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClass.java @@ -30,6 +30,7 @@ import org.springframework.core.io.Resource; import org.springframework.core.type.AnnotationMetadata; import org.springframework.core.type.StandardAnnotationMetadata; import org.springframework.core.type.classreading.MetadataReader; +import org.springframework.util.Assert; import org.springframework.util.ClassUtils; /** @@ -55,19 +56,66 @@ final class ConfigurationClass { private String beanName; + private final boolean imported; + + /** + * Create a new {@link ConfigurationClass} with the given name. + * @param metadataReader reader used to parse the underlying {@link Class} + * @param beanName must not be {@code null} + * @throws IllegalArgumentException if beanName is null (as of Spring 3.1.1) + * @see ConfigurationClass#ConfigurationClass(Class, boolean) + */ public ConfigurationClass(MetadataReader metadataReader, String beanName) { + Assert.hasText(beanName, "bean name must not be null"); this.metadata = metadataReader.getAnnotationMetadata(); this.resource = metadataReader.getResource(); this.beanName = beanName; + this.imported = false; } + /** + * Create a new {@link ConfigurationClass} representing a class that was imported + * using the {@link Import} annotation or automatically processed as a nested + * configuration class (if imported is {@code true}). + * @param metadataReader reader used to parse the underlying {@link Class} + * @param beanName name of the {@code @Configuration} class bean + * @since 3.1.1 + */ + public ConfigurationClass(MetadataReader metadataReader, boolean imported) { + this.metadata = metadataReader.getAnnotationMetadata(); + this.resource = metadataReader.getResource(); + this.imported = imported; + } + + /** + * Create a new {@link ConfigurationClass} with the given name. + * @param clazz the underlying {@link Class} to represent + * @param beanName name of the {@code @Configuration} class bean + * @throws IllegalArgumentException if beanName is null (as of Spring 3.1.1) + * @see ConfigurationClass#ConfigurationClass(Class, boolean) + */ public ConfigurationClass(Class clazz, String beanName) { + Assert.hasText(beanName, "bean name must not be null"); this.metadata = new StandardAnnotationMetadata(clazz); this.resource = new DescriptiveResource(clazz.toString()); this.beanName = beanName; + this.imported = false; } + /** + * Create a new {@link ConfigurationClass} representing a class that was imported + * using the {@link Import} annotation or automatically processed as a nested + * configuration class (if imported is {@code true}). + * @param clazz the underlying {@link Class} to represent + * @param beanName name of the {@code @Configuration} class bean + * @since 3.1.1 + */ + public ConfigurationClass(Class clazz, boolean imported) { + this.metadata = new StandardAnnotationMetadata(clazz); + this.resource = new DescriptiveResource(clazz.toString()); + this.imported = imported; + } public AnnotationMetadata getMetadata() { return this.metadata; @@ -81,6 +129,15 @@ final class ConfigurationClass { return ClassUtils.getShortName(getMetadata().getClassName()); } + /** + * Return whether this configuration class was registered via @{@link Import} or + * automatically registered due to being nested within another configuration class. + * @since 3.1.1 + */ + public boolean isImported() { + return this.imported; + } + public void setBeanName(String beanName) { this.beanName = beanName; } @@ -181,5 +238,4 @@ final class ConfigurationClass { } } - } diff --git a/org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClassBeanDefinitionReader.java b/org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClassBeanDefinitionReader.java index 1ed1d786df4..f2e437bf057 100644 --- a/org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClassBeanDefinitionReader.java +++ b/org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClassBeanDefinitionReader.java @@ -122,32 +122,43 @@ class ConfigurationClassBeanDefinitionReader { * Register the {@link Configuration} class itself as a bean definition. */ private void doLoadBeanDefinitionForConfigurationClassIfNecessary(ConfigurationClass configClass) { - if (configClass.getBeanName() != null) { - // a bean definition already exists for this configuration class -> nothing to do + if (!configClass.isImported()) { return; } - // no bean definition exists yet -> this must be an imported configuration class (@Import). BeanDefinition configBeanDef = new GenericBeanDefinition(); String className = configClass.getMetadata().getClassName(); configBeanDef.setBeanClassName(className); + MetadataReader reader; + try { + reader = this.metadataReaderFactory.getMetadataReader(className); + } + catch (IOException ex) { + throw new IllegalStateException("Could not create MetadataReader for class " + className); + } if (ConfigurationClassUtils.checkConfigurationClassCandidate(configBeanDef, this.metadataReaderFactory)) { - String configBeanName = BeanDefinitionReaderUtils.registerWithGeneratedName((AbstractBeanDefinition)configBeanDef, this.registry); + Map configAttributes = + reader.getAnnotationMetadata().getAnnotationAttributes(Configuration.class.getName()); + + // has the 'value' attribute of @Configuration been set? + String configBeanName = (String) configAttributes.get("value"); + if (StringUtils.hasText(configBeanName)) { + // yes -> register the configuration class bean with this name + this.registry.registerBeanDefinition(configBeanName, configBeanDef); + } + else { + // no -> register the configuration class bean with a generated name + configBeanName = BeanDefinitionReaderUtils.registerWithGeneratedName((AbstractBeanDefinition)configBeanDef, this.registry); + } configClass.setBeanName(configBeanName); if (logger.isDebugEnabled()) { logger.debug(String.format("Registered bean definition for imported @Configuration class %s", configBeanName)); } } else { - try { - MetadataReader reader = this.metadataReaderFactory.getMetadataReader(className); - AnnotationMetadata metadata = reader.getAnnotationMetadata(); - this.problemReporter.error( - new InvalidConfigurationImportProblem(className, reader.getResource(), metadata)); - } - catch (IOException ex) { - throw new IllegalStateException("Could not create MetadataReader for class " + className); - } + AnnotationMetadata metadata = reader.getAnnotationMetadata(); + this.problemReporter.error( + new InvalidConfigurationImportProblem(className, reader.getResource(), metadata)); } } diff --git a/org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClassParser.java b/org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClassParser.java index 00528aa87d7..465826c531c 100644 --- a/org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClassParser.java +++ b/org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClassParser.java @@ -119,8 +119,7 @@ class ConfigurationClassParser { /** * Parse the specified {@link Configuration @Configuration} class. * @param clazz the Class to parse - * @param beanName may be null, but if populated represents the bean id - * (assumes that this configuration class was configured via XML) + * @param beanName must not be null (as of Spring 3.1.1) */ public void parse(Class clazz, String beanName) throws IOException { processConfigurationClass(new ConfigurationClass(clazz, beanName)); @@ -167,7 +166,7 @@ class ConfigurationClassParser { MetadataReader reader = this.metadataReaderFactory.getMetadataReader(memberClassName); AnnotationMetadata memberClassMetadata = reader.getAnnotationMetadata(); if (ConfigurationClassUtils.isConfigurationCandidate(memberClassMetadata)) { - processConfigurationClass(new ConfigurationClass(reader, null)); + processConfigurationClass(new ConfigurationClass(reader, true)); } } @@ -300,7 +299,7 @@ class ConfigurationClassParser { else { // the candidate class not an ImportSelector or ImportBeanDefinitionRegistrar -> process it as a @Configuration class this.importStack.registerImport(importingClassMetadata.getClassName(), candidate); - processConfigurationClass(new ConfigurationClass(reader, null)); + processConfigurationClass(new ConfigurationClass(reader, true)); } } this.importStack.pop(); diff --git a/org.springframework.context/src/test/java/org/springframework/context/annotation/AbstractCircularImportDetectionTests.java b/org.springframework.context/src/test/java/org/springframework/context/annotation/AbstractCircularImportDetectionTests.java index 1a02ed5297f..57d42c88c8e 100644 --- a/org.springframework.context/src/test/java/org/springframework/context/annotation/AbstractCircularImportDetectionTests.java +++ b/org.springframework.context/src/test/java/org/springframework/context/annotation/AbstractCircularImportDetectionTests.java @@ -42,7 +42,7 @@ public abstract class AbstractCircularImportDetectionTests { public void simpleCircularImportIsDetected() throws Exception { boolean threw = false; try { - newParser().parse(loadAsConfigurationSource(A.class), null); + newParser().parse(loadAsConfigurationSource(A.class), "A"); } catch (BeanDefinitionParsingException ex) { assertTrue("Wrong message. Got: " + ex.getMessage(), ex.getMessage().contains( @@ -59,7 +59,7 @@ public abstract class AbstractCircularImportDetectionTests { public void complexCircularImportIsDetected() throws Exception { boolean threw = false; try { - newParser().parse(loadAsConfigurationSource(X.class), null); + newParser().parse(loadAsConfigurationSource(X.class), "X"); } catch (BeanDefinitionParsingException ex) { assertTrue("Wrong message. Got: " + ex.getMessage(), diff --git a/org.springframework.context/src/test/java/org/springframework/context/annotation/configuration/ImportTests.java b/org.springframework.context/src/test/java/org/springframework/context/annotation/configuration/ImportTests.java index 8756425e146..1866ea80405 100644 --- a/org.springframework.context/src/test/java/org/springframework/context/annotation/configuration/ImportTests.java +++ b/org.springframework.context/src/test/java/org/springframework/context/annotation/configuration/ImportTests.java @@ -25,6 +25,7 @@ import test.beans.TestBean; import org.springframework.beans.factory.parsing.BeanDefinitionParsingException; import org.springframework.beans.factory.support.DefaultListableBeanFactory; import org.springframework.beans.factory.support.RootBeanDefinition; +import org.springframework.context.annotation.AnnotationConfigApplicationContext; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.context.annotation.ConfigurationClassPostProcessor; @@ -53,13 +54,6 @@ public class ImportTests { assertThat(beanFactory.getBeanDefinitionCount(), equalTo(expectedCount)); } - @Test - public void testProcessImports() { - int configClasses = 2; - int beansInClasses = 2; - assertBeanDefinitionCount((configClasses + beansInClasses), ConfigurationWithImportAnnotation.class); - } - @Test public void testProcessImportsWithAsm() { int configClasses = 2; @@ -315,4 +309,36 @@ public class ImportTests { static class ConfigAnnotated { } static class NonConfigAnnotated { } + + // ------------------------------------------------------------------------ + + /** + * Test that values supplied to @Configuration(value="...") are propagated as the + * bean name for the configuration class even in the case of inclusion via @Import + * or in the case of automatic registration via nesting + */ + @Test + public void reproSpr9023() { + AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(); + ctx.register(B.class); + ctx.refresh(); + System.out.println(ctx.getBeanFactory()); + assertThat(ctx.getBeanNamesForType(B.class)[0], is("config-b")); + assertThat(ctx.getBeanNamesForType(A.class)[0], is("config-a")); + } + + @Configuration("config-a") + static class A { } + + @Configuration("config-b") + @Import(A.class) + static class B { } + + @Test + public void testProcessImports() { + int configClasses = 2; + int beansInClasses = 2; + assertBeanDefinitionCount((configClasses + beansInClasses), ConfigurationWithImportAnnotation.class); + } + } From 871336a8c8cb53a4643b9ec64202bef58cb4c999 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Fri, 3 Feb 2012 12:14:36 -0500 Subject: [PATCH 11/61] Better support for @SessionAttributes in clustered environments A list of "known" session attributes (listed in @SessionAttributes) was gradually built as attributes get added to the model. In a failover scenario that knowledge is lost causing session attributes to be potentially re-initialized via @ModelAttribute methods. With this change @SessionAttributes listed by name are immediately added to he list of "known" session attributes thus this knowledge is not lost after a failover. Attributes listed by type however still must be discovered as they get added to the model. --- .../resources/changelog.txt | 1 + .../annotation/SessionAttributesHandler.java | 90 ++++++++++--------- .../SessionAttributesHandlerTests.java | 52 +++++------ 3 files changed, 73 insertions(+), 70 deletions(-) diff --git a/build-spring-framework/resources/changelog.txt b/build-spring-framework/resources/changelog.txt index ae57f45e774..b680d5faa62 100644 --- a/build-spring-framework/resources/changelog.txt +++ b/build-spring-framework/resources/changelog.txt @@ -34,6 +34,7 @@ Changes in version 3.1.1 (2012-02-06) * add normalize() method to UriComponents * remove empty path segments from input to UriComponentsBuilder * add fromRequestUri(request) and fromCurrentRequestUri() methods to ServletUriCommonentsBuilder +* improve @SessionAttributes handling to provide better support for clustered sessions Changes in version 3.1 GA (2011-12-12) -------------------------------------- diff --git a/org.springframework.web/src/main/java/org/springframework/web/method/annotation/SessionAttributesHandler.java b/org.springframework.web/src/main/java/org/springframework/web/method/annotation/SessionAttributesHandler.java index fd7b4e42cf6..5e6080db13e 100644 --- a/org.springframework.web/src/main/java/org/springframework/web/method/annotation/SessionAttributesHandler.java +++ b/org.springframework.web/src/main/java/org/springframework/web/method/annotation/SessionAttributesHandler.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2011 the original author or authors. + * Copyright 2002-2012 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -31,16 +31,16 @@ import org.springframework.web.bind.support.SessionStatus; import org.springframework.web.context.request.WebRequest; /** - * Manages controller-specific session attributes declared via - * {@link SessionAttributes @SessionAttributes}. Actual storage is - * performed via {@link SessionAttributeStore}. - * - *

    When a controller annotated with {@code @SessionAttributes} adds - * attributes to its model, those attributes are checked against names and - * types specified via {@code @SessionAttributes}. Matching model attributes - * are saved in the HTTP session and remain there until the controller calls + * Manages controller-specific session attributes declared via + * {@link SessionAttributes @SessionAttributes}. Actual storage is + * delegated to a {@link SessionAttributeStore} instance. + * + *

    When a controller annotated with {@code @SessionAttributes} adds + * attributes to its model, those attributes are checked against names and + * types specified via {@code @SessionAttributes}. Matching model attributes + * are saved in the HTTP session and remain there until the controller calls * {@link SessionStatus#setComplete()}. - * + * * @author Rossen Stoyanchev * @since 3.1 */ @@ -50,51 +50,53 @@ public class SessionAttributesHandler { private final Set> attributeTypes = new HashSet>(); - private final Set resolvedAttributeNames = Collections.synchronizedSet(new HashSet(4)); + private final Set knownAttributeNames = Collections.synchronizedSet(new HashSet(4)); private final SessionAttributeStore sessionAttributeStore; /** - * Creates a new instance for a controller type. Session attribute names/types - * are extracted from a type-level {@code @SessionAttributes} if found. + * Create a new instance for a controller type. Session attribute names and + * types are extracted from the {@code @SessionAttributes} annotation, if + * present, on the given type. * @param handlerType the controller type * @param sessionAttributeStore used for session access */ public SessionAttributesHandler(Class handlerType, SessionAttributeStore sessionAttributeStore) { Assert.notNull(sessionAttributeStore, "SessionAttributeStore may not be null."); this.sessionAttributeStore = sessionAttributeStore; - + SessionAttributes annotation = AnnotationUtils.findAnnotation(handlerType, SessionAttributes.class); if (annotation != null) { - this.attributeNames.addAll(Arrays.asList(annotation.value())); + this.attributeNames.addAll(Arrays.asList(annotation.value())); this.attributeTypes.addAll(Arrays.>asList(annotation.types())); - } + } + + this.knownAttributeNames.addAll(this.attributeNames); } /** - * Whether the controller represented by this instance has declared session - * attribute names or types of interest via {@link SessionAttributes}. + * Whether the controller represented by this instance has declared any + * session attributes through an {@link SessionAttributes} annotation. */ public boolean hasSessionAttributes() { - return ((this.attributeNames.size() > 0) || (this.attributeTypes.size() > 0)); + return ((this.attributeNames.size() > 0) || (this.attributeTypes.size() > 0)); } - + /** - * Whether the attribute name and/or type match those specified in the - * controller's {@code @SessionAttributes} annotation. - * + * Whether the attribute name or type match the names and types specified + * via {@code @SessionAttributes} in underlying controller. + * *

    Attributes successfully resolved through this method are "remembered" - * and used in {@link #retrieveAttributes(WebRequest)} and - * {@link #cleanupAttributes(WebRequest)}. In other words, retrieval and - * cleanup only affect attributes previously resolved through here. - * - * @param attributeName the attribute name to check; must not be null - * @param attributeType the type for the attribute; or {@code null} + * and subsequently used in {@link #retrieveAttributes(WebRequest)} and + * {@link #cleanupAttributes(WebRequest)}. + * + * @param attributeName the attribute name to check, never {@code null} + * @param attributeType the type for the attribute, possibly {@code null} */ public boolean isHandlerSessionAttribute(String attributeName, Class attributeType) { Assert.notNull(attributeName, "Attribute name must not be null"); if (this.attributeNames.contains(attributeName) || this.attributeTypes.contains(attributeType)) { - this.resolvedAttributeNames.add(attributeName); + this.knownAttributeNames.add(attributeName); return true; } else { @@ -103,8 +105,8 @@ public class SessionAttributesHandler { } /** - * Stores a subset of the given attributes in the session. Attributes not - * declared as session attributes via {@code @SessionAttributes} are ignored. + * Store a subset of the given attributes in the session. Attributes not + * declared as session attributes via {@code @SessionAttributes} are ignored. * @param request the current request * @param attributes candidate attributes for session storage */ @@ -112,23 +114,23 @@ public class SessionAttributesHandler { for (String name : attributes.keySet()) { Object value = attributes.get(name); Class attrType = (value != null) ? value.getClass() : null; - + if (isHandlerSessionAttribute(name, attrType)) { this.sessionAttributeStore.storeAttribute(request, name, value); } } } - + /** - * Retrieve "known" attributes from the session -- i.e. attributes listed - * in {@code @SessionAttributes} and previously stored in the in the model - * at least once. + * Retrieve "known" attributes from the session, i.e. attributes listed + * by name in {@code @SessionAttributes} or attributes previously stored + * in the model that matched by type. * @param request the current request - * @return a map with handler session attributes; possibly empty. + * @return a map with handler session attributes, possibly empty */ public Map retrieveAttributes(WebRequest request) { Map attributes = new HashMap(); - for (String name : this.resolvedAttributeNames) { + for (String name : this.knownAttributeNames) { Object value = this.sessionAttributeStore.retrieveAttribute(request, name); if (value != null) { attributes.put(name, value); @@ -138,13 +140,13 @@ public class SessionAttributesHandler { } /** - * Cleans "known" attributes from the session - i.e. attributes listed - * in {@code @SessionAttributes} and previously stored in the in the model - * at least once. + * Remove "known" attributes from the session, i.e. attributes listed + * by name in {@code @SessionAttributes} or attributes previously stored + * in the model that matched by type. * @param request the current request */ public void cleanupAttributes(WebRequest request) { - for (String attributeName : this.resolvedAttributeNames) { + for (String attributeName : this.knownAttributeNames) { this.sessionAttributeStore.cleanupAttribute(request, attributeName); } } @@ -158,5 +160,5 @@ public class SessionAttributesHandler { Object retrieveAttribute(WebRequest request, String attributeName) { return this.sessionAttributeStore.retrieveAttribute(request, attributeName); } - + } \ No newline at end of file diff --git a/org.springframework.web/src/test/java/org/springframework/web/method/annotation/SessionAttributesHandlerTests.java b/org.springframework.web/src/test/java/org/springframework/web/method/annotation/SessionAttributesHandlerTests.java index 06575004b95..8007e5143bc 100644 --- a/org.springframework.web/src/test/java/org/springframework/web/method/annotation/SessionAttributesHandlerTests.java +++ b/org.springframework.web/src/test/java/org/springframework/web/method/annotation/SessionAttributesHandlerTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2011 the original author or authors. + * Copyright 2002-2012 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -24,7 +24,6 @@ import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import java.util.HashSet; -import java.util.Map; import org.junit.Before; import org.junit.Test; @@ -39,17 +38,17 @@ import org.springframework.web.context.request.ServletWebRequest; /** * Test fixture with {@link SessionAttributesHandler}. - * + * * @author Rossen Stoyanchev */ public class SessionAttributesHandlerTests { private Class handlerType = SessionAttributeHandler.class; - + private SessionAttributesHandler sessionAttributesHandler; - + private SessionAttributeStore sessionAttributeStore; - + private NativeWebRequest request; @Before @@ -58,7 +57,7 @@ public class SessionAttributesHandlerTests { this.sessionAttributesHandler = new SessionAttributesHandler(handlerType, sessionAttributeStore); this.request = new ServletWebRequest(new MockHttpServletRequest()); } - + @Test public void isSessionAttribute() throws Exception { assertTrue(sessionAttributesHandler.isHandlerSessionAttribute("attr1", null)); @@ -72,14 +71,18 @@ public class SessionAttributesHandlerTests { sessionAttributeStore.storeAttribute(request, "attr1", "value1"); sessionAttributeStore.storeAttribute(request, "attr2", "value2"); sessionAttributeStore.storeAttribute(request, "attr3", new TestBean()); + sessionAttributeStore.storeAttribute(request, "attr4", new TestBean()); - // Resolve successfully handler session attributes once - assertTrue(sessionAttributesHandler.isHandlerSessionAttribute("attr1", null)); - assertTrue(sessionAttributesHandler.isHandlerSessionAttribute("attr3", TestBean.class)); + assertEquals("Named attributes (attr1, attr2) should be 'known' right away", + new HashSet(asList("attr1", "attr2")), + sessionAttributesHandler.retrieveAttributes(request).keySet()); - Map attributes = sessionAttributesHandler.retrieveAttributes(request); + // Resolve 'attr3' by type + sessionAttributesHandler.isHandlerSessionAttribute("attr3", TestBean.class); - assertEquals(new HashSet(asList("attr1", "attr3")), attributes.keySet()); + assertEquals("Named attributes (attr1, attr2) and resolved attribute (att3) should be 'known'", + new HashSet(asList("attr1", "attr2", "attr3")), + sessionAttributesHandler.retrieveAttributes(request).keySet()); } @Test @@ -88,14 +91,16 @@ public class SessionAttributesHandlerTests { sessionAttributeStore.storeAttribute(request, "attr2", "value2"); sessionAttributeStore.storeAttribute(request, "attr3", new TestBean()); - // Resolve successfully handler session attributes once - assertTrue(sessionAttributesHandler.isHandlerSessionAttribute("attr1", null)); - assertTrue(sessionAttributesHandler.isHandlerSessionAttribute("attr3", TestBean.class)); - sessionAttributesHandler.cleanupAttributes(request); - + assertNull(sessionAttributeStore.retrieveAttribute(request, "attr1")); - assertNotNull(sessionAttributeStore.retrieveAttribute(request, "attr2")); + assertNull(sessionAttributeStore.retrieveAttribute(request, "attr2")); + assertNotNull(sessionAttributeStore.retrieveAttribute(request, "attr3")); + + // Resolve 'attr3' by type + sessionAttributesHandler.isHandlerSessionAttribute("attr3", TestBean.class); + sessionAttributesHandler.cleanupAttributes(request); + assertNull(sessionAttributeStore.retrieveAttribute(request, "attr3")); } @@ -105,19 +110,14 @@ public class SessionAttributesHandlerTests { model.put("attr1", "value1"); model.put("attr2", "value2"); model.put("attr3", new TestBean()); - - // Resolve successfully handler session attributes once - assertTrue(sessionAttributesHandler.isHandlerSessionAttribute("attr1", null)); - assertTrue(sessionAttributesHandler.isHandlerSessionAttribute("attr2", null)); - assertTrue(sessionAttributesHandler.isHandlerSessionAttribute("attr3", TestBean.class)); - + sessionAttributesHandler.storeAttributes(request, model); - + assertEquals("value1", sessionAttributeStore.retrieveAttribute(request, "attr1")); assertEquals("value2", sessionAttributeStore.retrieveAttribute(request, "attr2")); assertTrue(sessionAttributeStore.retrieveAttribute(request, "attr3") instanceof TestBean); } - + @SessionAttributes(value = { "attr1", "attr2" }, types = { TestBean.class }) private static class SessionAttributeHandler { } From a9451d11f6effeeddb22c12009c83fe0bcdb36f6 Mon Sep 17 00:00:00 2001 From: Chris Beams Date: Sat, 4 Feb 2012 17:39:21 +0100 Subject: [PATCH 12/61] Relax -aspects dependence on -test (compile=>test) The spring-aspects Maven pom had an incorrect compile-scoped dependence on spring-test. In fact, spring-aspects only uses spring-test in its unit tests. The pom has been updated accordingly, meaning that use of spring-aspects in Maven-based applications will no longer require spring-test on the classpath at runtime. ivy.xml metadata did not need updating, as it was already correct. This change is only necessary on the 3.1.x line; in 3.2.x (master) Maven poms are generated automatically from Gradle dependency metadata, which is also already correct. Issue: SPR-9048 --- org.springframework.aspects/pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/org.springframework.aspects/pom.xml b/org.springframework.aspects/pom.xml index ba34ad1ebc5..74646897c12 100644 --- a/org.springframework.aspects/pom.xml +++ b/org.springframework.aspects/pom.xml @@ -61,7 +61,7 @@ org.springframework spring-test ${project.version} - compile + test javax.persistence From 856b77bc31046d416f2f236337d362517b1fda6f Mon Sep 17 00:00:00 2001 From: Chris Beams Date: Sat, 4 Feb 2012 22:29:05 +0100 Subject: [PATCH 13/61] Optimize ApplicationContextInitializer handling - Perform early check whether any ACI classes have been declared and exit immediately if not, avoiding any other processing - Polish method names in ContextLoaderTests Issue: SPR-8991 --- .../web/context/ContextLoaderTests.java | 6 +++--- .../web/context/ContextLoader.java | 15 +++++++++++---- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/org.springframework.web.servlet/src/test/java/org/springframework/web/context/ContextLoaderTests.java b/org.springframework.web.servlet/src/test/java/org/springframework/web/context/ContextLoaderTests.java index e228f24f5eb..a515820f359 100644 --- a/org.springframework.web.servlet/src/test/java/org/springframework/web/context/ContextLoaderTests.java +++ b/org.springframework.web.servlet/src/test/java/org/springframework/web/context/ContextLoaderTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2011 the original author or authors. + * Copyright 2002-2012 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -116,7 +116,7 @@ public final class ContextLoaderTests { } @Test - public void testContextLoaderListenerWithRegisteredContextConfigurer() { + public void testContextLoaderListenerWithRegisteredContextInitializer() { MockServletContext sc = new MockServletContext(""); sc.addInitParameter(ContextLoader.CONFIG_LOCATION_PARAM, "org/springframework/web/context/WEB-INF/ContextLoaderTests-acc-context.xml"); @@ -132,7 +132,7 @@ public final class ContextLoaderTests { } @Test - public void testContextLoaderListenerWithUnkownContextConfigurer() { + public void testContextLoaderListenerWithUnkownContextInitializer() { MockServletContext sc = new MockServletContext(""); // config file doesn't matter. just a placeholder sc.addInitParameter(ContextLoader.CONFIG_LOCATION_PARAM, diff --git a/org.springframework.web/src/main/java/org/springframework/web/context/ContextLoader.java b/org.springframework.web/src/main/java/org/springframework/web/context/ContextLoader.java index 1f32562deb9..25c8fa14cb9 100644 --- a/org.springframework.web/src/main/java/org/springframework/web/context/ContextLoader.java +++ b/org.springframework.web/src/main/java/org/springframework/web/context/ContextLoader.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2011 the original author or authors. + * Copyright 2002-2012 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -23,11 +23,11 @@ import java.util.List; import java.util.Map; import java.util.Properties; import java.util.concurrent.ConcurrentHashMap; + import javax.servlet.ServletContext; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; - import org.springframework.beans.BeanUtils; import org.springframework.beans.factory.access.BeanFactoryLocator; import org.springframework.beans.factory.access.BeanFactoryReference; @@ -463,11 +463,18 @@ public class ContextLoader { * @see ApplicationContextInitializer#initialize(ConfigurableApplicationContext) */ protected void customizeContext(ServletContext servletContext, ConfigurableWebApplicationContext applicationContext) { + List>> initializerClasses = + determineContextInitializerClasses(servletContext); + + if (initializerClasses.size() == 0) { + // no ApplicationContextInitializers have been declared -> nothing to do + return; + } + ArrayList> initializerInstances = new ArrayList>(); - for (Class> initializerClass : - determineContextInitializerClasses(servletContext)) { + for (Class> initializerClass : initializerClasses) { Class contextClass = applicationContext.getClass(); Class initializerContextClass = GenericTypeResolver.resolveTypeArgument(initializerClass, ApplicationContextInitializer.class); From 025b8abfaf1c2236328389337869dddcd5466fd7 Mon Sep 17 00:00:00 2001 From: Chris Beams Date: Sat, 4 Feb 2012 21:53:20 +0100 Subject: [PATCH 14/61] Allow ACI classes access to servlet context params Prior to this commit, StandardServletEnvironment's servlet context PropertySource remained stubbed out until it the ServletContext became available and could be replaced during the refresh() of its enclosing WebApplicationContext. This behavior is acceptable in most cases. However, if the user has declared an ApplicationContextInitializer that attempts to access servlet context-params via the Environment API, this result in a kind of 'false negative', i.e. the context-param key and value are actually present in the ServletContext, but the PropertySource representing servlet context params is still a stub at this point, meaning that it returns an empty result in all cases. With this change, WebApplicationContextUtils#initServletPropertySources is invoked eagerly by the ContextLoader if any ACI classes have been declared. This swaps out the servlet context property source stub for the real thing just in time for ACIs to use it if necessary. Extra guard logic has been added to #initServletPropertySources to ensure idempotency -- once the stub has been replaced, the method never attempts the replacement again, e.g. during the normal context refresh() when this method will be called again. Issue: SPR-8991 --- .../web/context/ContextLoaderTests.java | 23 +++++++++++++++++++ .../web/context/ContextLoader.java | 8 +++++++ .../support/WebApplicationContextUtils.java | 18 ++++++++++----- 3 files changed, 43 insertions(+), 6 deletions(-) diff --git a/org.springframework.web.servlet/src/test/java/org/springframework/web/context/ContextLoaderTests.java b/org.springframework.web.servlet/src/test/java/org/springframework/web/context/ContextLoaderTests.java index a515820f359..836e17a5ff5 100644 --- a/org.springframework.web.servlet/src/test/java/org/springframework/web/context/ContextLoaderTests.java +++ b/org.springframework.web.servlet/src/test/java/org/springframework/web/context/ContextLoaderTests.java @@ -17,6 +17,7 @@ package org.springframework.web.context; import static org.hamcrest.CoreMatchers.equalTo; +import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.CoreMatchers.notNullValue; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -131,6 +132,19 @@ public final class ContextLoaderTests { assertThat(wac.getServletContext().getAttribute("initialized"), notNullValue()); } + @Test + public void testRegisteredContextInitializerCanAccessServletContextParamsViaEnvironment() { + MockServletContext sc = new MockServletContext(""); + // config file doesn't matter. just a placeholder + sc.addInitParameter(ContextLoader.CONFIG_LOCATION_PARAM, + "/org/springframework/web/context/WEB-INF/empty-context.xml"); + + sc.addInitParameter("someProperty", "someValue"); + sc.addInitParameter(ContextLoader.CONTEXT_INITIALIZER_CLASSES_PARAM, EnvApplicationContextInitializer.class.getName()); + ContextLoaderListener listener = new ContextLoaderListener(); + listener.contextInitialized(new ServletContextEvent(sc)); + } + @Test public void testContextLoaderListenerWithUnkownContextInitializer() { MockServletContext sc = new MockServletContext(""); @@ -324,6 +338,15 @@ public final class ContextLoaderTests { } } + private static class EnvApplicationContextInitializer implements ApplicationContextInitializer { + public void initialize(ConfigurableWebApplicationContext applicationContext) { + // test that ApplicationContextInitializers can access ServletContext properties + // via the environment (SPR-8991) + String value = applicationContext.getEnvironment().getRequiredProperty("someProperty"); + assertThat(value, is("someValue")); + } + } + private static interface UnknownApplicationContext extends ConfigurableApplicationContext { void unheardOf(); } diff --git a/org.springframework.web/src/main/java/org/springframework/web/context/ContextLoader.java b/org.springframework.web/src/main/java/org/springframework/web/context/ContextLoader.java index 25c8fa14cb9..79c3d8744ca 100644 --- a/org.springframework.web/src/main/java/org/springframework/web/context/ContextLoader.java +++ b/org.springframework.web/src/main/java/org/springframework/web/context/ContextLoader.java @@ -44,6 +44,7 @@ import org.springframework.util.Assert; import org.springframework.util.ClassUtils; import org.springframework.util.ObjectUtils; import org.springframework.util.StringUtils; +import org.springframework.web.context.support.WebApplicationContextUtils; /** * Performs the actual initialization work for the root application context. @@ -487,6 +488,13 @@ public class ContextLoader { Collections.sort(initializerInstances, new AnnotationAwareOrderComparator()); + // eagerly attempt to initialize servlet property sources in case initializers + // below depend on accessing context-params via the Environment API. Note that + // depending on application context implementation, this initialization will be + // attempted again during context refresh. + WebApplicationContextUtils.initServletPropertySources( + applicationContext.getEnvironment().getPropertySources(), servletContext); + for (ApplicationContextInitializer initializer : initializerInstances) { initializer.initialize(applicationContext); } diff --git a/org.springframework.web/src/main/java/org/springframework/web/context/support/WebApplicationContextUtils.java b/org.springframework.web/src/main/java/org/springframework/web/context/support/WebApplicationContextUtils.java index eb607d1c318..9b9426d112f 100644 --- a/org.springframework.web/src/main/java/org/springframework/web/context/support/WebApplicationContextUtils.java +++ b/org.springframework.web/src/main/java/org/springframework/web/context/support/WebApplicationContextUtils.java @@ -16,6 +16,9 @@ package org.springframework.web.context.support; +import static org.springframework.web.context.support.StandardServletEnvironment.SERVLET_CONFIG_PROPERTY_SOURCE_NAME; +import static org.springframework.web.context.support.StandardServletEnvironment.SERVLET_CONTEXT_PROPERTY_SOURCE_NAME; + import java.io.Serializable; import java.util.Collections; import java.util.Enumeration; @@ -32,6 +35,7 @@ import javax.servlet.http.HttpSession; import org.springframework.beans.factory.ObjectFactory; import org.springframework.beans.factory.config.ConfigurableListableBeanFactory; import org.springframework.core.env.MutablePropertySources; +import org.springframework.core.env.PropertySource.StubPropertySource; import org.springframework.util.Assert; import org.springframework.util.ClassUtils; import org.springframework.web.context.ConfigurableWebApplicationContext; @@ -247,13 +251,15 @@ public abstract class WebApplicationContextUtils { public static void initServletPropertySources( MutablePropertySources propertySources, ServletContext servletContext, ServletConfig servletConfig) { Assert.notNull(propertySources, "propertySources must not be null"); - if(servletContext != null && propertySources.contains(StandardServletEnvironment.SERVLET_CONTEXT_PROPERTY_SOURCE_NAME)) { - propertySources.replace(StandardServletEnvironment.SERVLET_CONTEXT_PROPERTY_SOURCE_NAME, - new ServletContextPropertySource(StandardServletEnvironment.SERVLET_CONTEXT_PROPERTY_SOURCE_NAME, servletContext)); + if(servletContext != null && + propertySources.contains(SERVLET_CONTEXT_PROPERTY_SOURCE_NAME) && + propertySources.get(SERVLET_CONTEXT_PROPERTY_SOURCE_NAME) instanceof StubPropertySource) { + propertySources.replace(SERVLET_CONTEXT_PROPERTY_SOURCE_NAME, new ServletContextPropertySource(SERVLET_CONTEXT_PROPERTY_SOURCE_NAME, servletContext)); } - if(servletConfig != null && propertySources.contains(StandardServletEnvironment.SERVLET_CONFIG_PROPERTY_SOURCE_NAME)) { - propertySources.replace(StandardServletEnvironment.SERVLET_CONFIG_PROPERTY_SOURCE_NAME, - new ServletConfigPropertySource(StandardServletEnvironment.SERVLET_CONFIG_PROPERTY_SOURCE_NAME, servletConfig)); + if(servletConfig != null && + propertySources.contains(SERVLET_CONFIG_PROPERTY_SOURCE_NAME) && + propertySources.get(SERVLET_CONFIG_PROPERTY_SOURCE_NAME) instanceof StubPropertySource) { + propertySources.replace(SERVLET_CONFIG_PROPERTY_SOURCE_NAME, new ServletConfigPropertySource(SERVLET_CONFIG_PROPERTY_SOURCE_NAME, servletConfig)); } } From 2ffa4725cdd24d7feeb1e5d16cdd82316488fcdf Mon Sep 17 00:00:00 2001 From: Thomas Risberg Date: Fri, 3 Feb 2012 16:41:57 -0500 Subject: [PATCH 15/61] Allow SELECT statements in ResourceDatabasePopulator ResourceDatabasePopulator is a component that underlies the database initialization support within Spring's jdbc: namespace, e.g.: Prior to this commit, ResourceDatabasePopulator#executeSqlScript's use of Statement#executeUpdate(sql) precluded the possibility of SELECT statements because returning a result is not permitted by this method and results in an exception being thrown. Whether this behavior is a function of the JDBC specification or an idiosyncracy of certain implementations does not matter as the issue can be worked around entirely. This commit eliminates use of #executeUpdate(sql) in favor of #execute(sql) followed by a call to #getUpdateCount, effectively allowing any kind of SQL statement to be executed during database initialization. Issue: SPR-8932 --- .../init/ResourceDatabasePopulator.java | 5 +++-- .../init/DatabasePopulatorTests.java | 21 +++++++++++++++---- .../datasource/init/db-test-data-select.sql | 3 +++ 3 files changed, 23 insertions(+), 6 deletions(-) create mode 100644 org.springframework.jdbc/src/test/resources/org/springframework/jdbc/datasource/init/db-test-data-select.sql diff --git a/org.springframework.jdbc/src/main/java/org/springframework/jdbc/datasource/init/ResourceDatabasePopulator.java b/org.springframework.jdbc/src/main/java/org/springframework/jdbc/datasource/init/ResourceDatabasePopulator.java index 3da2b77fd72..b33a9d008a0 100644 --- a/org.springframework.jdbc/src/main/java/org/springframework/jdbc/datasource/init/ResourceDatabasePopulator.java +++ b/org.springframework.jdbc/src/main/java/org/springframework/jdbc/datasource/init/ResourceDatabasePopulator.java @@ -181,9 +181,10 @@ public class ResourceDatabasePopulator implements DatabasePopulator { for (String statement : statements) { lineNumber++; try { - int rowsAffected = stmt.executeUpdate(statement); + stmt.execute(statement); + int rowsAffected = stmt.getUpdateCount(); if (logger.isDebugEnabled()) { - logger.debug(rowsAffected + " rows affected by SQL: " + statement); + logger.debug(rowsAffected + " returned as updateCount for SQL: " + statement); } } catch (SQLException ex) { diff --git a/org.springframework.jdbc/src/test/java/org/springframework/jdbc/datasource/init/DatabasePopulatorTests.java b/org.springframework.jdbc/src/test/java/org/springframework/jdbc/datasource/init/DatabasePopulatorTests.java index 8154fa526ca..04d59698cce 100644 --- a/org.springframework.jdbc/src/test/java/org/springframework/jdbc/datasource/init/DatabasePopulatorTests.java +++ b/org.springframework.jdbc/src/test/java/org/springframework/jdbc/datasource/init/DatabasePopulatorTests.java @@ -20,8 +20,6 @@ import static org.junit.Assert.assertEquals; import java.sql.Connection; -import javax.sql.DataSource; - import org.junit.After; import org.junit.Test; import org.springframework.core.io.ClassRelativeResourceLoader; @@ -49,7 +47,7 @@ public class DatabasePopulatorTests { assertEquals(name, jdbcTemplate.queryForObject("select NAME from T_TEST", String.class)); } - private void assertUsersDatabaseCreated(DataSource db) { + private void assertUsersDatabaseCreated() { assertEquals("Sam", jdbcTemplate.queryForObject("select first_name from users where last_name = 'Brannen'", String.class)); } @@ -191,7 +189,22 @@ public class DatabasePopulatorTests { connection.close(); } - assertUsersDatabaseCreated(db); + assertUsersDatabaseCreated(); + } + + @Test + public void testBuildWithSelectStatements() throws Exception { + databasePopulator.addScript(resourceLoader.getResource("db-schema.sql")); + databasePopulator.addScript(resourceLoader.getResource("db-test-data-select.sql")); + Connection connection = db.getConnection(); + try { + databasePopulator.populate(connection); + } finally { + connection.close(); + } + + assertEquals(1, jdbcTemplate.queryForInt("select COUNT(NAME) from T_TEST where NAME='Keith'")); + assertEquals(1, jdbcTemplate.queryForInt("select COUNT(NAME) from T_TEST where NAME='Dave'")); } } diff --git a/org.springframework.jdbc/src/test/resources/org/springframework/jdbc/datasource/init/db-test-data-select.sql b/org.springframework.jdbc/src/test/resources/org/springframework/jdbc/datasource/init/db-test-data-select.sql new file mode 100644 index 00000000000..a000cb1b96a --- /dev/null +++ b/org.springframework.jdbc/src/test/resources/org/springframework/jdbc/datasource/init/db-test-data-select.sql @@ -0,0 +1,3 @@ +insert into T_TEST (NAME) values ('Keith'); +insert into T_TEST (NAME) values ('Dave'); +select NAME from T_TEST where NAME = 'Keith'; From 9fb6e2313ccf54fc3b2936f33e470f9eb1418a5c Mon Sep 17 00:00:00 2001 From: Thomas Risberg Date: Mon, 6 Feb 2012 00:02:01 -0500 Subject: [PATCH 16/61] Fix single quote parsing in NamedParameterUtils Prior to this change, single quotes were incorrectly parsed by NamedParameterUtils#parseSqlStatement, resulting in incorrect parameter counts: ParsedSql sql = NamedParameterUtils .parseSqlStatement("SELECT 'foo''bar', :xxx FROM DUAL"); assert sql.getTotalParameterCount() == 0 // incorrect, misses :xxx That is, presence of the single-quoted string caused the parser to overlook the named parameter :xxx. This commit fixes the parsing error such that: ParsedSql sql = NamedParameterUtils .parseSqlStatement("SELECT 'foo''bar', :xxx FROM DUAL"); assert sql.getTotalParameterCount() == 1 // correct Issue: SPR-8280 --- .../core/namedparam/NamedParameterUtils.java | 16 +++++++---- .../namedparam/NamedParameterUtilsTests.java | 28 ++++++++++++++++++- 2 files changed, 38 insertions(+), 6 deletions(-) diff --git a/org.springframework.jdbc/src/main/java/org/springframework/jdbc/core/namedparam/NamedParameterUtils.java b/org.springframework.jdbc/src/main/java/org/springframework/jdbc/core/namedparam/NamedParameterUtils.java index 1ab1f86bff7..436f78b9fc9 100644 --- a/org.springframework.jdbc/src/main/java/org/springframework/jdbc/core/namedparam/NamedParameterUtils.java +++ b/org.springframework.jdbc/src/main/java/org/springframework/jdbc/core/namedparam/NamedParameterUtils.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2011 the original author or authors. + * Copyright 2002-2012 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -85,12 +85,18 @@ public abstract class NamedParameterUtils { int escapes = 0; int i = 0; while (i < statement.length) { - int skipToPosition = skipCommentsAndQuotes(statement, i); - if (i != skipToPosition) { - if (skipToPosition >= statement.length) { + int skipToPosition = i; + while (i < statement.length) { + skipToPosition = skipCommentsAndQuotes(statement, i); + if (i == skipToPosition) { break; } - i = skipToPosition; + else { + i = skipToPosition; + } + } + if (i >= statement.length) { + break; } char c = statement[i]; if (c == ':' || c == '&') { diff --git a/org.springframework.jdbc/src/test/java/org/springframework/jdbc/core/namedparam/NamedParameterUtilsTests.java b/org.springframework.jdbc/src/test/java/org/springframework/jdbc/core/namedparam/NamedParameterUtilsTests.java index ed4c54e0ee8..48875c60f07 100644 --- a/org.springframework.jdbc/src/test/java/org/springframework/jdbc/core/namedparam/NamedParameterUtilsTests.java +++ b/org.springframework.jdbc/src/test/java/org/springframework/jdbc/core/namedparam/NamedParameterUtilsTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2011 the original author or authors. + * Copyright 2002-2012 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -268,4 +268,30 @@ public class NamedParameterUtilsTests { assertEquals(expectedSql, newSql); } + /* + * SPR-8280 + */ + @Test + public void parseSqlStatementWithQuotedSingleQuote() { + String sql = "SELECT ':foo'':doo', :xxx FROM DUAL"; + ParsedSql psql = NamedParameterUtils.parseSqlStatement(sql); + assertEquals(1, psql.getTotalParameterCount()); + assertEquals("xxx", psql.getParameterNames().get(0)); + } + + @Test + public void parseSqlStatementWithQuotesAndCommentBefore() { + String sql = "SELECT /*:doo*/':foo', :xxx FROM DUAL"; + ParsedSql psql = NamedParameterUtils.parseSqlStatement(sql); + assertEquals(1, psql.getTotalParameterCount()); + assertEquals("xxx", psql.getParameterNames().get(0)); + } + + @Test + public void parseSqlStatementWithQuotesAndCommentAfter() { + String sql2 = "SELECT ':foo'/*:doo*/, :xxx FROM DUAL"; + ParsedSql psql2 = NamedParameterUtils.parseSqlStatement(sql2); + assertEquals(1, psql2.getTotalParameterCount()); + assertEquals("xxx", psql2.getParameterNames().get(0)); + } } From a3bb3769ba0135f7c02fb98164fa8f6a39f4c5f8 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Mon, 6 Feb 2012 18:04:27 -0500 Subject: [PATCH 17/61] Tighten FlashMapManager for use with alternative storage options Ensure that both FlashMapManager methods - the one invoked at the start of a request and the one invoked before a redirect, update the underlying storage fully since it's not guaranteed that both will be invoked on any given request. Also move the logic to remove expired FlashMap instances to the metohd invoked at the start of a request to ensure the check is made frequently enough. SPR-8997 --- .../web/servlet/DispatcherServlet.java | 10 +- .../springframework/web/servlet/FlashMap.java | 42 +----- .../web/servlet/FlashMapManager.java | 31 ++-- .../support/AbstractFlashMapManager.java | 140 +++++++++--------- .../support/SessionFlashMapManager.java | 11 +- .../web/servlet/view/RedirectView.java | 2 +- .../support/AbstractFlashMapManagerTests.java | 87 ++++++----- 7 files changed, 144 insertions(+), 179 deletions(-) diff --git a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/DispatcherServlet.java b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/DispatcherServlet.java index 7bac3dfb1ba..d6eddaf7d7e 100644 --- a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/DispatcherServlet.java +++ b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/DispatcherServlet.java @@ -840,14 +840,14 @@ public class DispatcherServlet extends FrameworkServlet { request.setAttribute(LOCALE_RESOLVER_ATTRIBUTE, this.localeResolver); request.setAttribute(THEME_RESOLVER_ATTRIBUTE, this.themeResolver); request.setAttribute(THEME_SOURCE_ATTRIBUTE, getThemeSource()); + + FlashMap inputFlashMap = this.flashMapManager.retrieveAndUpdate(request, response); + if (inputFlashMap != null) { + request.setAttribute(INPUT_FLASH_MAP_ATTRIBUTE, Collections.unmodifiableMap(inputFlashMap)); + } request.setAttribute(OUTPUT_FLASH_MAP_ATTRIBUTE, new FlashMap()); request.setAttribute(FLASH_MAP_MANAGER_ATTRIBUTE, this.flashMapManager); - Map flashMap = this.flashMapManager.getFlashMapForRequest(request); - if (flashMap != null) { - request.setAttribute(INPUT_FLASH_MAP_ATTRIBUTE, flashMap); - } - try { doDispatch(request, response); } diff --git a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/FlashMap.java b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/FlashMap.java index cb6450ca89b..08b7f736a4a 100644 --- a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/FlashMap.java +++ b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/FlashMap.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2011 the original author or authors. + * Copyright 2002-2012 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -32,12 +32,11 @@ import org.springframework.util.StringUtils; *

    A FlashMap can be set up with a request path and request parameters to * help identify the target request. Without this information, a FlashMap is * made available to the next request, which may or may not be the intended - * recipient. On a redirect, the target URL is known and for example - * {@code org.springframework.web.servlet.view.RedirectView} has the - * opportunity to automatically update the current FlashMap with target - * URL information. + * recipient. On a redirect, the target URL is known and a FlashMap can be + * updated with that information. This is done automatically when the + * {@code org.springframework.web.servlet.view.RedirectView} is used. * - *

    Annotated controllers will usually not use this type directly. + *

    Note: annotated controllers will usually not use FlashMap directly. * See {@code org.springframework.web.servlet.mvc.support.RedirectAttributes} * for an overview of using flash attributes in annotated controllers. * @@ -58,25 +57,6 @@ public final class FlashMap extends HashMap implements Comparabl private int timeToLive; - private final int createdBy; - - /** - * Create a new instance with an id uniquely identifying the creator of - * this FlashMap. - * @param createdBy identifies the FlashMapManager instance that created - * and will manage this FlashMap instance (e.g. via a hashCode) - */ - public FlashMap(int createdBy) { - this.createdBy = createdBy; - } - - /** - * Create a new instance. - */ - public FlashMap() { - this.createdBy = 0; - } - /** * Provide a URL path to help identify the target request for this FlashMap. * The path may be absolute (e.g. /application/resource) or relative to the @@ -96,7 +76,6 @@ public final class FlashMap extends HashMap implements Comparabl /** * Provide request parameters identifying the request for this FlashMap. - * Null or empty keys and values are skipped. * @param params a Map with the names and values of expected parameters. */ public FlashMap addTargetRequestParams(MultiValueMap params) { @@ -112,8 +91,8 @@ public final class FlashMap extends HashMap implements Comparabl /** * Provide a request parameter identifying the request for this FlashMap. - * @param name the expected parameter name, skipped if {@code null} - * @param value the expected parameter value, skipped if {@code null} + * @param name the expected parameter name, skipped if empty or {@code null} + * @param value the expected value, skipped if empty or {@code null} */ public FlashMap addTargetRequestParam(String name, String value) { if (StringUtils.hasText(name) && StringUtils.hasText(value)) { @@ -151,13 +130,6 @@ public final class FlashMap extends HashMap implements Comparabl } } - /** - * Whether the given id matches the id of the creator of this FlashMap. - */ - public boolean isCreatedBy(int createdBy) { - return this.createdBy == createdBy; - } - /** * Compare two FlashMaps and prefer the one that specifies a target URL * path or has more target URL parameters. Before comparing FlashMap diff --git a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/FlashMapManager.java b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/FlashMapManager.java index 8440bd636c5..078713194b9 100644 --- a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/FlashMapManager.java +++ b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/FlashMapManager.java @@ -16,44 +16,43 @@ package org.springframework.web.servlet; -import java.util.Map; - import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; /** * A strategy interface for retrieving and saving FlashMap instances. * See {@link FlashMap} for a general overview of flash attributes. - * + * * @author Rossen Stoyanchev * @since 3.1 - * + * * @see FlashMap */ public interface FlashMapManager { /** - * Get a Map with flash attributes saved by a previous request. - * See {@link FlashMap} for details on how FlashMap instances - * identifies the target requests they're saved for. - * If found, the Map is removed from the underlying storage. + * Find a FlashMap saved by a previous request that matches to the current + * request, remove it from underlying storage, and also remove other + * expired FlashMap instances. + *

    This method is invoked in the beginning of every request in contrast + * to {@link #saveOutputFlashMap}, which is invoked only when there are + * flash attributes to be saved - i.e. before a redirect. * @param request the current request - * @return a read-only Map with flash attributes or {@code null} + * @param response the current response + * @return a FlashMap matching the current request or {@code null} */ - Map getFlashMapForRequest(HttpServletRequest request); + FlashMap retrieveAndUpdate(HttpServletRequest request, HttpServletResponse response); /** - * Save the given FlashMap, in some underlying storage, mark the beginning - * of its expiration period, and remove other expired FlashMap instances. - * The method has no impact if the FlashMap is empty and there are no - * expired FlashMap instances to be removed. + * Save the given FlashMap, in some underlying storage and set the start + * of its expiration period. *

    Note: Invoke this method prior to a redirect in order - * to allow saving the FlashMap in the HTTP session or perhaps in a response + * to allow saving the FlashMap in the HTTP session or in a response * cookie before the response is committed. * @param flashMap the FlashMap to save * @param request the current request * @param response the current response */ - void save(FlashMap flashMap, HttpServletRequest request, HttpServletResponse response); + void saveOutputFlashMap(FlashMap flashMap, HttpServletRequest request, HttpServletResponse response); } diff --git a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/support/AbstractFlashMapManager.java b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/support/AbstractFlashMapManager.java index 0cfc3b2037d..e745cfb47b8 100644 --- a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/support/AbstractFlashMapManager.java +++ b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/support/AbstractFlashMapManager.java @@ -19,7 +19,6 @@ package org.springframework.web.servlet.support; import java.util.ArrayList; import java.util.Collections; import java.util.List; -import java.util.Map; import java.util.concurrent.CopyOnWriteArrayList; import javax.servlet.http.HttpServletRequest; @@ -50,6 +49,8 @@ public abstract class AbstractFlashMapManager implements FlashMapManager { private UrlPathHelper urlPathHelper = new UrlPathHelper(); + private static final Object writeLock = new Object(); + /** * Set the amount of time in seconds after a {@link FlashMap} is saved * (at request completion) and before it expires. @@ -81,34 +82,30 @@ public abstract class AbstractFlashMapManager implements FlashMapManager { return this.urlPathHelper; } - /** - * {@inheritDoc} - *

    Does not cause an HTTP session to be created. - */ - public final Map getFlashMapForRequest(HttpServletRequest request) { - List flashMaps = retrieveFlashMaps(request); - if (CollectionUtils.isEmpty(flashMaps)) { + public final FlashMap retrieveAndUpdate(HttpServletRequest request, HttpServletResponse response) { + List allMaps = retrieveFlashMaps(request); + if (CollectionUtils.isEmpty(allMaps)) { return null; } if (logger.isDebugEnabled()) { - logger.debug("Retrieved FlashMap(s): " + flashMaps); + logger.debug("Retrieved FlashMap(s): " + allMaps); } - List result = new ArrayList(); - for (FlashMap flashMap : flashMaps) { - if (isFlashMapForRequest(flashMap, request)) { - result.add(flashMap); - } + List mapsToRemove = getExpiredFlashMaps(allMaps); + FlashMap match = getMatchingFlashMap(allMaps, request); + if (match != null) { + mapsToRemove.add(match); } - if (!result.isEmpty()) { - Collections.sort(result); + if (!mapsToRemove.isEmpty()) { if (logger.isDebugEnabled()) { - logger.debug("Found matching FlashMap(s): " + result); + logger.debug("Removing FlashMap(s): " + allMaps); + } + synchronized (writeLock) { + allMaps = retrieveFlashMaps(request); + allMaps.removeAll(mapsToRemove); + updateFlashMaps(allMaps, request, response); } - FlashMap match = result.remove(0); - flashMaps.remove(match); - return Collections.unmodifiableMap(match); } - return null; + return match; } /** @@ -118,10 +115,44 @@ public abstract class AbstractFlashMapManager implements FlashMapManager { */ protected abstract List retrieveFlashMaps(HttpServletRequest request); + /** + * Return a list of expired FlashMap instances contained in the given list. + */ + private List getExpiredFlashMaps(List allMaps) { + List result = new ArrayList(); + for (FlashMap map : allMaps) { + if (map.isExpired()) { + result.add(map); + } + } + return result; + } + + /** + * Return a FlashMap contained in the given list that matches the request. + * @return a matching FlashMap or {@code null} + */ + private FlashMap getMatchingFlashMap(List allMaps, HttpServletRequest request) { + List result = new ArrayList(); + for (FlashMap flashMap : allMaps) { + if (isFlashMapForRequest(flashMap, request)) { + result.add(flashMap); + } + } + if (!result.isEmpty()) { + Collections.sort(result); + if (logger.isDebugEnabled()) { + logger.debug("Found matching FlashMap(s): " + result); + } + return result.get(0); + } + return null; + } + /** * Whether the given FlashMap matches the current request. - * The default implementation uses the target request path and query params - * saved in the FlashMap. + * The default implementation uses the target request path and query + * parameters saved in the FlashMap. */ protected boolean isFlashMapForRequest(FlashMap flashMap, HttpServletRequest request) { if (flashMap.getTargetRequestPath() != null) { @@ -142,37 +173,21 @@ public abstract class AbstractFlashMapManager implements FlashMapManager { return true; } - /** - * {@inheritDoc} - *

    The FlashMap, if not empty, is saved to the HTTP session. - */ - public final void save(FlashMap flashMap, HttpServletRequest request, HttpServletResponse response) { - Assert.notNull(flashMap, "FlashMap must not be null"); - - List flashMaps = retrieveFlashMaps(request); - if (flashMap.isEmpty() && (flashMaps == null)) { + public final void saveOutputFlashMap(FlashMap flashMap, HttpServletRequest request, HttpServletResponse response) { + if (CollectionUtils.isEmpty(flashMap)) { return; } - synchronized (this) { - boolean update = false; - flashMaps = retrieveFlashMaps(request); - if (!CollectionUtils.isEmpty(flashMaps)) { - update = removeExpired(flashMaps); - } - if (!flashMap.isEmpty()) { - String path = decodeAndNormalizePath(flashMap.getTargetRequestPath(), request); - flashMap.setTargetRequestPath(path); - flashMap.startExpirationPeriod(this.flashMapTimeout); - if (logger.isDebugEnabled()) { - logger.debug("Saving FlashMap=" + flashMap); - } - flashMaps = (flashMaps == null) ? new CopyOnWriteArrayList() : flashMaps; - flashMaps.add(flashMap); - update = true; - } - if (update) { - updateFlashMaps(flashMaps, request, response); - } + String path = decodeAndNormalizePath(flashMap.getTargetRequestPath(), request); + flashMap.setTargetRequestPath(path); + flashMap.startExpirationPeriod(this.flashMapTimeout); + if (logger.isDebugEnabled()) { + logger.debug("Saving FlashMap=" + flashMap); + } + synchronized (writeLock) { + List allMaps = retrieveFlashMaps(request); + allMaps = (allMaps == null) ? new CopyOnWriteArrayList() : allMaps; + allMaps.add(flashMap); + updateFlashMaps(allMaps, request, response); } } @@ -197,25 +212,4 @@ public abstract class AbstractFlashMapManager implements FlashMapManager { protected abstract void updateFlashMaps(List flashMaps, HttpServletRequest request, HttpServletResponse response); - /** - * Remove expired FlashMap instances from the given List. - */ - protected boolean removeExpired(List flashMaps) { - List expired = new ArrayList(); - for (FlashMap flashMap : flashMaps) { - if (flashMap.isExpired()) { - if (logger.isTraceEnabled()) { - logger.trace("Removing expired FlashMap: " + flashMap); - } - expired.add(flashMap); - } - } - if (expired.isEmpty()) { - return false; - } - else { - return flashMaps.removeAll(expired); - } - } - } diff --git a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/support/SessionFlashMapManager.java b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/support/SessionFlashMapManager.java index ef6183e952f..527f89d6335 100644 --- a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/support/SessionFlashMapManager.java +++ b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/support/SessionFlashMapManager.java @@ -25,7 +25,7 @@ import javax.servlet.http.HttpSession; import org.springframework.web.servlet.FlashMap; /** - * Stores {@link FlashMap} instances in the HTTP session. + * Store and retrieve {@link FlashMap} instances to and from the HTTP session. * * @author Rossen Stoyanchev * @since 3.1.1 @@ -35,9 +35,10 @@ public class SessionFlashMapManager extends AbstractFlashMapManager{ private static final String FLASH_MAPS_SESSION_ATTRIBUTE = SessionFlashMapManager.class.getName() + ".FLASH_MAPS"; /** - * Retrieve saved FlashMap instances from the HTTP session. - * @param request the current request - * @return a List with FlashMap instances or {@code null} + * Retrieve saved FlashMap instances from the HTTP Session. + *

    Does not cause an HTTP session to be created but may update it if a + * FlashMap matching the current request is found or there are expired + * FlashMap to be removed. */ @SuppressWarnings("unchecked") protected List retrieveFlashMaps(HttpServletRequest request) { @@ -46,7 +47,7 @@ public class SessionFlashMapManager extends AbstractFlashMapManager{ } /** - * Save the given FlashMap instances in the HTTP session. + * Save the given FlashMap instance, if not empty, in the HTTP session. */ protected void updateFlashMaps(List flashMaps, HttpServletRequest request, HttpServletResponse response) { request.getSession().setAttribute(FLASH_MAPS_SESSION_ATTRIBUTE, flashMaps); diff --git a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/view/RedirectView.java b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/view/RedirectView.java index 0f931d776bb..43f15df2e9d 100644 --- a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/view/RedirectView.java +++ b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/view/RedirectView.java @@ -276,7 +276,7 @@ public class RedirectView extends AbstractUrlBasedView implements SmartView { } FlashMapManager flashMapManager = RequestContextUtils.getFlashMapManager(request); - flashMapManager.save(flashMap, request, response); + flashMapManager.saveOutputFlashMap(flashMap, request, response); sendRedirect(request, response, targetUrl.toString(), this.http10Compatible); } diff --git a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/support/AbstractFlashMapManagerTests.java b/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/support/AbstractFlashMapManagerTests.java index ccef7060b9b..f4b80f6b1ad 100644 --- a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/support/AbstractFlashMapManagerTests.java +++ b/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/support/AbstractFlashMapManagerTests.java @@ -26,7 +26,6 @@ import static org.junit.Assert.assertTrue; import java.util.ArrayList; import java.util.Arrays; import java.util.List; -import java.util.Map; import java.util.concurrent.CopyOnWriteArrayList; import javax.servlet.http.HttpServletRequest; @@ -60,7 +59,7 @@ public class AbstractFlashMapManagerTests { } @Test - public void getFlashMapForRequestByPath() { + public void retrieveAndUpdateMatchByPath() { FlashMap flashMap = new FlashMap(); flashMap.put("key", "value"); flashMap.setTargetRequestPath("/path"); @@ -68,16 +67,15 @@ public class AbstractFlashMapManagerTests { this.flashMapManager.setFlashMaps(flashMap); this.request.setRequestURI("/path"); - Map inputFlashMap = this.flashMapManager.getFlashMapForRequest(this.request); + FlashMap inputFlashMap = this.flashMapManager.retrieveAndUpdate(this.request, this.response); assertEquals(flashMap, inputFlashMap); - assertEquals("Input FlashMap should have been removed", 0, this.flashMapManager.getFlashMaps().size()); } // SPR-8779 @Test - public void getFlashMapForRequestByOriginatingPath() { + public void retrieveAndUpdateMatchByOriginatingPath() { FlashMap flashMap = new FlashMap(); flashMap.put("key", "value"); flashMap.setTargetRequestPath("/accounts"); @@ -86,14 +84,14 @@ public class AbstractFlashMapManagerTests { this.request.setAttribute(WebUtils.FORWARD_REQUEST_URI_ATTRIBUTE, "/accounts"); this.request.setRequestURI("/mvc/accounts"); - Map inputFlashMap = this.flashMapManager.getFlashMapForRequest(this.request); + FlashMap inputFlashMap = this.flashMapManager.retrieveAndUpdate(this.request, this.response); assertEquals(flashMap, inputFlashMap); assertEquals("Input FlashMap should have been removed", 0, this.flashMapManager.getFlashMaps().size()); } @Test - public void getFlashMapForRequestByPathWithTrailingSlash() { + public void retrieveAndUpdateMatchWithTrailingSlash() { FlashMap flashMap = new FlashMap(); flashMap.put("key", "value"); flashMap.setTargetRequestPath("/path"); @@ -101,14 +99,14 @@ public class AbstractFlashMapManagerTests { this.flashMapManager.setFlashMaps(flashMap); this.request.setRequestURI("/path/"); - Map inputFlashMap = this.flashMapManager.getFlashMapForRequest(this.request); + FlashMap inputFlashMap = this.flashMapManager.retrieveAndUpdate(this.request, this.response); assertEquals(flashMap, inputFlashMap); assertEquals("Input FlashMap should have been removed", 0, this.flashMapManager.getFlashMaps().size()); } @Test - public void getFlashMapForRequestWithParams() { + public void retrieveAndUpdateMatchByParams() { FlashMap flashMap = new FlashMap(); flashMap.put("key", "value"); flashMap.addTargetRequestParam("number", "one"); @@ -116,19 +114,19 @@ public class AbstractFlashMapManagerTests { this.flashMapManager.setFlashMaps(flashMap); this.request.setParameter("number", (String) null); - Map inputFlashMap = this.flashMapManager.getFlashMapForRequest(this.request); + FlashMap inputFlashMap = this.flashMapManager.retrieveAndUpdate(this.request, this.response); assertNull(inputFlashMap); assertEquals("FlashMap should not have been removed", 1, this.flashMapManager.getFlashMaps().size()); this.request.setParameter("number", "two"); - inputFlashMap = this.flashMapManager.getFlashMapForRequest(this.request); + inputFlashMap = this.flashMapManager.retrieveAndUpdate(this.request, this.response); assertNull(inputFlashMap); assertEquals("FlashMap should not have been removed", 1, this.flashMapManager.getFlashMaps().size()); this.request.setParameter("number", "one"); - inputFlashMap = this.flashMapManager.getFlashMapForRequest(this.request); + inputFlashMap = this.flashMapManager.retrieveAndUpdate(this.request, this.response); assertEquals(flashMap, inputFlashMap); assertEquals("Input FlashMap should have been removed", 0, this.flashMapManager.getFlashMaps().size()); @@ -137,7 +135,7 @@ public class AbstractFlashMapManagerTests { // SPR-8798 @Test - public void getFlashMapForRequestWithMultiValueParam() { + public void retrieveAndUpdateMatchWithMultiValueParam() { FlashMap flashMap = new FlashMap(); flashMap.put("name", "value"); flashMap.addTargetRequestParam("id", "1"); @@ -146,20 +144,20 @@ public class AbstractFlashMapManagerTests { this.flashMapManager.setFlashMaps(flashMap); this.request.setParameter("id", "1"); - Map inputFlashMap = this.flashMapManager.getFlashMapForRequest(this.request); + FlashMap inputFlashMap = this.flashMapManager.retrieveAndUpdate(this.request, this.response); assertNull(inputFlashMap); assertEquals("FlashMap should not have been removed", 1, this.flashMapManager.getFlashMaps().size()); this.request.addParameter("id", "2"); - inputFlashMap = this.flashMapManager.getFlashMapForRequest(this.request); + inputFlashMap = this.flashMapManager.retrieveAndUpdate(this.request, this.response); assertEquals(flashMap, inputFlashMap); assertEquals("Input FlashMap should have been removed", 0, this.flashMapManager.getFlashMaps().size()); } @Test - public void getFlashMapForRequestSortOrder() { + public void retrieveAndUpdateSortMultipleMatches() { FlashMap emptyFlashMap = new FlashMap(); FlashMap flashMapOne = new FlashMap(); @@ -174,28 +172,44 @@ public class AbstractFlashMapManagerTests { this.flashMapManager.setFlashMaps(emptyFlashMap, flashMapOne, flashMapTwo); this.request.setRequestURI("/one/two"); - Map inputFlashMap = this.flashMapManager.getFlashMapForRequest(this.request); + FlashMap inputFlashMap = this.flashMapManager.retrieveAndUpdate(this.request, this.response); assertEquals(flashMapTwo, inputFlashMap); + assertEquals("Input FlashMap should have been removed", 2, this.flashMapManager.getFlashMaps().size()); } @Test - public void saveFlashMapEmpty() throws InterruptedException { + public void retrieveAndUpdateRemoveExpired() throws InterruptedException { + List flashMaps = new ArrayList(); + for (int i=0; i < 5; i++) { + FlashMap expiredFlashMap = new FlashMap(); + expiredFlashMap.startExpirationPeriod(-1); + flashMaps.add(expiredFlashMap); + } + this.flashMapManager.setFlashMaps(flashMaps); + this.flashMapManager.retrieveAndUpdate(this.request, this.response); + + assertEquals("Expired instances should be removed even if the saved FlashMap is empty", + 0, this.flashMapManager.getFlashMaps().size()); + } + + @Test + public void saveOutputFlashMapEmpty() throws InterruptedException { FlashMap flashMap = new FlashMap(); - this.flashMapManager.save(flashMap, this.request, this.response); + this.flashMapManager.saveOutputFlashMap(flashMap, this.request, this.response); List allMaps = this.flashMapManager.getFlashMaps(); assertNull(allMaps); } @Test - public void saveFlashMap() throws InterruptedException { + public void saveOutputFlashMap() throws InterruptedException { FlashMap flashMap = new FlashMap(); flashMap.put("name", "value"); this.flashMapManager.setFlashMapTimeout(-1); // expire immediately so we can check expiration started - this.flashMapManager.save(flashMap, this.request, this.response); + this.flashMapManager.saveOutputFlashMap(flashMap, this.request, this.response); List allMaps = this.flashMapManager.getFlashMaps(); assertNotNull(allMaps); @@ -204,67 +218,52 @@ public class AbstractFlashMapManagerTests { } @Test - public void saveFlashMapDecodeTargetPath() throws InterruptedException { + public void saveOutputFlashMapDecodeTargetPath() throws InterruptedException { FlashMap flashMap = new FlashMap(); flashMap.put("key", "value"); flashMap.setTargetRequestPath("/once%20upon%20a%20time"); - this.flashMapManager.save(flashMap, this.request, this.response); + this.flashMapManager.saveOutputFlashMap(flashMap, this.request, this.response); assertEquals("/once upon a time", flashMap.getTargetRequestPath()); } @Test - public void saveFlashMapNormalizeTargetPath() throws InterruptedException { + public void saveOutputFlashMapNormalizeTargetPath() throws InterruptedException { FlashMap flashMap = new FlashMap(); flashMap.put("key", "value"); flashMap.setTargetRequestPath("."); this.request.setRequestURI("/once/upon/a/time"); - this.flashMapManager.save(flashMap, this.request, this.response); + this.flashMapManager.saveOutputFlashMap(flashMap, this.request, this.response); assertEquals("/once/upon/a", flashMap.getTargetRequestPath()); flashMap.setTargetRequestPath("./"); this.request.setRequestURI("/once/upon/a/time"); - this.flashMapManager.save(flashMap, this.request, this.response); + this.flashMapManager.saveOutputFlashMap(flashMap, this.request, this.response); assertEquals("/once/upon/a/", flashMap.getTargetRequestPath()); flashMap.setTargetRequestPath(".."); this.request.setRequestURI("/once/upon/a/time"); - this.flashMapManager.save(flashMap, this.request, this.response); + this.flashMapManager.saveOutputFlashMap(flashMap, this.request, this.response); assertEquals("/once/upon", flashMap.getTargetRequestPath()); flashMap.setTargetRequestPath("../"); this.request.setRequestURI("/once/upon/a/time"); - this.flashMapManager.save(flashMap, this.request, this.response); + this.flashMapManager.saveOutputFlashMap(flashMap, this.request, this.response); assertEquals("/once/upon/", flashMap.getTargetRequestPath()); flashMap.setTargetRequestPath("../../only"); this.request.setRequestURI("/once/upon/a/time"); - this.flashMapManager.save(flashMap, this.request, this.response); + this.flashMapManager.saveOutputFlashMap(flashMap, this.request, this.response); assertEquals("/once/only", flashMap.getTargetRequestPath()); } - @Test - public void saveFlashMapAndRemoveExpired() throws InterruptedException { - List flashMaps = new ArrayList(); - for (int i=0; i < 5; i++) { - FlashMap flashMap = new FlashMap(); - flashMap.startExpirationPeriod(-1); - flashMaps.add(flashMap); - } - this.flashMapManager.setFlashMaps(flashMaps); - this.flashMapManager.save(new FlashMap(), request, response); - - assertEquals("Expired instances should be removed even if the saved FlashMap is empty", - 0, this.flashMapManager.getFlashMaps().size()); - } - private static class TestFlashMapManager extends AbstractFlashMapManager { From 3ec78e2c0429ec3a5f76bbe2358a65638d57f0b1 Mon Sep 17 00:00:00 2001 From: Arjen Poutsma Date: Tue, 7 Feb 2012 15:40:50 +0100 Subject: [PATCH 18/61] SPR-9093: UriTemplate not throwing IllegalArgumentException when URIVariables map missing values --- .../org/springframework/web/util/UriComponents.java | 6 +++++- .../springframework/web/util/UriTemplateTests.java | 11 ++++++++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/org.springframework.web/src/main/java/org/springframework/web/util/UriComponents.java b/org.springframework.web/src/main/java/org/springframework/web/util/UriComponents.java index 6123223e792..54cbcef437e 100644 --- a/org.springframework.web/src/main/java/org/springframework/web/util/UriComponents.java +++ b/org.springframework.web/src/main/java/org/springframework/web/util/UriComponents.java @@ -1002,6 +1002,9 @@ public final class UriComponents { } public Object getValue(String name) { + if (!this.uriVariables.containsKey(name)) { + throw new IllegalArgumentException("Map has no value for '" + name + "'"); + } return this.uriVariables.get(name); } } @@ -1010,6 +1013,7 @@ public final class UriComponents { * URI template variables backed by a variable argument array. */ private static class VarArgsTemplateVariables implements UriTemplateVariables { + private final Iterator valueIterator; public VarArgsTemplateVariables(Object... uriVariableValues) { @@ -1018,7 +1022,7 @@ public final class UriComponents { public Object getValue(String name) { if (!valueIterator.hasNext()) { - throw new IllegalArgumentException("Not enough variable values available to expand [" + name + "]"); + throw new IllegalArgumentException("Not enough variable values available to expand '" + name + "'"); } return valueIterator.next(); } diff --git a/org.springframework.web/src/test/java/org/springframework/web/util/UriTemplateTests.java b/org.springframework.web/src/test/java/org/springframework/web/util/UriTemplateTests.java index f0099328cd6..ba2d7a796d2 100644 --- a/org.springframework.web/src/test/java/org/springframework/web/util/UriTemplateTests.java +++ b/org.springframework.web/src/test/java/org/springframework/web/util/UriTemplateTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2011 the original author or authors. + * Copyright 2002-2012 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -89,6 +89,15 @@ public class UriTemplateTests { assertEquals("Invalid expanded template", new URI("http://example.com/hotel%20list/Z%C3%BCrich"), result); } + @Test(expected = IllegalArgumentException.class) + public void expandMapUnboundVariables() throws Exception { + Map uriVariables = new HashMap(2); + uriVariables.put("booking", "42"); + uriVariables.put("bar", "1"); + UriTemplate template = new UriTemplate("http://example.com/hotels/{hotel}/bookings/{booking}"); + template.expand(uriVariables); + } + @Test public void expandEncoded() throws Exception { UriTemplate template = new UriTemplate("http://example.com/hotel list/{hotel}"); From 7a170e8005f962ee287eacb88382a2ed6c3ffbfa Mon Sep 17 00:00:00 2001 From: Arjen Poutsma Date: Tue, 7 Feb 2012 15:41:49 +0100 Subject: [PATCH 19/61] Updated IntelliJ project for JUnit 4.9.0 --- spring-framework.ipr | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/spring-framework.ipr b/spring-framework.ipr index afc4643b983..49dc2d880c4 100644 --- a/spring-framework.ipr +++ b/spring-framework.ipr @@ -445,7 +445,7 @@