[FILEUPLOAD] RfC: Proposed API changes for streaming

[Jochen Wiedmann <jo...@gmail.com>]

Hi,

first of all, let me say thanks for giving me the possibility to contribute
to the project as a committer. Let's hope, I'll be doing fine. ;-)

As already written, the changes required for streaming are nontrivial. I'd
like to discuss them even if I were an experienced project member. Which I
am not, so please be alert even more.

The main reason for API changes is, that the existing interface FileItem is
to powerful. Therefore I propose to introduce a new interface, let's call it
SlimFileItem, or StreamingFileItem, with the following signature:

    public interface SlimFileItem extends Serializable {
        InputStream getInputStream() throws IOException;
        String getContentType();
        String getName();
        String getFieldName();
        boolean isFormField();
    }

As you can see, the FileItem can be changed to extend SlimFileItem.
Additionally, FileItem has a semantic difference: It allows to invoke
getInputStream() more than once, while SlimFileItem should throw an
IllegalStateException on the second invocation. So far, these changes should
not introduce a problem.

The SlimFileItem is not to be created by a factory: The need for a factory
is mainly caused by the fact, that the FileItem's data needs to be written.
SlimFileItem is best implemented as an inner class of the MultipartStream.

So the API changes can be reduced to an additional method

    Iterator /* SlimFileItem */ getItemIterator(RequestContext ctx)
      throws FileUploadException;

in FileUploadBase. The existing method

    List parseRequest(RequestContext ctx) throws FileUploadException;

would be changed to use getItemIterator internally and convert the
SlimFileItem instances into FileItem.

Finally, a suggestion for simplification: Let's change the
FileUploadException to extend IOException, and not Exception. It will avoid
a lot of casting, type checking, and conversions. Besides, it also makes
sense in the semantic level. At least, I do thing so.

The only argument I can see against the exception change is the following:
People will see compiler errors when using constructs like

    try {
       ...
    } catch (IOException e) {
       ...
    } catch (FileUploadException e) {
       ...
    }

The obvious solution would be to change the order. IMO, that's acceptable.


Regards for any comments,

Jochen

---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org

Re: [FILEUPLOAD] RfC: Proposed API changes for streaming

[Jochen Wiedmann <jo...@gmail.com>]

Martin Cooper wrote:

> Let's hope. ;-) Please add youself to the list of developers in the
> project.xml file.

Done.


> This looks interesting. However, given the non-trivial nature of the
> changes, and the "stylistic" effect on the way FileUpload works, I
> wonder if
> this wouldn't be better incorporated into the FileUpload 2 design, rather
> than being retrofitted into FileUpload 1.x.

I am open for both. In either way, we can start in a branch. That leaves
everything open, doesn't it? I have one concern, though: Please let's try to
have a at least a beta release within short time, say three months?


> Well, I guess I don't agree here. If FileItem extended  SimFileItem, then I
> could pass a FileItem to methods that accept a SlimFileItem. At that point,
> that SlimFileItem may or may not throw an exception if getInputStream is
> called more than once. That's bad API behaviour.

Personally I have no problem with my suggestion: If a method would accept a
StreamingFileItem, then that method should not expect, that getInputStream()
cannot be used more than once, so that's okay.

However, let's say that FileItem doesn't extend StreamingFileItem. Fine with me.


> SlimFileItem is best implemented as an inner class of the MultipartStream.
> 
> 
> Hmm. Not if you expect to expose SlimFileItem as part of the public API,
> which you are suggesting with the getItemIterator method below.

That's something I do not understand. The StreamingFileItem is an interface,
which is exposed. What's the problem, if the implementation is an inner class?


> Multipartstream isn't (supposed to be) part of the public API, so exposing
> an inner class of it through the public API is a no-no.

That's good to know. I get you right, that this means we may change that
classes API, may we?


> No. A FileUploadException isn't always related to IO, so extending
> IOException would simply be wrong.

Ok.


Attached you find a proposed patch, that takes a first step: The
MultipartStream receives an inner class ItemInputStream. This inner class is
used by readBodyData. (Obviously, the same InputStream will later be
returned by the StreamingFileItem.)


Jochen

Re: [FILEUPLOAD] RfC: Proposed API changes for streaming

[Martin Cooper <ma...@apache.org>]

On 6/9/06, Jochen Wiedmann <jo...@gmail.com> wrote:
>
>
> Hi,
>
> first of all, let me say thanks for giving me the possibility to
> contribute
> to the project as a committer. Let's hope, I'll be doing fine. ;-)


Let's hope. ;-) Please add youself to the list of developers in the
project.xml file.

As already written, the changes required for streaming are nontrivial.


This looks interesting. However, given the non-trivial nature of the
changes, and the "stylistic" effect on the way FileUpload works, I wonder if
this wouldn't be better incorporated into the FileUpload 2 design, rather
than being retrofitted into FileUpload 1.x.

Sadly, there is no one place where the goals for FileUpload 2 are written
down right now. Some are in existing enhancement requests, some are
mentioned in random old e-mail threads, and some are still just in my head.
Now that there are two of us, I guess I'd better try to find some time to
write things down. ;-)

I'd
> like to discuss them even if I were an experienced project member. Which I
> am not, so please be alert even more.


I've had coffee. I'm alert. ;-)

The main reason for API changes is, that the existing interface FileItem is
> to powerful.


By "too powerful", you mean that it tackles two tasks - providing an
interface to the incoming data, and implicitly providing storage for it.
That's true, and if we want to separate those functions, then perhaps that
calls for a more radical separation than you are suggesting.

Therefore I propose to introduce a new interface, let's call it
> SlimFileItem, or StreamingFileItem, with the following signature:


I don't like "Slim". "Stream" or "Streaming" would be better. But I'm
wondering if the relationship between this and the existing notion of a
FileItem should be so direct. I'm really thinking off the top of my head
here, and haven't thought it through, though, so I don't have any more
concrete ideas at this point. It's worth some thought, though.

    public interface SlimFileItem extends Serializable {
>         InputStream getInputStream() throws IOException;
>         String getContentType();
>         String getName();
>         String getFieldName();
>         boolean isFormField();
>     }
>
> As you can see, the FileItem can be changed to extend SlimFileItem.
> Additionally, FileItem has a semantic difference: It allows to invoke
> getInputStream() more than once, while SlimFileItem should throw an
> IllegalStateException on the second invocation. So far, these changes
> should
> not introduce a problem.


Well, I guess I don't agree here. If FileItem extended  SimFileItem, then I
could pass a FileItem to methods that accept a SlimFileItem. At that point,
that SlimFileItem may or may not throw an exception if getInputStream is
called more than once. That's bad API behaviour.

The SlimFileItem is not to be created by a factory: The need for a factory
> is mainly caused by the fact, that the FileItem's data needs to be
> written.


Sort of. It's really that FileItem is accomplishing two different tasks, as
I noted above.

SlimFileItem is best implemented as an inner class of the MultipartStream.


Hmm. Not if you expect to expose SlimFileItem as part of the public API,
which you are suggesting with the getItemIterator method below.
Multipartstream isn't (supposed to be) part of the public API, so exposing
an inner class of it through the public API is a no-no.

So the API changes can be reduced to an additional method
>
>     Iterator /* SlimFileItem */ getItemIterator(RequestContext ctx)
>       throws FileUploadException;
>
> in FileUploadBase. The existing method
>
>     List parseRequest(RequestContext ctx) throws FileUploadException;
>
> would be changed to use getItemIterator internally and convert the
> SlimFileItem instances into FileItem.
>
> Finally, a suggestion for simplification: Let's change the
> FileUploadException to extend IOException, and not Exception.


No. A FileUploadException isn't always related to IO, so extending
IOException would simply be wrong.

--
Martin Cooper


It will avoid
> a lot of casting, type checking, and conversions. Besides, it also makes
> sense in the semantic level. At least, I do thing so.
>
> The only argument I can see against the exception change is the following:
> People will see compiler errors when using constructs like
>
>     try {
>        ...
>     } catch (IOException e) {
>        ...
>     } catch (FileUploadException e) {
>        ...
>     }
>
> The obvious solution would be to change the order. IMO, that's acceptable.
>
>
> Regards for any comments,
>
> Jochen
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: commons-dev-help@jakarta.apache.org
>
>