-
Notifications
You must be signed in to change notification settings - Fork 43
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
fix(project-access): check for files in srv folder treats directories as files and throws exception #2923
base: main
Are you sure you want to change the base?
fix(project-access): check for files in srv folder treats directories as files and throws exception #2923
Conversation
🦋 Changeset detectedLatest commit: d60e9df The changes in this PR will be included in the next version bump. This PR includes changesets to release 40 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
if (await fileExists(srvFolderPath)) { | ||
const fileSystemFiles = await readDirectory(srvFolderPath); | ||
for (const file of fileSystemFiles) { | ||
const filePath = join(srvFolderPath, file); | ||
if (await fileExists(filePath)) { | ||
const fileContent = await readFile(filePath); | ||
memFs.write(filePath, fileContent); | ||
return true; |
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.
Only downside with this approach is that we would accept files that exist in the file system but have been deleted in memFS as existing files.
But as this has not been checked in the erroneous version of the code either it should not harm 🤷🏻
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.
@heimwege, could we create a map from memFs.dump()
before line 90 and check if the file is deleted in memFs here?
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.
Sure. But to be honest I would prefer the findBy approach as described in the description at the very top. Is there anything that prevents us from going that direction?
…access/check-files-in-srv-folder-of-CAP-project # Conflicts: # packages/project-access/src/project/cap.ts
@Klaus-Keller I refactored to the approach I would prefer because for me this would be more reusing what we already have instead of doing the same thing over again at a different place. Can you please check? @kjose90 Any comment from your side? |
|
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've added types for finder2 recently with #2926 🙈
#2922
project-access checkFilesInSrvFolder currently throws an exception in case the CAP srv folder contains subfolders because those are currently being treated like files and
readFiles
dumps on folders.@kjose90 would the proposed fix work for you?