You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@struts.apache.org by "Copilot (via GitHub)" <gi...@apache.org> on 2025/10/18 10:59:50 UTC
Re: [PR] WW-5326 Upgrade to OGNL 3.5.x and StrutsContext [struts]
Copilot commented on code in PR #1249:
URL: https://github.com/apache/struts/pull/1249#discussion_r2442158045
##########
pom.xml:
##########
@@ -120,7 +120,7 @@
<log4j2.version>2.24.3</log4j2.version>
<maven-surefire-plugin.version>3.5.2</maven-surefire-plugin.version>
<mockito.version>5.15.2</mockito.version>
- <ognl.version>3.3.5</ognl.version>
+ <ognl.version>3.5.0-SNAPSHOT</ognl.version>
Review Comment:
Snapshot dependency (3.5.0-SNAPSHOT) with an activeByDefault snapshot repository profile will leak into all builds. Remove or disable the ognl profile and pin a released OGNL version before merging to avoid unintended snapshot consumption.
```suggestion
<ognl.version>3.4.2</ognl.version>
```
##########
core/src/main/java/org/apache/struts2/util/StrutsTypeConverter.java:
##########
@@ -89,5 +91,5 @@ protected Object performFallbackConversion(Map context, Object o, Class toClass)
* @param o the object to be converted
* @return the converted String
*/
- public abstract String convertToString(Map context, Object o);
+ public abstract String convertToString(StrutsContext context, Object o);
Review Comment:
Javadoc above still references Map-based conversion semantics but method signatures now use StrutsContext. Update documentation to reflect new parameter type and any behavioral differences to prevent confusion in custom converter implementations.
##########
core/src/main/java/org/apache/struts2/ActionContext.java:
##########
@@ -232,11 +242,17 @@ public Map<String, Object> getApplication() {
* Gets the context map.
*
* @return the context map.
+ * @deprecated since 7.1.0, use {@link #getStrutsContext()} instead
*/
+ @Deprecated(since = "7.1.0", forRemoval = true)
public Map<String, Object> getContextMap() {
return context;
}
+ public StrutsContext getStrutsContext() {
+ return context;
Review Comment:
Deprecation notice recommends getStrutsContext(), but getContextMap() still returns the StrutsContext instance directly (which is no longer a plain Map in user intent). Clarify in javadoc that returned object implements Map only for legacy compatibility and encourage migration steps.
##########
core/src/main/java/org/apache/struts2/conversion/impl/XWorkBasicConverter.java:
##########
@@ -185,40 +184,40 @@ private Class doConvertToClass(Object value) {
return clazz;
}
- private Object doConvertToCollection(Map<String, Object> context, Object o, Member member, String prop, Object value, Class toType) {
- TypeConverter converter = container.getInstance(CollectionConverter.class);
+ private Object doConvertToCollection(StrutsContext context, Object o, Member member, String prop, Object value, Class<?> toType) {
+ TypeConverter<StrutsContext> converter = container.getInstance(CollectionConverter.class);
if (converter == null) {
throw new TypeConversionException("TypeConverter with name [#0] must be registered first! Converter: "+ StrutsConstants.STRUTS_CONVERTER_COLLECTION);
}
return converter.convertValue(context, o, member, prop, value, toType);
}
- private Object doConvertToArray(Map<String, Object> context, Object o, Member member, String prop, Object value, Class toType) {
- TypeConverter converter = container.getInstance(ArrayConverter.class);
+ private Object doConvertToArray(StrutsContext context, Object o, Member member, String prop, Object value, Class<?> toType) {
+ TypeConverter<StrutsContext> converter = container.getInstance(ArrayConverter.class);
if (converter == null) {
throw new TypeConversionException("TypeConverter with name [#0] must be registered first! Converter: "+ StrutsConstants.STRUTS_CONVERTER_ARRAY);
}
return converter.convertValue(context, o, member, prop, value, toType);
}
- private Object doConvertToDate(Map<String, Object> context, Object value, Class toType) {
- TypeConverter converter = container.getInstance(DateConverter.class);
+ private Object doConvertToDate(StrutsContext context, Object value, Class<?> toType) {
+ TypeConverter<StrutsContext> converter = container.getInstance(DateConverter.class);
if (converter == null) {
throw new TypeConversionException("TypeConverter with name [#0] must be registered first! Converter: "+ StrutsConstants.STRUTS_CONVERTER_DATE);
}
return converter.convertValue(context, null, null, null, value, toType);
}
- private Object doConvertToNumber(Map<String, Object> context, Object value, Class toType) {
- TypeConverter converter = container.getInstance(NumberConverter.class);
+ private Object doConvertToNumber(StrutsContext context, Object value, Class<?> toType) {
+ TypeConverter<StrutsContext> converter = container.getInstance(NumberConverter.class);
if (converter == null) {
throw new TypeConversionException("TypeConverter with name [#0] must be registered first! Converter: "+ StrutsConstants.STRUTS_CONVERTER_NUMBER);
}
return converter.convertValue(context, null, null, null, value, toType);
}
- private Object doConvertToString(Map<String, Object> context, Object value) {
- TypeConverter converter = container.getInstance(StringConverter.class);
+ private Object doConvertToString(StrutsContext context, Object value) {
Review Comment:
Internal helper methods all accept StrutsContext but some ignore it (e.g., doConvertToString). If context is not used, consider removing the parameter to simplify signatures or document future planned use to justify retention.
##########
core/src/main/java/org/apache/struts2/conversion/impl/InstantiatingNullHandler.java:
##########
@@ -145,7 +148,7 @@ public Object nullPropertyValue(Map<String, Object> context, Object target, Obje
return null;
}
- private Object createObject(Class clazz, Object target, String property, Map<String, Object> context) throws Exception {
+ private Object createObject(Class clazz, Object target, String property, StrutsContext context) throws Exception {
Review Comment:
Javadoc above still refers to ObjectFactory#buildBean(Class, java.util.Map); update the reference to ObjectFactory#buildBean(Class, StrutsContext) to prevent outdated guidance.
##########
core/src/main/java/org/apache/struts2/util/StrutsTypeConverter.java:
##########
@@ -67,7 +69,7 @@ public Object convertValue(Map context, Object o, Class toClass) {
* @param toClass the class to convert to
* @return The fallback conversion
*/
- protected Object performFallbackConversion(Map context, Object o, Class toClass) {
+ protected Object performFallbackConversion(StrutsContext context, Object o, Class toClass) {
Review Comment:
Javadoc above still references Map-based conversion semantics but method signatures now use StrutsContext. Update documentation to reflect new parameter type and any behavioral differences to prevent confusion in custom converter implementations.
##########
pom.xml:
##########
@@ -209,6 +209,23 @@
</plugins>
</build>
</profile>
+ <!-- TODO: remove or disable before merging -->
+ <profile>
+ <id>ognl</id>
+ <activation>
+ <activeByDefault>true</activeByDefault>
+ </activation>
+ <repositories>
+ <repository>
+ <id>oss-snapshots</id>
+ <url>https://oss.sonatype.org/content/repositories/snapshots/</url>
+ <layout>default</layout>
+ <snapshots>
+ <enabled>true</enabled>
+ </snapshots>
+ </repository>
+ </repositories>
+ </profile>
Review Comment:
Snapshot dependency (3.5.0-SNAPSHOT) with an activeByDefault snapshot repository profile will leak into all builds. Remove or disable the ognl profile and pin a released OGNL version before merging to avoid unintended snapshot consumption.
```suggestion
```
##########
core/src/main/java/org/apache/struts2/ObjectFactory.java:
##########
@@ -156,7 +157,7 @@ public Object buildAction(String actionName, String namespace, ActionConfig conf
* @return object for the given type
* @throws Exception in case of any error
*/
- public Object buildBean(Class clazz, Map<String, Object> extraContext) throws Exception {
+ public <T> T buildBean(Class<T> clazz, StrutsContext extraContext) throws Exception {
Review Comment:
The injectInternal parameter in buildBean(String, StrutsContext, boolean) is unused; either implement conditional logic based on it or remove the parameter to avoid dead API surface and potential confusion.
##########
core/src/main/java/org/apache/struts2/util/StrutsTypeConverter.java:
##########
@@ -80,7 +82,7 @@ protected Object performFallbackConversion(Map context, Object o, Class toClass)
* @param toClass the class to convert to
* @return the converted object
*/
- public abstract Object convertFromString(Map context, String[] values, Class toClass);
+ public abstract Object convertFromString(StrutsContext context, String[] values, Class toClass);
Review Comment:
Javadoc above still references Map-based conversion semantics but method signatures now use StrutsContext. Update documentation to reflect new parameter type and any behavioral differences to prevent confusion in custom converter implementations.
##########
core/src/main/java/org/apache/struts2/conversion/TypeConverter.java:
##########
@@ -44,7 +45,7 @@ public interface TypeConverter {
* @return Converted value of type toType or TypeConverter.NoConversionPossible to indicate that the
* conversion was not possible.
*/
- Object convertValue(Map<String, Object> context, Object target, Member member, String propertyName, Object value, Class toType);
+ Object convertValue(C context, Object target, Member member, String propertyName, Object value, Class<?> toType);
Review Comment:
Introducing a generic TypeConverter<C extends StrutsContext> changes the public API; existing code obtaining TypeConverter instances without a type parameter will compile with raw types and produce unchecked warnings. Consider providing a non-generic convenience interface or updating all retrieval sites to use parameterized types to maintain type safety.
##########
core/src/main/java/org/apache/struts2/conversion/TypeConverter.java:
##########
@@ -29,7 +30,7 @@
* @author Luke Blanshard (blanshlu@netscape.net)
* @author Drew Davidson (drew@ognl.org)
*/
-public interface TypeConverter {
+public interface TypeConverter<C extends StrutsContext> {
Review Comment:
Introducing a generic TypeConverter<C extends StrutsContext> changes the public API; existing code obtaining TypeConverter instances without a type parameter will compile with raw types and produce unchecked warnings. Consider providing a non-generic convenience interface or updating all retrieval sites to use parameterized types to maintain type safety.
##########
core/src/main/java/org/apache/struts2/ObjectFactory.java:
##########
@@ -180,7 +181,7 @@ protected Object injectInternalBeans(Object obj) {
* @return object for the given type
* @throws Exception in case of any error
*/
- public Object buildBean(String className, Map<String, Object> extraContext) throws Exception {
+ public <T> T buildBean(String className, StrutsContext extraContext) throws Exception {
Review Comment:
The injectInternal parameter in buildBean(String, StrutsContext, boolean) is unused; either implement conditional logic based on it or remove the parameter to avoid dead API surface and potential confusion.
##########
core/src/main/java/org/apache/struts2/ObjectFactory.java:
##########
@@ -193,8 +194,8 @@ public Object buildBean(String className, Map<String, Object> extraContext) thro
* @return object for the given type
* @throws Exception in case of any error
*/
- public Object buildBean(String className, Map<String, Object> extraContext, boolean injectInternal) throws Exception {
- Class clazz = getClassInstance(className);
+ public <T> T buildBean(String className, StrutsContext extraContext, boolean injectInternal) throws Exception {
+ Class<T> clazz = getClassInstance(className);
return buildBean(clazz, extraContext);
Review Comment:
The injectInternal parameter in buildBean(String, StrutsContext, boolean) is unused; either implement conditional logic based on it or remove the parameter to avoid dead API surface and potential confusion.
##########
core/src/main/java/org/apache/struts2/util/StrutsTypeConverter.java:
##########
@@ -67,7 +69,7 @@ public Object convertValue(Map context, Object o, Class toClass) {
* @param toClass the class to convert to
* @return The fallback conversion
*/
- protected Object performFallbackConversion(Map context, Object o, Class toClass) {
+ protected Object performFallbackConversion(StrutsContext context, Object o, Class toClass) {
Review Comment:
Changing convertValue/performFallbackConversion to accept StrutsContext alters all existing custom StrutsTypeConverter implementations; provide an explicit @since tag and migration guidance (e.g., how to obtain previous Map entries from StrutsContext) to reduce upgrade friction.
##########
core/src/main/java/org/apache/struts2/DefaultActionInvocation.java:
##########
@@ -350,8 +351,13 @@ protected void createAction(Map<String, Object> contextMap) {
}
}
+ @Deprecated
protected Map<String, Object> createContextMap() {
- ActionContext actionContext;
+ return createStrutsContext();
+ }
+
+ protected StrutsContext createStrutsContext() {
Review Comment:
createContextMap() now delegates to createStrutsContext() but its return type remains Map<String,Object>; update method javadoc (or add one) to indicate it returns a StrutsContext instance for backward compatibility and mark recommended replacement explicitly to guide migration.
##########
core/src/main/java/org/apache/struts2/factory/StrutsResultFactory.java:
##########
@@ -49,7 +50,7 @@ public void setReflectionProvider(ReflectionProvider provider) {
}
@Override
- public Result buildResult(ResultConfig resultConfig, Map<String, Object> extraContext) throws Exception {
+ public Result buildResult(ResultConfig resultConfig, StrutsContext extraContext) throws Exception {
Review Comment:
Public factory signature changed from Map<String,Object> to StrutsContext; this is a breaking change for third-party extensions implementing ResultFactory. Consider providing an overloaded legacy method or a migration note in release documentation.
##########
core/src/main/java/org/apache/struts2/conversion/impl/XWorkBasicConverter.java:
##########
@@ -75,7 +74,7 @@ public void setContainer(Container container) {
}
@Override
- public Object convertValue(Map<String, Object> context, Object o, Member member, String propertyName, Object value, Class toType) {
+ public Object convertValue(StrutsContext context, Object o, Member member, String propertyName, Object value, Class<?> toType) {
Review Comment:
Internal helper methods all accept StrutsContext but some ignore it (e.g., doConvertToString). If context is not used, consider removing the parameter to simplify signatures or document future planned use to justify retention.
##########
core/src/main/java/org/apache/struts2/factory/StrutsResultFactory.java:
##########
@@ -82,7 +83,7 @@ protected void setParameters(Map<String, Object> extraContext, Result result, Ma
}
}
- protected void setParameter(Result result, String name, String value, Map<String, Object> extraContext) {
+ protected void setParameter(Result result, String name, String value, StrutsContext extraContext) {
Review Comment:
Public factory signature changed from Map<String,Object> to StrutsContext; this is a breaking change for third-party extensions implementing ResultFactory. Consider providing an overloaded legacy method or a migration note in release documentation.
##########
core/src/main/java/org/apache/struts2/util/StrutsTypeConverter.java:
##########
@@ -45,7 +46,8 @@
*
*/
public abstract class StrutsTypeConverter extends DefaultTypeConverter {
- public Object convertValue(Map context, Object o, Class toClass) {
+
+ public Object convertValue(StrutsContext context, Object o, Class toClass) {
Review Comment:
Changing convertValue/performFallbackConversion to accept StrutsContext alters all existing custom StrutsTypeConverter implementations; provide an explicit @since tag and migration guidance (e.g., how to obtain previous Map entries from StrutsContext) to reduce upgrade friction.
##########
core/src/main/java/org/apache/struts2/conversion/impl/XWorkBasicConverter.java:
##########
@@ -185,40 +184,40 @@ private Class doConvertToClass(Object value) {
return clazz;
}
- private Object doConvertToCollection(Map<String, Object> context, Object o, Member member, String prop, Object value, Class toType) {
- TypeConverter converter = container.getInstance(CollectionConverter.class);
+ private Object doConvertToCollection(StrutsContext context, Object o, Member member, String prop, Object value, Class<?> toType) {
Review Comment:
Internal helper methods all accept StrutsContext but some ignore it (e.g., doConvertToString). If context is not used, consider removing the parameter to simplify signatures or document future planned use to justify retention.
##########
core/src/main/java/org/apache/struts2/conversion/impl/XWorkBasicConverter.java:
##########
@@ -185,40 +184,40 @@ private Class doConvertToClass(Object value) {
return clazz;
}
- private Object doConvertToCollection(Map<String, Object> context, Object o, Member member, String prop, Object value, Class toType) {
- TypeConverter converter = container.getInstance(CollectionConverter.class);
+ private Object doConvertToCollection(StrutsContext context, Object o, Member member, String prop, Object value, Class<?> toType) {
+ TypeConverter<StrutsContext> converter = container.getInstance(CollectionConverter.class);
if (converter == null) {
throw new TypeConversionException("TypeConverter with name [#0] must be registered first! Converter: "+ StrutsConstants.STRUTS_CONVERTER_COLLECTION);
}
return converter.convertValue(context, o, member, prop, value, toType);
}
- private Object doConvertToArray(Map<String, Object> context, Object o, Member member, String prop, Object value, Class toType) {
- TypeConverter converter = container.getInstance(ArrayConverter.class);
+ private Object doConvertToArray(StrutsContext context, Object o, Member member, String prop, Object value, Class<?> toType) {
+ TypeConverter<StrutsContext> converter = container.getInstance(ArrayConverter.class);
if (converter == null) {
throw new TypeConversionException("TypeConverter with name [#0] must be registered first! Converter: "+ StrutsConstants.STRUTS_CONVERTER_ARRAY);
}
return converter.convertValue(context, o, member, prop, value, toType);
}
- private Object doConvertToDate(Map<String, Object> context, Object value, Class toType) {
- TypeConverter converter = container.getInstance(DateConverter.class);
+ private Object doConvertToDate(StrutsContext context, Object value, Class<?> toType) {
Review Comment:
Internal helper methods all accept StrutsContext but some ignore it (e.g., doConvertToString). If context is not used, consider removing the parameter to simplify signatures or document future planned use to justify retention.
##########
core/src/main/java/org/apache/struts2/conversion/impl/XWorkBasicConverter.java:
##########
@@ -185,40 +184,40 @@ private Class doConvertToClass(Object value) {
return clazz;
}
- private Object doConvertToCollection(Map<String, Object> context, Object o, Member member, String prop, Object value, Class toType) {
- TypeConverter converter = container.getInstance(CollectionConverter.class);
+ private Object doConvertToCollection(StrutsContext context, Object o, Member member, String prop, Object value, Class<?> toType) {
+ TypeConverter<StrutsContext> converter = container.getInstance(CollectionConverter.class);
if (converter == null) {
throw new TypeConversionException("TypeConverter with name [#0] must be registered first! Converter: "+ StrutsConstants.STRUTS_CONVERTER_COLLECTION);
}
return converter.convertValue(context, o, member, prop, value, toType);
}
- private Object doConvertToArray(Map<String, Object> context, Object o, Member member, String prop, Object value, Class toType) {
- TypeConverter converter = container.getInstance(ArrayConverter.class);
+ private Object doConvertToArray(StrutsContext context, Object o, Member member, String prop, Object value, Class<?> toType) {
+ TypeConverter<StrutsContext> converter = container.getInstance(ArrayConverter.class);
if (converter == null) {
throw new TypeConversionException("TypeConverter with name [#0] must be registered first! Converter: "+ StrutsConstants.STRUTS_CONVERTER_ARRAY);
}
return converter.convertValue(context, o, member, prop, value, toType);
}
- private Object doConvertToDate(Map<String, Object> context, Object value, Class toType) {
- TypeConverter converter = container.getInstance(DateConverter.class);
+ private Object doConvertToDate(StrutsContext context, Object value, Class<?> toType) {
+ TypeConverter<StrutsContext> converter = container.getInstance(DateConverter.class);
if (converter == null) {
throw new TypeConversionException("TypeConverter with name [#0] must be registered first! Converter: "+ StrutsConstants.STRUTS_CONVERTER_DATE);
}
return converter.convertValue(context, null, null, null, value, toType);
}
- private Object doConvertToNumber(Map<String, Object> context, Object value, Class toType) {
- TypeConverter converter = container.getInstance(NumberConverter.class);
+ private Object doConvertToNumber(StrutsContext context, Object value, Class<?> toType) {
Review Comment:
Internal helper methods all accept StrutsContext but some ignore it (e.g., doConvertToString). If context is not used, consider removing the parameter to simplify signatures or document future planned use to justify retention.
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org