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

LocalFileAdapter file deletion bug #2948

Closed
kasonde opened this issue May 12, 2020 · 8 comments
Closed

LocalFileAdapter file deletion bug #2948

kasonde opened this issue May 12, 2020 · 8 comments

Comments

@kasonde
Copy link

kasonde commented May 12, 2020

Bug report

Describe the bug

In the documentation for file adapters, we're provided with a nifty hook to delete existing files before/after new ones are changed / deleted. Unfortunately the existingItem shown in the docs does not carry a file property as show. So the condition is always false and files are never deleted.

To Reproduce

Steps to reproduce the behaviour. Please provide code snippets or a repository:

  1. Go to 'https://www.npmjs.com/package/@keystonejs/file-adapters'
  2. Setup a file field and a local file adapter as shown in the docs
  3. Upload a file, and attempt to delete it.
  4. File remains undeleted.

Expected behaviour

I expect the existing file or in this case existingItem.file to be deleted.

System information

  • OS: macOS
  • Node: 10.13.0
@Vultraz
Copy link
Contributor

Vultraz commented May 12, 2020

I can't repro this 🤔 I tested, and the code in the docs works fine.

@kasonde
Copy link
Author

kasonde commented May 12, 2020

I see. That's odd. For reference, here's my config

I'm using MongoDB as my DB.

`
const fileAdapter = new LocalFileAdapter({
src: "./uploads/test_people",
path: "/uploads/test_people"
});

keystone.createList("TestPersonImage", {
fields: {
image: {
type: File,
adapter: fileAdapter,
hooks: {
beforeChange: async ({ existingItem }) => {
console.log("Field: ", existingItem);
if (existingItem && existingItem.file) {
await fileAdapter.delete(existingItem.file);
}
},
}
}
},
// hooks: {
// beforeDelete: async ({ existingItem }) => {
// console.log(existingItem);
// await fileAdapter.delete(existingItem.filename);
// },
// }
})

keystone.createList("TestPerson", {
fields: {
name: {
type: Text,
},
images: {
type: Relationship,
ref: "TestPersonImage",
many: true,
}
}
});
`

Note that I'm working directly with the TestPersonImage list and not the parent list. I'm performing CRUD from the admin panel.

@Vultraz
Copy link
Contributor

Vultraz commented May 12, 2020

You need to have both sets of hooks from the example enabled. The first is a field-level hook; it handles the case where the file - and just the file - actually changed. The other is a list-level hook; it handles the case where the entire list entry was deleted.

More info on hooks here: https://www.keystonejs.com/api/hooks/

@kasonde
Copy link
Author

kasonde commented May 14, 2020

@Vultraz the code above has it omitted. This because I was probably 10 tries in at that point pulling things out and putting them back in. But I have it noted down that even with both, it still doesn't work for me.

@MadeByMike
Copy link
Contributor

@kasonde I've also been able to get this working... do you think you could post an example project to gitHub that has the bug? That way we can see if there's something different about your local environment or ideally get an example that allows us to reproduce the issue.

Sorry about the trouble. If it turns out to be config, we can still improve the docs based on your experience!

@tshih5
Copy link

tshih5 commented Aug 12, 2020

@MadeByMike I initially had the same issue with files not getting removed when I attempted to delete them. The way I fixed it is replacing the .file property from existingItem.file to the actual name of the field when I defined it. In @kasonde 's case, the field name is "image", so I would replace existingItem.file with existingItem.image.

I'm fairly certain the docs explaining the file adapters don't say anything about using the field name as the property for existingItem, so it could be confusing for some readers.

@stale
Copy link

stale bot commented Dec 11, 2020

It looks like there hasn't been any activity here in over 6 months. Sorry about that! We've flagged this issue for special attention. It wil be manually reviewed by maintainers, not automatically closed. If you have any additional information please leave us a comment. It really helps! Thank you for you contribution. :)

@stale stale bot added the needs-review label Dec 11, 2020
@bladey
Copy link
Contributor

bladey commented Apr 8, 2021

Keystone 5 has officially moved into active maintenance mode as we push towards the next major new version Keystone Next, you can find out more information about this transition here.

In an effort to sustain the project going forward, we're cleaning up and closing old issues such as this one. If you feel this issue is still relevant for Keystone Next, please let us know.

@bladey bladey closed this as completed Apr 8, 2021
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

5 participants