Skip to content

🌱 (catalogd) serveFile for all endpoint instead of serveContent #1723

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

Closed
wants to merge 1 commit into from
Closed

🌱 (catalogd) serveFile for all endpoint instead of serveContent #1723

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
7 changes: 1 addition & 6 deletions catalogd/internal/storage/localdir.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,13 +210,8 @@ func (s *LocalDirV1) handleV1All(w http.ResponseWriter, r *http.Request) {
defer s.m.RUnlock()

catalog := r.PathValue("catalog")
catalogFile, catalogStat, err := s.catalogData(catalog)
if err != nil {
httpError(w, err)
return
}
w.Header().Add("Content-Type", "application/jsonl")
http.ServeContent(w, r, "", catalogStat.ModTime(), catalogFile)
http.ServeFile(w, r, catalogFilePath(s.catalogDir(catalog)))
}
Copy link
Member

Choose a reason for hiding this comment

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

Something I found from internet might be useful for the context

Feature	                     http.ServeFile                     http.ServeContent
Source	                     Directly from disk              Any io.ReadSeeker (e.g., memory, disk, network)
Headers                      Auto-detected                    Manually specified
Performance             Optimized for static files     Flexible but requires more control
Use Case	            Serving files from disk	     Serving custom or in-memory content


func (s *LocalDirV1) handleV1Query(w http.ResponseWriter, r *http.Request) {
Expand Down
2 changes: 1 addition & 1 deletion catalogd/internal/storage/localdir_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ func TestLocalDirServerHandler(t *testing.T) {
{
name: "Server returns 404 when non-existent catalog is queried",
expectedStatusCode: http.StatusNotFound,
expectedContent: "404 Not Found",
expectedContent: "404 page not found",
Copy link
Member

Choose a reason for hiding this comment

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

Oh this is interesting. I actually don't love this (because we aren't serving pages), but I also think it doesn't actually matter at all. It's just my OCD.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. I guess it's something we have to live with if we want to serveFile

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the motivation for using serveFile instead of ServeContent as it is? Is there any specific reason to prefer one over the other?

Copy link
Member

Choose a reason for hiding this comment

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

Might be worth looking at the implementations to see if there really is a significant difference. If it's just a difference of who opens the file, then maybe sticking with ServeContent is better because then 404 errors are consistent.

Copy link
Member

Choose a reason for hiding this comment

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

I took a look. ServeFile has a bunch of handling for directories, and then eventually calls serveContent. I think we close this and keep things as they are?

Sorry for the noise @anik120 I know I was the one that originally suggested this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries, I'm good with it 👍🏽

URLPath: "/catalogs/non-existent-catalog/api/v1/all",
},
{
Expand Down
Loading