MudFileUpload: fix AppendMultipleFiles keeping stale file references (#9586)

[igotinfected]

Description

Resolves #9586
Resolves #9795

Fixed an issue where uploading files multiple times with AppendMultipleFiles and attempting to read the file contents would throw an exception indicating that those files no longer exist.

Problem explained in #9586 (comment).

Whats it boils down to is that when using AppendMultipleFiles in MudFileUpload we append "new" files to our list of "old" files. The references to old files however no longer exist in the HTML input, causing an error when trying to read those files.

Currently users have to disable AppendMultipleFiles and make use of the file references with every upload.

The fix proposed here consists creating new InputFile instances with every upload, so that the previous file references are retained. This implementation implies the following:

• there are multiple file input controls in HTML, each with their unique id
• only one input is "active" at a time
• all but the newest inputs are destroyed after ClearAsync is called
• users can no longer provide their own input ids since we now have multiple inputs at once (⚠️ could be considered a breaking change)

This also fixes an issue where the FilesChanged callback would not be triggered when uploading the same file twice (despite the file contents having changed) (#9795).

How Has This Been Tested?

Visually with test components and with some basic unit tests.

Type of Changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation (fix or improvement to the website or code docs)

Checklist

• The PR is submitted to the correct branch (dev).
• My code follows the code style of this project.
• I've added relevant tests.

[igotinfected]

It could be possible to test against this by playing around with IBrowserFiles, will try to look into that soon.

Need to do a bunch of regression testing for this since it messes with multiple HTML inputs.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.80%. Comparing base (28bc599) to head (5d607ce).
Report is 582 commits behind head on dev.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #9600      +/-   ##
==========================================
+ Coverage   89.82%   90.80%   +0.97%     
==========================================
  Files         412      406       -6     
  Lines       11878    12830     +952     
  Branches     2364     2485     +121     
==========================================
+ Hits        10670    11650     +980     
+ Misses        681      625      -56     
- Partials      527      555      +28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[igotinfected]

Can't test against the actual bug with bUnit since it's a browser error (in JS). Will do more manual regression testing before publishing the PR.

[igotinfected]

Ready for review. I will do some more some manual testing on all browsers today to make sure I didn't miss any edge cases.

[Mr-Technician]

LGTM pending the one change

[ScarletKuro]

@henon should be this moved to v8 or this breaking change is fine?

[henon]

Let's play this safe and release it with v8. This is not urgent is it? And we have decided to start the v8 process now anyway, so it won't be long.

[igotinfected]

Nope not urgent, I prefer playing it safe too, thanks!

[ScarletKuro]

Do we merge this in v8? @henon

[henon]

Yes! Thanks @igotinfected

[henon]

@igotinfected What would you say we should write in the migration guide about this?

[igotinfected]

@igotinfected What would you say we should write in the migration guide about this?

(The fix description)
Fixed an issue where uploading files multiple times with AppendMultipleFiles and attempting to read the file contents would throw an exception indicating that those files no longer exist.

(The breaking change description)
The component now generates a new input with every file change to be able to retain references to previous files. As such, it is no longer possible to supply your own id to the underlying input.

... or something similar!

[henon]

So what should people do who rely on that id? What workaround can we propose to them?

[igotinfected]

Hmm I can't think of a scenario where you would absolutely need the id of the underlying input other than for automated UI testing purposes. You could use a different selector instead, for example by adding your own custom attribute like data-test-id that would be applied to all inputs though with the current implementation, so you'd have to do something like:

@* razor code *@
<MudFileUpload data-test-id="my-test-id" ...

// UI test code
var inputs = document.querySelectorAll('[data-test-id="my-test-id"]');
var activeInput = inputs[inputs.length - 1];
...

We can probably mess around with the @attributes if needed to only apply it to the active input if it causes issues for people down the line but I don't think it will be necessary.

[henon]

Thanks, I think this explanation is enough. People can find it when they follow the link to this issue.

[henon]

Added to the v8 migration guide at #9953

[alfred0dev]

I did update to version MudBlazor 7.15.0 through nuget package but error still persist, anything i should do?

[igotinfected]

I did update to version 7.15.0 from nuget package but error still persist, anything i should do?

This will only be available with v8, you could try it out with one of the released v8 previews in the meantime!