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

crypto: extract throwInvalidArgType function #22947

Closed
wants to merge 1 commit into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Sep 19, 2018

This commit extracts the throwing of ERR_INVALID_ARG_TYPE which is done
in identical ways in a few places in cipher.js.

The motivation for this is that I think it improves readability enough to
warrant a commit even though I'm aware that we should avoid commits with
only these sort of refactoring.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the crypto Issues and PRs related to the crypto subsystem. label Sep 19, 2018
@danbev
Copy link
Contributor Author

danbev commented Sep 19, 2018

@tniessen
Copy link
Member

Not opposed to this change, but the name throwInvalidArgType is too generic IMO. It is only used for certain types and the function name should reflect that, even though I currently don't have a better idea.

@danbev
Copy link
Contributor Author

danbev commented Sep 20, 2018

Not opposed to this change, but the name throwInvalidArgType is too generic IMO

Yeah, I agree. I was struggling to come up with a good name. Let me try to come up with a better name for the function.

@addaleax
Copy link
Member

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 22, 2018
throw new ERR_INVALID_ARG_TYPE(
name,
['string', 'Buffer', 'TypedArray', 'DataView'],
Copy link
Member

Choose a reason for hiding this comment

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

This change seems like a mistake. The toBuf() function accepts a string and transform that into a buffer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The motivation for this changes was that toBuf() would convert any string passed in to a Buffer, and therefore isArrayBufferView would not be passed a string for the password, key, and iv cases.

Note that my first commit also updated the update function which does not call toBuf() which was a mistake and this as fixed in the latest commit, so update will also include the string in its throws cause.

Does that still sound incorrect?

Copy link
Member

Choose a reason for hiding this comment

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

The error message corresponds to the possible values for the user, not to possible transformations we do internally before throwing the error. So string should still be valid in all of these cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry about the late reply. I see your point now, I'll revert my latest change. Thanks!

@BridgeAR BridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 23, 2018
['string', 'Buffer', 'TypedArray', 'DataView'],
key
);
throwInvalidArrayBufferView('key', key);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I would prefer constructing the error object in the function and throwing it explicitly here. I think it would be better during the reviews and one less line in the stack trace 😉.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense. I'll update shortly. Thanks!

@danbev
Copy link
Contributor Author

danbev commented Sep 25, 2018

@danbev
Copy link
Contributor Author

danbev commented Sep 25, 2018

Re-run of failing node-test-commit-smartos and node-test-commit-linux.

This commit extracts the throwing of ERR_INVALID_ARG_TYPE which is done
in identical ways in a few places in cipher.js.

The motivation for this is that I think it improves readability enough to
warrant a commit even though I'm aware that we should avoid commits with
only these sort of refactoring.
@danbev
Copy link
Contributor Author

danbev commented Sep 27, 2018

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 27, 2018
@danbev
Copy link
Contributor Author

danbev commented Oct 2, 2018

@danbev
Copy link
Contributor Author

danbev commented Oct 2, 2018

Re-run of failing node-test-commit-arm

@danbev
Copy link
Contributor Author

danbev commented Oct 3, 2018

Landed in b25e6ab.

@danbev danbev closed this Oct 3, 2018
@danbev danbev deleted the cipher_throwInvalidArgType branch October 3, 2018 06:14
danbev added a commit that referenced this pull request Oct 3, 2018
This commit extracts the throwing of ERR_INVALID_ARG_TYPE which is done
in identical ways in a few places in cipher.js.

The motivation for this is that I think it improves readability enough to
warrant a commit even though I'm aware that we should avoid commits with
only these sort of refactoring.

PR-URL: #22947
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request Oct 4, 2018
This commit extracts the throwing of ERR_INVALID_ARG_TYPE which is done
in identical ways in a few places in cipher.js.

The motivation for this is that I think it improves readability enough to
warrant a commit even though I'm aware that we should avoid commits with
only these sort of refactoring.

PR-URL: #22947
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
This commit extracts the throwing of ERR_INVALID_ARG_TYPE which is done
in identical ways in a few places in cipher.js.

The motivation for this is that I think it improves readability enough to
warrant a commit even though I'm aware that we should avoid commits with
only these sort of refactoring.

PR-URL: #22947
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
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. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants