commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Martin Cooper" <mart...@apache.org>
Subject Re: [FILEUPLOAD] RfC: Proposed API changes for streaming
Date Sat, 10 Jun 2006 16:55:07 GMT
On 6/9/06, Jochen Wiedmann <jochen.wiedmann@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
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message