GODRIVER-1548 Add gridfs.File type #353
Conversation
mongo/gridfs/download_stream.go
Outdated
| @@ -161,6 +215,11 @@ func (ds *DownloadStream) Skip(skip int64) (int64, error) { | |||
| return skip, nil | |||
| } | |||
|
|
|||
| // GetFile returns a File object representing the file being downloaded. | |||
| func (ds *DownloadStream) GetFile() File { | |||
There was a problem hiding this comment.
Is the file meant to not be stored as a pointer? Couldn't the metadata be quite large. This would end up with a copy every time you get the file instead of just copying the pointer.
ie return *File instead and store as a pointer to the file.
There was a problem hiding this comment.
I opted to return a value instead of a pointer because the DownloadStream type uses parts of the file (e.g. the file length) and offering a pointer would let a user mutate the stream's state. The stream doesn't validate its state after creation because it's not mutable by users, so this could cause problems with unknown consequences.
If you're concerned about metadata size, I can update the DownloadStream type to store the relevant information it needs from File upon creation and then return a pointer to the underlying File. That way, it's not accessing anything that's potentially been mutated by a user and also avoiding the large copy.
There was a problem hiding this comment.
Ah that is a good concern.
Metadata doesn't really have any size limits besides the total bson doc limit and considering most fields have been pushed into it + not wanting to limit how people use the metadata. Anyways I think it'd be a good idea to not tie ourselves into supporting a potential issue here.
I would say either update to a pointer with a clear comment that all fields are read only(you could modify metadata after unmarshaling if you wished though safely) or implement your internal copy idea. Either way I'd say release as with a *File even if it's just a comment preventing bad usage for now.
There was a problem hiding this comment.
Updated to store/return a *File and defensively copy the necessary fields in newDownloadStream.
|
LGTM I propose storing/returning the |
No description provided.