WW-5251 Reinstate deleted interfaces with transparent compat

[kusalk]

Partially reverts #670 to restore compatibility with deprecated interfaces
Refs WW-5251

[kusalk]

@brianandle If you bundle these classes with your Struts application, it should restore compatibility.

[brianandle]

This would need to be something like the following since the setParameters takes a Map:

       Map<String, String[]> newParams = new HashMap<String, String[]>();
        for(String key : parameters.keySet()){
            Parameter p = parameters.get(key);
            if(p instanceof Parameter.Request){
            	if(p.isMultiple()) {
            		newParams.put(key, p.getMultipleValues());
            	} else {
            		newParams.put(key, new String[] {p.getValue()});
            	}
            }
        }
       setParameters(newParams);

[brianandle]

We're also missing the restoration of org.apache.struts2.interceptor.RequestAware which would be something like:

public interface RequestAware extends org.apache.struts2.action..ServletRequestAware {

   void setRequest(Map<String,Object> request);

    default void withServletRequest(HttpServletRequest request) {
		setRequest(request.getParameterMap());
	}
}

[brianandle]

I created https://issues.apache.org/jira/browse/WW-5403 as well which highlights some other classes/methods I found that (probably not just the ones listed) that were removed in a minor when they really, IMHO, should have been removed only in the next major (7.x).

I would worry what other newly deprecated, in 6.x, classes/methods were removed in a minor vs waiting until 7.x. Unless the class/method exposed a significant security risk on it's own.

[kusalk]

As far as I know Struts has never adhered to SemVer. I noticed deprecations and deletions happening in successive minor versions when I first began contributing so I just went with it too. Maybe with 7.0.0 we can start adhering to it strictly if this is what the community/users would prefer?

[brianandle]

From Struts 6.0.0 announcement page.

Version change

You can be surprised by the version change, previously we have been using Struts 2.5.x versioning schema, but this was a bit misleading. Struts 2 is a different framework than Struts 1 and its versioning is supposed to start with 1.0.0, yet that never happened. With each breaking changes release (like Struts 2.5), we had been only upgrading the MINOR part of the versioning schema. To fix that problem as from Struts 2 ver. 6.0.0 (aka Struts 2.6) we adopt a proper SemVer to avoid such confusion.

[kusalk]

Yes that was the approach (bundled compatibility jar) I took when upgrading my Struts application. This technique can be used for the renamed marker interfaces only however. The remaining API changes are easy enough to find and replace in your codebase.

I'll leave it to @lukaszlenart to decide whether we want to restore full API compatibility with Struts 6.0.0. Given you're upgrading from 2.5 and you're the first (as far as I know) to complain, I'm inclined to say no.

However, I'm very much in favour of ensuring there are no more SemVer violations henceforth.

[lukaszlenart]

Using the four numbers format is an old approach to marking a given release a security hotfix. Anyway, restoring an old behaviour can be dangerous. I'm fine with restoring the old interfaces, yet I would be more careful brining back changes related to ActionContext - this is was a source of lot of security vulnerabilities.

[brianandle]

That makes sense to not revert all of the ActionContext change. It might make sense to add back getName(), since it was never marked deprecated, marking deprecated and removing in 7.0.0.

I agree that reverting something that would re-expose a security issue wouldn't make sense. If the revert in #897 was done for 6.4.0.0 then from our implementation we'd be good to go. I created WW-5403 thinking about the community at large and that it could cause adoption issues.

[kusalk]

Let me fix ParameterAware and add RequestAware to this PR

[kusalk]

@brianandle I believe this PR should be equivalent to #897 now - let me know if there's any issues

[sonarcloud]

Quality Gate failed

Failed conditions
8 Security Hotspots
28.6% Coverage on New Code (required ≥ 80%)
4.0% Duplication on New Code (required ≤ 3%)
E Reliability Rating on New Code (required ≥ A)
E Security Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

[brianandle]

To match the existing signature, which caused build issues for us, this needs to be:

setParameters(Map<String, String[]> map);

[kusalk]

Ah good catch, not sure how I managed to drop the generic types here

[brianandle]

I did check the other files in the review and it was just this one.

We were able to build fine and it's going through our automation, what we have anyway, tonight.

[lukaszlenart]

Sorry, I merged wrong PR :( Should I revert it?

[brianandle]

@lukaszlenart I believe it's the correct one, this seems like a better approach vs the revert, but the ParameterAware still needs an additional change by @kusalk. Or you could fix the method signature (see comment)

[brianandle]

Thanks @kusalk that other PR looks good to me. Commented there too