Edge cases in Upload

[SimonLab]

I've spotted some possible errors with the following code:

imgup/lib/app/upload.ex

Lines 23 to 28 in 81de078

	def upload(image) do
	# Create `CID` from file contents so filenames are unique
	#
	{:ok, file_binary} = File.read(image.path)
	file_cid = Cid.cid(file_binary)
	file_name = "#{file_cid}.#{Enum.at(MIME.extensions(image.content_type), 0)}"

• {:ok, file_binary} = File.read(image.path)
  This will break if File.read returns an error, {:errror, reason} as the match will fail, see https://hexdocs.pm/elixir/1.12/File.html#read/1

• file_cid = Cid.cid(file_binary)
  The Cid.cid function returns a string, however the string can represent an error: invalid data type, see https://hexdocs.pm/excid/Cid.html#cid/1
  

Not an error in itself but I think we can make this a bit more readable:

file_extension = image.content_type
|> MIME.extensions()
|> List.first()

file_name = "#{file_cid}.#{file_extension}" 

Also MIME.extension can return an empty list, so in this case the file extension would be nil. Do you still want to upload the file if the extension is unknown? If yes maybe the file name should be just the file_cid
see https://hexdocs.pm/mime/MIME.html#extensions/1-examples

[nelsonic]

Good call. If you have time, please PR the update. 🙏
We are currently busy trying to debug #71 🙃

[LuchoTurtle]

Going to work on this.
Because the current code is based on try...rescue, I think we can get away with pattern matching to provide more meaningful feedback to the person using the API, specifically stating where the error occurred (whether it was creating CID, extension, etc...).

Implementing should be easy, testing might be slightly harder and probably will have to use mock to get it cover all possible scenarios.

[nelsonic]

All in favour of fixing edge cases. 👍
Please write the test first. 🧑‍💻
Or at least drop a comment in this thread sharing what you're thinking. 💭

Note: Mocking is fine in a library e.g: elixir-auth-google
but should be avoided in an App such as this one.

[nelsonic]

Getting the following error when attempting to upload on: https://imgup.fly.dev/liveview

Couldn't upload files to S3. Open an issue on Github and contact the repo owner.

New issue: #88

[nelsonic]

Really not a fan of having mocks for edge cases that we haven't seen in the real world.
Would much rather have deliberately crafted files that simulate the edge cases
such as empty or corrupted files.
Mocking is a false positive.
Unless we have actual tests this is always going to be incomplete.

[nelsonic]

I now realise that it was a mistake for me to apply a priority-1 label to this issue. 😞
There were several issues contained within this "Edge Cases" issue
and they should have have been split out into multiple issues.

The reason It's taken me so long to review the PR is that it covers many things.
We should have addressed each one individually.
If the CID fails because the file is not readable, that's one thing.
If the file cannot be read that's another
If we cannot upload to S3 that's a whole other error.
We definitely don't want to Mock anything unless we absolutely have no other choice.

I've now spent more time on this issue and PR than I have on my own work for the last 2 weeks.
That has completely sucked my energy and I'm not happy about it.
I've learned my lesson. Don't work on anything that wasn't requested by a person using the product.