Now I see there is a following commit which reverts the changes in the diff. So it seems it was bad commit, not just bad diff for commit... All is fine.
On Mon, Oct 31, 2011 at 3:59 PM, Martin Grigorov <[email protected]> wrote: > Hi Pedro, > > How exactly you did this change ? > The diff for ServletWebRequest below looks wrong and scary. I see that > the actual source of that class is OK but for some reason the diff is > wrong. > Even ViewVC shows the same problem: > http://svn.apache.org/viewvc/wicket/trunk/wicket-core/src/main/java/org/apache/wicket/protocol/http/servlet/ServletWebRequest.java?r1=1186162&r2=1190589&diff_format=h > > I remember once Peter Ertl also confused me with such commit notification. > Can you remember the steps you did to commit this change ? > I would like to avoid them in future :-) > > On Sat, Oct 29, 2011 at 12:31 AM, <[email protected]> wrote: >> Author: pedro >> Date: Fri Oct 28 21:31:23 2011 >> New Revision: 1190589 >> >> URL: http://svn.apache.org/viewvc?rev=1190589&view=rev >> Log: >> Making sure that client URL is context relative even while responding errors >> Issue: WICKET-4168 >> >> Modified: >> >> wicket/trunk/wicket-core/src/main/java/org/apache/wicket/protocol/http/servlet/ServletWebRequest.java >> >> wicket/trunk/wicket-core/src/test/java/org/apache/wicket/protocol/http/servlet/ServletWebRequestTest.java >> >> Modified: >> wicket/trunk/wicket-core/src/main/java/org/apache/wicket/protocol/http/servlet/ServletWebRequest.java >> URL: >> http://svn.apache.org/viewvc/wicket/trunk/wicket-core/src/main/java/org/apache/wicket/protocol/http/servlet/ServletWebRequest.java?rev=1190589&r1=1190588&r2=1190589&view=diff >> ============================================================================== >> --- >> wicket/trunk/wicket-core/src/main/java/org/apache/wicket/protocol/http/servlet/ServletWebRequest.java >> (original) >> +++ >> wicket/trunk/wicket-core/src/main/java/org/apache/wicket/protocol/http/servlet/ServletWebRequest.java >> Fri Oct 28 21:31:23 2011 >> @@ -16,445 +16,120 @@ >> */ >> package org.apache.wicket.protocol.http.servlet; >> >> -import java.nio.charset.Charset; >> -import java.util.ArrayList; >> -import java.util.Arrays; >> -import java.util.Collections; >> -import java.util.Enumeration; >> -import java.util.HashMap; >> -import java.util.List; >> -import java.util.Locale; >> -import java.util.Map; >> -import java.util.Set; >> - >> -import javax.servlet.ServletRequest; >> -import javax.servlet.http.Cookie; >> import javax.servlet.http.HttpServletRequest; >> >> -import org.apache.wicket.protocol.http.RequestUtils; >> -import org.apache.wicket.request.IRequestParameters; >> -import org.apache.wicket.request.IWritableRequestParameters; >> +import org.apache.wicket.MarkupContainer; >> +import org.apache.wicket.Page; >> +import org.apache.wicket.markup.IMarkupResourceStreamProvider; >> +import org.apache.wicket.markup.html.WebPage; >> +import org.apache.wicket.protocol.http.WebApplication; >> +import org.apache.wicket.protocol.http.mock.MockHttpServletRequest; >> import org.apache.wicket.request.Url; >> import org.apache.wicket.request.http.WebRequest; >> -import org.apache.wicket.util.lang.Args; >> -import org.apache.wicket.util.lang.Bytes; >> -import org.apache.wicket.util.lang.Checks; >> -import org.apache.wicket.util.string.PrependingStringBuffer; >> -import org.apache.wicket.util.string.StringValue; >> -import org.apache.wicket.util.string.Strings; >> -import org.apache.wicket.util.string.UrlUtils; >> -import org.apache.wicket.util.time.Time; >> -import org.apache.wicket.util.upload.FileItemFactory; >> -import org.apache.wicket.util.upload.FileUploadException; >> -import org.slf4j.Logger; >> -import org.slf4j.LoggerFactory; >> +import org.apache.wicket.util.resource.IResourceStream; >> +import org.apache.wicket.util.resource.StringResourceStream; >> +import org.apache.wicket.util.tester.WicketTester; >> +import org.junit.Assert; >> +import org.junit.Test; >> >> /** >> - * {@link WebRequest} subclass that wraps a {@link HttpServletRequest} >> object. >> - * >> - * @author Matej Knopp >> - * @author Juergen Donnerstag >> - * @author Igor Vaynberg >> + * Tests for {@link ServletWebRequest} >> */ >> -public class ServletWebRequest extends WebRequest >> +public class ServletWebRequestTest extends Assert >> { >> - private final HttpServletRequest httpServletRequest; >> - >> - private final Url url; >> - >> - private final String filterPrefix; >> - >> - private final ErrorAttributes errorAttributes; >> - >> - /** >> - * Construct. >> - * >> - * @param httpServletRequest >> - * @param filterPrefix >> - * contentPath + filterPath, used to extract the actual >> {@link Url} >> - */ >> - public ServletWebRequest(HttpServletRequest httpServletRequest, >> String filterPrefix) >> - { >> - this(httpServletRequest, filterPrefix, null); >> - } >> >> /** >> - * Construct. >> - * >> - * @param httpServletRequest >> - * @param filterPrefix >> - * contentPath + filterPath, used to extract the actual >> {@link Url} >> - * @param url >> + * Tests that {@link ServletWebRequest#getClientUrl()} returns the >> current url + the query >> + * string when this is not error dispatched request. When the >> request is error dispatched it >> + * returns just the request uri to the error page without the query >> string >> */ >> - public ServletWebRequest(HttpServletRequest httpServletRequest, >> String filterPrefix, Url url) >> + @Test >> + public void wicket3599() >> { >> - Args.notNull(httpServletRequest, "httpServletRequest"); >> - Args.notNull(filterPrefix, "filterPrefix"); >> + MockHttpServletRequest httpRequest = new >> MockHttpServletRequest(null, null, null); >> + httpRequest.setURL("/" + httpRequest.getContextPath() + >> "/request/Uri"); >> + httpRequest.setParameter("some", "parameter"); >> >> - this.httpServletRequest = httpServletRequest; >> - this.filterPrefix = filterPrefix; >> + ServletWebRequest webRequest = new >> ServletWebRequest(httpRequest, "/"); >> + Url clientUrl = webRequest.getClientUrl(); >> + assertEquals("request/Uri?some=parameter", >> clientUrl.toString()); >> >> - errorAttributes = ErrorAttributes.of(httpServletRequest); >> + // simulates a request that has errors metadata >> + httpRequest.setAttribute("javax.servlet.error.request_uri", >> + "/" + httpRequest.getContextPath() + >> "/any/source/of/error"); >> + ServletWebRequest errorWebRequest = new >> ServletWebRequest(httpRequest, "/"); >> + Url errorClientUrl = errorWebRequest.getClientUrl(); >> >> - if (url != null) >> - { >> - this.url = url; >> - } >> - else >> - { >> - this.url = getUrl(httpServletRequest, filterPrefix); >> - } >> + assertEquals("any/source/of/error", >> errorClientUrl.toString()); >> } >> >> /** >> - * Returns base url without context or filter mapping. >> - * <p> >> - * Example: if current url is >> - * >> - * <pre> >> - * >> http://localhost:8080/context/filter/mapping/wicket/bookmarkable/com.foo.Page?1&id=2 >> - * </pre> >> - * >> - * the base url is <em>wicket/bookmarkable/com.foo.Page</em> >> - * </p> >> - * >> - * @see org.apache.wicket.request.Request#getClientUrl() >> + * <a >> href="https://issues.apache.org/jira/browse/WICKET-4168";>WICKET-4168</a> >> */ >> - @Override >> - public Url getClientUrl() >> + @Test >> + public void testClientURLIsContextRelativeInErrorResponses() >> { >> - if (errorAttributes != null && >> !Strings.isEmpty(errorAttributes.getRequestUri())) >> - { >> - return >> setParameters(Url.parse(errorAttributes.getRequestUri(), getCharset())); >> - } >> - else if (!isAjax()) >> - { >> - return getUrl(httpServletRequest, filterPrefix); >> - } >> - else >> - { >> - String base = null; >> + MockHttpServletRequest httpRequest = new >> MockHttpServletRequest(null, null, null); >> + httpRequest.setURL(httpRequest.getContextPath() + >> "/request/Uri"); >> >> - base = getHeader(HEADER_AJAX_BASE_URL); >> + String problematiURI = httpRequest.getContextPath() + >> "/any/source/of/error"; >> >> - if (base == null) >> - { >> - base = >> getRequestParameters().getParameterValue(PARAM_AJAX_BASE_URL).toString(null); >> - } >> + httpRequest.setAttribute("javax.servlet.error.request_uri", >> problematiURI); >> >> - Checks.notNull(base, "Current ajax request is >> missing the base url header or parameter"); >> + ServletWebRequest errorWebRequest = new >> ServletWebRequest(httpRequest, ""); >> >> - return setParameters(Url.parse(base, getCharset())); >> - } >> - } >> + Url errorClientUrl = errorWebRequest.getClientUrl(); >> >> - private Url setParameters(Url url) >> - { >> - url.setPort(httpServletRequest.getServerPort()); >> - url.setHost(httpServletRequest.getServerName()); >> - url.setProtocol(httpServletRequest.getScheme()); >> - return url; >> - } >> + assertEquals("any/source/of/error", >> errorClientUrl.toString()); >> >> - private Url getUrl(HttpServletRequest request, String filterPrefix) >> - { >> - if (filterPrefix.length() > 0 && !filterPrefix.endsWith("/")) >> - { >> - filterPrefix += "/"; >> - } >> - StringBuilder url = new StringBuilder(); >> - String uri = request.getRequestURI(); >> - uri = Strings.stripJSessionId(uri); >> - final int start = request.getContextPath().length() + >> filterPrefix.length() + 1; >> - url.append(uri.substring(start)); >> - >> - if (errorAttributes == null) >> - { >> - String query = request.getQueryString(); >> - if (!Strings.isEmpty(query)) >> - { >> - url.append('?'); >> - url.append(query); >> - } >> - } >> - >> - return setParameters(Url.parse(url.toString(), >> getCharset())); >> } >> >> /** >> - * Returns the prefix of Wicket filter (without the leading /) >> - * >> - * @return Wicket filter prefix >> + * https://issues.apache.org/jira/browse/WICKET-4123 >> */ >> - public String getFilterPrefix() >> - { >> - return filterPrefix; >> - } >> - >> - @Override >> - public List<Cookie> getCookies() >> - { >> - Cookie[] cookies = httpServletRequest.getCookies(); >> - List<Cookie> result = (cookies == null) ? >> Collections.<Cookie> emptyList() >> - : Arrays.asList(cookies); >> - return Collections.unmodifiableList(result); >> - } >> - >> - >> - @Override >> - public Locale getLocale() >> - { >> - return httpServletRequest.getLocale(); >> - } >> - >> - @Override >> - public Time getDateHeader(String name) >> - { >> - try >> - { >> - long value = httpServletRequest.getDateHeader(name); >> - >> - if (value == -1) >> - { >> - return null; >> - } >> - >> - return Time.millis(value); >> - } >> - catch (IllegalArgumentException e) >> - { >> - // per spec thrown if the header contains a value >> that cannot be converted to a date >> - return null; >> - } >> - } >> - >> - @Override >> - public String getHeader(String name) >> + @Test >> + public void useCustomServletWebRequest() >> { >> - return httpServletRequest.getHeader(name); >> - } >> - >> - @SuppressWarnings("unchecked") >> - @Override >> - public List<String> getHeaders(String name) >> - { >> - List<String> result = new ArrayList<String>(); >> - Enumeration<String> e = httpServletRequest.getHeaders(name); >> - while (e.hasMoreElements()) >> - { >> - result.add(e.nextElement()); >> - } >> - return Collections.unmodifiableList(result); >> - } >> - >> - private Map<String, List<StringValue>> postParameters = null; >> - >> - private static boolean isMultiPart(ServletRequest request) >> - { >> - String contentType = request.getContentType(); >> - return contentType != null && >> contentType.toLowerCase().contains("multipart"); >> - } >> - >> - protected Map<String, List<StringValue>> generatePostParameters() >> - { >> - Map<String, List<StringValue>> postParameters = new >> HashMap<String, List<StringValue>>(); >> - >> - IRequestParameters queryParams = getQueryParameters(); >> - >> - @SuppressWarnings("unchecked") >> - Map<String, String[]> params = >> getContainerRequest().getParameterMap(); >> - for (Map.Entry<String, String[]> param : params.entrySet()) >> + WebApplication application = new WebApplication() >> { >> - final String name = param.getKey(); >> - final String[] values = param.getValue(); >> - >> - // build a mutable list of query params that have >> the same name as the post param >> - List<StringValue> queryValues = >> queryParams.getParameterValues(name); >> - if (queryValues == null) >> - { >> - queryValues = Collections.emptyList(); >> - } >> - else >> - { >> - queryValues = new >> ArrayList<StringValue>(queryValues); >> - } >> - >> - // the list that will contain accepted post param >> values >> - List<StringValue> postValues = new >> ArrayList<StringValue>(); >> - >> - for (String value : values) >> + @Override >> + public Class<? extends Page> getHomePage() >> { >> - StringValue val = StringValue.valueOf(value); >> - if (queryValues.contains(val)) >> - { >> - // if a query param with this value >> exists remove it and continue >> - queryValues.remove(val); >> - } >> - else >> - { >> - // there is no query param with this >> value, assume post >> - postValues.add(val); >> - } >> + return CustomRequestPage.class; >> } >> >> - if (!postValues.isEmpty()) >> + @Override >> + protected WebRequest >> newWebRequest(HttpServletRequest servletRequest, String filterPath) >> { >> - postParameters.put(name, postValues); >> + return new >> CustomServletWebRequest(servletRequest, filterPath); >> } >> - } >> - return postParameters; >> - } >> + }; >> >> - private Map<String, List<StringValue>> getPostRequestParameters() >> - { >> - if (postParameters == null) >> - { >> - postParameters = generatePostParameters(); >> - } >> - return postParameters; >> + WicketTester tester = new WicketTester(application); >> + tester.startPage(new CustomRequestPage()); >> } >> >> - private final IRequestParameters postRequestParameters = new >> IWritableRequestParameters() >> + private static class CustomRequestPage extends WebPage implements >> IMarkupResourceStreamProvider >> { >> - public void reset() >> - { >> - getPostRequestParameters().clear(); >> - } >> + private static final long serialVersionUID = 1L; >> >> - public void setParameterValues(String key, List<StringValue> >> values) >> + private CustomRequestPage() >> { >> - getPostRequestParameters().put(key, values); >> + assertTrue(getRequest() instanceof >> CustomServletWebRequest); >> } >> >> - public Set<String> getParameterNames() >> + public IResourceStream >> getMarkupResourceStream(MarkupContainer container, >> + Class<?> containerClass) >> { >> - return >> Collections.unmodifiableSet(getPostRequestParameters().keySet()); >> + return new StringResourceStream("<html></html>"); >> } >> - >> - public StringValue getParameterValue(String name) >> - { >> - List<StringValue> values = >> getPostRequestParameters().get(name); >> - if (values == null || values.isEmpty()) >> - { >> - return StringValue.valueOf((String)null); >> - } >> - else >> - { >> - return values.iterator().next(); >> - } >> - } >> - >> - public List<StringValue> getParameterValues(String name) >> - { >> - List<StringValue> values = >> getPostRequestParameters().get(name); >> - if (values != null) >> - { >> - values = >> Collections.unmodifiableList(values); >> - } >> - return values; >> - } >> - }; >> - >> - @Override >> - public IRequestParameters getPostParameters() >> - { >> - return postRequestParameters; >> - } >> - >> - @Override >> - public Url getUrl() >> - { >> - return new Url(url); >> } >> >> - @Override >> - public ServletWebRequest cloneWithUrl(Url url) >> + private static class CustomServletWebRequest extends >> ServletWebRequest >> { >> - return new ServletWebRequest(httpServletRequest, >> filterPrefix, url) >> + public CustomServletWebRequest(HttpServletRequest >> httpServletRequest, String filterPrefix) >> { >> - @Override >> - public IRequestParameters getPostParameters() >> - { >> - // don't parse post parameters again >> - return >> ServletWebRequest.this.getPostParameters(); >> - } >> - }; >> - } >> - >> - /** >> - * Creates multipart web request from this request. >> - * >> - * @param maxSize >> - * @param upload >> - * upload identifier for {@link UploadInfo} >> - * @return multipart request >> - * @throws FileUploadException >> - */ >> - public MultipartServletWebRequest newMultipartWebRequest(Bytes >> maxSize, String upload) >> - throws FileUploadException >> - { >> - return new >> MultipartServletWebRequestImpl(getContainerRequest(), filterPrefix, maxSize, >> - upload); >> - } >> - >> - /** >> - * Creates multipart web request from this request. >> - * >> - * @param maxSize >> - * @param upload >> - * upload identifier for {@link UploadInfo} >> - * @param factory >> - * @return multipart request >> - * @throws FileUploadException >> - */ >> - public MultipartServletWebRequest newMultipartWebRequest(Bytes >> maxSize, String upload, >> - FileItemFactory factory) throws FileUploadException >> - { >> - return new >> MultipartServletWebRequestImpl(getContainerRequest(), filterPrefix, maxSize, >> - upload, factory); >> - } >> - >> - private static final Logger logger = >> LoggerFactory.getLogger(ServletWebRequest.class); >> - >> - @Override >> - public String getPrefixToContextPath() >> - { >> - PrependingStringBuffer buffer = new PrependingStringBuffer(); >> - Url filterPrefixUrl = Url.parse(filterPrefix, getCharset()); >> - for (int i = 0; i < filterPrefixUrl.getSegments().size() - >> 1; ++i) >> - { >> - buffer.prepend("../"); >> + super(httpServletRequest, filterPrefix); >> } >> - return buffer.toString(); >> - } >> - >> - @Override >> - public Charset getCharset() >> - { >> - return RequestUtils.getCharset(httpServletRequest); >> - } >> - >> - @Override >> - public HttpServletRequest getContainerRequest() >> - { >> - return httpServletRequest; >> - } >> - >> - @Override >> - public String getContextPath() >> - { >> - return >> UrlUtils.normalizePath(httpServletRequest.getContextPath()); >> - } >> - >> - @Override >> - public String getFilterPath() >> - { >> - return UrlUtils.normalizePath(filterPrefix); >> - } >> - >> - @Override >> - public boolean shouldPreserveClientUrl() >> - { >> - return errorAttributes != null && >> !Strings.isEmpty(errorAttributes.getRequestUri()); >> } >> } >> >> Modified: >> wicket/trunk/wicket-core/src/test/java/org/apache/wicket/protocol/http/servlet/ServletWebRequestTest.java >> URL: >> http://svn.apache.org/viewvc/wicket/trunk/wicket-core/src/test/java/org/apache/wicket/protocol/http/servlet/ServletWebRequestTest.java?rev=1190589&r1=1190588&r2=1190589&view=diff >> ============================================================================== >> --- >> wicket/trunk/wicket-core/src/test/java/org/apache/wicket/protocol/http/servlet/ServletWebRequestTest.java >> (original) >> +++ >> wicket/trunk/wicket-core/src/test/java/org/apache/wicket/protocol/http/servlet/ServletWebRequestTest.java >> Fri Oct 28 21:31:23 2011 >> @@ -54,12 +54,34 @@ public class ServletWebRequestTest exten >> Url clientUrl = webRequest.getClientUrl(); >> assertEquals("request/Uri?some=parameter", >> clientUrl.toString()); >> >> - // error dispatched >> - httpRequest.setAttribute("javax.servlet.error.request_uri", >> "/some/error/url"); >> + // simulates a request that has errors metadata >> + httpRequest.setAttribute("javax.servlet.error.request_uri", >> + "/" + httpRequest.getContextPath() + >> "/any/source/of/error"); >> ServletWebRequest errorWebRequest = new >> ServletWebRequest(httpRequest, "/"); >> Url errorClientUrl = errorWebRequest.getClientUrl(); >> >> - assertEquals("/some/error/url", errorClientUrl.toString()); >> + assertEquals("any/source/of/error", >> errorClientUrl.toString()); >> + } >> + >> + /** >> + * <a >> href="https://issues.apache.org/jira/browse/WICKET-4168";>WICKET-4168</a> >> + */ >> + @Test >> + public void testClientURLIsContextRelativeInErrorResponses() >> + { >> + MockHttpServletRequest httpRequest = new >> MockHttpServletRequest(null, null, null); >> + httpRequest.setURL(httpRequest.getContextPath() + >> "/request/Uri"); >> + >> + String problematiURI = httpRequest.getContextPath() + >> "/any/source/of/error"; >> + >> + httpRequest.setAttribute("javax.servlet.error.request_uri", >> problematiURI); >> + >> + ServletWebRequest errorWebRequest = new >> ServletWebRequest(httpRequest, ""); >> + >> + Url errorClientUrl = errorWebRequest.getClientUrl(); >> + >> + assertEquals("any/source/of/error", >> errorClientUrl.toString()); >> + >> } >> >> /** >> >> >> > > > > -- > Martin Grigorov > jWeekend > Training, Consulting, Development > http://jWeekend.com > -- Martin Grigorov jWeekend Training, Consulting, Development http://jWeekend.com
