Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WW-4873 Makes ActionInvocation not serializable and InvocationSessionStore$InvocationContext transient #192

Merged
merged 2 commits into from Dec 16, 2017

Conversation

@yasserzamani
Copy link
Member

yasserzamani commented Dec 14, 2017

⚠️ For simpler review, please review each commit separately.

Why serialize and deserialize methods of ActionInvocation deleted? ActionInvocation is a large complex object which contains request, response, container, value stack and action context. It's hard to maintain it serializable e.g. user may want to store a private not serializable object in action. And It's not a good practice (CWE-579: J2EE Bad Practices: Non-serializable Object Stored in Session).

ℹ️ I know I also deleted that.container = actionContext.getContainer(); (replacing restored invocation container with current container) with ActionInvocation.deserialize method. I saw it's not needed and even it's better for restored default invocation to work with itself stored container to keep consistency. Anyway currently it's really not needed because only TokenSessionStoreInterceptor uses it and it only passes the invocation to result and result is not aware about DefaultActionInvocation (only knows ActionInvocation):

if ((result != null) && (savedInvocation.getProxy().getExecuteResult())) {
    result.execute(savedInvocation);
}
@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 14, 2017

Coverage Status

Changes Unknown when pulling 2941416 on yasserzamani:WW-4873_2 into ** on apache:master**.

@lukaszlenart

This comment has been minimized.

Copy link
Member

lukaszlenart commented Dec 14, 2017

wtf :\

@lukaszlenart

This comment has been minimized.

Copy link
Member

lukaszlenart commented Dec 14, 2017

Looks lik a known issue lemurheavy/coveralls-public#351

@yasserzamani

This comment has been minimized.

Copy link
Member Author

yasserzamani commented Dec 15, 2017

That is weird. https://coveralls.io/github/apache/struts always claims it's a FIRST BUILD ON MASTER. To test coveralls.io, I simulated at https://coveralls.io/github/yasserzamani/struts but it works there!

Another thing is it jumps from #1 to #606 at https://coveralls.io/repos/188307/builds?page=2, I'm not sure if it's a problem but https://coveralls.io/repos/52976/builds?page=64 is not like this.

@lukaszlenart

This comment has been minimized.

Copy link
Member

lukaszlenart commented Dec 16, 2017

I think I have fixed it, I forgot that I had setup the build on Travis to be triggered only by PRs - now also changes to the master branch trigger a build :)

@lukaszlenart

This comment has been minimized.

Copy link
Member

lukaszlenart commented Dec 16, 2017

Looks good to me, 👍

@lukaszlenart lukaszlenart merged commit 8b608e3 into apache:master Dec 16, 2017
3 checks passed
3 checks passed
Jenkins This pull request looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on master at 46.183%
Details
@yasserzamani yasserzamani deleted the yasserzamani:WW-4873_2 branch Dec 16, 2017
@yasserzamani

This comment has been minimized.

Copy link
Member Author

yasserzamani commented Dec 16, 2017

Works :) 👍 example

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.