cocoon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Grzegorz Kossakowski <g...@tuffmail.com>
Subject Re: SSF Secrets or valid enhancements?
Date Wed, 07 Jan 2009 13:17:46 GMT
Gabriel Gruber pisze:
> 
> Hello Folks,

Hello Gabriel,

> over the holidays I continued my last years playings with Apache Wicket
> and Cocoon. This time a little bit more successful. I have a working
> prototype now, where a Wicket Servlet is called by the Servlet Service
> Framework and the resulting xhtml is postprocessed by some XSLTs inside
> cocoon. Even Wicket Ajax is working fine!

Nice. We've been discussing such a setup a long time ago (two years ago?) but nobody really
cared to
implement this. I'm glad to see that SSF can work as we designed it. :-) (kudos to Daniel
Fagerstrom)

> However in order to get it running I had to make some changes in SSF,
> because I discovered these problems:
> 
> a) I found no way to foreward the request parameters from the original
> HTTP POST Request to the Wicket Servlet, alltough my pipelines looked
> correct. But after debugging I discovered that the created
> ServletServiceRequest object just didn't contain the request parameters
> of the original request.
> 
> b) SSF is not capeable of handling redirection of called HTTP servlets
> correctly. (HTTP Response 302)

What do you mean exactly? SSF will pass this kind of response back to browser. Would you expect
it
do something else?

> c) some Methods of SSFs 'ServletServiceRespose' class are not
> implemented, but needed by the Wicket Servlet, namely: encodeRedirectUrl
> and encodeUrl..

Yep, SSF contains quite a lot of TODO markers. Most of these methods are very straightforward
to
implement. Patches are welcome.

> In the end it turned out that b) is not a problem, if you change the
> RequestCycleStrategy inside Wicket (to use not redirection at all...).
> 
> So in order to fix these issues for me I had to make some changes to
> cocoon-servlet-service-impl and cocoon-servlet-service-components. While
> this patches worked for my usecase I would like to see those changes
> integrated into trunk obviously ;-)
> 
> I case of the ServletServiceRequest object I was a bit puzzled because
> in different posts on the mailing list I read that the "... req params &
> attrs  and session are passed/shared..." which was the thing I needed.
> however it didnot work for me. Looking at the code it seemed to me that
> only request parameters which where appended to the called URL (like
> myServlet?param1=a&param2=b)where actually passed, while does which were
> POSTED where NOT passed.

Strictly speaking, parameters sent using POST method are not parameters in HTTP spec sense
as far as
I remember. Anyway, from Servlets are handling these both methods in an unified way.

The main reason why this is not happening is that it's not that easy to implement, see
http://thread.gmane.org/gmane.text.xml.cocoon.devel/73088/focus=73137 and answers to this
post which
tries to clarify the problem.

> As the current behavior is somehow different to my new implementation, I
> suggest to implement a marker to switch between old and new behavior...
> 
>     *public* String getParameter(String name) {
>            
>             *if* (useParentRequest)                          
>                     *return* (String)
> *this*.parentRequest.getParameter(name);
>            
>         *return* (String) *this*.parameters.getValue(name);
>     }
> 
> How this marker could be implemented? F.i. via another special Postfix
> in the servlet-Name (like the one which is used to identify full
> qualified servlet-class-names)

-1
Current implementation is not final one and if we break back-compatibility slightly in order
to make
implementation complete I'm all for it.

> If I am wrong with my assumptions, please tell me how I can reach the
> desired result without any modifications of this cocoon classes.

As you see it has to be modified and it's been already discussed but the solution is not that
simple
and requires careful consideration of all possible cases.

> Attached are my patches...

Comments inline.

> ### Eclipse Workspace Patch 1.0
> #P cocoon-servlet-service-components
> Index: src/main/java/org/apache/cocoon/servletservice/components/ServletSource.java
> ===================================================================
> --- src/main/java/org/apache/cocoon/servletservice/components/ServletSource.java	(revision
731081)
> +++ src/main/java/org/apache/cocoon/servletservice/components/ServletSource.java	(working
copy)
> @@ -84,7 +84,8 @@
>                         + "The servlet returned " + servletConnection.getResponseCode()
+ " response code.");
>              
>              // FIXME: This is not the most elegant solution
> -            if (servletConnection.getResponseCode() != HttpServletResponse.SC_OK) {
> +            int rc = servletConnection.getResponseCode(); 
> +            if (rc != HttpServletResponse.SC_OK) {
>                  //most probably, servlet returned 304 (not modified) and we need to
perform second request to get data
>  
>                  //
> @@ -94,7 +95,15 @@
>                  //       and, as a result, GET request instead of POST.
>                  //
>  
> -                servletConnection = createServletConnection(location);
> +            	if (rc == HttpServletResponse.SC_FOUND) {

Using SC_MOVED_TEMPORARILY is preferred as it's more obvious and stays close to terminology
used in
HTTP spec.
What about SC_MOVED_PERMANENTLY?

These two redirects should be handled in unified way as long as you don't deal with caching
as
defined in HTTP spec.

> +            		// if request sent a redirect status code
> +            		// we shall connect to the new location
> +            		String redirectUrl = servletConnection.getRedirectURI().toString();
> +            		servletConnection = createServletConnection(redirectUrl);

Here you assume that getRedirectURI() returns *internal* URI right (with servlet: protocol)?
Without
testing this assumption is wrong.

> +            	} else {
> +            		servletConnection = createServletConnection(location);
> +            	}
> +            	
>                  servletConnection.connect();
>              }


> ### Eclipse Workspace Patch 1.0
> #P cocoon-servlet-service-impl
> Index: src/main/java/org/apache/cocoon/servletservice/util/ServletServiceResponse.java
> ===================================================================
> --- src/main/java/org/apache/cocoon/servletservice/util/ServletServiceResponse.java	(revision
731081)
> +++ src/main/java/org/apache/cocoon/servletservice/util/ServletServiceResponse.java	(working
copy)
> @@ -20,6 +20,7 @@
>  import java.io.OutputStream;
>  import java.io.OutputStreamWriter;
>  import java.io.PrintWriter;
> +import java.net.URI;
>  import java.text.ParseException;
>  import java.text.SimpleDateFormat;
>  import java.util.Date;
> @@ -31,6 +32,7 @@
>  import javax.servlet.http.Cookie;
>  import javax.servlet.http.HttpServletResponse;
>  
> +
>  /**
>   * Creates a {@link HttpServletResponse} object that is usable for internal block calls.
>   *
> @@ -45,6 +47,7 @@
>      private boolean committed;
>      private Locale locale;
>      private int statusCode;
> +    private URI url;

This is non-obvious variable as response usually does not care about to which url it is the
response. Some comment would be useful why it's needed.

>      private Map headers;
>  
> @@ -53,6 +56,11 @@
>       */
>      final SimpleDateFormat dateFormat = new SimpleDateFormat("EEE', 'dd' 'MMM' 'yyyy'
'HH:mm:ss' 'Z", Locale.US);
>  
> +    public ServletServiceResponse(URI url) {
> +    	this.url = url;
> +        headers = new HashMap();
> +        statusCode = HttpServletResponse.SC_OK;
> +    }
>  
>      public ServletServiceResponse() {
>          headers = new HashMap();
> @@ -83,23 +91,19 @@
>      }
>  
>      public String encodeRedirectUrl(String url) {
> -        // TODO Auto-generated method stub
> -        return null;
> +        return url;
>      }
>  
>      public String encodeRedirectURL(String url) {
> -        // TODO Auto-generated method stub
> -        return null;
> +        return url;
>      }
>  
>      public String encodeUrl(String url) {
> -        // TODO Auto-generated method stub
> -        return null;
> +        return url;
>      }
>  
>      public String encodeURL(String url) {
> -        // TODO Auto-generated method stub
> -        return null;
> +        return url;
>      }

Quick check of Javadocs[1] reveals:
Encodes the specified URL by including the session ID in it, or, if encoding is not needed,
returns
the URL unchanged.

I can't see an argument for assumption that encoding is not needed here. Could you elaborate?

>      public void flushBuffer() throws IOException {
> @@ -191,8 +195,39 @@
>      }
>  
>      public void sendRedirect(String location) throws IOException {
> -        // TODO Auto-generated method stub
> +    	
> +        if (!location.startsWith("servlet:"))
> +        {
> +            StringBuffer buf = getRootURL(url);
> +//            if (location.startsWith("/"))
> +//                buf.append(URIUtil.canonicalPath(location));
> +//            else
> +//            {
> +//                String path=url.getPath();
> +//                String parent=(path.endsWith("/"))?path:URIUtil.parentPath(path);
> +//                location=URIUtil.canonicalPath(URIUtil.addPaths(parent,location));
> +//                if (!location.startsWith("/"))
> +//                    buf.append('/');
> +//                buf.append(location);
> +//            }
> +            buf.append(location);
> +            location=buf.toString();
> +        }
> +    	
> +    	// build redirect location
> +    	setHeader("Location",location);
> +        setStatus(HttpServletResponse.SC_MOVED_TEMPORARILY);
>      }
> +    
> +    public StringBuffer getRootURL(URI uri)
> +    {
> +        StringBuffer url = new StringBuffer();
> +        String scheme = uri.getScheme();
> +        url.append(scheme);
> +        url.append(":");
> +    	url.append(uri.getSchemeSpecificPart());
> +        return url;
> +    }

Cannot parse this fragment. Why ServletServiceRepsonse has to care about protocol of location
argument in sendRedirect()?

>      public void setBufferSize(int size) {
>          // TODO Implement buffering, for the moment ignore.
> Index: src/main/java/org/apache/cocoon/servletservice/AbsoluteServletConnection.java
> ===================================================================
> --- src/main/java/org/apache/cocoon/servletservice/AbsoluteServletConnection.java	(revision
731081)
> +++ src/main/java/org/apache/cocoon/servletservice/AbsoluteServletConnection.java	(working
copy)
> @@ -77,7 +77,7 @@
>              throw iae;
>          }
>          this.request = new ServletServiceRequest(reqUri, CallFrameHelper.getRequest());
> -        this.response = new ServletServiceResponse();
> +        this.response = new ServletServiceResponse(this.uri);
>      }
>  
>      /**
> Index: src/main/java/org/apache/cocoon/servletservice/ServletConnection.java
> ===================================================================
> --- src/main/java/org/apache/cocoon/servletservice/ServletConnection.java	(revision 731081)
> +++ src/main/java/org/apache/cocoon/servletservice/ServletConnection.java	(working copy)
> @@ -113,6 +113,12 @@
>       * @return a URI representing this servlet connection.
>       */
>      URI getURI();
> +    
> +    /**
> +    * Get a URI representing the redirect URI incase of responsecode of 302
> +    * 
> +    * @return a URI representing the redirect servlet connection
> +    */
> +   URI getRedirectURI();
>  
> -
>  }
> Index: src/main/java/org/apache/cocoon/servletservice/util/ServletServiceRequest.java
> ===================================================================
> --- src/main/java/org/apache/cocoon/servletservice/util/ServletServiceRequest.java	(revision
731081)
> +++ src/main/java/org/apache/cocoon/servletservice/util/ServletServiceRequest.java	(working
copy)
> @@ -123,6 +123,8 @@
>      private Parameters parameters;
>  
>      private ServletServiceContext context;
> +    
> +    private boolean useParentRequest = true;
>  
>      /**
>       * @param uri
> @@ -255,18 +257,31 @@
>      //
>  
>      public String getParameter(String name) {
> +    	
> +    	if (useParentRequest)  			
> +    		return (String) this.parentRequest.getParameter(name);
> +    	
>          return (String) this.parameters.getValue(name);
>      }
>  
>      public String[] getParameterValues(String name) {
> +    	if (useParentRequest)
> +    		return this.parentRequest.getParameterValues(name);
> +    	
>          return this.parameters.getValues(name);
>      }
>  
>      public Enumeration getParameterNames() {
> +    	if (useParentRequest)
> +    		return this.parentRequest.getParameterNames();
> +    	
>          return this.parameters.getNames();
>      }
>  
>      public Map getParameterMap() {
> +    	if (useParentRequest)
> +    		return this.parentRequest.getParameterMap();
> +    	
>          return this.parameters.getValues();
>      }

I've already given my opinion on using special marker. Would like to hear what others think
about it.

> Index: src/main/java/org/apache/cocoon/servletservice/AbstractServletConnection.java
> ===================================================================
> --- src/main/java/org/apache/cocoon/servletservice/AbstractServletConnection.java	(revision
731081)
> +++ src/main/java/org/apache/cocoon/servletservice/AbstractServletConnection.java	(working
copy)
> @@ -22,6 +22,7 @@
>  import java.io.InputStream;
>  import java.io.OutputStream;
>  import java.net.URI;
> +import java.net.URISyntaxException;
>  
>  import javax.servlet.ServletContext;
>  import javax.servlet.ServletException;
> @@ -194,5 +195,15 @@
>          }
>  
>      }
> +    
> +    public URI getRedirectURI() {
> +    	String redirectLocation = this.response.getHeader("Location");
> +    	try {
> +			return new URI(redirectLocation);
> +		} catch (URISyntaxException e) {
> +			this.logger.error("problems creating correct URI...", e);
> +			return null;
> +		}
> +    }
>  
>  }
> Index: src/main/java/org/apache/cocoon/servletservice/RelativeServletConnection.java
> ===================================================================
> --- src/main/java/org/apache/cocoon/servletservice/RelativeServletConnection.java	(revision
731081)
> +++ src/main/java/org/apache/cocoon/servletservice/RelativeServletConnection.java	(working
copy)
> @@ -79,7 +79,7 @@
>  
>          // prepare request and response objects
>          this.request = new ServletServiceRequest(reqUri, CallFrameHelper.getRequest());
> -        this.response = new ServletServiceResponse();
> +        this.response = new ServletServiceResponse(reqUri);
>  
>          if(this.logger.isDebugEnabled()) {
>              this.logger.debug("Resolving relative servlet URI " + this.uri.toASCIIString());

There are a few more minor things to comment on but for this iteration let's focus on most
important
stuff.

> WDYT?
> 
> By the way if it is interesting for the public I can post my
> wicket-cocoon integration sample for others!

Some folks asked for it in the past and we considered this ourselves so I think this is interesting
to others.

Thanks for your offer and for work on implementing missing pieces of SSF.


-- 
Best regards,
Grzegorz Kossakowski

Mime
View raw message