Infer content type in alloc fs stat endpoint #5907
Conversation
client/allocdir/alloc_dir.go
Outdated
| // detectContentType tries to infer the file type by reading the first | ||
| // 512 bytes of the file. Json file extensions are special cased. | ||
| func detectContentType(fileInfo os.FileInfo, path string) string { | ||
| contentType := "unknown" |
There was a problem hiding this comment.
I would default to application/octet-stream and consider returning rather than assigning to variable.
There was a problem hiding this comment.
+1 to this. It's essentially how I'd interpret unknown anyway, might as well use the proper generic binary type.
| } | ||
| } | ||
| // Special case json files | ||
| if strings.HasSuffix(path, ".json") { |
There was a problem hiding this comment.
Given that this is a cheaper check, I'd consider checking it first.
There was a problem hiding this comment.
Probably an unnecessary caution, but it's possible that a *.json file isn't actually json. Could be useful to only do this more targeted content typing if the file sniffing content type is in fact text.
client/allocdir/alloc_dir_test.go
Outdated
| fileInfo, err := os.Stat(imgPath) | ||
| require.Nil(err) | ||
| res := detectContentType(fileInfo, imgPath) | ||
| require.Equal("image/png", res) |
There was a problem hiding this comment.
more nit picking - seems like a good candidate for table testing - I would test go source files and maybe some application/octet-stream file as well.
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This PR adds a new field to
AllocFileInfo, used in the stat endpoint. It uses http.DetectContentType to infer content type for files.