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: issues-unsubscr...@struts.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org