Skip to content

GODRIVER-1969 Use the unified spec test runner for GridFS tests. #1121

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

Merged
matthewdale merged 3 commits into from
Nov 10, 2022
Merged

GODRIVER-1969 Use the unified spec test runner for GridFS tests. #1121

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2f1e9d5
GODRIVER-1969 Use the unified spec test runner for GridFS tests.
matthewdale Nov 6, 2022
7fac490
Update mongo/integration/unified/gridfs_bucket_operation_execution.go
matthewdale Nov 9, 2022
1593bf2
Use a skipTestError instead of the skippedTestDescriptions for unsupp…
matthewdale Nov 9, 2022
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
5 changes: 5 additions & 0 deletions mongo/gridfs/download_stream.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,11 @@ func (ds *DownloadStream) fillBuffer(ctx context.Context) error {
_ = ds.cursor.Close(ctx)
return ds.cursor.Err()
}
// 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
Copy link
Collaborator

@qingyang-hu qingyang-hu Nov 8, 2022

Choose a reason for hiding this comment

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

What's the difference between ErrWrongIndex and ErrWrongSize?

Copy link
Contributor

@benjirewis benjirewis Nov 8, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@matthewdale matthewdale Nov 9, 2022

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@benjirewis benjirewis Nov 9, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@matthewdale matthewdale Nov 9, 2022

Choose a reason for hiding this comment

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

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.

}
return errNoMoreChunks
}

Expand Down
Loading