Skip to content
This repository has been archived by the owner. It is now read-only.

RF-12224: FileUpload - initial support for multiple file selection #78

Closed
wants to merge 6 commits into from

Conversation

@lfryc
Copy link
Contributor

commented Jun 12, 2013

Opening pull request for a review.

public class FileUploadRendererBase extends RendererBase {

private static int parseMaxFilesQuantity(String maxFilesQuantity) {

This comment has been minimized.

Copy link
@lfryc

lfryc Jun 12, 2013

Author Contributor

Couldn't we just change maxFilesQuantity attribute to Integer?

This comment has been minimized.

Copy link
@simone83

simone83 Jun 12, 2013

Yes of course we could, so I've changed related getter inside the AbstractFileUpload. See next comment.

acceptedTypes.trim();
if (acceptedTypes.length() == 0)
return result;
String[] types = acceptedTypes.split(",");

This comment has been minimized.

Copy link
@lfryc

lfryc Jun 12, 2013

Author Contributor

Arrays.asList(acceptedTypes.toLowerCase().replaceAll("\s+", "").split(','))

This comment has been minimized.

Copy link
@lfryc

lfryc Jun 12, 2013

Author Contributor

We should move this logic to AbstractFileUpload

This comment has been minimized.

Copy link
@simone83

simone83 Jun 12, 2013

This logic has been moved in that pull request #79. I forgot to add the other boolean method acceptsFile as you suggests and to add the import of java.util.List. Can you do that for me? Thanx

break;
if (clientId.equals(file.getParameterName())) {
if (acceptedTypes.isEmpty()

This comment has been minimized.

Copy link
@lfryc

lfryc Jun 12, 2013

Author Contributor

Let's introduce new method in AbstractFileUpload and call it boolean acceptsFile(UploadedFile file);

@simone83

This comment has been minimized.

Copy link

commented Jun 12, 2013

:+1 . In this method we also need (but I'm sure that we cannot do that modifing only few sources :( ): <-- ahahah

  • check the maxFilesUpload not only per-request but with all files uploaded. If not, we have the problem if an user first uploads K<N (let maxFilesUpload=N) files, then another request for other K<N files and that is going done because it will only check if K<N and it does. In the new fileupload.js I sent you today this is fixed, but we also do this fix at serverside (remember? It plays the game in a sort of amleto-boolean-logic : upload the whole files block or not upload?).
    This is not a great bug for now because the adviced user can avoid that problem inserting some logic inside the listener...
  • another, similar bug of above, but related to the duplicates problem. Now we don't care about that at server-side and we have not a complete file list at server-side. Only the user's have that. We have only the files sent in the multipart per-request... so if we have > 1 request for upload several files, how can we do the check for duplicated filenames?
    We need to hack the javascript code for include an hidden attribute which contains the updated value of __getItemTotalCount() inside, so we can read that at server-side (dirty but hacky) if boolean isNoDuplicate() returns true. Similar as above, the adviced user can avoid this problem with a check of duplicates inside the bean which the listener is implemented.

OoooooooooooooookaY?

^ not intentionally, I've mirrored the worry face :( |I'm a mirror| ): !!!

@simone83

This comment has been minimized.

We have to add this method and then send the event if and only if isUploadable(file) returns true . This will clean the code a little.

public boolean isUploadable(UploadedFile uploadedFile) {

    if (uploadedFile == null) return false;

    String fileName = uploadedFile.getName();

    List<String> acceptedTypesList = (List<String>) (Arrays.asList(getAcceptedTypes().replaceAll("\\s+","").toLowerCase().split(",")));

    if (acceptedTypesList.isEmpty())

        return true;

    else if (acceptedTypesList.contains("") && acceptedTypesList.size() <= 1)

        return true; //fix the bug if no file suffix has specified but there are commas so empty string elements are in list!

    int lastIndexOfDot = fileName.lastIndexOf('.');

    if (lastIndexOfDot > -1) {

        String suffix = fileName.substring(lastIndexOfDot + 1).toLowerCase();

        return (acceptedTypesList).contains(suffix);

    }

    return false; 

}
//Multiple upload support - by Simone Cinti
var fileName;
var length=0;
var multipleInputFiles = null;

This comment has been minimized.

Copy link
@simone83

simone83 Jun 14, 2013

we have to add

var itemsCount = this.__getTotalItemCount();

multipleInputFiles = this.input.prop("files");
length = multipleInputFiles.length;
if (this.maxFilesQuantity) {
if (length > this.maxFilesQuantity) {

This comment has been minimized.

Copy link
@simone83

simone83 Jun 14, 2013

the if block from 126 to 128 has to be removed

this.inputContainer.append(this.input);
this.input.change(this.addProxy);
this.__updateButtons();
richfaces.Event.fire(this.element, "onfileselect", fileName);
}

This comment has been minimized.

Copy link
@simone83

simone83 Jun 14, 2013

from line 167, add this following code:
itemsCount++;
if (this.maxFilesQuantity && (itemsCount >= this.maxFilesQuantity)) {
this.addButton.hide();
break;
}

So when total items count is greather than maxFilesUploaded it will break the loop and will disable the Add button. So in rf-fu-lst only maxFilesUploaded will be added.

@lfryc lfryc referenced this pull request Jul 2, 2013
* Get a list of accepted types from {@link #getAcceptedTypes()} attribute.
*/
public List<String> getAcceptedTypesList() {
String acceptedTypes = this.getAcceptedTypes();

This comment has been minimized.

Copy link
@bleathem

bleathem Jul 4, 2013

Contributor

This value should be cached in the component using the StateHelper, something like:
String acceptedTypes = (String) getStateHelper().eval("acceptedTypes");

This comment has been minimized.

Copy link
@lfryc

lfryc Jul 26, 2013

Author Contributor

Actually, the we delegate to abstract method, which is implemented by UIFileUpload as:

public String getAcceptedTypes() {
        String value = (String) getStateHelper().eval(Properties.acceptedTypes);
        return value;
    }
@Override
public void queueEvent(FacesEvent event) {
if (event instanceof FileUploadEvent) {
queuedFileUploadEvents += 1;

This comment has been minimized.

Copy link
@bleathem

bleathem Jul 4, 2013

Contributor

Do we have any threading concerns here?

This comment has been minimized.

Copy link
@lfryc

lfryc Jul 4, 2013

Author Contributor

oh, right, I did wrong assumption that JSF isn't multi-threaded.. ;-) going to fix that

if (name != null) {
int i = name.lastIndexOf('.');
if (i > 0) {
return name.substring(i + 1);

This comment has been minimized.

Copy link
@bleathem

bleathem Jul 4, 2013

Contributor

Will we get an exception here for a file named: "emptyextension." ?

This comment has been minimized.

Copy link
@simone83

simone83 Jul 5, 2013

yes, I've been tested it and it will throw an exception if you try to upload a file with empty extension. Right.

This comment has been minimized.

Copy link
@bleathem

bleathem Jul 5, 2013

Contributor

An empty file extension is legal on unix-like OS'es, and may be valid for certain use cases.

This comment has been minimized.

Copy link
@simone83

simone83 Jul 6, 2013

surely, is legal in winblows os'es too, but the question is that: the user specifies an acceptedTypes list-comma separated string inside the page in the rich:fileUpload component, and this must be a list of extensions without the '.' dot. In my use-case it was correctly rejected because i have specified a list of acceptedTypes="GIF, PNG", excluding the empty extension.

Note: Inside the component reference spec. the sintax (with or without dots?) is not explicitly reported...


acceptedTypes

The acceptedTypes parameter defines comma separated list of file extensions accepted by component. The component does not provide any feedback when rejecting file. For introducing feedback for rejection, use ontyperejected parameter


...so if we implicit take the assumption (as the example source code in showcase does) that we must pass a string of comma separated extensions without dot '.' prefixes, the only way the user can specify an empty file extension is when he inserts a comma followed by zero or more spaces (or vice-versa if it is the first entry in the acceptedTypes string).

If you try to do that (but now defining the acceptedTypes attribute, including the empty string between commas) the actual implementation will upload correctly the "withoutextension." file and rejects the incorrect file types at server-side, but at client-side there's something wrong because it will display in list all the files however (rejected or not)...


I investigate about this problem and we must change a line inside fileupload.js@__accept() method:
fileupload.js@line:309
CURRENT: var extension = this.acceptedTypes[i];
NEW FIX : var extension = "." + this.acceptedTypes[i];

with this NEW FIX the next line of code will behave as we expect:
result = fileName.indexOf(extension, fileName.length - extension.length) !== -1;
otherwise the indexOf will fail because extension is an empty string in our case
(I supposed acceptedTypes =", PNG, GIF" to include the empty extesions files)


I think, however, the use of empty string inside the acceptedTypes for the empty extension it is not the best choice in terms of clairness, so, for instance, we could modify the acceptedTypes requiring the "." prefix, to emprove that, then the user will specify an acceptedTypes=".,.JPG,.PNG" for example...but we're wasting time IMHO.

So, finally, i think we can:

  1. leave all as-is in the actual implemention, and
  2. correct the bug i reported before related to missed rejects displayed in list at client-side, and
  3. eventually, we could better explain the usage/syntax of acceptedTypes field inside the VDL doc of 4.3.3 release, either in case of empty file extension and, more generally, reporting explicitly the correct sintax of this field without the '.' prefix. WDYT?

This comment has been minimized.

Copy link
@lfryc

lfryc Jul 8, 2013

Author Contributor

I think that it is sufficient to provide option to pass "" empty extension,
e.g.: "jpg,png,"

It will mean "no extension" as well as "empty extension" (isn't it
incredibly rare case?).

On Sat, Jul 6, 2013 at 4:29 PM, simone83 notifications@github.com wrote:

In input/ui/src/main/java/org/richfaces/request/BaseUploadedFile.java:

@@ -73,4 +73,16 @@ public String getContentType() {
Closeables.closeQuietly(is);
}
}
+

  • @override
  • public String getFileExtension() {
  •    String name = this.getName();
    
  •    if (name != null) {
    
  •        int i = name.lastIndexOf('.');
    
  •        if (i > 0) {
    
  •            return name.substring(i + 1);
    

surely, is legal in winblows os'es too, but the question is that: the user
specifies an acceptedTypes list-comma separated string inside the page in
the rich:fileUpload component, and this must be a list of extensions
without the '.' dot.
Inside the component reference spec. the use of dots however is not

explicitly required...

acceptedTypes

The acceptedTypes parameter defines comma separated list of file
extensions accepted by component. The component does not provide any
feedback when rejecting file. For introducing feedback for rejection, use

ontyperejected parameter

...so if we implicit take the assumption (as the example source code in
showcase does) that we must pass a string of comma separated extensions
without dot '.' prefixes, the only way the user can specify an empty file
extension is when he inserts a comma followed by 0 or more spaces or
vice-versa if it is the first entry in the acceptedTypes string.
If you try to do that, the actual implementation will upload each file
without care of extension (i have to look inside code deeply to understand
if it's a bug or it is a sort of failed validation of acceptedTypes list
behaviour, then it will disable the effects of acceptedTypes attribute at
all...try it yourself!).
So we have to modify the acceptedTypes to require the "." prefix, then the
user can specify an acceptedTypes=".,.JPG,.PNG" for example...otherwise, as
it is now, it make sense to reject the "noextension." files because they
have and empty extension that it will represented by an empty string and
the users actually cannot specify that inside the acceptedTypes because as
i sayed before inserting a comma followed by an empty string will change
the expected behaviour, disabling the per-extension upload filter ...WDYT?


Reply to this email directly or view it on GitHubhttps://github.com//pull/78/files#r5052766
.

This comment has been minimized.

Copy link
@simone83

simone83 Jul 8, 2013

yes, for server-side, but the problem still remain at client-side (so i proposed the var extension = "." + this.acceptedTypes[i]; at line 309 of fileupload.js)...try it yourself to upload (in multiple upload mode) two files: one file with no extesions and the other with an extension that will be rejected, specyfing the acceptedTypes=",png,jpg" for example and see what happens...

@bleathem

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2013

This looks really good, thanks for taking the time to create the patch @simone83! I made a couple comments inline.

Regarding the need for a multiple=true/false attribute for backwards compatibility, it's hard to say. I'd like to see what the usage of the fileupload component looks like with this new feature in place. I'll build the component in this branch, and try it in the showcase to try it out.

@bleathem

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2013

Ok, I tired it in the showcase - it works great. I can't think of a use case for requiring users to select only a single file at a time, particularly with the getMaxFilesQuantity attribute that can be set to 1. @lfryc can you think of a use case requiring this?

@bleathem

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2013

Possible bug: I tried uploading 2 files, the first one threw an exception that the filesize is too big, this blocked the upload of the 2nd file which was acceptable. Clicking the upload button a 2nd time allowed the upload time of the 2nd file to proceed.

IMO, if a file is invalid to be uploaded, it should not block the remaining files waiting to be uploaded.

@simone83

This comment has been minimized.

Copy link

commented Jul 5, 2013

Brian, thank you. I'm glad to join in richfaces community with great guys like you and lukas and give my contrib when i can. Concerning the getMaxFilesQuantity set to be 1 in case of single file at a time, note also we must mantain backward compatibility with some browsers like IE7 that does not recognize the multiple attribute in html component. In my first patch, and its subsequently changes it works in both versions of browsers. If u take a look inside fileupload.js in fact you can see the support for oldiest browsers:
so:
1) var multipleInputFiles = this.input.prop("files"); --> for multiple support
2) var fileName = this.input.val(); --> for old browsers single upload support

then the things should works on oldiest IE browsers and in newest Firefox too: the code in fileupload.js recognize if multiple upload is supported then will go in multiple mode, else it works in single upload mode (it does an if (this.input.prop("files")) so if it is true we are running on a browser that supports multiple upload, else nothing to do :( we have to go in single upload mode).
So we can not control the multiple feature only by maxFilesQuantity parameter because that structural differences, and we must auto-detect first what mode (single or multiple) the client browser -on we are running on - supports.
I think you know that as best as I know but ...guess it is best to explain all that tricks however...agree??!?! :)

@simone83

This comment has been minimized.

Copy link

commented Jul 5, 2013

Notice also we can do that (valid only on browsers that supports the multiple attribute):

var multipleInputFiles = this.input.prop("files"); 

for (var i = 0; i < multipleInputFiles.length; i++) {
  var fileName = multipleInputFiles[i].name; //this is the file name
  var fileSize = multipleInputFiles[i].size; //this is the file size
}

so we can act inside fileupload.js for implement client-side verification of maximum-file size exceed treshold,
so for example we can add a new exception: FileSizeExceededException.

function FileSizeExceededException(fileName) {
    this.name = "FileSizeExceededException";
    this.message = "The size of file " + fileName + " is greater than max size allowed";
    this.fileName = fileName;
}
  • in __addItems we can add this check inside for loop:
    if (multipleInputFiles[i].size > MAX_SIZE) throw new FileSizeExceededException(multipleInputFiles[i].name);
    • and adding the binding to a new rf event "onsizeexceeded", either in fileupload.js and in component VDL

Notice also for mantaining the compatibility with single mode upload, where we can't rely on size attribute, we also need to delegate at user-implementation inside the bean listener to check the max treshold of a file size.
WDYT?

@bleathem

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2013

Setting maxFilesQuantity=1 means the user can upload only a single file (total), not one file at a time. Form the VDL doc, maxFilesQuantity:

Defines maximum number of files allowed to be uploaded. After a number of files in the list equals to the value of this attribute, "Add" button disappears and nothing could be uploaded even if you clear the whole list. In order to upload files again you should rerender the component

But like I was saying, I don't think we need to provide an option to limit file selection to one at a time. If the browser supports it, let's run with it.


As for the Exception thrown while uploading a group of files, we should either:

  1. look at modifying our exception handling to allow for updating the remaining files
  2. modify the display to clearly indicate that the upload of the remaining files has failed

My preference is for 1)

@simone83

This comment has been minimized.

Copy link

commented Jul 5, 2013

Ok,i agree...that behavior of maxfilesquantity was well knew by me but probably I misread your related post reading "can" instead of "can't" sorry:(

@simone83

This comment has been minimized.

Copy link

commented Jul 6, 2013

Brian, I've done a new post (new re-edited for last time) concerning the "withoutextension." files,please read it because it is important...in it i describe a NEW FIX that resolves an issue in case of acceptedTypes=",EXT1,EXT2,...,EXTN"
including the empty string supporting the empty file-extension. Otherwise the actual implemention will fail if you try to upload a file without extension and any other file in same multiple-upload request, due the indexOf in fileupload.js@310. We can also avoid this fix but we have to change the syntax/logic of acceptedTypes field including the "." on the extension name...

...thank you!
Simone

@bleathem

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2013

We don't have to go so far as to get the acceptedTypes attribute to work with empty extensions, but rather we should have the component fail when a file with an empty extension is uploaded (for example if I leave acceptedTypes unspecified).

@lfryc

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2013

I'm going to work on merging this into RF 4.5.0 branch.

@lfryc

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2013

Merged: 72417a9

@lfryc lfryc closed this Jul 26, 2013

@lfryc lfryc deleted the RF-12224-multi-file-upload branch Feb 12, 2014

lfryc added a commit to richfaces/richfaces that referenced this pull request Mar 19, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants
You can’t perform that action at this time.