SpEL: Unnecessary conversion of Map<?, ?> arguments [SPR-8518]

[spring-projects-issues]

Grzegorz Grzybek opened SPR-8518 and commented

@kdonald

In revision 4483 there was a change in org.springframework.expression.spel.support.ReflectionHelper.convertArguments(TypeConverter, Object[], Object, int[], Integer). Now all arguments are converted.

I have a problem with SpEL, where root object is map (but it's a general problem with Map arguments) - it's a problem w Java generics, where type arguments may be deducted from method parameters, but not from actual arguments:

// method
void mapArg(Map<String, Object> arg1);
// type descriptor for method parameter
new TypeDescriptor(MethodParameter.forMethodOrConstructor(methodOrCtor, i));
// type descriptor for actual argument
TypeDescriptor.forObject(argument)

Spring converts Map<String, Object> because it determines it's Map<?, ?>, which results in new Map being passed to actual method. When the map is EvaluationContext's root object, it's not the root map which is passed to SpEL invoked method, so I can't use SpEL to fill passed map...

I know it can't be resolved because of type erasure - but can this be specified in documentation somehow?

regards
Grzegorz Grzybek

---

Affects: 3.1 M2

Issue Links:

• 3.0.6 Map to Map conversion shallow copying maps causing unexpected results on upgrade from 3.0.5 [SPR-8714] #13356 3.0.6 Map to Map conversion shallow copying maps causing unexpected results on upgrade from 3.0.5

[spring-projects-issues]

Keith Donald commented

Can you provide a test case, perhaps over at https://github.com/SpringSource/spring-framework-issues, that demonstrates the issue from your perspective? I understand the change we made, but I'm having a hard time understanding the exact problem you have with it. A test case illustrating the "unnecessary" behavior would help me, as would additional information about the problem (such as the actual EL expression you're evaluating here). Thanks!

[spring-projects-issues]

Grzegorz Grzybek commented

Sorry I'm late - I'm on holidays and I don't have my laptop here :)
So here's from my memory:

1. I've written an XML-templating engine which uses SpEL as internal expression language.
2. SpEL expressions are evaluated while processing (by VTD-XML by the way - it's about 1000x faster than DOM) the document
3. the root object passed to the engine (and then available to all expressions as #root) is a HashMap<String, Object> - it doesn't only hold some initial values, but (and that's the point) collects some data produced during XML processing
4. Particularly: at the beginning of XML processing there is one SpEL invocation of my custom method (using SpEl function registered in EvaluationContext) which does some XPath and puts the result into #root, i.e. the Map
5. Consecutive SpEL invocations made in the context of deeply nested XML nodes use the XPath results.

So he only way (which worked up until 3.1.0.M1) is to use "shared map". It was beautiful discovery of SpEL's power to use Map as #root object together with org.springframework.context.expression.MapAccessor - it allowed me to implement the needed functionality.

I hope this explains a bit - I know I've described the technical reason why it's not working (type erasure) and not what my code is - hop I got better this time.

I don't have an idea how this could be solved in SpEL implementations. Of course I could register an identity map converter, but I'd rather like this special case to be handled (I mean the case, where arguments without type parameter info are passed to methods with generic parameters - Collection<?> should IMO not be converted when passed to method with Collection<String> parameter) - the case where type information is unavailable.

regards
Grzegorz Grzybek

[spring-projects-issues]

Keith Donald commented

Keith Donald 9:24 AM
hey
the issue of collection conversion semantics (when to create a converted copy) has been raised: https://jira.springsource.org/browse/SPR-8518
In the context of SpEL here Grzegorz needs a shared map instance to be passed around, so the copying we're doing is breaking his system.

Juergen Hoeller 9:26 AM
hmm
we anticipated some such problem, didn't we

Keith Donald 9:26 AM
In his case he's passing a Map<?, ?> to a Map<String, Object> method.
its creating a copy and invoking them method with the copy now
which breaks him since he expects the Map<?, ?> to be shared - the methods he is invoking via SpEL are supposed to fill the map.
yes, we anticipated some issues along these lines.

Juergen Hoeller 9:28 AM
we don't have to copy the Map in the untyped case, do we
we just did it for completeness

Keith Donald 9:28 AM
I modified SpEL to pass every argument of a constructor or method reference invocation through the conversion service.
which will break people who are depending on shared collections to be passed around by SpEL.

Juergen Hoeller 9:29 AM
yeah... I haven't thought of that use case before

Keith Donald 9:29 AM
me neither.

Juergen Hoeller 9:30 AM
so I guess we need to switch the approach again, allowing for the original collection to be passed in if no conversion is applicable

Keith Donald 9:30 AM
The algorithm in the collection converters now always creates a copy.
"if no conversion is applicable" rules need to be fully defined.

Juergen Hoeller 9:31 AM
I know, which is why it was easier to always create a copy

Keith Donald 9:31 AM
yes.
in any case, I do like it that SpEL simply invokes the CS for every argument it is binding... before it was doing all this complicated checking to determine if conversion was necessary itself.
That checking code had some issues and really made the code messy.
I'd prefer if the CS handle that decision and SpEL simply delegate to it.

Juergen Hoeller 9:32 AM
yeah... it does suggest that passing through an original is the norm though
we're relying on CS to return the original in the common case
if we're always calling it

Keith Donald 9:33 AM
so this is an excercise requriing definition of thes erules for each collection converter e.g. MapToMap.
could we adopt bean wrapper collection conversion rules?

Juergen Hoeller 9:35 AM
which is the other end of the spectrum basically: only use the copy when actually necessary, even if a temporary copy has been created already
the problem there is String-to-String conversion and that sort of stuff
or even nested collections
there are quite a few cases where we'll only find out whether conversion was necessary through actually attempting conversion of each element and seeing what we end up with

Keith Donald 9:36 AM
I see.
so you're saying we might end up doing the work, creating the copy, but throwing the copy away in preference of the original if it's equal to the original.

Juergen Hoeller 9:37 AM
which is why the temporary copy is being used, and each converted element compared with the original element to find out whether anything changed
yeah - we're comparing identity, actually, not equality
so if we end up with the exact same element references, we'll return the original

[spring-projects-issues]

Grzegorz Grzybek commented

Here's a patch:

Index: src/test/java/org/springframework/expression/spel/MapAccessTests.java
===================================================================
--- src/test/java/org/springframework/expression/spel/MapAccessTests.java	(revision 4681)
+++ src/test/java/org/springframework/expression/spel/MapAccessTests.java	(working copy)
@@ -60,6 +60,26 @@
 	}
 
 	@Test
+	public void testCustomMapAccessorWithMapAsRootObject() throws Exception {
+		ExpressionParser parser = new SpelExpressionParser();
+		StandardEvaluationContext ctx = TestScenarioCreator.getTestEvaluationContext();
+		ctx.addPropertyAccessor(new MapAccessor());
+		Map<String, Object> map = new HashMap<String, Object>();
+		map.put("obj", new ClassWithMethod());
+		ctx.setRootObject(map);
+		
+		Expression expr = parser.parseExpression("obj.putIntoMap(#root)");
+		expr.getValue(ctx);
+		Assert.assertEquals(2, map.size());
+	}
+
+	public class ClassWithMethod {
+		public Object putIntoMap(Map<String, Object> map) {
+			return map.put("k", "v");
+		}
+	}
+
+	@Test
 	public void testVariableMapAccess() throws Exception {
 		ExpressionParser parser = new SpelExpressionParser();
 		StandardEvaluationContext ctx = TestScenarioCreator.getTestEvaluationContext();

It seems it doesn't work only with methods on objects. Functions registered in EvaluationContext work fine (so: an inconsequence).

I think the problem in SpEL (actually ReflectiveMethodResolver) is mixing arguments (runtime objects) with parameters (also runtime, but at type level) metadata. We can't find types of keys and values on arguments, which is possible for method parameters. Maybe ReflectionHelper.compareArguments should return ArgumentsMatchInfo which would handle special (not exactly) case of passing HashMap<?, ?> to Map<String, Object> (and LinkedList<?> to List<String>, etc.) - MapToMap converter is not what I expect (Principle of Least Surprise?). I understand the need for checking whether map keys/values need conversion, and taking into account type erasure - the check is always done because runtime map arguments don't have information about keys/values types.

So maybe it's not a bug (the above mentioned inconsequence should be) - I can always register identity MapToMap converter, but please clarify this behaviour :)

regards
Grzegorz Grzybek

[spring-projects-issues]

Rossen Stoyanchev commented

A fix has been made to check if a copy of the source collection is required in CollectionToCollectionConverter. This is the case either if the source collection is not assignable to the target collection type or if any collection elements actually undergo type conversion. If none of these are true the original source collection is returned.

If you could validate this in your code that would be much appreciated!

[spring-projects-issues]

Grzegorz Grzybek commented

My tests went fine, thanks for the fix!

[spring-projects-issues]

Juergen Hoeller commented

Backported to 3.0.7, for release in late December.

Juergen