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.

Sign up for GitHub

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

Jump to bottom

Do not call sprintf with one argument #63

Open
hekike opened this issue Jan 2, 2019 · 6 comments
Open

Do not call sprintf with one argument #63

hekike opened this issue Jan 2, 2019 · 6 comments

Comments

@hekike
Copy link

hekike commented Jan 2, 2019 •
edited
Loading

PR https://cr.joyent.us/#/c/5299

Safe user inputs in messages

Addresses the issue when someone builds message manually and it contains user-input with formatting characters instead of using sprintf.

For example:
In this case VError throws without this change:

var myInput = "encoded%stext"
new VError(myInput + " invalid input")

Related to #57

Performance Improvement

Calling sprintf is expensive, do not call when zero arguments are passed.
In some scenarios, an application needs to create hundreds of VError(s).

To test the change's effect:

for(var i = 0; i < 10000; i++) {
        new mod_verror.VError('Foo')
}

On my 2017 MBP 3.5Ghz, I see a 27% performance improvement after the change.

Before:
screen shot 2018-12-31 at 10 29 47 am
@hekike hekike mentioned this issue Jan 2, 2019
Closed
@davepacheco
Copy link
Contributor

Thanks for submitting this!

I really don't like the change in behavior regarding format specifiers like '%s' in a single-argument string. Similar ideas have been discussed extensively in #57, #8, and a few others. In particular, this question and the reply under it. I get that this is a rough edge, and I'm sorry for the pain it causes, but I find the alternative painful, too (inconsistent behavior regarding cases that are otherwise the same from a user's perspective).

#57 proposes an alternate constructor that would avoid this particular rough edge for people who want that. I think that's the way to go here.

@davepacheco
Copy link
Contributor

As an example, I think it's pretty confusing that VError('%s') would produces a valid Error with message '%s' while VError('%s %s', 'one') would blow up.

@hekike
Copy link
Author

hekike commented Jan 14, 2019

@davepacheco thanks for the feedback, I picked up the work from #57 with adding the modifications you asked in the CL: https://cr.joyent.us/#/c/4394

New CL: https://cr.joyent.us/#/c/5360/

Please review.

@davepacheco davepacheco mentioned this issue Jan 26, 2019
Closed
@gabegorelick
Copy link

Will this be done soon?

@davepacheco
Copy link
Contributor

@gabegorelick I did provide feedback on the change back in January but I believe the author went a different route. I just tried to pick it up myself and apply the feedback but there's a little more work left than I realized. The new change is here:

https://cr.joyent.us/#/c/6608/

The main difference from review 5360 is that I've refactored the code as I suggested in 5360.
Unfortunately, the test suite doesn't pass. I'm not sure if it wasn't run before or if my changes made it worse, but it looks to me like more work is needed to separate out the tests that can be run with PError from those that can't (because they test format strings). Since I haven't fixed that, I don't know if there are other regressions lurking.

I think this is the best starting point if somebody wants to pick up the test suite work. I'm not sure when I'll get to that again.

@hekike hekike mentioned this issue Aug 18, 2019
Merged
trentm pushed a commit that referenced this issue Oct 24, 2019
@trentm
#63 Do not call sprintf with one argument
092c9f8 
Portions contributed by: David Pacheco <dap@joyent.com>
@joyent-automation joyent-automation mentioned this issue Oct 24, 2019
Closed
@trentm trentm mentioned this issue Oct 24, 2019
Closed
@trentm
Copy link
Contributor

trentm commented Oct 24, 2019

My history of verror work that led to the skipPrintf option to VError and the PError class.
(@davepacheco has an earlier history here.)

My current intent:

raphinesse added a commit to raphinesse/cordova-common that referenced this issue Dec 12, 2019
@raphinesse
feat(CordovaError): support for error cause & more
48fee1d 
This commit bases CordovaError on the popular joyent/node-verror.

We actually use @netflix/nerror, a VError fork for now. That's because
we do not want printf style error formatting support and that forks
allows to disable it. There's [ongoing work][1] to integrate that change
into the original VError.

So basically CordovaError behaves like PError but with all the static
methods from VError and different parameter ordering for its
constructor.

One change that could break some existing tests in repositories that use
cordova-common is that `toString` (for errors without a cause argument)
now behaves like the Error default again:

    new CordovaError('foo').toString();
    // old result: 'foo'
    // new result: 'CordovaError: foo'

[1]: TritonDataCenter/node-verror#63 (comment)
@raphinesse raphinesse mentioned this issue Dec 13, 2019
Merged
raphinesse added a commit to raphinesse/cordova-common that referenced this issue Dec 13, 2019
@raphinesse
feat(CordovaError): support for error cause & more
b85e6d3 
This commit bases CordovaError on the popular [joyent/node-verror].

We actually use @netflix/nerror, a VError fork, for now. That's because
we do not want printf style error formatting support and that fork
allows to disable it. There's [ongoing work][1] to integrate that change
into the original VError.

So basically CordovaError behaves like PError but with all the static
methods from VError and different parameter ordering for its
constructor.

One change that could break some existing tests in repositories that use
cordova-common is that `toString` (for errors without a cause argument)
now behaves like the Error default again:

    new CordovaError('foo').toString();
    // old result: 'foo'
    // new result: 'CordovaError: foo'

[joyent/node-verror]: https://github.com/joyent/node-verror
[1]: TritonDataCenter/node-verror#63 (comment)
raphinesse added a commit to raphinesse/cordova-common that referenced this issue Dec 15, 2019
@raphinesse
feat(CordovaError): support for error cause & more
a6101a4 
This commit bases CordovaError on the popular [joyent/node-verror].

We actually use @netflix/nerror, a VError fork, for now. That's because
we do not want printf style error formatting support and that fork
allows to disable it. There's [ongoing work][1] to integrate that change
into the original VError.

So basically CordovaError behaves like PError but with all the static
methods from VError and different parameter ordering for its
constructor.

One change that could break some existing tests in repositories that use
cordova-common is that `toString` (for errors without a cause argument)
now behaves like the Error default again:

    new CordovaError('foo').toString();
    // old result: 'foo'
    // new result: 'CordovaError: foo'

[joyent/node-verror]: https://github.com/joyent/node-verror
[1]: TritonDataCenter/node-verror#63 (comment)
raphinesse added a commit to raphinesse/cordova-common that referenced this issue Jan 9, 2020
@raphinesse
feat(CordovaError): support for error cause & more
3f8c8ee 
This commit bases CordovaError on the popular [joyent/node-verror].

We actually use @netflix/nerror, a VError fork, for now. That's because
we do not want printf style error formatting support and that fork
allows to disable it. There's [ongoing work][1] to integrate that change
into the original VError.

So basically CordovaError behaves like PError but with all the static
methods from VError and different parameter ordering for its
constructor.

One change that could break some existing tests in repositories that use
cordova-common is that `toString` (for errors without a cause argument)
now behaves like the Error default again:

    new CordovaError('foo').toString();
    // old result: 'foo'
    // new result: 'CordovaError: foo'

[joyent/node-verror]: https://github.com/joyent/node-verror
[1]: TritonDataCenter/node-verror#63 (comment)
raphinesse added a commit to raphinesse/cordova-common that referenced this issue Jan 12, 2020
@raphinesse
feat(CordovaError): support for error cause & more
0920aaf 
This commit bases CordovaError on the popular [joyent/node-verror].

We actually use @netflix/nerror, a VError fork, for now. That's because
we do not want printf style error formatting support and that fork
allows to disable it. There's [ongoing work][1] to integrate that change
into the original VError.

So basically CordovaError behaves like PError but with all the static
methods from VError and different parameter ordering for its
constructor.

One change that could break some existing tests in repositories that use
cordova-common is that `toString` (for errors without a cause argument)
now behaves like the Error default again:

    new CordovaError('foo').toString();
    // old result: 'foo'
    // new result: 'CordovaError: foo'

[joyent/node-verror]: https://github.com/joyent/node-verror
[1]: TritonDataCenter/node-verror#63 (comment)
raphinesse added a commit to raphinesse/cordova-common that referenced this issue Mar 14, 2020
@raphinesse
feat(CordovaError): support for error cause & more
569793d 
This commit bases CordovaError on the popular [joyent/node-verror].

We actually use @netflix/nerror, a VError fork, for now. That's because
we do not want printf style error formatting support and that fork
allows to disable it. There's [ongoing work][1] to integrate that change
into the original VError.

So basically CordovaError behaves like PError but with all the static
methods from VError and different parameter ordering for its
constructor.

One change that could break some existing tests in repositories that use
cordova-common is that `toString` (for errors without a cause argument)
now behaves like the Error default again:

    new CordovaError('foo').toString();
    // old result: 'foo'
    // new result: 'CordovaError: foo'

[joyent/node-verror]: https://github.com/joyent/node-verror
[1]: TritonDataCenter/node-verror#63 (comment)
raphinesse added a commit to raphinesse/cordova-common that referenced this issue Mar 14, 2020
@raphinesse
feat(CordovaError): support for error cause & more
e25f155 
This commit bases CordovaError on the popular [joyent/node-verror].

We actually use @netflix/nerror, a VError fork, for now. That's because
we do not want printf style error formatting support and that fork
allows to disable it. There's [ongoing work][1] to integrate that change
into the original VError.

So basically CordovaError behaves like PError but with all the static
methods from VError and different parameter ordering for its
constructor.

One change that could break some existing tests in repositories that use
cordova-common is that `toString` (for errors without a cause argument)
now behaves like the Error default again:

    new CordovaError('foo').toString();
    // old result: 'foo'
    // new result: 'CordovaError: foo'

[joyent/node-verror]: https://github.com/joyent/node-verror
[1]: TritonDataCenter/node-verror#63 (comment)
raphinesse added a commit to apache/cordova-common that referenced this issue Mar 14, 2020
@raphinesse
feat(CordovaError): support for error cause & more (#121)
fe6a73f 
This commit bases CordovaError on the popular [joyent/node-verror].

We actually use @netflix/nerror, a VError fork, for now. That's because
we do not want printf style error formatting support and that fork
allows to disable it. There's [ongoing work][1] to integrate that change
into the original VError.

So basically CordovaError behaves like PError but with all the static
methods from VError and different parameter ordering for its
constructor.

One change that could break some existing tests in repositories that use
cordova-common is that `toString` (for errors without a cause argument)
now behaves like the Error default again:

    new CordovaError('foo').toString();
    // old result: 'foo'
    // new result: 'CordovaError: foo'

[joyent/node-verror]: https://github.com/joyent/node-verror
[1]: TritonDataCenter/node-verror#63 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@trentm @davepacheco @gabegorelick @hekike