Thanks! LGTM 👍 just curious why you want to keep an expression which always fails!

Very good question, indeed. ;-)

The short answer is that it will always fail but will always be evaluated anyway. So better avoiding the cost of parsing in any case...

Some more details on the case below.

This is because this single expression can be evaluated through a chain, and the way the method ognl.ASTChain.getValueBody(OgnlContext, Object) is built can lead to method ognl.OgnlRuntime.getProperty(OgnlContext, Object, Object) throwing a OgnlException("source is null for getProperty(null, \"" + name + "\")") (line 2733).
Hence the expression is not cached, before this code change.

Here are a few values of thename attribute of method ognl.OgnlRuntime.getProperty(OgnlContext, Object, Object) creating this scenario:

actionMapping
opensymphony
apache
valueStack
actiontag
servlet
Application__
Request__

And a stack trace for the actionMapping case (Struts 2.5.20):

Daemon Thread [http-nio-8080-exec-6] (Suspended (breakpoint at line 2733 in OgnlRuntime))   
    owns: NioEndpoint$NioSocketWrapper  (id=294)    
    OgnlRuntime.getProperty(OgnlContext, Object, Object) line: 2733 
    ASTProperty.getValueBody(OgnlContext, Object) line: 114 
    ASTProperty(SimpleNode).evaluateGetValueBody(OgnlContext, Object) line: 212 
    ASTProperty(SimpleNode).getValue(OgnlContext, Object) line: 258 
    ASTChain.getValueBody(OgnlContext, Object) line: 141    
    ASTChain(SimpleNode).evaluateGetValueBody(OgnlContext, Object) line: 212    
    ASTChain(SimpleNode).getValue(OgnlContext, Object) line: 258    
    Ognl.getValue(Object, Map, Object, Class) line: 470 
    Ognl.getValue(Object, Map, Object) line: 434    
    OgnlUtil$2.execute(Object) line: 401    
    OgnlUtil.compileAndExecute(String, Map<String,Object>, OgnlTask<T>) line: 442   
    OgnlUtil.getValue(String, Map<String,Object>, Object) line: 399 
    OgnlValueStack.getValueUsingOgnl(String) line: 293  
    OgnlValueStack.tryFindValue(String) line: 276   
    OgnlValueStack.tryFindValueWhenExpressionIsNotNull(String) line: 258    
    OgnlValueStack.findValue(String, boolean) line: 238 
    OgnlValueStack.findValue(String) line: 300  
    StrutsRequestWrapper.getAttribute(String) line: 94  
    PrepareOperations.findActionMapping(HttpServletRequest, HttpServletResponse, boolean) line: 181 
    PrepareOperations.findActionMapping(HttpServletRequest, HttpServletResponse) line: 165  
    StrutsPrepareFilter.doFilter(ServletRequest, ServletResponse, FilterChain) line: 90 
    ApplicationFilterChain.internalDoFilter(ServletRequest, ServletResponse) line: 189  
    ApplicationFilterChain.doFilter(ServletRequest, ServletResponse) line: 162  
...

Here, the Struts layer attempts to find the actionMapping to use, which is first evaluated using OGNL onto the ValueStack - and in this specific case, there is no such entry in the stack - before it actually gets into the struts configuration to find the appropriate mapping.

I did spend some time to check how method ognl.ASTChain.getValueBody(OgnlContext, Object) works, and I'm actually puzzled to see that the source attribute is actually the result of the previous step in the chain (which can be null, leading to the exception being raised). But I'm not sure whether this is a problem or a by-design choice (and I sure don't have enough background onto the OGNL internals to decide).

Would you say that this sequence of event is normal, or is it something that should not occur?

Let me know, and I'll open another issue - I can reproduce it 100% so it's easy to investigate if you find it useful...

Hi @davoustp.

Thanks for producing this PR in response to the JIRA issue. 👍 The proposed change addresses the JIRA issue that was raised, by adding any expression/tree that can be "successfully parsed" to the expression cache.

The original logic (prior to the change) would only add the expression/tree to the expression cache if the expression could be successfully executed (in addition to being "successfully parsed"). Looking at the history of OgnlUtil (as far back as I can see in the repository), it looks like the logic considered an expression "valid" to put in the expression cache if-and-only-if it could be successfully executed. It seems like that may have been an intentional decision based on the original code comment ?

There may be a trade-off in execution performance (by allowing more expressions to be considered "valid" and cached) and memory utilization (due to the larger number of potential expressions now being cached).

It may be difficult to predict if the change in this PR might result in unexpected memory overhead, for some applications, due to potentially increased expression cache size.

Having expressions considered "valid" if-and-only-if they can be executed successfully may have provided a limiting factor to keep the cache size more manageable in the past, but that is just my speculation.

This change may be safer to attempt for the 2.6.x branch (as opposed to PR #503 for 2.5.x), since it introduces a new behaviour for the expression cache.

Thoughts ?

Thanks for producing this PR in response to the JIRA issue. 👍

My pleasure, that's normal to contribute back when we find something useful for the community (OSS rocks).

Looking at the history of OgnlUtil (as far back as I can see in the repository), it looks like the logic considered an expression "valid" to put in the expression cache if-and-only-if it could be successfully executed. It seems like that may have been an intentional decision based on the original code comment ?

Yep, that's my belief as well, the logic/behaviour is not documented, neither as a comment nor in commit history, so we're stuck in a guess game.

There may be a trade-off in execution performance (by allowing more expressions to be considered "valid" and cached) and memory utilization (due to the larger number of potential expressions now being cached).

Very true, as with any caching strategy: you trade off CPU or latency against memory.

Having expressions considered "valid" if-and-only-if they can be executed successfully may have provided a limiting factor to keep the cache size more manageable in the past, but that is just my speculation.

Definitely. However, I do think that this is a weird way to implement memory usage control.
The problem I see is that the fact that an expression evaluates successfully or not does not depend on the expression itself, but on the context it's evaluated against.
Such an expression may fail to execute 1000 times, and succeeds on the next attempt (because using a different context which allows the expression to evaluate successfully). So all in a sudden, the expression gets cached...
All in all, the condition to cache a valid expression entirely depends onto the context we throw at it.
Weird, isn't it?

If we care about memory control, I would then rather go using a real cache with a proper eviction policy, such as https://github.com/ben-manes/caffeine (I use it a lot, it's rock stable, highly efficient and extremely flexible).
This would allow to evict least frequently used expressions, which can even be balanced against its cost to build if needed. This could be bounded or not, time-based...
(we'd need to check the minimum JDK version supported by Struts2, though, since caffeine requires JDK 8 minimum if I remember correctly)

This way, any application generating a very large set of expressions would still get the benefit of caching for the more frequently used expressions without growing the cache too large. And expressions get cached irrespective of any evaluation result.

A small set of configuration knobs might be required as well (to allow user control on cache retention duration, bounded/unbounded and upper bound if needed...).
And going this way could only go into the 2.6.x branch of course.

This change may be safer to attempt for the 2.6.x branch (as opposed to PR #503 for 2.5.x), since it introduces a new behaviour for the expression cache.

If we prefer keeping the changes low, yes, of course, we could have that change in 2.6.x only.

I tried looking at ASTChain processing, and the behaviour you mentioned above, by debugging really basic application. From what I can determine so far, the cases where the source can be null looks like it was probably a design choice. It seems to happen for compound node statements where there is no value for the "parent node", so the "child node" cannot have a value. I am not 100% certain, though, so if someone can see and explain things clearly, please feel free to comment. :)

Looking into using a full-fledged cache (like cache2k or caffeine, possibly using the JSR107/JCache API to be provider-agnostic) for the expression cache could be interesting. Alternatively, implementing a slightly more sophisticated customization of the existing cache (without introducing any new dependencies), could be interesting too. That could be a new feature request for 2.6.x.

As for this PR and 2.6.x, given the explanation and arguments you have provided, I think it seems reasonable to apply to 2.6.x, and see how things behave. 👍

Thanks @JCgH4164838Gh792C124B5 !

I do have the same analysis as yours re: the ASTChain logic - this looks by design, but no clue as to whether this is normal or not.

I opened up the new issue https://issues.apache.org/jira/browse/OGNL-261 to continue to discuss about these OGNL internals and get this PR limited to the OGNL expression caching scope.

Thanks a lot @davoustp for the explanations. My only concern was cache as with caching all the valid expressions DDoS can be easily performed - yet I think this can be also improved by using a better caching mechanism.

I opt to merge this PR, but only support those changes in Struts 2.6.x - I wouldn't introduce such behaviour in 2.5.x (also I just want to focus on 2.6.x branch, tbh)

Hi @lukaszlenart,
Please feel free to accept and merge this PR and reject PR #503 (Struts 2.5.x), as you see fit.
(I do understand the concern about being focused ;-) )

Thanks a lot @davoustp 💯