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

Delete does not remove files #262

Closed
nanohard opened this issue Jul 30, 2018 · 7 comments
Closed

Delete does not remove files #262

nanohard opened this issue Jul 30, 2018 · 7 comments

Comments

@nanohard
Copy link
Contributor

nanohard commented Jul 30, 2018

Deleting content or files/uploads will only remove the reference from the database? but will not remove the actual file from the filesystem in the uploads directory.

To be clear, the "quick-delete" button doesn't work at all; I'm speaking of the delete button after you select the item.

Correction:
The "quick-delete" does work, but going too fast for several in a row results in an unknown 500 error.

@olliephillips
Copy link
Contributor

olliephillips commented Mar 25, 2019

It's not a complete fix for this issue I don't think Steve. I think the errors mentioned there are 500 errors. I saw 400 error on this. It also doesn't delete the file. I mentioned that in the PR but maybe it was missed as I added the comment later. I shall dig them out and add them in the issue itself.

@olliephillips
Copy link
Contributor

olliephillips commented Mar 25, 2019

Extracted from my PR comment, and referring to physical file not being deleted from uploads folder.

What is the intended functionality here? I see no major problem, unless uploading large files, or a file containing sensitive information is uploaded by mistake, in which case, user would be under impression it was gone by deleting it, but the file would be available to http/s calls at api/uploads/filepath

@nilslice
Copy link
Contributor

@olliephillips - ah my mistake -- I had thought the file deletion code was fine, but my URL action was incorrect (which correcting the endpoint would have solved).

That is not the case, though, and the bug is still present? I missed that point, sorry!

I probably can't get to this until later in the week, but the issue is that there is no code to delete the file from disk: https://github.com/ponzu-cms/ponzu/blob/master/system/admin/handlers.go#L2202 (oops...)

I don't recall if the values we get from the admin request contain the filepath or not, so the fix may involve a DB lookup to get the file URL, which would be translated to the path on disk (they should be the same, though will need to be split & re-joined using the system filepath separator).

I'm just putting this out there in case you have any cycles free and want to give the fix a go. Otherwise I can get to this soon!

@nilslice
Copy link
Contributor

What is the intended functionality here?

Just for posterity, the intended action is certainly that the file would be deleted from disk.. I think this was just an error on my end a long time ago, and I never realized that the code just had not been written! 😆

@olliephillips
Copy link
Contributor

I came to same conclusion. On inspection there's no code to remove from disk, and that a DB lookup might be necessary since I think the admin UI only has knowledge of ID.

Now I know that we want to delete the physical file, I'm happy to have a look at this, yes.

Re the 500 error. There are a couple of places that the delete routine might return 500s. One might pertain to a memory limit (4MB). I'll attempt to reproduce what the OP reported to confirm. If I can fix that I will.

@nilslice
Copy link
Contributor

Now I know that we want to delete the physical file, I'm happy to have a look at this, yes.

Awesome! thank you!

Re the 500 error. There are a couple of places that the delete routine might return 500s. One might pertain to a memory limit (4MB). I'll attempt to reproduce what the OP reported to confirm. If I can fix that I will.

Oh, I missed this initially:

The "quick-delete" does work, but going too fast for several in a row results in an unknown 500 error.

This may be because there is a slight delay when the ponzu database re-sorts itself when content is changed. When the delete action is done, some work is put on a goroutine, which is probably a premature optimization... but I didn't want the UI to be held up by a "delete & re-sort". Not 100% sure this is the issue, but this can be reproduced when trying to click the "Delete" buttons on a normal content type many times in a row rapidly.

@olliephillips
Copy link
Contributor

PR #301 offers the delete physical file functionality. Will look at the 500 error (taking on board your last comment) within the week and update here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants