changed Map iterations to use entrySet when both key and value are used

[sdutry]

Changes

Changed iterations over the keySet of Maps when the values were (also) used.

Reported by

Sonar build

• see: Iterate over the "entrySet" instead of the "keySet"

Encountered possible problems

I did not change 1 of the warnings, because i think the code could throw a ConcurrentModificationException

• see: CheckboxInterceptor.java#L63-L80

[sdutry]

I did not change 1 of the warnings, because i think the code could throw a ConcurrentModificationException

see: CheckboxInterceptor.java#L63-L80

I was wrong here,
The HttpParameters class overrides the keySet() method and returns an unmodifiable copy of the underlying keySet
see: HttpParameters.java#L139-L142

Given the entrySet() method doesn't return a copy, this can't be changed as simply as the others.

[lukaszlenart]

But it returns https://github.com/apache/struts/blob/master/core/src/main/java/org/apache/struts2/dispatcher/HttpParameters.java#L150-L152 ?

[sdutry]

But it returns https://github.com/apache/struts/blob/master/core/src/main/java/org/apache/struts2/dispatcher/HttpParameters.java#L150-L152 ?

While it does, i don't know if this is sufficient. The keySet() method returns a unmodifiable view of a newly created set where as the entrySet() returns a unmodifiable view of the actual entrySet().

I'll test later today if the tests still work if i change it.

[yasserzamani]

The keySet() method returns a unmodifiable view of a newly created set where as the entrySet() returns a unmodifiable view of the actual entrySet().

I tested and you were right :) when I remove new TreeSet< there, CheckboxInterceptorTest fails!

As we finally should fix it, I tested something as below and seems work.

Index: core/src/main/java/org/apache/struts2/interceptor/CheckboxInterceptor.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- core/src/main/java/org/apache/struts2/interceptor/CheckboxInterceptor.java	(revision 47a4a57d2d7fc4d5ab1765f6bd31bf1b0f0d583c)
+++ core/src/main/java/org/apache/struts2/interceptor/CheckboxInterceptor.java	(revision )
@@ -26,6 +26,7 @@
 import org.apache.struts2.dispatcher.HttpParameters;
 
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.Map;
 
 /**
@@ -60,12 +61,14 @@
         HttpParameters parameters = ai.getInvocationContext().getParameters();
         Map<String, Parameter> extraParams = new HashMap<>();
 
-        for (String name : parameters.keySet()) {
+        HashSet<String> checkboxes = new HashSet<String>();
+        for (Map.Entry<String, Parameter> param : parameters.entrySet()) {
+            String name = param.getKey();
             if (name.startsWith("__checkbox_")) {
                 String checkboxName = name.substring("__checkbox_".length());
 
-                Parameter value = parameters.get(name);
-                parameters = parameters.remove(name);
+                Parameter value = param.getValue();
+                checkboxes.add(name);
                 if (value.isMultiple()) {
               	    LOG.debug("Bypassing automatic checkbox detection due to multiple checkboxes of the same name: {}", name);
                     continue;
@@ -78,6 +81,7 @@
                 }
             }
         }
+        parameters.remove(checkboxes);
 
 
         ai.getInvocationContext().getParameters().appendAll(extraParams);

[sdutry]

@yasserzamani
added the change.

[yasserzamani]

Great! nice work! I reviewed and seems it's ready to go :)

[sdutry]

Great! nice work! I reviewed and seems it's ready to go :)

Going to wait till after the next official struts release before merging.

[lukaszlenart]

We can go ahead if you want, if there be any release blocker in 2.5.14 I will create a branch off the STRUTS_2_5_14 tag to fix that and I will prepare new packages