Use server suggested filename when downloading

[781flyingdutchman]

Issue

At the moment (V7.10) the downloader requires the filename field in a DownloadTask to be set to a valid filename for tasks that are enqueued (or downloaded). Upon request (issue #37) we've added a method withSuggestedFilename to the DownloadTask class that does a HEAD request to the server to get the Content-Disposition and uses that to create a copy of the DownloadTask with the filename set to the server suggested filename.
This approach works ok, but is not elegant (adds a server request when the Content-Disposition would also be available on download), is complex (see issue #145) and does not handle errors well (issue #167).

Ideally, we offer an option to tell the downloader that the filename needs to be taken from the server upon download. The reason I've hesitated with that is that thus far all fields in a task are essentially final, so the Task object you get back from the downloader in status and progress updates is essentially the same object as the one you fed in (i.e. all fields are the same). That makes a lot of things easier - eg you can use either the one passed to you or the one you stored when calling download. The only field that can be different is retriesRemaining and that is an 'internal' field, only supposed to be used by the downloader itself to manage retries. If we create a DownloadTask that does not have a valid filename and get one passed to us with a valid filename then this can lead to confusion.

Proposal

That said, I think it's worth accepting this issue for this special case. What I am proposing is:

• If the filename field is set to '?' this indicates that you want the downloader to create that filename based on the server response
• The task passed to you in a TaskStatusUpdate or a TaskProgressUpdate will have its filename field set to a proper filename as soon as possible.

The condition as soon as possible is important, because, for example, when you get the status update TaskStatus.enqueued the filename will still not be known. Similarly, I am concerned that on iOS we may not be able to determine the filename with confidence until the download has completed.
You should therefore use the filename field only when it is no longer '?', for which we'll introduce a getter on DownloadTask: bool get hasFilename => filename != '?';

Alternatives

• The alternative to directly changing the task passed to you in a TaskStatusUpdate or a TaskProgressUpdate is to add a field filename to TaskStatusUpdate or TaskProgressUpdate, but I think that is more confusing (because now there are two places where the filename is stored) and doesn't add much.
• Instead of using '?' to indicate we want the downloader to create a filename for us, we could use another string, e.g. '*' or 'suggest'. I think '?' is simple, clear (because it is not case sentsitive and not a valid filename) and expressive in that it indicates that we don't know.
• Instead of the getter name hasFilename we could use hasSuggestedFilename (though false could be interpreted as 'there is a valid filename, just not a suggested one') or hasValidFilename (though that suggests we test the actual validity of the filename, and I'd rather not sign up for that)

[rekire]

I would suggest to make the file name nullable. I know that is a breaking change which you want to avoid, but you get a very clear compile error that you might need to add question marks. There is basically no migration path required for that, mentioning this is of cause always helpful.

My assumption for this suggestion is, that when you don't set the filename then you don't care about it in first place. You just want to have that file.

Regarding mutable vs. immutable you have already a state so this is already mutable, when I understood you correctly. You could add ChangeNotifier to DownloadTask or its parent Task and notify this way the user.

Your suggestion of using "?" is clear to me, but not intuitive and will lead to a special handling on the client side, which is might be not really required. This is explicit vs. implicit, again my assumption is that a user of this library just want that file, so it implicit doesn't matter. A fallback to the filename from the url should be applied out of my view so that when you run final task = await DownloadTask(url: 'https://example.com/robots.txt') always .withSuggestedFilename(unique: true) is used as default.

[github-worst-company]

Instead of all the complications, I suggest leaving it as is, because of all the extra getters being added and then it changing midway etc, can get complicated for the user.

IMO documentation should be added for filename which states that the filename property will be null at first (unless provided by the user), and upon completion of download, the filename will be set to a random string or the suggested filename if using withSuggestedFilename(). This is much more explicit and doesn't require adding anything extra to the class.

Although it has this issue:

  final task = DownloadTask(url: '....');
  await FileDownloader().download(task);
  task.filename; // filename will be the same upon download finish unless mutated by ".download()"

And I also suggest using .withSuggestedFilename(unique: true) as default as this is the safest option because the user might not know that the file has some random name with no extension and he could be searching for the file using path provider, but the server request for getting the name should be after download completion, as this will pretty much secure that the request won't throw an error because the it can't connect to the server, the error handling will be done by the task being enqueued or downloaded, if either of those fail then the server request won't happen but if they finish downloading, then the request can be sent.

Then there can be a function of withRandomFilename() while withSuggestedFilename() would be deprecated.

Either way, it does look like there will be some breaking changes.

[781flyingdutchman]

Thank both for your reply.

@rekire the filename currently is nullable and per your comment "when you don't set the filename then you don't care about it in first place, you just want to have that file." a null filename creates a random one instantly, so the Task is 'complete' with a filename that will be steady. Changing that behavior is a major breaking change, and not something I think is worth doing. It's also worth noting that having the server suggest the filename is not what the majority of users of this package want. Most users just want to download a file from a server and do something with it, and are happy setting a filename themselves, or couldn't care less about the filename. Developers that create 'catalog' like apps (e.g. with music or video downloads obtained from a site) appear especially interested in the server suggested filename functionality - they are the ones who are going to have to add some complexity to get what they want.

The introduction of '?' as a special filename case gives you the option to get the filename at the time of download instead of trying to obtain it beforehand using withSuggestedFilename. Per #167 this has the advantage that an error connecting to the server will lead to a retry that also retries the filename,whereas obtaining it beforehand may 'miss' the suggested filename if an error occurs at that time (for example, witSuggestedFilename cannot wait for an internet connection, whereas as DownloadTask does). That said, it's an option: nothing changes for users who don't use '?'. I think this is what @deadlyrazer describes as well - and that is solved when using '?' instead of with SuggestedFilename.

Both of you have commented that the approach introduces extra complexity - which is true - and that it would be preferable to set the filename in the original task once obtained from the server. The latter is also true, but impossible: the task that you enqueue may no longer exist by the time the download starts/completes in the background, so the only way to feed the filename back is via the status and progress updates.

I've implemented this (along with a few other changes) now for Android and Desktop on the dev branch of the repo (iOS is still in progress). The use of '?' allows this to be implemented without any breaking change. and the complexity introduced as a result is limited only to those users using this functionality (and is unavoidable) so I still feel this strikes the right balance.

I'd love for you to try it out. Here's how I have descibed it in the readme.

[wreppun]

I was looking for a way to access the response headers (with 'Content-Type'), and stumbled upon this issue.

Not to throw another wrench into things, but have you considered something like

final String Function(String serverPath, Map<String, String?> responseHeaders)? filenameBuilder;

with an exposed Future<String> builtFilename (or something).

Then you could supply a default builder (whatever is implemented now), as well as letting the user manage that themself.

[781flyingdutchman]

Thanks, appreciate the input! If I understand you correctly this would be a modification of the withSuggestedFilename method by adding an optional filenameBuilder that would create the actual filename from the serverPath and responseHeaders, correct?

This could work, though I am curious 1) in what scenarios this would be useful, and 2) how more helpful this is than just doing your own request using the Task you already have (if you look at the implementation of the withSuggestedFilename method you'll see it's a simple HEAD request, and a more complicating parsing of the headers etc.).

If your suggestion is offering an option for such a builder as part of the background task, then I'd say that's too complicated, as in that mode you don't have easy access to the Dart side, and you cannot supply a 'native' builder.

Let me know your thoughts.

[github-worst-company]

I've tried it in the dev branch and it's pretty nice, the only annoyance is that the original task can't be modified, so the new task from the TaskStatusUpdate return from .download will have to be used, but that will be needed if we want no breaking changes.

Although I don't see much point in having filename in progress/status updates but it should be useful for the few users that may use it.

Only big issue is that using "?" is quite implicit and just reading the code without having read any documentation would make it quite confusing.

I would suggest having filename: DownloadTask.suggestedFileName,, suggestedFileName would be a static getter on DownloadTask.

Otherwise, I like it so far.

[781flyingdutchman]

Yes it is a bit annoying, but is unavoidable done this also needs to work for the enqueue approach where you're not waiting for the result, and the result may happen far in the future after an app restart. You therefore simply cannot rely on the original task being there.
Good idea on static getter, I'll add that.

[781flyingdutchman]

Implemented in V7.11.0

[781flyingdutchman]

@wreppun I've added the option to pass you own builder to withSuggestedFilename