Skip to content

WW-5534 Simplify ProxyUtil, add OgnlCache#computeIfAbsent #1236

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kusalk merged 2 commits into from
Mar 5, 2025
Merged

WW-5534 Simplify ProxyUtil, add OgnlCache#computeIfAbsent #1236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8579a10
WW-5534 Simplify ProxyUtil, add OgnlCache#computeIfAbsent
kusalk Feb 27, 2025
117b9c7
WW-5534 Add coverage for OgnlUtil#getBeanInfo exception propagation
kusalk Mar 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion core/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 -->
Copy link
Member Author

@kusalk kusalk Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

<groupId>org.springframework</groupId>
<artifactId>spring-aop</artifactId>
<optional>true</optional>
</dependency>

<dependency>
<groupId>org.springframework</groupId>
Expand Down
24 changes: 12 additions & 12 deletions core/src/main/java/org/apache/struts2/interceptor/ChainingInterceptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,11 @@

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.struts2.result.ActionChainResult;
import org.apache.struts2.ActionInvocation;
import org.apache.struts2.StrutsConstants;
import org.apache.struts2.Unchainable;
import org.apache.struts2.inject.Inject;
import org.apache.struts2.result.ActionChainResult;
import org.apache.struts2.result.Result;
import org.apache.struts2.util.CompoundRoot;
import org.apache.struts2.util.ProxyUtil;
Expand Down Expand Up @@ -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)) {
Copy link
Member Author

@kusalk kusalk Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing much to see here, just denested this code for slight readability improvement

continue;
}
Object action = invocation.getAction();
Class<?> editable = null;
if (ProxyUtil.isProxy(action)) {
editable = ProxyUtil.ultimateTargetClass(action);
}
reflectionProvider.copy(object, action, ctxMap, prepareExcludes(), includes, editable);
}
}

Expand All @@ -204,9 +205,8 @@ private boolean shouldCopy(Object o) {
return o != null && !(o instanceof Unchainable);
}

@SuppressWarnings("unchecked")
private List prepareList(CompoundRoot root) {
List list = new ArrayList(root);
private List<Object> prepareList(CompoundRoot root) {
var list = new ArrayList<>(root);
list.remove(0);
Collections.reverse(list);
return list;
Expand Down
34 changes: 25 additions & 9 deletions core/src/main/java/org/apache/struts2/ognl/OgnlCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Member Author

@kusalk kusalk Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Function<? super K, ? extends V> mappingFunction) {
requireNonNull(mappingFunction);
if (get(key) == null) {
putIfAbsent(key, mappingFunction.apply(key));
}
return get(key);
}

void put(Key key, Value value);
void put(K key, V value);

void putIfAbsent(Key key, Value value);
void putIfAbsent(K key, V value);

int size();

Expand Down
7 changes: 7 additions & 0 deletions core/src/main/java/org/apache/struts2/ognl/OgnlCaffeineCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
import com.github.benmanes.caffeine.cache.Cache;
import com.github.benmanes.caffeine.cache.Caffeine;

import java.util.function.Function;

/**
* <p>This OGNL Cache implementation is backed by {@link Caffeine} which uses the Window TinyLfu algorithm.</p>
*
Expand Down Expand Up @@ -46,6 +48,11 @@ public V get(K key) {
return cache.getIfPresent(key);
}

@Override
public V computeIfAbsent(K key, Function<? super K, ? extends V> mappingFunction) {
return cache.asMap().computeIfAbsent(key, mappingFunction);
}

@Override
public void put(K key, V value) {
cache.put(key, value);
Expand Down
16 changes: 11 additions & 5 deletions core/src/main/java/org/apache/struts2/ognl/OgnlDefaultCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.function.Function;

/**
* <p>Basic OGNL cache implementation.</p>
Expand Down Expand Up @@ -45,16 +46,21 @@ public V get(K key) {
return ognlCache.get(key);
}

@Override
public V computeIfAbsent(K key, Function<? super K, ? extends V> mappingFunction) {
return ognlCache.computeIfAbsent(key, mappingFunction);
}

@Override
public void put(K key, V value) {
ognlCache.put(key, value);
this.clearIfEvictionLimitExceeded();
clearIfEvictionLimitExceeded();
}

@Override
public void putIfAbsent(K key, V value) {
ognlCache.putIfAbsent(key, value);
this.clearIfEvictionLimitExceeded();
clearIfEvictionLimitExceeded();
}

@Override
Expand All @@ -69,12 +75,12 @@ public void clear() {

@Override
public int getEvictionLimit() {
return this.cacheEvictionLimit.get();
return cacheEvictionLimit.get();
}

@Override
public void setEvictionLimit(int cacheEvictionLimit) {
this.cacheEvictionLimit.set(cacheEvictionLimit);
public void setEvictionLimit(int newCacheEvictionLimit) {
cacheEvictionLimit.set(newCacheEvictionLimit);
}

/**
Expand Down
6 changes: 6 additions & 0 deletions core/src/main/java/org/apache/struts2/ognl/OgnlLRUCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.function.Function;

/**
* <p>A basic OGNL LRU cache implementation.</p>
Expand Down Expand Up @@ -54,6 +55,11 @@ public V get(K key) {
return ognlLRUCache.get(key);
}

@Override
public V computeIfAbsent(K key, Function<? super K, ? extends V> mappingFunction) {
return ognlLRUCache.computeIfAbsent(key, mappingFunction);
}

@Override
public void put(K key, V value) {
ognlLRUCache.put(key, value);
Expand Down
31 changes: 18 additions & 13 deletions core/src/main/java/org/apache/struts2/ognl/OgnlUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,6 @@
*/
package org.apache.struts2.ognl;

import org.apache.struts2.conversion.impl.XWorkConverter;
import org.apache.struts2.inject.Container;
import org.apache.struts2.inject.Inject;
import org.apache.struts2.ognl.accessor.RootAccessor;
import org.apache.struts2.util.CompoundRoot;
import org.apache.struts2.util.reflection.ReflectionException;
import ognl.ClassResolver;
import ognl.Ognl;
import ognl.OgnlContext;
Expand All @@ -35,7 +29,12 @@
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.struts2.StrutsConstants;
import org.apache.struts2.ognl.OgnlGuard;
import org.apache.struts2.conversion.impl.XWorkConverter;
import org.apache.struts2.inject.Container;
import org.apache.struts2.inject.Inject;
import org.apache.struts2.ognl.accessor.RootAccessor;
import org.apache.struts2.util.CompoundRoot;
import org.apache.struts2.util.reflection.ReflectionException;

import java.beans.BeanInfo;
import java.beans.IntrospectionException;
Expand Down Expand Up @@ -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 -> {
Copy link
Member Author

@kusalk kusalk Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

try {
return Introspector.getBeanInfo(k, Object.class);
} catch (IntrospectionException e) {
throw new IllegalArgumentException(e);
}
});
} catch (IllegalArgumentException e) {
Copy link
Member Author

@kusalk kusalk Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (e.getCause() instanceof IntrospectionException innerEx) {
throw innerEx;
}
return beanInfo;
throw e;
}
}

Expand Down
Loading
Loading