search

keystonejs / keystone-storage-adapter-s3

⚠️ Archived - Legacy S3 Storage Adapter for KeystoneJS
MIT License
17 stars 55 forks source link

Replacing path.resolve with path.join in _resolveFilename method #19

Closed vitalbone closed 6 years ago

vitalbone commented 7 years ago

The current logic in the _resolveFilename method uses path.resolve, which "resolves a sequence of paths or path segments into an absolute path."

This means our filename can resolve to: /usr/{User}/we/are/not/where/we/want/to/be/{bucket}/our-files-live-here/{filename}

This is not ideal as the logic default will, in most cases, resolve to the user system root directory. This 'resolvedFilename' would make more sense if the filename is relative to the directory in the s3 bucket, specified in with path in the adapter config. ie, {bucket}/our-files-live-here/{filename}

Let me know if I'm off the mark here. @blargity and I had a look and this is what we though would be a good solution, as it solved our use case, but it may need more thinking. Happy to hear any thoughts / suggestions.

mikehazell commented 6 years ago

Thanks for this @vitalbone. This issue was really a failure to actually require the path option to have a leading slash. The latest release resolves this be adding the slash if it is missing rather than throwing an error.

path.resolve will work as expected (ie. not look at the local file path) if it has a leading slash.