Skip to content

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

Merged
nodejs-github-bot merged 3 commits into
nodejs:masterfrom
fracsinus:fs
Dec 10, 2021
Merged

fs: fix length option being ignored during read()#40906
nodejs-github-bot merged 3 commits into
nodejs:masterfrom
fracsinus:fs

Conversation

@fracsinus

@fracsinusfracsinus commented Nov 21, 2021

Copy link
Copy Markdown
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.

@fracsinus
fs: fix length option being ignored during read()
4948c56
Currently, `length` in an options object is ignored.
@nodejs-github-botnodejs-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
ronag
ronag reviewed Nov 21, 2021
View reviewed changes
Comment threadlib/internal/fs/promises.js Outdated
ronag
ronag requested changes Nov 21, 2021
View reviewed changes

@ronagronag left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Needs a test

@aduh95

aduh95 commented Nov 21, 2021

Copy link
Copy Markdown
Contributor

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?

@fracsinus@ronag
fs: use nullish coalescing to respect zero-length reads
8fdb45b
Co-authored-by: Robert Nagy <ronagy@icloud.com>
@ronagronag 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

ronag commented Nov 22, 2021

Copy link
Copy Markdown
Member

@nodejs/fs

@github-actionsgithub-actionsBot 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.

Sign in to view
@ronag
ronag requested a review from aduh95November 22, 2021 10:22
aduh95
aduh95 reviewed Nov 22, 2021
View reviewed changes

@aduh95aduh95 left a comment

Copy link
Copy Markdown
Contributor

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?

Comment threadtest/parallel/test-fs-promises-file-handle-read.js Outdated
Comment threadtest/parallel/test-fs-promises-file-handle-read.js Outdated
@aduh95aduh95 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
@github-actionsgithub-actionsBot mentioned this pull request Nov 23, 2021
Open
45 tasks
@fracsinus@aduh95
fs: add test for zero-length read
3d49687
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@aduh95aduh95 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-actionsgithub-actionsBot 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.

Sign in to view
This was referenced Nov 24, 2021
Open
Open
@nodejs-github-bot

nodejs-github-bot commented Nov 25, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/41093/

@github-actionsgithub-actionsBot mentioned this pull request Nov 26, 2021
Open
33 tasks
@github-actionsgithub-actionsBot mentioned this pull request Nov 27, 2021
Open
27 tasks
@nodejs-github-bot

nodejs-github-bot commented Nov 28, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/41162/

@aduh95aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 10, 2021
@nodejs-github-botnodejs-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:masterDec 10, 2021
@nodejs-github-bot

nodejs-github-bot commented Dec 10, 2021

Copy link
Copy Markdown
Collaborator

Landed in 10493b4

danielleadams pushed a commit that referenced this pull request Dec 14, 2021
@fracsinus@danielleadams
fs: fix length option being ignored during read()
e25671c
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>
@danielleadamsdanielleadams mentioned this pull request Dec 14, 2021
Merged
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
@fracsinus@danielleadams
fs: fix length option being ignored during read()
6a98aa3
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
@fracsinus@danielleadams
fs: fix length option being ignored during read()
8a790a9
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 Feb 1, 2022
@fracsinus@danielleadams
fs: fix length option being ignored during read()
67c6170
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>
@danielleadamsdanielleadams mentioned this pull request Feb 1, 2022
Merged
@coderabbitaicoderabbitaiBot mentioned this pull request Jul 1, 2026
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lpincalpincalpinca approved these changes
@ronagronagronag approved these changes
@aduh95aduh95aduh95 approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.fsIssues and PRs related to the fs subsystem / file system.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@fracsinus@aduh95@ronag@nodejs-github-bot@lpinca@targos