Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: Filenames on S3 should be the cid of the content ... #62

Closed
1 task
nelsonic opened this issue Jun 6, 2023 · 6 comments · Fixed by #87
Closed
1 task

Feat: Filenames on S3 should be the cid of the content ... #62

nelsonic opened this issue Jun 6, 2023 · 6 comments · Fixed by #87
Assignees
Labels
chore a tedious but necessary task often paying technical debt enhancement New feature or enhancement of existing functionality priority-2 Second highest priority, should be worked on as soon as the Priority-1 issues are finished T25m Time Estimate 25 Minutes technical A technical issue that requires understanding of the code, infrastructure or dependencies

Comments

@nelsonic
Copy link
Member

nelsonic commented Jun 6, 2023

At present Unique File Names are using DateTime.utc_now():

image

This is does not take advantage of cid.
The idea of cid is that when a file is uploaded the name of the file is based on the actual content of the file ...
Therefore we need to read the binary of the file and feed that into cid/1 such that when the same file is uploaded again, it will have the same filename on S3.
The reason we want to do this is simple: we avoid duplicates.

Todo

  • update the function key variable to use a cid of the actual contents of the file.
@nelsonic nelsonic added enhancement New feature or enhancement of existing functionality T25m Time Estimate 25 Minutes chore a tedious but necessary task often paying technical debt technical A technical issue that requires understanding of the code, infrastructure or dependencies labels Jun 6, 2023
@LuchoTurtle
Copy link
Member

With the changes introduced in

imgup/lib/app/upload.ex

Lines 22 to 27 in 2f51c88

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)}"
, I believe this issue is no longer relevant and should be closed.

@nelsonic
Copy link
Member Author

The upload.ex is only used for the API.

@LuchoTurtle
Copy link
Member

Because the current code used in the LiveView make the upload from Javascript (followed by the File Upload guide in https://hexdocs.pm/phoenix_live_view/uploads.html), because it's been requested that the LiveView uses the upload/1 function that was implemented, I'm going to create a new page to use LiveView but use this function. I'm not going to refactor the code of the pre-existing LiveView because it would essentially be overhauled.

In order to not lose the previous work made, I'm going to create a new page that is "clientless", which uses the upload/1 function that was implemented.

@nelsonic
Copy link
Member Author

nelsonic commented Jul 4, 2023

Indeed the guide written by Chris https://github.com/phoenixframework/phoenix_live_view/blob/v0.19.3/guides/server/uploads.md#L1 uses the JS "direct" uploads method with signing. That is preferable in certain circumstances. If we can read the contents of the file on the device/browser we could use the JS implementation of CID to create the filename and thus continue using the direct upload approach. But I find that it requires considerably more code and isn't well tested. 💭

If you end up making a "clientless" version that uses the upload/1 function, please document + test as much as possible. 🙏

@nelsonic nelsonic added the priority-2 Second highest priority, should be worked on as soon as the Priority-1 issues are finished label Jul 4, 2023
@LuchoTurtle
Copy link
Member

The closed PR #64 directly addresses this. It generates the CID from the contents of the file within the JS code.
Because using JS to push the file was not initially intended, I closed off the PR. If relevant, I might open it back up again to finish it.

@LuchoTurtle
Copy link
Member

And yes, I'm doing the "clientless" version now on a different live view page. I want to get this done before doing other issues (like the image classifier) so I can have a more solid foundation to work on it.

I'm branching off from #86 (review) so I don't have merge conflicts later on 👌

LuchoTurtle added a commit that referenced this issue Jul 6, 2023
LuchoTurtle added a commit that referenced this issue Jul 6, 2023
LuchoTurtle added a commit that referenced this issue Jul 6, 2023
LuchoTurtle added a commit that referenced this issue Jul 10, 2023
LuchoTurtle added a commit that referenced this issue Jul 10, 2023
LuchoTurtle added a commit that referenced this issue Jul 10, 2023
LuchoTurtle added a commit that referenced this issue Jul 17, 2023
# Conflicts:
#	test/app_web/api_test.exs
LuchoTurtle added a commit that referenced this issue Jul 17, 2023
LuchoTurtle added a commit that referenced this issue Jul 17, 2023
idk how this was changed in the first place.
LuchoTurtle added a commit that referenced this issue Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore a tedious but necessary task often paying technical debt enhancement New feature or enhancement of existing functionality priority-2 Second highest priority, should be worked on as soon as the Priority-1 issues are finished T25m Time Estimate 25 Minutes technical A technical issue that requires understanding of the code, infrastructure or dependencies
Projects
Status: Done
2 participants