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

lib/querystring: convert to using internal/errors #15565

Merged
merged 1 commit into from Oct 28, 2017
Merged

Conversation

@ramimoshe
Copy link
Contributor

@ramimoshe ramimoshe commented Sep 23, 2017

covert lib/querystring.js over to using lib/internal/errors.js

ref: #11273

Copy link
Member

@BridgeAR BridgeAR left a comment

This needs a test and the error type should not change.

lib/querystring.js Outdated
@@ -178,7 +179,7 @@ function qsEscape(str) {
if (i < str.length)
c2 = str.charCodeAt(i) & 0x3FF;
else
throw new URIError('URI malformed');
throw new errors.TypeError('ERR_QUERYSTRING_URI_INVALID', 'URI malformed');

This comment has been minimized.

@BridgeAR

BridgeAR Sep 23, 2017
Member

The error type URIError should stay as it is. If that type does not yet exist in the internal/errors it has to be created.
Out of my perspective a better name for the internal type would also be ERR_INVALID_URI (I am still thinking about if it would be a good idea to combine this with the existing ERR_INVALID_URL type or not) and the error message itself should be moved in the internal errors as static value.

This comment has been minimized.

@refack

refack Sep 23, 2017
Member

As a temporary workaround this could be done with:

else {
  const e = new URIError('URI malformed');
  e.code = 'ERR_INVALID_URI';
  throw e;
}

This comment has been minimized.

@refack

refack Sep 23, 2017
Member

Since this is the only place in /lib/ where URIError is used, I'm not sure it should not be changed...
/cc @jasnell

This comment has been minimized.

@BridgeAR

BridgeAR Sep 25, 2017
Member

This recommendation is bad out of my perspective. It is not similar to the internal/errors by just adding the error code. It is neither a NodeError, nor would it receive any further changes in case something changes in the internal/errors.
I would also like to keep the error type as URIError even if it is the only occurrence in lib. It is a single line that has to be added to the internal/errors to add that type and use it from then on.

This comment has been minimized.

@ramimoshe

ramimoshe Sep 26, 2017
Author Contributor

Thank for your comments
but I am not fully understand you
@BridgeAR @refack do you mean to add "URIError" to module.exports in internal/errors?
e.g:

module.exports = exports = {
  message,
  URIError, 
  ....

or using the exported Error with URIError
e.g:

throw new errors.Error(URIError)
@BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Sep 23, 2017

@ramimoshe Thanks a lot for your PR! This goes in the right direction and just needs a bit more work 😃

@refack refack self-assigned this Sep 23, 2017
@ramimoshe
Copy link
Contributor Author

@ramimoshe ramimoshe commented Sep 23, 2017

thanks,
I changed the error to be ERR_INVALID_URL, and I did a small refactor

lib/internal/errors.js Outdated
@@ -261,7 +261,7 @@ E('ERR_NO_LONGER_SUPPORTED', '%s is no longer supported');
E('ERR_OUT_OF_RANGE', 'The "%s" argument is out of range');
E('ERR_OUTOFMEMORY', 'Out of memory');
E('ERR_PARSE_HISTORY_DATA', 'Could not parse history data in %s');
E('ERR_QUERYSTRING_URI_INVALID', '%s');
E('ERR_INVALID_URL', 'URI malformed');

This comment has been minimized.

@refack

refack Sep 23, 2017
Member

So we have a new lint rule that makes sure the error code are sorted in ASCIIbetical order.
It would have made you notice https://github.com/nodejs/node/blob/4518d94e6104323b06781ba608e59208d35a6129/lib/internal/errors.js#L243
So IMHO make this ERR_INVALID_URI (so that we can keep the message text the same), and move it to L243 above the previous ERR_INVALID_URL

@refack
Copy link
Member

@refack refack commented Sep 23, 2017

Thanks for following up.
Now all that's left is to fix the tests:
https://github.com/nodejs/node/blob/4518d94e6104323b06781ba608e59208d35a6129/test/parallel/test-querystring-escape.js#L16
https://github.com/nodejs/node/blob/4518d94e6104323b06781ba608e59208d35a6129/test/parallel/test-url-parse-invalid-input.js#L28
https://github.com/nodejs/node/blob/4518d94e6104323b06781ba608e59208d35a6129/test/parallel/test-querystring.js#L273-L276
with something like:

common.expectsError(
  () => qs.stringify({ foo: '\udc00' }),
  {
    code: 'ERR_INVALID_URI',
    type: TypeError,
    message: 'URI malformed'
  }
);

or just as the validator for assert.throws

common.expectsError({
  code: 'ERR_INVALID_URI',
  type: TypeError,
  message: 'URI malformed'
});

I did a small refactor

👍

if (i < str.length)
c2 = str.charCodeAt(i) & 0x3FF;
else
throw new URIError('URI malformed');

This comment has been minimized.

@lpinca

lpinca Sep 24, 2017
Member

I'm not sure if this should to be changed to use internal/errors. URIError seems fine to me.

This comment has been minimized.

@BridgeAR

BridgeAR Sep 25, 2017
Member

I would definitely recommend to change this to use internal/errors but it should be kept as URIError as I mentioned above (#15565 (comment)).

@ramimoshe
Copy link
Contributor Author

@ramimoshe ramimoshe commented Sep 24, 2017

@refack
I am not sure how to update 'decodeURIComponent' function
so "node/test/parallel/test-querystring.js Lines 273 to 276 " test is still checking URIError

 // invalid surrogate pair throws URIError 
 assert.throws(function() { 
   qs.stringify({ foo: '\udc00' }); 
 }, /^URIError: URI malformed$/); 

StackTrace from the test (after changing):

URIError: URI malformed
    at decodeURIComponent (<anonymous>)
    at Url.parse (url.js:292:19)
    at Object.urlParse [as parse] (url.js:98:5)
    at Object.<anonymous> (/Users/rami.moshe/Projects/forks/node/test/parallel/test-url-parse-invalid-input.js:27:5)
    at Module._compile (module.js:600:30)
    at Object.Module._extensions..js (module.js:611:10)
    at Module.load (module.js:521:32)
    at tryModuleLoad (module.js:484:12)
    at Function.Module._load (module.js:476:3)
    at Function.Module.runMain (module.js:641:10)
@BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Sep 25, 2017

@ramimoshe you can not update the decodeURIComponent function and you should not even try. It is a native function.

The error type should definitely not be changed because it would be inconsistent in that case what error we might return. Please switch back to the URIError and just add that type to the internal/errors as I pointed out.

@BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Sep 30, 2017

Ping @ramimoshe

@ramimoshe
Copy link
Contributor Author

@ramimoshe ramimoshe commented Sep 30, 2017

Thank for your comments
but I am not fully understand you
@BridgeAR @refack do you mean to add "URIError" to module.exports in internal/errors?
e.g:

module.exports = exports = {
  message,
  URIError, 
  ....

or using the exported Error with URIError
e.g:

throw new errors.Error(URIError)
@BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Sep 30, 2017

This is how you can add the URIError to the internal/errors.

module.exports = exports = {
  message,
  Error: makeNodeError(Error),
  TypeError: makeNodeError(TypeError),
  RangeError: makeNodeError(RangeError),
  URIError: makeNodeError(URIError), // <---- 
  AssertionError,
  E // This is exported only to facilitate testing.
};

And this is how you would use it in a different file.

const errors = require('internal/errors');
// ...
throw new errors.URIError('ERR_INVALID_URI');
@joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Oct 8, 2017

I think I have brought this up in a URL PR before but couldn't find the link. The ECMAScript spec says URIError should only be raised by built-in functions, i.e. functions like decodeURIComponent() that are implemented as part of the language, we should not raise that in our codebase anyway.

(Going to board so no time to dig the link to spec)

@refack
refack approved these changes Oct 8, 2017
@refack
Copy link
Member

@refack refack commented Oct 8, 2017

I couldn't find a reference in the ECMAScript spec (image because the spec takes a while to load)
image


The Web IDL spec only excludes SyntaxError - https://heycam.github.io/webidl/#idl-exceptions

image

@refack
Copy link
Member

@refack refack commented Oct 8, 2017

@ramimoshe, sorry for the delay.

so "node/test/parallel/test-querystring.js Lines 273 to 276 " test is still checking URIError

Use common.expectsError, specifying the type and code. Example

common.expectsError(
() => zlib.deflateSync(i),
{
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError,
message: 'The "buffer" argument must be one of type string, Buffer, ' +
'TypedArray, or DataView'
}
);

@joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Oct 14, 2017

@refack IMO

Indicates that one of the global URI handling functions was used in a way that is incompatible with its definition.

means it excludes user land methods from throwing a URIError, because those are not "global URI handling functions". Also the WHATWG URL doesn't throw URIError either, it throws TypeError in similar situations.

Anyway, I am still fine with throwing URIError in this PR because that's what we do at the moment.

Copy link
Member

@joyeecheung joyeecheung left a comment

This would need to update test/parallel/test-querystring.js tests in order to pass the tests...

@joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Oct 14, 2017

Also, test/parallel/test-querystring-escape.js needs to be updated as well.

@ramimoshe
Copy link
Contributor Author

@ramimoshe ramimoshe commented Oct 19, 2017

@joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Oct 19, 2017

@ramimoshe Nope, looks like a flake because the failing test should not go through the query string module.

@ramimoshe
Copy link
Contributor Author

@ramimoshe ramimoshe commented Oct 25, 2017

@joyeecheung do i have to do something or just wait to approving?

@joyeecheung joyeecheung dismissed stale reviews from BridgeAR and themself Oct 25, 2017

Fixed

@joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Oct 25, 2017

@ramimoshe This is semver-major so needs at least 2 TSC approvals... @nodejs/tsc

@jasnell jasnell added this to In Progress in Error Codes Oct 25, 2017
@joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Oct 28, 2017

https://ci.nodejs.org/job/node-test-linter/13033/console

not ok 11 - /usr/home/iojs/build/workspace/node-test-linter/lib/internal/errors.js
  ---
  message: '"ERR_INVALID_URI" is not documented in doc/api/errors.md'
  severity: error
  data:
    line: 291
    column: 1
    ruleId: documented-errors
  messages:
    - message: doc/api/errors.md does not have an anchor for "ERR_INVALID_URI"
      severity: error
      data:
        line: 291
        column: 1
        ruleId: documented-errors
  ...

@ramimoshe Do you have time to document this error code? If not I can do that, just don't want to delay this PR for too long.

@ramimoshe
Copy link
Contributor Author

@ramimoshe ramimoshe commented Oct 28, 2017

@joyeecheung - yes, i will fix it

Copy link
Member

@joyeecheung joyeecheung left a comment

PR-URL: #15565
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@refack refack force-pushed the ramimoshe:master branch to 9788e96 Oct 28, 2017
@refack refack merged commit 9788e96 into nodejs:master Oct 28, 2017
@refack
Copy link
Member

@refack refack commented Oct 28, 2017

@ramimoshe Thanks!

@targos targos moved this from In Progress to Done in Error Codes Oct 29, 2017
Qard added a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
PR-URL: nodejs/node#15565
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Qard added a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
PR-URL: nodejs/node#15565
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
addaleax added a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
PR-URL: nodejs/node#15565
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@refack refack removed their assignment Oct 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Error Codes
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.