getRequestURL in HttpServletRequestWrapper returns the string "null" in the request URL

• Key: CACTUS-89
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Vincent Massol
• Reporter: Scott Leberknight
• Component/s: Framework
• Affects Version/s: 1.5
• Fix Version/s: 1.6

Environment

Operating System: All
Platform: All

Description

Learning Cactus, I developed a simple servlet called "Servlet1" and a
corresponding Cactus test called "TestServlet1". I am testing the doGet()
method. In the beginDoGet() method I do the following:

request.setURL("localhost:8080", "/testWeb", "/servlet1", null, "name=scott");

Note in the above that pathInfo is null. Then, in my testDoGet() method, I do
the following assertions (among others):

assertEquals("requestURI",
             "/testWeb/servlet1",
             request.getRequestURI()); // this passes

assertEquals("requestURL",
             "http://localhost:8080/testWeb/servlet1",
             request.getRequestURL().toString()); // this fails

The second assertion fails! The test runner reports that the actual value was:
http://localhost:8080/testWeb/servlet1null

Setting pathInfo as null in the setURL() method on the WebRequest in beginXXX()
(which the Cactus JavaDocs state is OK) produces the literal string "null" at
the end of the request URL when you call the getRequestURL() method in testXXX()
methods.

I did some digging into the Cactus 1.5 source code and found what apppears to be
an error in the getRequestURL() method in the
org.apache.cactus.server.HttpServletRequestWrapper class. I compared this
method's code to the implementation of getRequestURI() in
AbstractHttpServletRequestWrapper, which handles a null pathInfo correctly.

Basically the problem is that HttpServletRequestWrapper.getRequestURL() doesn't
check whether pathInfo is null when it creates the request URL. In contrast,
AbstractHttpServletRequestWrapper.getRequestURI() checks both servletPath and
pathInfo and prints an empty string into the StringBuffer if they are null. So,
I believe this original code snippet from getRequestURL():

result = new StringBuffer(this.url.getProtocol() + "://"
       + getServerName() + ":" + getServerPort() + getContextPath()
       + getServletPath() + getPathInfo());

can be fixed if it is changed to:

result = new StringBuffer(this.url.getProtocol() + "://"
       + getServerName() + ":" + getServerPort() + getContextPath()
       + ((getServletPath() == null) ? "" : getServletPath())
       + ((getPathInfo() == null) ? "" : getPathInfo());

While this fixes it, I think it could be improved by recognizing that the
request URI is simply part of the request URL and that we can get it by calling
the getRequestURI() method defined in HttpServletRequestWrapper's superclass,
and we could refactor as:

result = new StringBuffer(this.url.getProtocol() + "://"
       + getServerName() + ":" + getServerPort()
       + getRequestURI());

Hopefully this explanation is helpful and it is, in fact, a bug. I did research
whether this bug existed in Bugzilla and could not find anything about it.