search

IQSS / dataverse

Open source research data repository software
http://dataverse.org
Other
878 stars 490 forks source link

HTTP Errors during file upload not caught #4511

Closed qqmyers closed 6 years ago

qqmyers commented 6 years ago

It looks like the code in editFilesFragment.xhtml (v 4.8.5) does not catch HTTP errors and thus these failures leave the GUI showing a full progress bar for the file with no further updates. I suspect that this is what was being seen in issue #4439 . In our case, we had a server with nginx configured with a small upload size limit and I was receiving 413 (Request Entity Too Large) errors (rather than some timeout error you might expect for GB+ files). It's straight-forward to add a javascript onerror handler to the p:fileUpload element to provide some user feedback that there's been a failure. I'd be happy to submit one if I can get some guidance on what error action would make sense for dataverse (just a note to the right of the entry stating that "the upload of this file has failed () and it will not be saved to your dataset.", or?).

pdurbin commented 6 years ago

@qqmyers more feedback to the user about what's going on sounds great. I would say that for now you can put whatever message you want in a pull request and we can iterate on it. Thanks!

pdurbin commented 6 years ago

@qqmyers thanks for creating pull request #4522! I just dragged it to code review at https://waffle.io/IQSS/dataverse

qqmyers commented 6 years ago

Turned out it wasn't 'straight-forward' to get the status and other info across Chrome, Firefox, and Edge, but I was able to get them from the call stack. After that, I just wrote a simple message to the right of any entry that failed showing the error code (popup for the status text).

Doing this is different than how the error is reported when a file already exists in the dataset - where, as soon as there's one error, a message is posted, and the file list is deleted. A second error of this type replaces the first error message even though the second file no longer show in the list.

It should be possible to work more like the file already exists error and put a red message below, but it seems like it would be less useful to work exactly like it and only show one message at a time and remove the file list. (If I upload 10 files (or more) and a few fail, I'd want to see the list of failures somewhere. And I'd probably want the same if there were several files that already existed in the dataset...). For this reason, and not wanting to go to far in trying to figure out how to make the client-side detection of HTTP errors tie into the server-side reporting coming back through JSF, I decided to leave it as is for now. Something simple like using CSS to make the HTTP errors have the same styling as the file already exists error message might be worthwhile though...

pdurbin commented 6 years ago

@qqmyers thanks for the write up. This morning I helped @sekmiller switch to your fork and branch and he's taking a look.

matthew-a-dunlap commented 6 years ago

Thanks for sending this our way @qqmyers . I had to minorly change the code to get it running as the function being called was not the correct name.

Here is a sample screenshot demoing the functionality. To do this I hacked the handleFileUpload method to add an error to the HttpServletResponse:

screen shot 2018-03-21 at 1 06 42 pm

I noticed that if a user keeps uploading files, the javascript will begin to concat repeats of the error message. I have yet to dig into it in more depth. I thought it may be due to the errors being the same code, but as I uploaded more I ended up with another "matching code" and the concat did not happen.

qqmyers commented 6 years ago

@matthew-a-dunlap - can you tell me what function you changed, or put it in git? I had it working so I either made a cut/paste error or there's another browser difference (what browser did you test with?).

And, if I understand, you're creating fake errors for testing via the handleFileUpload method?

W.r.t. the repeats, The messages should only be happening once per upload attempt and they are tied to the name of the file being uploaded. I noticed in testing here that if I let current uploads complete (and fail), and then add a new file, all of the prior ones get a new upload attempt. For that case, I added code to remove messages upon an upload start. It's possible that that code is broken somehow or that there's some way for new files to be uploaded without the upload start method getting called again. In any case, I can try to take a look. If the info here gives you any more insight, let me know. - Thanks!

qqmyers commented 6 years ago
matthew-a-dunlap commented 6 years ago

@qqmyers Totally. Our team is going to discuss design around displaying these errors and get back about a good path to move forward on this. Thanks again!

matthew-a-dunlap commented 6 years ago

p.s. Its possible my fake errors in handleFileUpload are being added multiple times causing repeat errors.

matthew-a-dunlap commented 6 years ago

@qqmyers Our dev team discussed this and was hoping to find a solution that works with our normal JSF / PrimeFaces flow. I investigated solutions to this issue and was surprised at the number of steps it seemed to take to inside JSF / PrimeFaces. I tried asking last week on the PrimeFaces forum for guidance on this issue: https://forum.primefaces.org/viewtopic.php?f=3&t=55039&sid=c76fb7669bd36e2acc611e59b8a5c174 , some details of my investigation are in there. They have yet to reply but I am going to give it another day or two before finding another path.

Also FWIW, the issue with the double error labels seems to happen because the javascript matches on name, yet the user can upload multiple files of the same name. The concatenation of the error text then happens multiple times.

pdurbin commented 6 years ago

I don't know if this helps or not but I recently learned that PrimeFaces is on Gitter: https://gitter.im/primefaces/primefaces

qqmyers commented 6 years ago

@matthew-a-dunlap - thanks for the update. I agree it would be nice if Primefaces could handle it so hopefully they can provide some guidance. If javascript ends up being the best option, or if a javascript trigger just needs to send more than a filename to make it clear to JSF which entry had the error, I'd be happy to see what other info is available.

matthew-a-dunlap commented 6 years ago

My apologies for our slow reply on this @qqmyers . We are interested in bringing in this javascript fix after a few outstanding points are fixed/investigated:

qqmyers commented 6 years ago

Matthew, I'm out this week, but I can look at these things next week. There is a text version of the error message that's more useful. It's actually shower now if you hover over the error code. I'm guessing there must be some way to distinguish files, but we'll see. Thanks, Jim

On Apr 2, 2018 1:00 PM, matthew-a-dunlap notifications@github.com wrote:

My apologies for our slow reply on this @qqmyershttps://github.com/qqmyers . We are interested in bringing in this javascript fix after a few outstanding points are fixed/investigated:

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/IQSS/dataverse/issues/4511#issuecomment-377978478, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AGa4z-9ZWBdsZIxnhJ_nJ5FTphdj-6CUks5tklkTgaJpZM4Sst_p.

pdurbin commented 6 years ago

@qqmyers thanks for working on this and nice talking to you on the community call the other day. I haven't loaded up your code myself but I like that you're trying to improve the user experience when uploading files. Error handling is super important. I hope your users aren't seeing too many of these errors!

I'm updating the "dev efforts from the community" spreadsheet linked from https://groups.google.com/d/msg/dataverse-community/X2diSWYll0w/ikp1TGcfBgAJ and I just added this issue and your pull request to it so we don't forget to follow up. There's no rush. I'm going to pull this out of the "Development" column at https://waffle.io/IQSS/dataverse so we don't have to see it every morning at standup but please just ping us when you want us to take another look at the code.

One more thing that's on my mind is that we're pretty far behind on our version of PrimeFaces and I hope that whatever you come up with will continue to work once we upgrade. Just something to think about. Thanks!

qqmyers commented 6 years ago

@pdurbin , @matthew-a-dunlap - I've updated the request with a new commit that: uses the standard error message class for formatting, displays both the error numeric code and explanatory text (versus showing that as a hover-over title before), and, most significantly reworks how to identify the matching entry in the uploads table and to remove old error messages upon restarts.

To uniquely find the row, I'm now adding an attribute when the row is created that I can later find in the error event - loosely following a post from someone else trying to integrate javascript with PrimeFaces. I've tested with Firefox and Chrome on Win10 by repeatedly killing my network connection (we fixed the issue with our dev server that was causing file-size related 413 errors.). Hopefully rarely seen messages, but enough to give some clues when things go wrong...

djbrooke commented 6 years ago

Thanks @qqmyers - we'll take a look at this and provide feedback as soon as possible.

matthew-a-dunlap commented 6 years ago

This looks really great @qqmyers .The only thing I see needed is to merge development into your branch, and then our QA can take a look.

Below is a screenshot in case folks are interested in the messaging.

screen shot 2018-04-23 at 3 44 34 pm

pdurbin commented 6 years ago

@qqmyers hi! We just discussed this issue and pull request in sprint planning this afternoon. When you have a moment, please merge the latest from the "develop" branch into your pull request so we can advance this to QA in our kanban board. Thanks!

qqmyers commented 6 years ago

OK - merged again...

djbrooke commented 6 years ago

Thanks @qqmyers - moved over to QA.

pdurbin commented 6 years ago

@qqmyers thanks! I talk about how quickly pull requests can get stale over at http://guides.dataverse.org/en/4.8.6/developers/version-control.html . Much appreciated!