kusalk commented on code in PR #1236:
URL: https://github.com/apache/struts/pull/1236#discussion_r1977302915
##########
core/pom.xml:
##########
@@ -187,13 +187,19 @@
<artifactId>commons-text</artifactId>
</dependency>
- <!-- Optional used in org.apache.struts2.util.ProxyUtil to detect if
object is HibernateProxy -->
<dependency>
+ <!-- Optional used in org.apache.struts2.util.ProxyUtil to detect
if object is HibernateProxy -->
<groupId>org.hibernate</groupId>
<artifactId>hibernate-core</artifactId>
<version>5.6.15.Final</version>
<optional>true</optional>
</dependency>
+ <dependency>
+ <!-- Optional used in org.apache.struts2.util.ProxyUtil to detect
if object is Spring proxy -->
Review Comment:
I'm using the same optional dependency technique we used for Hibernate proxy
resolution - it allows us to avoid reflecting and duplicating code in
`ProxyUtil` leading to improved maintainability
##########
core/src/main/java/org/apache/struts2/ognl/OgnlCache.java:
##########
@@ -15,21 +15,37 @@
*/
package org.apache.struts2.ognl;
+import java.util.function.Function;
+
+import static java.util.Objects.requireNonNull;
+
/**
- * A basic cache interface for use with OGNL processing (such as Expression,
BeanInfo).
- * All OGNL caches will have an eviction limit, but setting an extremely high
value can
- * simulate an "effectively unlimited" cache.
+ * A basic cache interface for use with OGNL processing (such as Expression,
BeanInfo). Implementation must be
+ * thread-safe. All OGNL caches will have an eviction limit, but setting an
extremely high value can simulate an
+ * "effectively unlimited" cache.
*
- * @param <Key> The type for the cache key entries
- * @param <Value> The type for the cache value entries
+ * @param <K> The type for the cache key entries
+ * @param <V> The type for the cache value entries
*/
-public interface OgnlCache<Key, Value> {
+public interface OgnlCache<K, V> {
+
+ V get(K key);
- Value get(Key key);
+ /**
+ * @since 7.1
+ */
+ default V computeIfAbsent(K key,
Review Comment:
Adding a default implementation preserves API compatibility in case some
Struts users have defined their own `OgnlCache` implementations using the
extension points. It allows them to upgrade to Struts 7.1 without requiring any
additional changes
##########
core/src/main/java/org/apache/struts2/util/ProxyUtil.java:
##########
@@ -44,17 +48,14 @@
*
*/
public class ProxyUtil {
- private static final String SPRING_ADVISED_CLASS_NAME =
"org.springframework.aop.framework.Advised";
Review Comment:
Removed these fields which were previously used for reflection based code -
we now use the optional Spring dependency with catch clauses
##########
core/src/main/java/org/apache/struts2/util/ProxyUtil.java:
##########
@@ -65,15 +66,18 @@ public class ProxyUtil {
* object as fallback; never {@code null})
*/
public static Class<?> ultimateTargetClass(Object candidate) {
- Class<?> result = null;
- if (isSpringAopProxy(candidate))
- result = springUltimateTargetClass(candidate);
-
- if (result == null) {
- result = candidate.getClass();
- }
-
- return result;
+ return targetClassCache.computeIfAbsent(candidate, k -> {
Review Comment:
Added caching here too to uplift `SecurityMemberAccess` performance for
applications which have chosen to allow Hibernate and Spring proxies
##########
core/src/main/java/org/apache/struts2/ognl/OgnlUtil.java:
##########
@@ -676,13 +675,19 @@ public BeanInfo getBeanInfo(Object from) throws
IntrospectionException {
* @throws IntrospectionException is thrown if an exception occurs during
introspection.
*/
public BeanInfo getBeanInfo(Class<?> clazz) throws IntrospectionException {
- synchronized (beanInfoCache) {
- BeanInfo beanInfo = beanInfoCache.get(clazz);
- if (beanInfo == null) {
- beanInfo = Introspector.getBeanInfo(clazz, Object.class);
- beanInfoCache.putIfAbsent(clazz, beanInfo);
+ try {
+ return beanInfoCache.computeIfAbsent(clazz, k -> {
Review Comment:
By removing the `synchronized` blcok and using `#computeIfAbsent` we uplift
performance as now we only lock on the class name rather than whenever we enter
this method
##########
core/src/main/java/org/apache/struts2/interceptor/ChainingInterceptor.java:
##########
@@ -167,17 +167,18 @@ public String intercept(ActionInvocation invocation)
throws Exception {
}
private void copyStack(ActionInvocation invocation, CompoundRoot root) {
- List list = prepareList(root);
+ List<Object> list = prepareList(root);
Map<String, Object> ctxMap =
invocation.getInvocationContext().getContextMap();
for (Object object : list) {
- if (shouldCopy(object)) {
- Object action = invocation.getAction();
- Class<?> editable = null;
- if(ProxyUtil.isProxy(action)) {
- editable = ProxyUtil.ultimateTargetClass(action);
- }
- reflectionProvider.copy(object, action, ctxMap,
prepareExcludes(), includes, editable);
+ if (!shouldCopy(object)) {
Review Comment:
Nothing much to see here, just denested this code for slight readability
improvement
##########
core/src/main/java/org/apache/struts2/ognl/OgnlUtil.java:
##########
@@ -676,13 +675,19 @@ public BeanInfo getBeanInfo(Object from) throws
IntrospectionException {
* @throws IntrospectionException is thrown if an exception occurs during
introspection.
*/
public BeanInfo getBeanInfo(Class<?> clazz) throws IntrospectionException {
- synchronized (beanInfoCache) {
- BeanInfo beanInfo = beanInfoCache.get(clazz);
- if (beanInfo == null) {
- beanInfo = Introspector.getBeanInfo(clazz, Object.class);
- beanInfoCache.putIfAbsent(clazz, beanInfo);
+ try {
+ return beanInfoCache.computeIfAbsent(clazz, k -> {
+ try {
+ return Introspector.getBeanInfo(k, Object.class);
+ } catch (IntrospectionException e) {
+ throw new IllegalArgumentException(e);
+ }
+ });
+ } catch (IllegalArgumentException e) {
Review Comment:
This is rather ugly but it's to ensure backwards compatibility with respect
to the exception throwing behaviour. The lambda we pass to `#computeIfAbsent`
doesn't allow us to throw checked exceptions
--
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]
