WW-5336 Deprecate OgnlTool #735
Conversation
kusalk
force-pushed
the
WW-5336-deprecate-ognltool
branch
from
fdf00c1 to
23abcf7
Compare
|
Await merge of #731 first |
kusalk
force-pushed
the
WW-5336-deprecate-ognltool
branch
2 times, most recently
from
518d2ac to
740cb68
Compare
kusalk
force-pushed
the
WW-5336-deprecate-ognltool
branch
from
740cb68 to
35b0a68
Compare
| @@ -50,32 +61,30 @@ public class StrutsUtil { | |||
|
|
|||
| protected HttpServletRequest request; | |||
| protected HttpServletResponse response; | |||
| protected Map<String, Class> classes = new Hashtable<>(); | |||
| protected OgnlTool ognl; | |||
| protected Map<String, Class<?>> classes = new HashMap<>(); | |||
There was a problem hiding this comment.
This class is constructed per request thread so I don't believe we need concurrency handling here
| public Object bean(Object name) throws Exception { | ||
| String className = name.toString(); | ||
| Class<?> clazz = classes.get(className); | ||
| if (clazz == null) { |
There was a problem hiding this comment.
Using #computeIfAbsent here would change the exception handling semantics so I've forgone it
| @@ -124,6 +123,17 @@ public Object findValue(String expression, String className) throws ClassNotFoun | |||
| return stack.findValue(expression, Class.forName(className)); | |||
| } | |||
|
|
|||
| public Object findValue(String expr, Object context) { | |||
There was a problem hiding this comment.
Copied this here from OgnlTool
| return ognl.getValue(expr, ActionContext.getContext().getContextMap(), context); | ||
| } catch (OgnlException e) { | ||
| if (e.getReason() instanceof SecurityException) { | ||
| LOG.error(format("Could not evaluate this expression due to security constraints: [{0}]", expr), e); |
| selectedItems.add(i); | ||
| } | ||
| } | ||
| public List<ListEntry> makeSelectList(String selectedList, String list, String listKey, String listValue) { |
There was a problem hiding this comment.
Fixed cognitive complexity and readability
| @@ -198,7 +198,7 @@ | |||
| <bean type="ognl.MethodAccessor" name="com.opensymphony.xwork2.util.CompoundRoot" | |||
| class="com.opensymphony.xwork2.ognl.accessor.CompoundRootAccessor"/> | |||
|
|
|||
| <bean class="org.apache.struts2.views.jsp.ui.OgnlTool"/> | |||
| <bean class="org.apache.struts2.views.jsp.ui.OgnlTool"/> <!-- Deprecated since 6.3.0 --> | |||
There was a problem hiding this comment.
Maybe it's better to put the comment before the bean?
There was a problem hiding this comment.
Amended - I tend to favour the same line as sometimes comments can become detached from their intended line in highly concurrent teams
|
|
||
| List<ListEntry> listMade = strutsUtil.makeSelectList("#mySelectedList", "#myList", null, null); | ||
|
|
||
| assertEquals(listMade.size(), 3); |
There was a problem hiding this comment.
for assertEquals, the expected value should be the first argument
There was a problem hiding this comment.
Seems all the assertions in this class are mis-ordered, I've amended the ones I've touched :)
|
Kudos, SonarCloud Quality Gate passed! |
|
Hope this is the last change and I can prepare another RC or even test build and call for vote :) |
I opened 2 more security enhancement cards that I intend to work on next week or the week after. Up to you whether you want these in 6.3.0 or a later release |
WW-5336
I couldn't see any value in retaining OgnlTool as its own class so I've deprecated it and merged the method into StrutsUtil.