Re: [PATCH] Add test getHeader() and getHeaderDate() functionality for headers.h

["Ilya A. Volynets-Evenbakh" <ilya@total-knowledge.com>]

Please fix and resend (meaning - back out last commit from your git,
and recommit with fixes)
See below for comments.

Sergey wrote:
> From: Sergey Jukov <sergey@total-knowledge.com>
>
> ---
>  ChangeLog           |    4 ++--
>  engine.xml          |    2 --
>  headers/headers.cpp |   22 +++++++++++++++++-----
>  headers/headers.h   |    9 +++++++++
>  4 files changed, 28 insertions(+), 9 deletions(-)
>
> diff --git a/ChangeLog b/ChangeLog
> index 83a0a1d..9f4ba79 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,5 +1,5 @@
> -Sergey Jukov <sergey@total-knowledge>                Fri, 9 Sep 2006 10:30:00 -0800 
> -- Add extra path test functionality to headers.cpp.
> +Sergey Jukov <sergey@total-knowledge>                Tue, 12 Sep 2006 10:30:00 -0800 
> +- Add getHeader() and getDateHeader() test functionality to headers.cpp.
>  	
>  Sergey Jukov <sergey@total-knowledge.com>            Mon, 4 Sep 2006 22:10:03 -0800
>  - Add Utils servlet test scripts
> diff --git a/engine.xml b/engine.xml
> index 0851951..4dec2a8 100644
> --- a/engine.xml
> +++ b/engine.xml
> @@ -5,8 +5,6 @@
>  <app name="test">
>      <servlet name="IndexServlet" dso="./debug/index/IndexServlet.so"/>
>      <servlet name="HelloServlet" dso="./debug/hello/HelloServlet.so"/>
> -<!--    <servlet name="FirstTestServlet"
> -dso="./debug/firsttest/FirstTestServlet.so"/> -->
>   
Why is this here? I don't have the first test servlet, so should not
need this stuff.
>      <servlet name="FileUploadServlet" dso="./debug/fileupload/FileUploadServlet.so"/>
>      <servlet name="RedirectServlet" dso="./debug/redirect/RedirectServlet.so"/>
>      <servlet name="CookiesServlet" dso="./debug/cookies/CookiesServlet.so"/>
> diff --git a/headers/headers.cpp b/headers/headers.cpp
> index 1b6897b..e026299 100644
> --- a/headers/headers.cpp
> +++ b/headers/headers.cpp
> @@ -23,7 +23,7 @@ void HeadersServlet::service(servlet::Ht
>  {
>  	std::ostream &out = resp.getOutputStream();
>  	std::auto_ptr< std::vector<std::string> > headerNames = req.getHeaderNames();
> -	resp.setHeader("Pragma", "no-cache");
> +	//resp.setHeader("Pragma", "no-cache");
>   
Any reason for getting rid of this? If yes - don't comment things out -
just remove them.
>  	resp.setHeader("X-Test-Header", "MyHeader");
>  	renderHeader(out);
>  	out<<"<PRE>";
> @@ -41,12 +41,17 @@ void HeadersServlet::service(servlet::Ht
>  
>  	out << "\n==========\nHTTP Method = "+req.getMethod();
>  
> -	//std:: string methodName = "X-Test-Header";
> -	//out << "\nInteger Header = "+req.getIntHeader(methodName);
> -	out << "\nPath = "+req.getPathInfo();
> +	std:: string testHeaderName = "If-Modified-Since";
>  
> +	//out << "\nInteger Header = "+req.getIntHeader(methodName); unimplemented
> +	//out << "\nPath = "+req.getPathInfo(); unimplemented
> +	//out << "\nPath Translated = "+req.getPathTranslated(); unimplemented
> +	//out << "\nAuthentication Type = "+req.getAuthType(); unimplemented
>   
Again - it should either be here or not - no need to _add_ all this
commented out code.
>  
> -	//out << "\nPath Translated = "+req.getPathTranslated();
> +	out << "\nHeader = "+req.getHeader(testHeaderName); 
> +	std:: string dateHeader = LongToString(req.getDateHeader(testHeaderName));
> +
> +	out << "\nDate Header = "+dateHeader; 
>  
>  	out<<"</PRE>";
>  
> @@ -63,4 +68,11 @@ void HeadersServlet::renderFooter(std::o
>  	out<<"\n </body>\n</html>";
>  }
>  
> +std::string HeadersServlet::LongToString(long l)   //convert long to string
> +{
> +        std:: stringstream s;
> +        s << l;
> +        return s.str();
> +}
>   
There are few problems with this function.
1. It's useless.
2. If you need to perform this multiple times, create single
std::stringstream and use it every time.
3. In general, this falls into "utility functions" category, so it
should be static, and probably somewhere
    in some sort of common utility class, which is reused.

> +
>  EXPORT_SERVLET(HeadersServlet)
> diff --git a/headers/headers.h b/headers/headers.h
> index 2814471..811dd24 100644
> --- a/headers/headers.h
> +++ b/headers/headers.h
> @@ -24,6 +24,9 @@ #include <servlet/HttpServlet.h>
>  #include <servlet/HttpServletRequest.h>
>  #include <servlet/HttpServletResponse.h>
>  #include <iostream>
> +#include <sstream>
> +
> +
>  
>  /**
>  This servlet demonstrates how to set response headers, and look
> @@ -36,6 +39,10 @@ For example, in URL 
>  http://www.total-knowledge.com/csp/HeadersServlet/add/path?key=value
>  extra path information is "/add/path". Returns null if there was no 
>  extra path information provided.
> +It displays HTTP method with which this request was made, work of getHeader()
> +which returns the value of specified request header(null if there was no such 
> +header in HTTP request) and getDateHeader() which returns -1 if the request 
> +doesn't have a header. Example provided for If-Modified-Since HTTP request header.
>  
>  @author Ilya A. Volynets-Evenbakh
>  */
> @@ -44,8 +51,10 @@ class HeadersServlet : public servlet::H
>  private:
>  	void renderHeader(std::ostream&);
>  	void renderFooter(std::ostream&);
> +	std::string LongToString(long l);
>  public:
>  	virtual void service(servlet::HttpServletRequest& req, servlet::HttpServletResponse& resp);
> +
>   
White space-only changes again?
>  };
>  
>  #endif
>   

-- 
Ilya A. Volynets-Evenbakh
Total Knowledge. CTO
http://www.total-knowledge.com