Skip to content

CXX-2650 Fix limit value in open_download_stream #927

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

Jump to bottom
Merged
merged 2 commits into from
Feb 7, 2023
Merged

CXX-2650 Fix limit value in open_download_stream #927

merged 2 commits into from
Feb 7, 2023

Conversation

chino540off
Copy link
Contributor

Hello,

Here is a patch to fix the limit value computed to returned the correct number of chunks in a partial download.
Without this patch, a download::read on 2 chunks throws an execption because the chunks cursor was exhausted.

Cheers

@chino540off
Copy link
Contributor Author

I've just add a unit test to demonstrate the issue.

@chino540off chino540off force-pushed the fix_limit_in_open_downstream branch from 4b2cfd0 to fbee925 Compare January 25, 2023 10:06
@kkloberdanz
Copy link
Contributor

Hi @chino540off, thank you for the contribution! We are happy to have your fix. Would you please submit the PR to be merged into the master branch? Since this is a bug fix, we can then cherry-pick it into the releases/v3.7 branch after it has been merged into master.

Once again, thank you!

@chino540off chino540off changed the base branch from releases/v3.7 to master February 2, 2023 08:17
@chino540off chino540off force-pushed the fix_limit_in_open_downstream branch from fbee925 to 88edfe5 Compare February 2, 2023 08:18
@chino540off
Copy link
Contributor Author

chino540off commented Feb 2, 2023 •
edited
Loading

Hello @kkloberdanz, you're welcome.

The branch has been rebased over master. Please tell me if it misses something.

cheers,

kkloberdanz
kkloberdanz approved these changes Feb 2, 2023
View reviewed changes
Copy link
Contributor

@kkloberdanz kkloberdanz left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@chino540off
Copy link
Contributor Author

@kkloberdanz some steps are failing in the CI (evergreen, and evergreen/mongohouse-ubuntu). Is there something I can do ? I cannot access to get some details.

@kkloberdanz
Copy link
Contributor

Hi @chino540off, the failing test is a known broken test. We are working on fixing it, but it is not a blocker for your PR. @kevinAlbs is planning to review your PR once he has the time. After we get Kevin's review, we can proceed with this PR. Thank you for you patience!

@kevinAlbs kevinAlbs changed the title Fix limit value in open_download_stream CXX-2650 Fix limit value in open_download_stream Feb 7, 2023
kevinAlbs
kevinAlbs approved these changes Feb 7, 2023
View reviewed changes
Copy link
Collaborator

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution @chino540off.

@kevinAlbs kevinAlbs merged commit 1da6947 into mongodb:master Feb 7, 2023
kevinAlbs pushed a commit that referenced this pull request Feb 7, 2023
@chino540off @kevinAlbs
CXX-2650 Fix limit value in open_download_stream (#927)
d309410 
* [mongocxx/gridfs/bucket] Fix the cursor limit value in _open_download_stream

* [mongocxx/gridfs/utest] Add a unit test to ensure limit value

---------

Co-authored-by: Olivier Detour <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevinAlbs kevinAlbs kevinAlbs approved these changes

@kkloberdanz kkloberdanz kkloberdanz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chino540off @kkloberdanz @kevinAlbs