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.

Sign up for GitHub

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(5xx): should return 4xx when file not found #1291

Merged
merged 1 commit into from
Apr 16, 2024
Merged

fix(5xx): should return 4xx when file not found #1291

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions src/services/db/GitFileSystemService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,7 @@ export default class GitFileSystemService {
repoName: string,
filePath: string,
isStaging: boolean
): ResultAsync<string, GitFileSystemError> {
): ResultAsync<string, GitFileSystemError | NotFoundError> {
const efsVolPath = isStaging
? EFS_VOL_PATH_STAGING
: EFS_VOL_PATH_STAGING_LITE
Expand All @@ -333,6 +333,14 @@ export default class GitFileSystemService {
)

if (error instanceof GitError) {
// NOTE: While some path can be potentially normalised by Git (eg. images//path.png),
// it is not guaranteed to exist in HEAD. We simply return a not found error in this case.
if (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would you know why the request even contains things like //filename?

are these actually broken links that cms tries to resolve?

Copy link
Contributor Author

@kishore03109 kishore03109 Apr 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would you know why the request even contains things like //filename?

we have a markdown editor, so there are possibility for typo + the logs suggest this has occurred at least once.
we dont debounce too, to this could occur while the user is typing the correct string.

are these actually broken links that cms tries to resolve?

yes, intuitively should be able to resolve them gracefully by 4xx rather than 5xx

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add comments for this check here so that we don't lose context of when this error statements occur?

error.message.includes("fatal: path") &&
error.message.includes("exists on disk, but not in 'HEAD'")
Comment on lines +339 to +340
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a blocker for now, because we can raise a PR to improve this + bug is fixed here but i wonder if error.message is a robust enough property for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like best effort, since this is directly reliant on the error message that git returns us
do have an alternative in mind?

) {
return new NotFoundError("File/Directory does not exist")
}
return new GitFileSystemError("Unable to determine Git blob hash")
}

Expand Down Expand Up @@ -1121,7 +1129,11 @@ export default class GitFileSystemService {
)
)
.andThen((directoryContents) => {
const resultAsyncs = directoryContents.map((directoryItem) => {
const IGNORED_FILES = [".git"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think the pattern we might wanna aim for here is to respect the filesystem's convention! in here, this means that hidden files/folders (prefixed by a `.) should be ignored rather than a specific instance.

wwdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm i rather keep this as .git since this actually is not the file system convention, but rather the .git system's convention. since they dont have a direct mapping, opting for the stricter check here.

const filteredDirectoryContents = directoryContents.filter(
(directoryItem) => !IGNORED_FILES.includes(directoryItem.name)
)
const resultAsyncs = filteredDirectoryContents.map((directoryItem) => {
const isDirectory = directoryItem.isDirectory()
const { name } = directoryItem
const path = directoryPath === "" ? name : `${directoryPath}/${name}`
Expand Down
37 changes: 37 additions & 0 deletions src/services/db/__tests__/GitFileSystemService.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,11 @@ const dirTree = {
},
}

const dirTreeWithIgnoredFiles = {
...dirTree,
".git": "fake git directory",
}

describe("GitFileSystemService", () => {
beforeEach(() => {
mockFs({
Expand Down Expand Up @@ -172,6 +177,18 @@ describe("GitFileSystemService", () => {

expect(result._unsafeUnwrapErr()).toBeInstanceOf(NotFoundError)
})

it("should ignore .git folder", async () => {
mockFs({
[EFS_VOL_PATH_STAGING]: dirTreeWithIgnoredFiles,
})
const result = await GitFileSystemService.listDirectoryContents(
"fake-repo",
"fake-empty-dir",
DEFAULT_BRANCH
)
expect(result._unsafeUnwrap()).toHaveLength(0)
})
})

describe("listPaginatedDirectoryContents", () => {
Expand Down Expand Up @@ -661,6 +678,26 @@ describe("GitFileSystemService", () => {

expect(result._unsafeUnwrapErr()).toBeInstanceOf(GitFileSystemError)
})

it("should return not found error for a non-existent file", async () => {
MockSimpleGit.cwd.mockReturnValueOnce({
revparse: jest
.fn()
.mockRejectedValueOnce(
new GitError(
undefined,
`fatal: path //fake-media-file.png exists on disk, but not in 'HEAD'`
)
),
})
const result = await GitFileSystemService.getGitBlobHash(
"fake-repo",
"fake-dir/%2ffake-media-file.png",
true
)

expect(result._unsafeUnwrapErr()).toBeInstanceOf(NotFoundError)
})
})

describe("getFilePathStats", () => {
Expand Down
Loading