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

Immutable context #125

Merged
merged 15 commits into from
May 3, 2018
Merged

Immutable context #125

merged 15 commits into from
May 3, 2018

Conversation

lukaszlenart
Copy link
Member

@lukaszlenart lukaszlenart commented Mar 24, 2017

This PR uses the latest OGNL version which drops access to #context via a magical key and makes OgnlContext a bit immutable.

OGNL 3.2 version notes

Implements WW-4927

@cnenning
Copy link
Member

Sounds like a very good idea! A short check showed that my apps are not affected 😆

@yasserzamani
Copy link
Member

Below is my new design which may be helpful as an example for whom is affected. Those are about access to %{#context['com.opensymphony.xwork2.dispatcher.HttpServletRequest'].requestURI}.

I wrote following Filter which operates after struts-prepare but before struts-execute. It supplies an utility into request scope:

package utils;

import java.io.IOException;

import javax.servlet.Filter;
import javax.servlet.FilterChain;
import javax.servlet.FilterConfig;
import javax.servlet.ServletException;
import javax.servlet.ServletRequest;
import javax.servlet.ServletResponse;
import javax.servlet.http.HttpServletRequest;

import org.apache.struts2.StrutsStatics;
import com.opensymphony.xwork2.ActionContext;
import com.opensymphony.xwork2.util.ValueStack;

/**
 * @author zamani
 *
 */
public class MYStrutsPrepareFilter implements Filter {

	private MYUtils MYUtils;

	@Override
	public void init(FilterConfig filterConfig) throws ServletException {
		MYUtils = new MYUtils();
	}

	@Override
	public void doFilter(ServletRequest req, ServletResponse res, FilterChain chain)
			throws IOException, ServletException {

		ValueStack stack = ActionContext.getContext().getValueStack();
		stack.setValue("#request['MYUtils']", MYUtils);
		
		chain.doFilter(req, res);
	}

	@Override
	public void destroy() {
		MYUtils = null;
	}

	
	public class MYUtils {
		public String getRequestURI() {
			HttpServletRequest httpsr = ((HttpServletRequest) ActionContext.getContext()
					.get(StrutsStatics.HTTP_REQUEST));
			return httpsr.getRequestURI();
		}
	}
}

Then "#request['MYUtils'].requestURI" can be used in jsp, instead.

@aleksandr-m
Copy link
Contributor

aleksandr-m commented Jun 1, 2017

Spotted same #context['com.opensymphony.xwork2.dispatcher.HttpServletRequest'] expression here. @yasserzamani What do you use it for?

@yasserzamani
Copy link
Member

yasserzamani commented Jun 2, 2017

@aleksandr-m , not me but my workmate has used it as an ID for each jsp. I do not know his work details but in general, he then uses this ID to decide where he should place the result after an AJAX call. For example a parent jsp that have div1, div2 and div3 calls an AJAX then if result was from '/APP/jsp/customer/error.jsp' then it puts the result in div3, if result was from '/APP/jsp/customer/payment.jsp' then it puts the result in div1, if result was from '/APP/jsp/customer/free.jsp' then it puts the result in div2.

@asfgit
Copy link

asfgit commented Jul 17, 2017

Can one of the admins verify this patch?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.0007%) to 46.168% when pulling a37eafa on lukaszlenart:immutable-context into 845ab41 on apache:master.

@coveralls
Copy link

coveralls commented Mar 20, 2018

Coverage Status

Coverage increased (+0.05%) to 46.214% when pulling 058409e on lukaszlenart:immutable-context into 845ab41 on apache:master.

@lukaszlenart
Copy link
Member Author

@apache/struts-committers this is ready for another look.

@@ -185,6 +187,10 @@ public void setContextMap(Map<String, Object> contextMap) {
* @return the context map.
*/
public Map<String, Object> getContextMap() {
Map<String, Object> context = getContext().context;
if (context instanceof OgnlContext) {
((OgnlContext) context).put(TypeConverter.TYPE_CONVERTER_CONTEXT_KEY, ((OgnlContext) context).getTypeConverter());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the deal with the type converter?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, probably it's a workaround :/ Need to re-think this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whooops... it can be thrown away, goot catch 👍

AccessibleObject accessible = (AccessibleObject) member;

if (!accessible.isAccessible()) {
result = Boolean.TRUE;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will result be used in restore method as state parameter? if so I think it should be Boolean.FALSE.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... this was copy/pasted from existing code but I think you are right.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done :)

@yasserzamani
Copy link
Member

LGTM 👍 Thanks!

@yasserzamani yasserzamani merged commit 57901ab into apache:master May 3, 2018
@lukaszlenart lukaszlenart deleted the immutable-context branch November 24, 2019 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants