GODRIVER-1969 Use the unified spec test runner for GridFS tests. #1121
Conversation
| // If there are no more chunks, but we didn't read the expected number of chunks, return an | ||
| // ErrWrongIndex error to indicate that we're missing chunks at the end of the file. | ||
| if ds.expectedChunk != ds.numChunks { | ||
| return ErrWrongIndex |
There was a problem hiding this comment.
What's the difference between ErrWrongIndex and ErrWrongSize?
There was a problem hiding this comment.
Our documentation says:
// ErrWrongIndex is used when the chunk retrieved from the server does not have the expected index.
...
// ErrWrongSize is used when the chunk retrieved from the server does not have the expected size.
ErrWrongIndex seems more accurate in this situation than ErrWrongSize. I could also see an argument for a new error type, something like ErrMissingChunks.
There was a problem hiding this comment.
I agree that the error name/description is kinda confusing and an ErrMissingChunk would be more clear. However, since it's caused by the same issue where ErrWrongIndex is currently used (i.e. a chunk is missing), I decided it was better to use the same error so any existing error checking code would continue working.
I originally decided to use ErrWrongIndex because the unified spec test describing the error it's expecting as "ChunkIsMissing". In the Go Driver, there isn't a "ChunkIsMissing" error, but the current spec test runner maps "ChunkIsMissing" to ErrWrongIndex.
I think we should use ErrWrongIndex in both cases, but maybe we can/should update the error message? Something like:
chunk is missing or index does not match expected index
There was a problem hiding this comment.
Using ErrWrongIndex SGTM, but I'm hesitant to change the message for ErrWrongIndex in case users are checking for the current message. That said, GridFS does not get much usage, and most users would probably do something like errors.Is(err, gridfs.ErrWrongIndex), anyway, so up to you.
There was a problem hiding this comment.
I'll leave it as-is and add a proposal to the Go Driver 2.0 scope to rename and/or update the description of the error.
| // If there are no more chunks, but we didn't read the expected number of chunks, return an | ||
| // ErrWrongIndex error to indicate that we're missing chunks at the end of the file. | ||
| if ds.expectedChunk != ds.numChunks { | ||
| return ErrWrongIndex |
There was a problem hiding this comment.
Our documentation says:
// ErrWrongIndex is used when the chunk retrieved from the server does not have the expected index.
...
// ErrWrongSize is used when the chunk retrieved from the server does not have the expected size.
ErrWrongIndex seems more accurate in this situation than ErrWrongSize. I could also see an argument for a new error type, something like ErrMissingChunks.
Co-authored-by: Benjamin Rewis <32186188+benjirewis@users.noreply.github.com>
…orted GridFS spec tests.
3 participants
GODRIVER-1969
Summary
DownloadStreamAPI that failed to return an error when chunks were missing at the end of a GridFS file.Background & Motivation
This is a prerequisite for completely removing the
x/bsonxpackage (GODRIVER-2416) because the current GridFS spec test runner relies heavily onbsonx.DocAPIs that have no analog for thebson.Dtype (see this comment for more details).