WW-5326 docs: add Jira reference to spec and plan

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2026-08-05 14:47:09 +00:00 · 2026-04-06 19:18:21 +02:00
parent 1dda92ed23
commit 803b5cbbd0
3 changed files with 1567 additions and 0 deletions
@@ -0,0 +1,272 @@
+# Hibernate Proxy Detection Optimization
+
+> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
+
+**Goal:** Eliminate `LinkageError` exceptions thrown when Hibernate is not on the classpath by detecting availability once at class-load time.
+
+**Architecture:** Add a static availability check in `StrutsProxyService` that probes for `org.hibernate.proxy.HibernateProxy` once during class initialization. All Hibernate-related methods short-circuit immediately when Hibernate is absent. Same pattern applied to deprecated `ProxyUtil`.
+
+**Tech Stack:** Java 17, JUnit 5, AssertJ, Mockito
+
+---
+
+### Task 1: Add Hibernate Availability Check to StrutsProxyService
+
+**Files:**
+- Modify: `core/src/main/java/org/apache/struts2/util/StrutsProxyService.java`
+- Test: `core/src/test/java/org/apache/struts2/util/StrutsProxyServiceTest.java`
+
+- [ ] **Step 1: Write the failing test — verify no LinkageError is thrown when Hibernate classes are used**
+
+The existing tests already call `isHibernateProxy()` and `isHibernateProxyMember()` with non-Hibernate objects. We need a test that verifies the short-circuit behavior works correctly. Add this test to `StrutsProxyServiceTest.java`:
+
+```java
+@Test
+public void isHibernateProxyDoesNotThrowWhenCalledRepeatedly() {
+    // Verify that calling isHibernateProxy many times for different objects
+    // does not cause performance issues (no exceptions thrown internally)
+    for (int i = 0; i < 1000; i++) {
+        assertThat(proxyService.isHibernateProxy(new Object())).isFalse();
+    }
+}
+
+@Test
+public void isHibernateProxyMemberDoesNotThrowWhenCalledRepeatedly() throws NoSuchMethodException {
+    Method method = Object.class.getMethod("toString");
+    for (int i = 0; i < 1000; i++) {
+        assertThat(proxyService.isHibernateProxyMember(method)).isFalse();
+    }
+}
+```
+
+- [ ] **Step 2: Run tests to verify they pass (baseline — these pass even without the fix because Hibernate IS on the test classpath)**
+
+Run: `mvn test -DskipAssembly -pl core -Dtest=StrutsProxyServiceTest#isHibernateProxyDoesNotThrowWhenCalledRepeatedly+isHibernateProxyMemberDoesNotThrowWhenCalledRepeatedly`
+Expected: PASS
+
+- [ ] **Step 3: Add static Hibernate availability flag to StrutsProxyService**
+
+In `core/src/main/java/org/apache/struts2/util/StrutsProxyService.java`, add a static availability check at the top of the class and modify the three Hibernate methods to short-circuit:
+
+```java
+// Add this field near the top of the class, after the class declaration:
+private static final boolean HIBERNATE_AVAILABLE = isHibernateAvailable();
+
+private static boolean isHibernateAvailable() {
+    try {
+        Class.forName("org.hibernate.proxy.HibernateProxy");
+        return true;
+    } catch (ClassNotFoundException e) {
+        return false;
+    }
+}
+```
+
+Then modify the three Hibernate methods to short-circuit:
+
+**`isHibernateProxy`** — change from:
+```java
+@Override
+public boolean isHibernateProxy(Object object) {
+    try {
+        return object != null && HibernateProxy.class.isAssignableFrom(object.getClass());
+    } catch (LinkageError ignored) {
+        return false;
+    }
+}
+```
+to:
+```java
+@Override
+public boolean isHibernateProxy(Object object) {
+    if (!HIBERNATE_AVAILABLE || object == null) {
+        return false;
+    }
+    try {
+        return HibernateProxy.class.isAssignableFrom(object.getClass());
+    } catch (LinkageError ignored) {
+        return false;
+    }
+}
+```
+
+**`isHibernateProxyMember`** — change from:
+```java
+@Override
+public boolean isHibernateProxyMember(Member member) {
+    try {
+        return hasMember(HibernateProxy.class, member);
+    } catch (LinkageError ignored) {
+        return false;
+    }
+}
+```
+to:
+```java
+@Override
+public boolean isHibernateProxyMember(Member member) {
+    if (!HIBERNATE_AVAILABLE) {
+        return false;
+    }
+    try {
+        return hasMember(HibernateProxy.class, member);
+    } catch (LinkageError ignored) {
+        return false;
+    }
+}
+```
+
+**`getHibernateProxyTarget`** — change from:
+```java
+@Override
+public Object getHibernateProxyTarget(Object object) {
+    try {
+        return Hibernate.unproxy(object);
+    } catch (LinkageError ignored) {
+        return object;
+    }
+}
+```
+to:
+```java
+@Override
+public Object getHibernateProxyTarget(Object object) {
+    if (!HIBERNATE_AVAILABLE) {
+        return object;
+    }
+    try {
+        return Hibernate.unproxy(object);
+    } catch (LinkageError ignored) {
+        return object;
+    }
+}
+```
+
+- [ ] **Step 4: Run the full StrutsProxyService test suite**
+
+Run: `mvn test -DskipAssembly -pl core -Dtest=StrutsProxyServiceTest`
+Expected: All tests PASS
+
+- [ ] **Step 5: Run the Spring integration test suite**
+
+Run: `mvn test -DskipAssembly -pl core -Dtest=StrutsProxyServiceSpringIntegrationTest`
+Expected: All tests PASS
+
+- [ ] **Step 6: Commit**
+
+```bash
+git add core/src/main/java/org/apache/struts2/util/StrutsProxyService.java core/src/test/java/org/apache/struts2/util/StrutsProxyServiceTest.java
+git commit -m "WW-5622 Optimize Hibernate proxy detection to avoid LinkageError exceptions
+
+Add static availability check for Hibernate classes in StrutsProxyService.
+When Hibernate is not on the classpath, all Hibernate-related methods
+short-circuit immediately without throwing/catching LinkageError.
+This eliminates a significant performance penalty for applications
+that don't use Hibernate."
+```
+
+---
+
+### Task 2: Apply Same Fix to Deprecated ProxyUtil
+
+**Files:**
+- Modify: `core/src/main/java/org/apache/struts2/util/ProxyUtil.java`
+
+- [ ] **Step 1: Add the same static availability check to ProxyUtil**
+
+In `core/src/main/java/org/apache/struts2/util/ProxyUtil.java`, add the same pattern:
+
+```java
+// Add after the isProxyMemberCache field:
+private static final boolean HIBERNATE_AVAILABLE = isHibernateAvailable();
+
+private static boolean isHibernateAvailable() {
+    try {
+        Class.forName("org.hibernate.proxy.HibernateProxy");
+        return true;
+    } catch (ClassNotFoundException e) {
+        return false;
+    }
+}
+```
+
+Then modify the three Hibernate methods in ProxyUtil identically to Task 1:
+
+**`isHibernateProxy`**:
+```java
+@Deprecated(since = "7.2")
+public static boolean isHibernateProxy(Object object) {
+    if (!HIBERNATE_AVAILABLE || object == null) {
+        return false;
+    }
+    try {
+        return HibernateProxy.class.isAssignableFrom(object.getClass());
+    } catch (LinkageError ignored) {
+        return false;
+    }
+}
+```
+
+**`isHibernateProxyMember`**:
+```java
+@Deprecated(since = "7.2")
+public static boolean isHibernateProxyMember(Member member) {
+    if (!HIBERNATE_AVAILABLE) {
+        return false;
+    }
+    try {
+        return hasMember(HibernateProxy.class, member);
+    } catch (LinkageError ignored) {
+        return false;
+    }
+}
+```
+
+**`getHibernateProxyTarget`**:
+```java
+@Deprecated(since = "7.2")
+public static Object getHibernateProxyTarget(Object object) {
+    if (!HIBERNATE_AVAILABLE) {
+        return object;
+    }
+    try {
+        return Hibernate.unproxy(object);
+    } catch (LinkageError ignored) {
+        return object;
+    }
+}
+```
+
+- [ ] **Step 2: Run existing ProxyUtil tests**
+
+Run: `mvn test -DskipAssembly -pl core -Dtest=ProxyUtilTest`
+Expected: PASS (or if no dedicated test exists, run the SecurityMemberAccess tests which exercise ProxyUtil indirectly)
+
+Run: `mvn test -DskipAssembly -pl core -Dtest=SecurityMemberAccessTest`
+Expected: PASS
+
+- [ ] **Step 3: Commit**
+
+```bash
+git add core/src/main/java/org/apache/struts2/util/ProxyUtil.java
+git commit -m "WW-5622 Apply same Hibernate availability optimization to deprecated ProxyUtil"
+```
+
+---
+
+### Task 3: Run Full Test Suite
+
+- [ ] **Step 1: Run all core tests**
+
+Run: `mvn test -DskipAssembly -pl core`
+Expected: All tests PASS
+
+- [ ] **Step 2: Run spring plugin tests (exercises proxy detection heavily)**
+
+Run: `mvn test -DskipAssembly -pl plugins/spring`
+Expected: All tests PASS
+
+- [ ] **Step 3: Run json plugin tests (StrutsJSONWriter has Hibernate-related class name checks)**
+
+Run: `mvn test -DskipAssembly -pl plugins/json`
+Expected: All tests PASS
@@ -0,0 +1,263 @@
+# OGNL 3.5.x Upgrade — Design Spec
+
+> **Jira:** [WW-5326](https://issues.apache.org/jira/browse/WW-5326)
+
+## Goal
+
+Upgrade Apache Struts from OGNL 3.4.10 to OGNL 3.5.0-BETA4+ and introduce `StrutsContext extends OgnlContext<StrutsContext>` as the framework's own OGNL evaluation context. This lays the foundation for treating OGNL as an execution sandbox with typed, Struts-specific context state.
+
+## Motivation
+
+- **Forward-looking maintenance**: stay current with OGNL development, avoid a larger migration later
+- **Real-world validation**: Struts is the primary consumer of OGNL — upgrading validates the 3.5.x generic API
+- **Java 17 baseline**: OGNL 3.5.x requires Java 17, aligning with Struts 7.x
+- **Type safety**: self-bounded generics (`OgnlContext<C>`) enable Struts to have a properly typed context instead of stringly-typed map entries
+- **Sandbox foundation**: `StrutsContext` is the first step toward isolated OGNL evaluation contexts (`OgnlRuntime` instance-based isolation is future work)
+
+## Non-Goals
+
+- Instance-based `OgnlRuntime` / true sandbox isolation (future OGNL work)
+- Consuming new OGNL features (null-safe operator `?.`, dual-mode evaluation) — those come as separate follow-ups
+- Behavioral changes to security model, accessor logic, or expression evaluation
+
+## Current State
+
+### OGNL Usage in Struts
+
+- **Version**: 3.4.10 (defined in root `pom.xml` as `ognl.version`)
+- **Core dependency**: `core/pom.xml` depends on `ognl:ognl`
+- **Context creation**: 3 call sites use `Ognl.createDefaultContext()`:
+  - `OgnlUtil.createDefaultContext()` (line 738)
+  - `OgnlValueStack.setRoot()` (line 124)
+  - `OgnlReflectionContextFactory.createDefaultContext()` (line 33)
+- **No custom OgnlContext subclass**: Struts uses `OgnlContext` directly
+- **Context state via map entries**: flags like `DENY_METHOD_EXECUTION`, `CREATE_NULL_OBJECTS`, `VALUE_STACK`, conversion state — all stored as stringly-typed map entries in `OgnlContext` and accessed via `ReflectionContextState` static methods
+
+### OGNL Interface Implementations in Struts
+
+| Interface | Struts Implementation |
+|---|---|
+| `MemberAccess` | `SecurityMemberAccess` |
+| `TypeConverter` | `OgnlTypeConverterWrapper` |
+| `ClassResolver` | `RootAccessor` (interface), `CompoundRootAccessor` (impl) |
+| `PropertyAccessor` | `RootAccessor`, `CompoundRootAccessor`, `ObjectProxyPropertyAccessor`, + 8 classes extending `ObjectPropertyAccessor`/`ListPropertyAccessor`/`MapPropertyAccessor`/etc. |
+| `MethodAccessor` | `RootAccessor`, `CompoundRootAccessor`, `XWorkMethodAccessor` |
+| `NullHandler` | `OgnlNullHandlerWrapper` |
+
+### Tiles Plugin OGNL Usage
+
+6-8 files in `plugins/tiles` use OGNL directly:
+- `ScopePropertyAccessor`, `AnyScopePropertyAccessor`, `NestedObjectDelegatePropertyAccessor`, `DelegatePropertyAccessor`
+- `OGNLAttributeEvaluator`, `PropertyAccessorDelegateFactory`, `TilesContextPropertyAccessorDelegateFactory`
+- Associated test files
+
+## OGNL 3.5.x Key API Changes
+
+### Self-Bounded Generics
+
+All core interfaces and classes are now generic with `<C extends OgnlContext<C>>`:
+
+```java
+public class OgnlContext<C extends OgnlContext<C>> implements Map<String, Object>
+public interface MemberAccess<C extends OgnlContext<C>>
+public interface ClassResolver<C extends OgnlContext<C>>
+public interface TypeConverter<C extends OgnlContext<C>>
+public interface PropertyAccessor<C extends OgnlContext<C>>
+public interface MethodAccessor<C extends OgnlContext<C>>
+public interface NullHandler<C extends OgnlContext<C>>
+public class ObjectPropertyAccessor<C extends OgnlContext<C>> implements PropertyAccessor<C>
+// ... all base accessor classes similarly parameterized
+```
+
+### OgnlContext Constructor Changes
+
+```java
+// New (memberAccess first, required non-null)
+public OgnlContext(MemberAccess<C> memberAccess, ClassResolver<C> classResolver, TypeConverter<C> typeConverter)
+
+// Deprecated (old parameter order)
+@Deprecated(forRemoval = true)
+OgnlContext(ClassResolver<C> classResolver, TypeConverter<C> typeConverter, MemberAccess<C> memberAccess)
+```
+
+### OgnlContext.Builder
+
+```java
+public static class Builder<C extends OgnlContext<C>> {
+    public Builder(Function<Builder<C>, C> provider)
+    public Builder<C> withMemberAccess(MemberAccess<C> memberAccess)
+    public Builder<C> withClassResolver(ClassResolver<C> classResolver)
+    public Builder<C> withTypeConverter(TypeConverter<C> converter)
+    public Builder<C> withRoot(Object value)
+    public C build()
+}
+```
+
+### Other Changes
+
+- `SecurityManager` support removed
+- Null-safe navigation operator (`?.`) added
+- `setRoot()` deprecated in favor of `withRoot()` (fluent)
+- Java 17 baseline
+
+### Unchanged
+
+- `Ognl` class remains abstract with only static methods (no instance-based evaluation)
+- `OgnlRuntime` remains a static utility (global accessor/cache registration)
+
+## Design
+
+### Approach: Direct StrutsContext Construction
+
+Struts creates `StrutsContext` directly, bypassing `Ognl.createDefaultContext()`. This gives Struts full ownership of context lifecycle and avoids the global-state issues of `Ognl.withBuilderProvider()`.
+
+### StrutsContext
+
+```java
+package org.apache.struts2.ognl;
+
+public class StrutsContext extends OgnlContext<StrutsContext> {
+
+    // Phase 1: just the constructor, delegate to super
+    public StrutsContext(SecurityMemberAccess memberAccess,
+                         RootAccessor resolver,
+                         OgnlTypeConverterWrapper converter) {
+        super(memberAccess, resolver, converter);
+    }
+
+    // Phase 2 (incremental): promote map entries to typed fields
+    // private ValueStack valueStack;
+    // private boolean reportErrorsOnNoProperty;
+    // private boolean throwExceptionOnFailure;
+    // private boolean createNullObjects;
+    // private boolean denyMethodExecution;
+    // private boolean denyIndexedAccessExecution;
+    // private String conversionPropertyFullName;
+    // private String currentPropertyPath;
+    // private Class<?> lastBeanClassAccessed;
+    // private String lastBeanPropertyAccessed;
+}
+```
+
+Phase 1 introduces the class with zero behavioral change — it's just an `OgnlContext` subclass. The typed fields are a follow-up.
+
+### Generic Type Ripple
+
+All OGNL interface implementations parameterize with `<StrutsContext>`:
+
+```java
+// Core interfaces
+public class SecurityMemberAccess implements MemberAccess<StrutsContext>
+public class OgnlTypeConverterWrapper implements ognl.TypeConverter<StrutsContext>
+public interface RootAccessor extends PropertyAccessor<StrutsContext>, MethodAccessor<StrutsContext>, ClassResolver<StrutsContext>
+public class OgnlNullHandlerWrapper implements ognl.NullHandler<StrutsContext>
+
+// Accessors (extend generic base classes)
+public class CompoundRootAccessor implements RootAccessor
+public class ObjectProxyPropertyAccessor implements PropertyAccessor<StrutsContext>
+public class ObjectAccessor extends ObjectPropertyAccessor<StrutsContext>
+public class ParameterPropertyAccessor extends ObjectPropertyAccessor<StrutsContext>
+public class HttpParametersPropertyAccessor extends ObjectPropertyAccessor<StrutsContext>
+public class XWorkObjectPropertyAccessor extends ObjectPropertyAccessor<StrutsContext>
+public class XWorkEnumerationAccessor extends EnumerationPropertyAccessor<StrutsContext>  // verify base class
+public class XWorkIteratorPropertyAccessor extends IteratorPropertyAccessor<StrutsContext>  // verify base class
+public class XWorkCollectionPropertyAccessor extends ObjectPropertyAccessor<StrutsContext>
+public class XWorkListPropertyAccessor extends ListPropertyAccessor<StrutsContext>
+public class XWorkMapPropertyAccessor extends MapPropertyAccessor<StrutsContext>
+public class XWorkMethodAccessor extends ObjectMethodAccessor<StrutsContext>
+```
+
+Method signatures change `OgnlContext` parameters to `StrutsContext` throughout.
+
+### Context Creation
+
+Replace `Ognl.createDefaultContext()` with direct construction:
+
+```java
+// OgnlUtil.createDefaultContext()
+protected StrutsContext createDefaultContext(Object root, ClassResolver<StrutsContext> resolver) {
+    if (resolver == null) {
+        resolver = container.getInstance(RootAccessor.class);
+    }
+    StrutsContext ctx = new StrutsContext(
+        container.getInstance(SecurityMemberAccess.class), resolver, defaultConverter);
+    ctx.withRoot(root);
+    return ctx;
+}
+
+// OgnlValueStack.setRoot()
+StrutsContext ognlContext = new StrutsContext(securityMemberAccess, accessor,
+    new OgnlTypeConverterWrapper(xworkConverter));
+ognlContext.withRoot(this.root);
+
+// OgnlReflectionContextFactory — already @Deprecated(forRemoval=true) since 6.8.0
+// Keep using Ognl.createDefaultContext(root) with raw type, or remove entirely
+```
+
+### Tiles Plugin
+
+The tiles plugin accessors operate on tiles-specific objects, not on `StrutsContext` directly. Options:
+- Parameterize with raw `OgnlContext` (use `PropertyAccessor<OgnlContext>`) if OGNL allows it
+- Use wildcard `PropertyAccessor<?>` if supported
+- Parameterize with `StrutsContext` if tiles always runs within a Struts context
+
+Decision: determine during implementation based on what compiles cleanly.
+
+### XWorkTypeConverterWrapper
+
+Currently casts `Map` context to `OgnlContext`. After upgrade, `ognl.TypeConverter<StrutsContext>` passes `StrutsContext` directly — the cast goes away. Struts' own `TypeConverter` interface (in `conversion` package) may also need its `convertValue` signature updated.
+
+### ReflectionContextState
+
+Initially unchanged — continues to work via `Map<String, Object>` interface that `StrutsContext` inherits from `OgnlContext`. Promoting to typed fields is a follow-up.
+
+## Implementation Phases
+
+### Phase 1: Version bump + StrutsContext + generics (this effort)
+
+1. Bump `ognl.version` to `3.5.0-BETA4` in root `pom.xml`
+2. Create `StrutsContext extends OgnlContext<StrutsContext>` (constructor only)
+3. Update all OGNL interface implementations with `<StrutsContext>` type parameter (~20 classes in core)
+4. Update method signatures: `OgnlContext` → `StrutsContext` in all accessor/handler implementations
+5. Replace `Ognl.createDefaultContext()` with direct `StrutsContext` construction (3 call sites)
+6. Update tiles plugin accessor classes (~6-8 files)
+7. Update test files (~50+ files referencing `OgnlContext`)
+8. Verify all tests pass
+
+### Phase 2: Typed context fields (follow-up)
+
+- Promote `ReflectionContextState` map entries to `StrutsContext` typed fields
+- Update accessors to use typed getters instead of `context.get("string.key")`
+- Deprecate `ReflectionContextState` static methods
+
+### Phase 3: Sandbox features (future, requires OGNL changes)
+
+- Instance-based `OgnlRuntime` (OGNL-side work)
+- Per-sandbox accessor registrations
+- Isolated evaluation engines
+
+## Risk Areas
+
+### OgnlRuntime global statics
+
+`OgnlRuntime.setPropertyAccessor(Class<?>, PropertyAccessor<C>)` is generic but the registration is global. Registering `PropertyAccessor<StrutsContext>` may cause unchecked warnings or issues when OGNL internally retrieves accessors with a different context type. May need raw types at registration boundary.
+
+### OGNL internal context preservation
+
+If OGNL internally creates new `OgnlContext` instances during expression evaluation (rather than preserving the passed-in `StrutsContext`), typed fields would be lost. BETA1 addressed "context root preservation during nested evaluations" but this needs runtime verification.
+
+### Tiles plugin type compatibility
+
+Tiles accessors may not naturally fit `StrutsContext` parameterization. Need to determine the right generic type during implementation.
+
+### OGNL BETA stability
+
+OGNL 3.5.0 is still in BETA. API changes may occur in subsequent releases. This is acceptable given the user is an OGNL contributor and can influence the API.
+
+## Expected Outcomes
+
+- Struts compiles and all tests pass against OGNL 3.5.0-BETA4
+- `StrutsContext` exists as the framework's OGNL context class
+- All OGNL interface implementations are properly parameterized with `<StrutsContext>`
+- Foundation is in place for typed context fields and eventual sandbox isolation
+- Any OGNL API issues discovered are reported/fixed upstream