Skip to content
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.

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

fs: fix length option being ignored during read() #40906

Merged
merged 3 commits into from
Dec 10, 2021

Conversation

fracsinus
Copy link
Contributor

Currently, calling read() with an options object always uses buffer.byteLength, ignoring length in the object.

First time opening a PR here, please let me know if anything doesn't conform to the guidelines.

Currently, `length` in an options object is ignored.
@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Nov 21, 2021
lib/internal/fs/promises.js Outdated Show resolved Hide resolved
Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

Needs a test

@aduh95
Copy link
Contributor

aduh95 commented Nov 21, 2021

Thanks for sending this PR, this looks indeed like an oversight.

Can you please add test cases in test/parallel/test-fs-promises-file-handle-read.js where the length argument is different from buffer.byteLength, and verify that the number of read bytes is as expected?

Co-authored-by: Robert Nagy <ronagy@icloud.com>
@ronag ronag added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. and removed needs-ci PRs that need a full CI run. labels Nov 22, 2021
@ronag
Copy link
Member

ronag commented Nov 22, 2021

@nodejs/fs

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 22, 2021
@nodejs-github-bot

This comment has been minimized.

@ronag ronag requested a review from aduh95 November 22, 2021 10:22
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Can you please add a test case where length is 0?

test/parallel/test-fs-promises-file-handle-read.js Outdated Show resolved Hide resolved
test/parallel/test-fs-promises-file-handle-read.js Outdated Show resolved Hide resolved
@aduh95 aduh95 added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Nov 22, 2021
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Nov 23, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 23, 2021
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 10, 2021
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 10, 2021
@nodejs-github-bot nodejs-github-bot merged commit 10493b4 into nodejs:master Dec 10, 2021
@nodejs-github-bot
Copy link
Collaborator

Landed in 10493b4

danielleadams pushed a commit that referenced this pull request Dec 13, 2021
Currently, `length` in an options object is ignored.

PR-URL: #40906
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit that referenced this pull request Dec 14, 2021
Currently, `length` in an options object is ignored.

PR-URL: #40906
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
Currently, `length` in an options object is ignored.

PR-URL: #40906
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
Currently, `length` in an options object is ignored.

PR-URL: #40906
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
Currently, `length` in an options object is ignored.

PR-URL: nodejs#40906
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
Currently, `length` in an options object is ignored.

PR-URL: #40906
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants