socketry / multipart-post

Adds multipart POST capability to net/http
MIT License
293 stars 72 forks source link

Feature request: Don't set "local.path" as a default filename to prevent it from being detected as a binary #69

Open ts-3156 opened 3 years ago

ts-3156 commented 3 years ago

Thank you for useful gem!

On some systems, a preview of an image is not displayed correctly when I specify "local.path" as a filename. To solve this behavior, could you simply set "file" as a default filename?

If you agree with this issue, I will create a pull request. I would like to know your opinion.


One of those systems is Slack for Mac. Starting at 2021/05/05 05:30 UTC, Slack client no longer correctly displays previews of images when the filename is "local.path".

Expectation Reality
Screenshot 2021-05-07 21 52 06 Screenshot 2021-05-07 21 47 43

The relevant code of this issue is here. https://github.com/socketry/multipart-post/blob/master/lib/multipart/post/composite_read_io.rb#L80-L88

ioquatix commented 3 years ago

Yes, something like this seems reasoanble.

I think if the path is not given, we can use nil. We could make it an optional keyword argument.

I am open to ideas, and agree the current implementation looks a bit odd.

ts-3156 commented 3 years ago

@ioquatix Thank you for your response!

I've created the most conservative diffs. Is this what you want? https://github.com/socketry/multipart-post/compare/master...ts-3156:set_file_as_default_filename

I think if the path is not given, we can use nil. We could make it an optional keyword argument.

Do you refer to local_path? As far as I tried, it's fine even if it is nil. However, I didn't include the change in that diffs because I'm not sure that not all gems that depend on the multipart-post gem will have the same result.


The code I am suggesting is very small. Besides, There's no need to rush. If you have a better idea, please feel free to close this issue without hesitation. It may be the easiest way for you.

ioquatix commented 3 years ago

I think we should try to fix the interface.

The problem I have with filename = "file" unless filename is because we could legitimately have a file called "file". In this case, we can't distinguish the case where no file name was given or the file was actually called "file". If we do need some filename, then why not make a method, e.g.

DEFAULT_FILE_NAME = "unspecified.dat"

def filename(default: DEFAULT_FILE_NAME)
  @filename || default
end

@Lewiscowles1986 do you have any opinion?

Lewiscowles1986 commented 3 years ago

I would use nil which I read as indeterminate for filename so to avoid reserved (cursed) tokens that are hard to reason about; then internally set if nil. You could also use empty string via String.new as that is an invalid file name.

What do you think of this?

I believe most network clients like outlook, thunderbird, all the browsers default to attachment.dat

ioquatix commented 1 year ago

I welcome a PR for this.