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

http: be more specific when reporting errors #14759

Closed
wants to merge 1 commit into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Aug 11, 2017

When reporting header content errors specify header name in the message
for easier debugging in user applications.

Fix: #14754

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

http

When reporting header content errors specify header name in the message
for easier debugging in user applications.

Fix: nodejs#14754
@indutny indutny added the http Issues or PRs related to the http subsystem. label Aug 11, 2017
@indutny indutny requested review from mscdex and jasnell August 11, 2017 01:59
@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Aug 11, 2017
@mscdex mscdex added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 11, 2017
@mscdex
Copy link
Contributor

mscdex commented Aug 11, 2017

How about more simply: 'The header content contains invalid characters: ' + name ?

@indutny
Copy link
Member Author

indutny commented Aug 11, 2017

@mscdex I don't mind. Although, both are consistent with existing error messages.

@@ -440,7 +440,8 @@ function storeHeader(self, state, key, value, validate) {
throw new Error('Header "%s" value must not be undefined', key);
} else if (checkInvalidHeaderChar(value)) {
debug('Header "%s" contains invalid characters', key);
throw new TypeError('The header content contains invalid characters');
throw new TypeError(
'The header content contains invalid characters ["' + key + '"]');
Copy link
Member

Choose a reason for hiding this comment

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

If the error messages are being changed, it would be nice to migrate the files to use internal/errors at the same time.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Specific change LGTM with a suggestion

@starkwang
Copy link
Contributor

The errors in lib/_http_outgoing.js is being migrated to use internal/errors(#14735). I've added the header name in the error message.

@jasnell
Copy link
Member

jasnell commented Aug 23, 2017

@indutny ... can you give this a rebase?

@indutny
Copy link
Member Author

indutny commented Aug 23, 2017

@jasnell is it relevant after #14735?

@jasnell
Copy link
Member

jasnell commented Aug 23, 2017

It appears your're right :-)

@jasnell jasnell closed this Aug 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Header name is not added in the error message for the case of invalid characters in the header
8 participants