kusalk commented on code in PR #766:
URL: https://github.com/apache/struts/pull/766#discussion_r1359722722
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlDefaultCache.java:
##########
@@ -30,10 +30,10 @@
public class OgnlDefaultCache<Key, Value> implements OgnlCache<Key, Value> {
private final ConcurrentHashMap<Key, Value> ognlCache;
- private final AtomicInteger cacheEvictionLimit = new AtomicInteger(25000);
+ private final AtomicInteger cacheEvictionLimit = new AtomicInteger();
Review Comment:
The overriding is a fair point - I'll move the initialisation of the whole
field into the constructor
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java:
##########
@@ -673,9 +673,15 @@ public void copy(final Object from, final Object to, final
Map<String, Object> c
* note if exclusions AND inclusions are supplied and
not null nothing will get copied.
* @param editable the class (or interface) to restrict property setting to
*/
- public void copy(final Object from, final Object to, final Map<String,
Object> context, Collection<String> exclusions, Collection<String> inclusions,
Class<?> editable) {
+ public void copy(final Object from,
+ final Object to,
+ final Map<String, Object> context,
+ Collection<String> exclusions,
+ Collection<String> inclusions,
+ Class<?> editable) {
if (from == null || to == null) {
- LOG.warn("Attempting to copy from or to a null source. This is
illegal and is bein skipped. This may be due to an error in an OGNL expression,
action chaining, or some other event.");
+ LOG.warn(
Review Comment:
Currently, this method is only called by `ChainingInterceptor` within
Struts, and from what I can tell, these parameters will never be null. Although
it is also a public method. Calling this method with null values is improper
usage and the stack trace will provide better context for such an issue to be
resolved. The previous warning message wasn't very helpful.
##########
core/src/main/resources/org/apache/struts2/default.properties:
##########
@@ -243,25 +243,25 @@ struts.ognl.enableExpressionCache=true
### For expressionCacheLRUMode true, the limit will ensure the cache does not
exceed
### that size, dropping the oldest (least-recently-used) expressions to add
new ones.
### NOTE: If not set, the default is 25000, which may be excessive.
-# struts.ognl.expressionCacheMaxSize=1000
+struts.ognl.expressionCacheMaxSize=10000
Review Comment:
Yeah I agree that it would be better to have the defaults in the code and
properties file be consistent (as well as the accompanying comments in the
properties file). I'll amend this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
