-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add checkDirExists endpoint + tests #38
Conversation
Tests are suggesting issue with the positive test:
|
@Xkonti Fixed the status 204 test by adding proper filter to the tested directories terms. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. There's just one last thing before I can merge it in. Thank you!
src/endpoints/dir/checkDirExists.ts
Outdated
let relativePath = query.path ? (query.path as string) : null; | ||
if (!isPathValid(relativePath)) { | ||
set.status = 400; | ||
return provideValidPathDirMsg; | ||
} | ||
relativePath = relativePath as string; | ||
|
||
// Get the full path to the directory | ||
const dirPath = join(getConfig().dataDir, relativePath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can get the relative path and absolute path at the same time using validateRelativePath
. This way we will avoid same checking logic in multiple spots:
// Performs validation of the `query.path`
const dirPathValidationResult = validateRelativePath(query.path);
if (dirPathValidationResult.isErr()) {
set.status = 400;
return provideValidPathDirMsg;
}
// Create variable to hold relative and absolute path
const dirPath = sourcePathValidationResult.value;
// Using the relative and absolute path:
let something = dirPath.relativePath;
let somethingElse = dirPath.absolutePath;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reused it. Though the absolutePath doesn't seems necessary for this endpoint, I just used the relativePath from the value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you're using the absolute path in line 19:
// Get the full path to the directory
const dirPath = join(getConfig().dataDir, relativePath);
Here's whot it looks like in validateRealativePath
:
file-api/src/utils/pathUtils.ts
Lines 27 to 30 in bd3b6fb
// Get the full path to the file | |
const relativePath = path as string; | |
const absolutePath = join(getConfig().dataDir, relativePath); | |
return ok({relativePath, absolutePath}); |
@Xkonti I didn't notice it was the same piece of logic, my bad. So I only use the already returned absolutePath this time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks awesome. Thanks!
This PR closes #13
I mostly created derived logic from checkIfFileExists.
I also created the relative tests. I was able to find how to test the status 400 properly, it fallbacks to 404.
I noticed a lot of test errors when executing
bun test
My coverage is 92%.