kusalk commented on code in PR #1573: URL: https://github.com/apache/struts/pull/1573#discussion_r2832605187
########## core/src/main/java/com/opensymphony/xwork2/util/StrutsProxyCacheFactoryBean.java: ########## @@ -0,0 +1,38 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package com.opensymphony.xwork2.util; + +import com.opensymphony.xwork2.inject.Inject; +import com.opensymphony.xwork2.ognl.ProxyCacheFactory; + +/** + * Bean that wires the ProxyCacheFactory to ProxyUtil during container initialization. + * <p> + * This bean is created by the container and receives the configured ProxyCacheFactory + * via dependency injection, then passes it to the static ProxyUtil class. + * + * @since 6.8.0 Review Comment: 6.9 right? ########## core/src/main/java/com/opensymphony/xwork2/util/ProxyUtil.java: ########## @@ -51,15 +51,64 @@ public class ProxyUtil { private static final String HIBERNATE_HIBERNATEPROXY_CLASS_NAME = "org.hibernate.proxy.HibernateProxy"; private static final int CACHE_MAX_SIZE = 10000; private static final int CACHE_INITIAL_CAPACITY = 256; - private static final OgnlCache<Class<?>, Boolean> isProxyCache = new DefaultOgnlCacheFactory<Class<?>, Boolean>( - CACHE_MAX_SIZE, OgnlCacheFactory.CacheType.WTLFU, CACHE_INITIAL_CAPACITY).buildOgnlCache(); - private static final OgnlCache<Member, Boolean> isProxyMemberCache = new DefaultOgnlCacheFactory<Member, Boolean>( - CACHE_MAX_SIZE, OgnlCacheFactory.CacheType.WTLFU, CACHE_INITIAL_CAPACITY).buildOgnlCache(); + + // Holder for the cache factory (set by container) + private static volatile ProxyCacheFactory<?, ?> cacheFactory; + + // Lazy-initialized caches + private static volatile OgnlCache<Class<?>, Boolean> isProxyCache; + private static volatile OgnlCache<Member, Boolean> isProxyMemberCache; + + /** + * Sets the cache factory. Called by the container during initialization. + * + * @param factory the cache factory to use for creating proxy caches + * @since 6.8.0 + */ + public static void setProxyCacheFactory(ProxyCacheFactory<?, ?> factory) { + cacheFactory = factory; + } + + @SuppressWarnings("unchecked") + private static OgnlCache<Class<?>, Boolean> getIsProxyCache() { + if (isProxyCache == null) { Review Comment: Could extract lazy initialisation logic into a separate utility class for better readability in this class, and it prevents anyone from accidentally accessing the uninitialised field directly ``` public class LazyRef<T> implements Supplier<T> { private volatile T data; private final Supplier<T> supplier; public LazyRef(Supplier<T> supplier) { this.supplier = supplier; } @Override public T get() { T result = data; if (result == null) { synchronized (this) { result = data; if (result == null) { data = result = supplier.get(); } } } return result; } public synchronized void reset() { data = null; } } ``` then the field declarations should become something like: ``` private static LazyRef<OgnlCache<Class<?>, Boolean>> isProxyCache = new LazyRef(() -> createCache()); private static LazyRef<OgnlCache<Member, Boolean>> isProxyMemberCache = new LazyRef(() -> createCache()); ``` ########## core/src/main/resources/org/apache/struts2/default.properties: ########## @@ -247,6 +247,14 @@ struts.ognl.beanInfoCacheType=wtlfu ### application-specific needs. struts.ognl.beanInfoCacheMaxSize=10000 +### Specifies the type of cache to use for proxy detection in ProxyUtil. +### Valid values: basic, lru, wtlfu. Default is 'basic' (no Caffeine dependency required). +### Use 'wtlfu' for better eviction policy if Caffeine is available. +struts.proxy.cacheType=basic Review Comment: All the other caches default to `wtlfu` (i.e. performant by default), let's do that here too ########## core/src/main/java/com/opensymphony/xwork2/util/ProxyUtil.java: ########## @@ -51,15 +51,64 @@ public class ProxyUtil { private static final String HIBERNATE_HIBERNATEPROXY_CLASS_NAME = "org.hibernate.proxy.HibernateProxy"; private static final int CACHE_MAX_SIZE = 10000; private static final int CACHE_INITIAL_CAPACITY = 256; - private static final OgnlCache<Class<?>, Boolean> isProxyCache = new DefaultOgnlCacheFactory<Class<?>, Boolean>( - CACHE_MAX_SIZE, OgnlCacheFactory.CacheType.WTLFU, CACHE_INITIAL_CAPACITY).buildOgnlCache(); - private static final OgnlCache<Member, Boolean> isProxyMemberCache = new DefaultOgnlCacheFactory<Member, Boolean>( - CACHE_MAX_SIZE, OgnlCacheFactory.CacheType.WTLFU, CACHE_INITIAL_CAPACITY).buildOgnlCache(); + + // Holder for the cache factory (set by container) + private static volatile ProxyCacheFactory<?, ?> cacheFactory; + + // Lazy-initialized caches + private static volatile OgnlCache<Class<?>, Boolean> isProxyCache; + private static volatile OgnlCache<Member, Boolean> isProxyMemberCache; + + /** + * Sets the cache factory. Called by the container during initialization. + * + * @param factory the cache factory to use for creating proxy caches + * @since 6.8.0 + */ + public static void setProxyCacheFactory(ProxyCacheFactory<?, ?> factory) { + cacheFactory = factory; Review Comment: This is a bug I'm pretty sure, since the fields are statically initialised once, when the container is reset or if a `ProxyUtil` method is called prior to injection, the injected factory won't take effect I think the following will fix it: ``` cacheFactory = factory; isProxyCache.reset(); isProxyMemberCache.reset(); ``` -- 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]
