Hi ,

this is my changes to configure the FileUploadBase

my mock classes to test the Click Pages

I've create a separate issue for #2 since it is unrelated to FileUpload. (CLK-327)

Having played with multipart requests today I am thinking the following:

#1 If fileSizeMax exception occurs, we store the exception (or exceptions in case of multiple fileUploads) in request scope and continue parsing the request. Form#isFormSubmission will still work correctly thus each FileField can display a fileSizeMax exception message.

#2 If sizeMax exception occurs, we store the exception but cannot continue parsing request. This means the request is broken. Page listeners won't be triggered and forms won't be processed. The developer will have to cater for this problem in the Page:

public MyPage extends Page {
  ...

  public void onPost() {
     if (getContext().hasErrors()) {

        String requestLength = Integer.toString(getContext().getRequest().getContentLength());
        String maxLength = "1024";

        Object args[] = new String {maxLength, requestLength};

        addModel("exception", getMessage("request-size-exceeded", args));

        return;
     }
     if (form.isFormSubmission()) {
       ...
     }
  }
  ...
}

In template we can have:

$exception

$form

If would be nice if #2 is more transparent though.

The other question is how to configure FileUploadBase.

We can add an entry in click.xml or add a createFileUploadBase in ClickServlet.

Personally I like configuring things in Java since its more flexible easy to javadoc and low maintenance.

However I dislike having two places where configuration can occur: click.xml and ClickServlet.

What do others think?

> The other question is how to configure FileUploadBase.
>
> We can add an entry in click.xml or add a createFileUploadBase in ClickServlet.
>
> Personally I like configuring things in Java since its more flexible easy to javadoc and low maintenance.
>
> However I dislike having two places where configuration can occur: click.xml and ClickServlet.
IMHO click.xml is not the place to configure individual frameworks (Cayenne or Hibernate
is not configured here either).

> What do others think?
To me this entire problem looks like a deficiency of commons-fileupload.

IMHO to make fileupload more user(and webframework) friendly, most issues should be
addressed by Apache fileupload itself (and we just update to the latest release).

In worst case I would prefer a property file like log4j.properties if this would work
without doing anything in click (like it's the case with log4j).

Regarding configuration that are component "instance" dependent, they should definitely go into the API.

Ahmed.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
Click-development mailing list
Click-development@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/click-development

Hi bob,

>>#1 If fileSizeMax exception occurs, we store the exception (or exceptions in case of multiple fileUploads) in request scope and continue parsing the request. >>Form#isFormSubmission will still work correctly thus each FileField can display a fileSizeMax exception message.

How do you do to continue the processing ? All the request.getParameter("foo") returns null after that... What´s the trick?

>>Personally I like configuring things in Java since its more flexible easy to javadoc and low maintenance.
>>However I dislike having two places where configuration can occur: click.xml and ClickServlet.

I agree. Any chance to contigure the FileField control ? Example: fileField.setSizeMax(x) ? Also.. is it possible to don´t have a request wrapper and the FileField by itself process the request in the onProcess method ? Not sure if this is possible...

Another thing about the Context creation... Now it is difficult to override the context creation it in the ClickServlet. Because even if we override the ClickServlet.getContext(). And create a subclass of Context, I can´t override nothing, because everything is in its constructor (the ClickRequestWrapper creation and fileUpload.parseRequest)... So I´m forced to call the super.constructor of Context, and this will parse the request in the super class. Understand? Not sure if I´m being clear ;)....

regards,

Hi Ricardo,

>>How do you do to continue the processing ? All the request.getParameter("foo") returns null after that... What´s the trick?

Have a look at FileUpload parseRequest method and notice that they bail once the exception is thrown. It looks possible to provide our own parseRequest method (~30 lines of code) and just continue parsing the request after exception is thrown.

    http://svn.apache.org/viewvc/commons/proper/fileupload/trunk/src/java/org/apache/commons/fileupload/FileUploadBase.java?revision=628386&view=markup

Nothing is ever as simple as that but it seems doable ;-)

>>I agree. Any chance to contigure the FileField control ? Example: fileField.setSizeMax(x) ?

Probably not, since we will need the sizeMax / fileSizeMax values before the Page is even created.

>>Also.. is it possible to don´t have a request wrapper and the FileField by itself process the request in the onProcess method ? Not sure if this is possible...

Don't think this is doable. Request should be parsed before Page is created so calls to getParameter("xxx") does not return null.

Thanks

Hi Ahmed,

>>In worst case I would prefer a property file like log4j.properties if this would work without doing anything in click (like it's the case with log4j).

Agreed that would be the preferred way however I can't find any such option. Also they only seem to release once a year, so we might wait awhile for such a feature :-(

Hi,

The last release is 1.2.1 - 2008-01-18, see here:

http://mirrors.uol.com.br/pub/apache/commons/fileupload/binaries/

http://commons.apache.org/fileupload/changes-report.html

it seems that they worked on some issues with FileSizeLimitExceededException ...

regards,

>>Another thing about the Context creation... Now it is difficult to override the context creation it in the ClickServlet. Because even if we override the ClickServlet.getContext(). And create a subclass of Context, I can´t override nothing, because >>everything is in its constructor (the ClickRequestWrapper creation and fileUpload.parseRequest)... So I´m forced to call the super.constructor of Context, and this will parse the request in the super class. Understand? Not sure if I´m being clear

Yes understood. In the past I also wanted to change Context to have a default no-arg constructor and a #init method that can be overridden.

However Context was designed to be immutable and thread safe, so its probably best we don't change it unless absolutely necessary. Also the Context constructor contains some tricky code.

The FileUpload API is core dependency in the Click framework as it supports multi-part request processing which is needed for both Page request autobinding and the FileField control.

As advocated by Ahmed in that I would like to see this configured in once place in click.xml, rather than through overriding ClickServlet methods. Unfortunately the FileUpload API is rather complicated and can't be configured easily via XML.

As Bob mentioned I don't think we should expect Apache fileupload to be revised to address our issues.

What I think we need to do is create a wrapper / helper class which can be configured in click.xml and which can parse the multi-part request parameters for us. This class would encapsulate the FileItemFactory, FileUploadBase and ServletRequestContext.

The ClickApp would create instances of these configured objects, for use in the Context and ClickRequestWrapper objects, being passed in via the constructor.

With regard to the Parsing exceptions, if we can continue processing that would be ideal. I think the exception should be be WARN logged, then squirreled away in the Context and raised when Context.getFileItem() is invoked for the failed file item.

In the Click design this is all ugly complex stuff, which the framework is trying to shield Click users from.

Working on this today revealed some problems:

#1 If sizeMax or fileSizeMax exception occurs and we keep parsing the request for form fields, then the entire request still needs to be read. So if a user submits a 200meg file, we will need to read in the entire 200meg *before* we can iterate to the next item in the request. Note that the uploading file is *not* written to disk it is purely streamed. I noticed that FileUpload 1.2.1 has a caveat where they close the inputStream once an exception is raised. Making this option impossible without hacking their source.

#2 Alternatively we can stop parsing the request store the exception and continue to the Page knowing that calling getParameter("xx") would return null. This means page autobinding won't work either.

I have implemented #1 and it seems to work using FileUpload 1.2 but resources are hogged with large files. Plus the code is quite complex.

#2 is how FileUpload is implemented by default but we get no parameters.

Neither choice is ideal but which one is better?

And if there are other avenues to explore let me know.

OK good progress Bob.

If FileUpload 1.2.1 close the stream on an exception, then I don't think we should pursue #1. I don't think we should be forking their codebase, and as you point out it raises other issues with resource hogging.

+1 for #2

regards Malcolm Edgar

+1 #2

no problem to stop the request...

we just need to set the error message somewhere and redirect to the page ... How the page can get this error message?

regards,

The intention is to throw the original FileUpload exception when you access the FileItem from the request.

regards Malcolm Edgar

>>The intention is to throw the original FileUpload exception when you access the FileItem from the request.

This is the conundrum with #2. No request parameters are available and so the form and its fields are not processed.

Field#bindRequestValue is never called thus FileField#getFileItem is never called.

What you end up with is a Post request that acts like a Get.

The FileSizeLimitExceededException and SizeLimitExceededException seems like Context related problems so I have methods Context#isValid and Context#getError.
 
The usage pattern would be:

    public void onPost() {
        if (!getContext().isValid()) {
            String errorMessage = getContext().getError();
            form.setError(errorMessage);
        }
    }

One would need to do the above if sizeLimit or fileSizeLimit is specified for file uploads. If you don't the page is just rendered as a normal Get request and no errors are displayed.

An alternative to the above is to throw an exception in ClickServlet and let ErrorPage handle it. But this makes the developers life more difficult because as Ricardo stated you really want to display the error message on the page with the form and fileField.

Or better we can handle the exception at the Page level:

    public void onPost() {
        if (! this.isValid()) {
            String errorMessage = getError();
            form.setError(errorMessage);
        }
    }

    public String getError() {

        Exception exception = (Exception) getRequest().getAttribute(PAGE_EXCEPTION);

        if (exception == null) {
            return null;
        }

        Long args[] = new Long(2);
        String message = null;

        if (exception instanceof SizeLimitExceededException) {
            message = (String) getMessages().get("request-size-limit-exceeded-error");
            args[0] = new Long(se.getActualSize());
            args[1] = new Long(se.getPermittedSize());
        }

        error = MessageFormat.format(message, args);
        return error;
    }

Makes localization of errors messages easier since we already support click-page.properties.

Looking into the FileUploadBase class it is really problematic for this use case (as you pointed out earlier), if an error occurs nothing gets returned, just an exception which has the name of the failed item in the message.

If we don't know what the form_name parameter is the form wont know that the request is intended for it. Then the whole Click event processing model breaks down, and we can't expose FileItem errors in the Form.

I think we need to hack our own version of this class to get some decent error handling behavior.

Probably something quite simple, in the parseRequest() method, catch the exception, create a custom exception which contain the processed file list, the name of the file item which cause the error, and the causing error. Then hopefully we can get the form_name, and do some descent error handling in the form.

regards Malcolm Edgar

> I think we need to hack our own version of this class to get some decent error handling behavior.
Yes, but please still submit it to the commons fileupload project too.
Maybe they will include it, the same way like was the case with Velocity improvements (but hopefully
not after so much time like it took with Velocity :) ).

Ahmed.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
Click-development mailing list
Click-development@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/click-development

>>Then hopefully we can get the form_name, and do some descent error handling in the form.

This is more or less how #1 worked. However you propose we stop processing when the first exception is raised. I will test it a bit. The assumption here is that the form_name is processed before any fileField's is processed. (From my testing this is indeed the case. The order of the fields are as they appear in the form).

Do note that the above can work for fileSizeMax but *not* sizeMax exceptions. If a SizeLimitExceededException is thrown #parseRequest is never called. FileUploadBase checks the size of request#getContentLength and then throws exception immediately. I have a workaround for this by temporarily setting sizeMax back to -1 so the request is parsed.

I have some time today and will try and checkin this new approach by tonight.

Hi all,

I liked the Page.getError() approach

regards,
Ricardo

Checked in changes for approach #1 as described by Malcolm.

FileField handles fileSizeMax exceptions and Form handles sizeMax exceptions. The FileUpload example was updated to show how multiple file uploads work.

Have not upgraded to FileUpload 1.2.1. It is not available on ibiblio yet. And FileUpload 1.2.1 breaks on Jetty 5.1.* but works on Jetty 6.*. I think its because the new FileUpload closes the stream once an exception occurs.

> Have not upgraded to FileUpload 1.2.1. It is not available on ibiblio yet.
> And FileUpload 1.2.1 breaks on Jetty 5.1.* but works on Jetty 6.*.
> I think its because the new FileUpload closes the stream once an exception occurs.
IMHO this it's not a problem with breaking Jetty5.1.

Ahmed.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
Click-development mailing list
Click-development@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/click-development

Every now and then between container restarts I receive this exception:

    Exception in thread "File Reaper" java.lang.NullPointerException
        at org.apache.commons.io.FileCleaner$Reaper.run(FileCleaner.java:213)

This is a FileUpload issue and as pointed out by Ricardo its fixed in FileUpload 1.2.1: http://commons.apache.org/fileupload/changes-report.html

Bob,

I tested it's working very well. thanks

translation of 'click-control.properties' to brazilian portuguese

Have checked in updates around this, to support page auto-mapped parameters when a file upload error occurs. Also to enable size configurations to be set using XML rather than writing a custom class. Note custom class is still supported.

Once again nice work on this Bob.

One thing that has been worrying me with the current design is that there is lot of logic embedded in the Context constructor. Initially this was a fairly simple immutable object which just held references, but it is growing to be more complex now with ThreadLocal support and ClickRequestWrapper support. I wonder if we have the ClickRequestWrapper initialized in the ClickServlet and have it passed into the constructor. This needs more thought in terms of ThreadLocal support and Jetty. This also has implications for increasing the size/complexity of ClickServlet.

regards Malcolm Edgar

>>This needs more thought in terms of ThreadLocal support and Jetty.
Heh I am already scared ;-)

I think this will be good. Moving the code to ClickServlet enables easier customization of Context dependencies. This in turn helps with the mock package.

Hi Malcolm,

Another advantage of moving the context construction code to ClickServlet (or somewhere else) is that the Context exception handling can be improved. I don't like the idea of passing the exception around as request attribute.

>> Have checked in updates around this, to support page auto-mapped parameters when a file upload error occurs

I just had a look at FileUploadService#parseRequest method and not sure if auto-mapped parameters would really matter in this case. Is auto-mapped parameters not suited for GET requests while FileUpload only occur on POST requests? Also we cannot determine which auto-mapped parameters will be parsed out so this seems a bit dangerous. Say for example you have a check if parameter X is available, and if it is you do something. The problem is that certain other parameters might not be available. To me it seems better to just fail completely (and display the file upload error message) rather than to going half way. Perhaps I am missing some other use cases?

Also note if we upgrade to FileUpload v1.2.1 before 1.5M1 we should add the following snippet to our FileUploadService#parseRequest method. This enables headers to be available on the FileItems. (only available from v1.2.1)

    if (fileItem instanceof FileItemHeadersSupport) {
        final FileItemHeaders fih = item.getHeaders();
        ((FileItemHeadersSupport) fileItem).setHeaders(fih);
    }
    items.add(fileItem);

regards

bob

I have logged an issue with FileUpload to improve exception handling and include more metadata for better error reporting.

https://issues.apache.org/jira/browse/FILEUPLOAD-154

This will enable us to specify the name of the uploaded file in the error message.

Hi Ricardo,

Your fileupload translation to brazilian portuguese won't work right now.

FileUpload 1.2.1 does not expose the file name that was uploaded, so the property had to be changed for now.

Here is what is in trunk:

file-size-limit-exceeded-error=The file is too large. {0} is limited to {1} bytes. Posted size was {2} bytes.
post-size-limit-exceeded-error=The request was rejected because it is limited to {0} bytes. Posted size was {1} bytes.

Note the file-size-limit-exceeded-error only take 3 parameters.

Could you translate that and patch it against trunk please?

kind regards

bob

fixed in 1.5-M1