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 ([email protected])
* @author Drew Davidson ([email protected])
*/
-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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
