You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@struts.apache.org by GitBox <gi...@apache.org> on 2020/11/13 16:21:50 UTC
[GitHub] [struts] lukaszlenart opened a new pull request #446: [WW-5096] Fix StaticParametersInterceptor param overwrite
lukaszlenart opened a new pull request #446:
URL: https://github.com/apache/struts/pull/446
Fixes [WW-5096](https://issues.apache.org/jira/browse/WW-5096)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] [struts] akash5885 commented on a change in pull request #446: [WW-5096] Fix StaticParametersInterceptor param overwrite
Posted by GitBox <gi...@apache.org>.
akash5885 commented on a change in pull request #446:
URL: https://github.com/apache/struts/pull/446#discussion_r523417890
##########
File path: core/src/main/java/com/opensymphony/xwork2/interceptor/StaticParametersInterceptor.java
##########
@@ -237,7 +237,8 @@ protected void addParametersToContext(ActionContext ac, Map<String, ?> newParams
}
} else {
if (newParams != null) {
- combinedParams = combinedParams.withExtraParams(newParams);
+ HttpParameters newHttpParameters = HttpParameters.create(newParams).build();
+ combinedParams = combinedParams.withParent(newHttpParameters);
}
if (previousParams != null) {
combinedParams = combinedParams.withParent(previousParams);
Review comment:
@lukaszlenart I think for this one means for previous params, combinedParams.withParent(previousParams) needs to replace with combinedParams.withExtraParams(previousParams) otherwise extra parameters which are in newParams and not in previousParams will be lost.
Please let me know if I'm missing anything here.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] coveralls commented on pull request #446: [WW-5096] Fix StaticParametersInterceptor param overwrite
Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #446:
URL: https://github.com/apache/struts/pull/446#issuecomment-729538643
[](https://coveralls.io/builds/35053884)
Coverage increased (+0.009%) to 47.305% when pulling **d591b7e3b956ad34e1204da9e60382cc2fc4b2bf on WW-5096-fix-static** into **482af41673a3883e904ea72391a5b4a03cbd5d94 on struts-2-5-x**.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] yasserzamani edited a comment on pull request #446: [WW-5096] Fix StaticParametersInterceptor param overwrite
Posted by GitBox <gi...@apache.org>.
yasserzamani edited a comment on pull request #446:
URL: https://github.com/apache/struts/pull/446#issuecomment-730446869
LGTM :100: Merely I can't recall if integration tests were done in PR builds but currently I see they aren't. Basically I remember `STRUTS_IT=true` was devised, defined by me and was required to be set in Travis params, any idea?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [GitHub] [struts] yasserzamani commented on pull request #446:
[WW-5096] Fix StaticParametersInterceptor param overwrite
Posted by Murugasen Kalimuthu <cb...@gmail.com>.
please unsubscribe me from this email group.
Thanks
On Thu, Nov 19, 2020 at 11:07 AM GitBox <gi...@apache.org> wrote:
>
> yasserzamani commented on pull request #446:
> URL: https://github.com/apache/struts/pull/446#issuecomment-730475892
>
>
> Ooops sorry that was on only master branch and PRs; I realized now
>
>
> ----------------------------------------------------------------
> 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.
>
> For queries about this service, please contact Infrastructure at:
> users@infra.apache.org
>
>
>
[GitHub] [struts] yasserzamani commented on pull request #446: [WW-5096] Fix StaticParametersInterceptor param overwrite
Posted by GitBox <gi...@apache.org>.
yasserzamani commented on pull request #446:
URL: https://github.com/apache/struts/pull/446#issuecomment-730475892
Ooops sorry that was on only master branch and PRs; I realized now
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] yasserzamani edited a comment on pull request #446: [WW-5096] Fix StaticParametersInterceptor param overwrite
Posted by GitBox <gi...@apache.org>.
yasserzamani edited a comment on pull request #446:
URL: https://github.com/apache/struts/pull/446#issuecomment-730446869
LGTM :100: Merely I can't recall if integration tests were done in PR builds but currently I see they aren't. Basically I remember `STRUTS_IT=true` was devised and defined by me and was required to be set in Travis params, any idea?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] yasserzamani commented on pull request #446: [WW-5096] Fix StaticParametersInterceptor param overwrite
Posted by GitBox <gi...@apache.org>.
yasserzamani commented on pull request #446:
URL: https://github.com/apache/struts/pull/446#issuecomment-730508873
OK I cherry picked against master; Lets merge if passed integration tests there as well :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] lukaszlenart commented on pull request #446: [WW-5096] Fix StaticParametersInterceptor param overwrite
Posted by GitBox <gi...@apache.org>.
lukaszlenart commented on pull request #446:
URL: https://github.com/apache/struts/pull/446#issuecomment-729489996
@akash5885 thanks a lot for you description and test case - based on that I was able to prepare a few unit tests to cover the logic. Please review and let me know.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] yasserzamani commented on pull request #446: [WW-5096] Fix StaticParametersInterceptor param overwrite
Posted by GitBox <gi...@apache.org>.
yasserzamani commented on pull request #446:
URL: https://github.com/apache/struts/pull/446#issuecomment-729024211
Looks like the bug has emerged by [here](https://github.com/apache/struts/commit/5508352ddb46417ccd44033064ea337da509c021#diff-680ecf9ee762502e27c18bab70f879a7772ed76880d1473061133487d2bc75b7) and we can just restore previous logic.
One test fails. And not sure if we simple have new test case from provided scenario?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] lukaszlenart commented on a change in pull request #446: [WW-5096] Fix StaticParametersInterceptor param overwrite
Posted by GitBox <gi...@apache.org>.
lukaszlenart commented on a change in pull request #446:
URL: https://github.com/apache/struts/pull/446#discussion_r523461964
##########
File path: core/src/main/java/com/opensymphony/xwork2/interceptor/StaticParametersInterceptor.java
##########
@@ -237,7 +237,8 @@ protected void addParametersToContext(ActionContext ac, Map<String, ?> newParams
}
} else {
if (newParams != null) {
- combinedParams = combinedParams.withExtraParams(newParams);
+ HttpParameters newHttpParameters = HttpParameters.create(newParams).build();
+ combinedParams = combinedParams.withParent(newHttpParameters);
}
if (previousParams != null) {
combinedParams = combinedParams.withParent(previousParams);
Review comment:
Not sure if I got your point, yet please check the latest change. The problem is that I don't have a test case to reproduce the problem.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] lukaszlenart merged pull request #446: [WW-5096] Fix StaticParametersInterceptor param overwrite
Posted by GitBox <gi...@apache.org>.
lukaszlenart merged pull request #446:
URL: https://github.com/apache/struts/pull/446
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] [struts] yasserzamani edited a comment on pull request #446: [WW-5096] Fix StaticParametersInterceptor param overwrite
Posted by GitBox <gi...@apache.org>.
yasserzamani edited a comment on pull request #446:
URL: https://github.com/apache/struts/pull/446#issuecomment-729024211
Looks like the bug has emerged by [here](https://github.com/apache/struts/commit/5508352ddb46417ccd44033064ea337da509c021#diff-680ecf9ee762502e27c18bab70f879a7772ed76880d1473061133487d2bc75b7) and we can just restore previous logic.
One test fails. And not sure if we simply can have new test case from provided scenario?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] akash5885 commented on pull request #446: [WW-5096] Fix StaticParametersInterceptor param overwrite
Posted by GitBox <gi...@apache.org>.
akash5885 commented on pull request #446:
URL: https://github.com/apache/struts/pull/446#issuecomment-728234065
HI @JCgH4164838Gh792C124B5 @lukaszlenart
Apologies I didn't mention exact scenario that triggered this bug. But now I have added scenario in JIRA ticket [WW-5096](https://issues.apache.org/jira/browse/WW-5096) which triggered this bug.
@JCgH4164838Gh792C124B5
yes, you are correct.original code of StaticParametersInterceptor addParametersToContext() was doing the same operations irrespective of overwrite is true or false.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] yasserzamani edited a comment on pull request #446: [WW-5096] Fix StaticParametersInterceptor param overwrite
Posted by GitBox <gi...@apache.org>.
yasserzamani edited a comment on pull request #446:
URL: https://github.com/apache/struts/pull/446#issuecomment-730446869
LGTM :100: Merely I can't recall if integration tests were done in PR builds but currently I see they aren't. Basically I remember `STRUTS_IT=true` was devised and defined by me and was required to be set in Travis params for JDK8 build only, any idea?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] JCgH4164838Gh792C124B5 commented on pull request #446: [WW-5096] Fix StaticParametersInterceptor param overwrite
Posted by GitBox <gi...@apache.org>.
JCgH4164838Gh792C124B5 commented on pull request #446:
URL: https://github.com/apache/struts/pull/446#issuecomment-727290131
Hi @lukaszlenart and @akash5885 .
It looks like the original code of `StaticParametersInterceptor` `addParametersToContext()` was doing the same operations if `overwrite` was `true `or `false`, just in reverse-order depending on the `boolean `value. I think that was what @akash5885 stated in the JIRA ?
Did the different ordering of the old code blocks have any effect at all, or was the end result the same whether `overwrite` was `true` or `false` ?
Is there a possibility for a manual test example or test case that clarifies how the original code was not working as intended, and how the updated code does ?
I am not sure I am understanding the original problem correctly, so I figured I would ask. 😄
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] yasserzamani commented on pull request #446: [WW-5096] Fix StaticParametersInterceptor param overwrite
Posted by GitBox <gi...@apache.org>.
yasserzamani commented on pull request #446:
URL: https://github.com/apache/struts/pull/446#issuecomment-730446869
LGTM :100: Merely I can't recall if integration tests were done in PR builds but currently I see they aren't. Basically I remember `STRUTS_IT=true` was defined by me and was required to be set in Travis params, any idea?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org