🌱 (catalogd) serveFile for all endpoint instead of serveContent #1723
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
anik120
changed the title
| name: "Server returns 404 when non-existent catalog is queried", | ||
| expectedStatusCode: http.StatusNotFound, | ||
| expectedContent: "404 Not Found", | ||
| expectedContent: "404 page not found", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Right. I guess it's something we have to live with if we want to serveFile
There was a problem hiding this comment.
What is the motivation for using serveFile instead of ServeContent as it is? Is there any specific reason to prefer one over the other?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
No worries, I'm good with it 👍🏽
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1723 +/- ##
==========================================
- Coverage 67.58% 67.45% -0.14%
==========================================
Files 59 59
Lines 4982 4977 -5
==========================================
- Hits 3367 3357 -10
- Misses 1371 1375 +4
- Partials 244 245 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| w.Header().Add("Content-Type", "application/jsonl") | ||
| http.ServeContent(w, r, "", catalogStat.ModTime(), catalogFile) | ||
| http.ServeFile(w, r, catalogFilePath(s.catalogDir(catalog))) | ||
| } |
There was a problem hiding this comment.
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
4 participants
Description
Reviewer Checklist