temp folder refactor #5000
temp folder refactor #5000
Conversation
mnaamani
force-pushed
the
colossus-specify-temp-folder
branch
from
5847164
to
06b200b
Compare
storage-node/src/commands/server.ts
Outdated
| logger.warn( | ||
| 'It is recommended to specify a unique file path for temporary files.' + | ||
| 'For now a temp folder under the uploads folder will be used.' + | ||
| 'In future this will be warning will become and error!' |
There was a problem hiding this comment.
In future this will be warning will become and error!
Can you please rephrase that?
There was a problem hiding this comment.
Will rephrase the whole warning to:
You did not specify a path to the temporary directory. A temp folder under the uploads folder will
be used. In a future release passing an absolute path to a temporary directory with the tempFolder argument
will become mandatory.
WDYT?
| if (flags.logFilePath && path.relative(flags.logFilePath, flags.uploads) === '') { | ||
| this.error('Paths for logs and uploads must be unique.') | ||
| } |
There was a problem hiding this comment.
| if (flags.logFilePath && path.relative(flags.logFilePath, flags.uploads) === '') { | |
| this.error('Paths for logs and uploads must be unique.') | |
| } | |
| if (flags.logFilePath && path.relative(flags.logFilePath, flags.uploads) === '..') { | |
| this.error('Paths for logs and uploads must be unique.') | |
| } |
Shouldn't it be path.relative(flags.logFilePath, flags.uploads) === '..', I tested the path.relative function as path.relative('/uploads/temp', '/uploads') and I got .. as return value
There was a problem hiding this comment.
Here is was only trying to protect against the two paths being the same location. I was not concerned with temp or logs being subfolders of the main uploads directory. That is why I was comparing with '' null string.
Originally I was doing if (lags.logFilePath === flags.uploads) { } but I decided to use path.relative() to take into account trailing / path separator..
Given that I added proper filtering of filenames when they are loaded from uploads directory, and if we assume the file names of logs and temp files will not clash with the object id name, perhaps this constraint is not really necessary at all?
| if (path.relative(tempFolder, flags.uploads) === '') { | ||
| this.error('Paths for temporary and uploads folders must be unique.') |
There was a problem hiding this comment.
| if (path.relative(tempFolder, flags.uploads) === '') { | |
| this.error('Paths for temporary and uploads folders must be unique.') | |
| if (path.relative(tempFolder, flags.uploads) === '..') { | |
| this.error('Paths for temporary and uploads folders must be unique.') |
Please see the other comment
There was a problem hiding this comment.
commented here #5000 (comment)
Added in 3dd8e0a |
Yes will do that next. |
Fixes #4977