Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Errors are passed with message : Unspecified Error #93

Closed
osher opened this Issue Jul 23, 2012 · 20 comments

Comments

Projects
None yet
4 participants

osher commented Jul 23, 2012

Yes, I see the fields reason, error, errid and all the rest.

But still, I'm quite sure that it's not to the point nor any help to pass Unspecified Error on the description and on the error message...

I'm quite sure there's a single place to fix this

{
    message: 'Unspecified error',
    stacktrace: [
        'Error: Unspecified error',
        '    at Request._callback (D:\\....
        '    at Request.callback (D:\\...
        '    at Request.<anonymous> (native)',
        '    at Request.<anonymous> (events.js:70:17)',
        '    at Request.emit (D:\\...
        '    at Request.<anonymous> (D:\\...
        '    at Request.<anonymous> (events.js:67:17)',
        '    at Request.emit (D:\\...
        '    at IncomingMessage.<anonymous> (D:\\...
    ],
    request: {
        headers: { content-type: 'application/json', accept: 'a
        callback: Function,
        uri: 'http://localhost:5984/ua/test',
        jar: false,
        method: 'GET'
    },
    scope: 'couch',
    name: 'Error',
    status-code: 404,
    description: 'Unspecified error',
    status_code: 404,
    error: 'not_found',
    errid: 'non_200',
    arguments: undefined,
    headers: {
        cache-control: 'must-revalidate',
        content-type: 'application/json',
        status-code: 404,
        uri: 'http://localhost:5984/ua/test',
        date: 'Mon, 23 Jul 2012 20:07:22 GMT'
    },
    stack: 'Error: Unspecified error\n    at Request._callback
Owner

dscape commented Jul 23, 2012

Can you tell me what's the issue? What would you like to see?

The unspecified error seems to be bubbled up from request (@mikeal).

I can fix this if you get me a reproducible test case and what you would except to happen.

Please re-open the issue when you have that, or feel free to simply submit a pull request.

@dscape dscape closed this Jul 23, 2012

osher commented Jul 24, 2012

W?????
I don't believe you closed it like this!

All you have to do, is pass the errs.create() a message property.
That's all.

As a result - inspecting the error, passing it to the console, passing it to the log, catching it in unit-testing frameworks (i personally use vows) - will all make much more sense.

Owner

dscape commented Jul 24, 2012

Feel free to submit a PR if you like with this change. Until then this stays closed.

Collaborator

mikeal commented Jul 24, 2012

is the bubbled error a socket error or an http code that doesn't match?

Owner

dscape commented Jul 24, 2012

Seems to be a simple 404. I think the issue here is that if you throw this the message gets shows, while the reason holds the actual message from couchdb (as it should)

@osher wanted this behavior to be changed and message to be the same in reason when reason is presented.

Collaborator

mikeal commented Jul 24, 2012

a 404 is an error you create, not one that I create, are you doing new Error("404 cause you dont know your data") or are you just passing a normal object?

Collaborator

mikeal commented Jul 24, 2012

the HTTP reason is not useful because the application tier is interpreting the 404 not as "not found" but as "this document does not exist in this database" and so it can create a much more applicable reason.

Owner

dscape commented Jul 24, 2012

Errors in nano or non 200s. This is one of them.

Collaborator

mikeal commented Jul 24, 2012

yeah, as long as it's an actually error instance you'll have the right tracebacks.

osher commented Jul 25, 2012

The tracebacks are not the issue - they are correct.
It's just instead of having the couch-error-proxied-message popped to your face, you need to inspect the object, and then see that its actually a couch error, and that the message is on the reason - which is nothing a newb will look for, and nothing that is provided with the Error#toString() implementation - like what you get when you concat an error object to your so-called user-friendly error log entry.

I could figure that the errors are created using errs, so basically it's about passing message to errs, or about creating a new Error(reason) and merging to it all the rest of the info you need to add

dscape added a commit that referenced this issue Jul 30, 2012

youurayy commented Sep 3, 2012

Hi, this isn't fixed.

This is when I try to delete an object that was already deleted (via nano.db.destroy().

If there is no .message in flatiron/errs/lib/errs.js:83, it gets created with the text "Unspecified Error".

And the .message is not yet there at nano.js:290, because the parsed that came from couch looks like {error:'not_found',reason:'deleted'}.

So the errs.merge() won't merge the properly constructed .message into the parsed Error, because errs.created() already created a .message field there.

Sorry no time for tests or pull req right now, already spent my time on figuring this out. Should be obvious by the info I provided, if not let me know please and will find the time.

This should be fixable by adding something like

          if(!parsed.message && (parsed.reason || parsed.error))
            parsed.message = (parsed.reason || parsed.error);

before the errs.handle(errs.merge(errs.create(parsed) line in nano.js. But then, I can't be sure if that's the right way to do it in respect to errs.js etc.

Owner

dscape commented Sep 4, 2012

Thanks @ypocat . I'll take a look later today

Owner

dscape commented Sep 4, 2012

@ypocat are you running the latest version of nano? check

nano/nano.js

Line 297 in 0160051

, "message" : parsed.reason || "couch returned "+status_code
. reason is being merged.

Please do send a PR with a broken test if you wish to show me wrong, but I can't reproduce this neither I found your claim to be true according to code.

Thanks for the report, hope you enjoy nano.

youurayy commented Sep 4, 2012

Hi @dscape, that's what I tried to explain - the .message is not merged, because the the errs.create(parsed) call precedes the errs.merge(<result of err.create(parsed)>, { ... .message: '...' }) call.

The errs.create(parsed) does create the .message on the object, thus the errs.merge() has no effect, as far as .message is concerned.

I have pointed my package.json to "nano": "https://github.com/dscape/nano.git" before looking at this, and I don't see any other relevant branches in this repo, so I think I have the latest code.

Also, thanks for nano. :)

Owner

dscape commented Sep 4, 2012

You seem to be wrong, as per https://github.com/flatiron/errs/blob/d1756bbf93706ab6a56334e59f7b3e39b31730bb/lib/errs.js#L148-#L156

Still I went out of my away and created a test case for you, but I couldn't reproduce what your claim:

specify("shared_error:bad_delete", timeout, function (assert) {
  nano.db.destroy("say_wat_wat", function (error, response) {
    console.log(error);
    assert.ok(error, "There must be an error");
    assert.ok(error.message, "A note is given");
    assert.ok(error.message !== 'Unspecified error');
  });
});

Outputs:

$ SPECIFY_REPORTER=compact node tests/shared/error.js
{ [Error: missing]
  name: 'Error',
  scope: 'couch',
  status_code: 404,
  'status-code': 404,
  request: 
   { method: 'DELETE',
     headers: 
      { 'content-type': 'application/json',
        accept: 'application/json' },
     uri: 'http://localhost:5984/say_wat_wat_in_the_',
     jar: false,
     callback: [Function] },
  headers: 
   { date: 'Tue, 04 Sep 2012 17:33:41 GMT',
     'content-type': 'application/json',
     'cache-control': 'must-revalidate',
     'status-code': 404,
     uri: 'http://localhost:5984/say_wat_wat_in_the_' },
  errid: 'non_200',
  error: 'not_found',
  reason: 'missing',
  description: 'Unspecified error',
  stacktrace: 
   [ 'Error: Unspecified error',
     '    at Request._callback (/Users/dscape/Desktop/dev/nano/nano.js:290:39)',
     '    at Request.init.self.callback (/Users/dscape/Desktop/dev/nano/node_modules/request/main.js:120:22)',
     '    at Request.EventEmitter.emit (events.js:91:17)',
     '    at Request.<anonymous> (/Users/dscape/Desktop/dev/nano/node_modules/request/main.js:555:16)',
     '    at Request.EventEmitter.emit (events.js:88:17)',
     '    at IncomingMessage.Request.start.self.req.self.httpModule.request.buffer (/Users/dscape/Desktop/dev/nano/node_modules/request/main.js:517:14)',
     '    at IncomingMessage.EventEmitter.emit (events.js:115:20)',
     '    at IncomingMessage._emitEnd (http.js:366:10)',
     '    at HTTPParser.parserOnMessageComplete [as onMessageComplete] (http.js:149:23)' ] }
✔ /tests/shared/error.js ................................................ 30/30

Can you please submit a PR showing me this behavior? I can't act on something I can't see.

Owner

dscape commented Sep 4, 2012

Oh I see, you are talking about the stacktrace?

youurayy commented Sep 4, 2012

Hey, I'm talking about the message. Let me check this in more depth and I will get back to you (with a PR if this is really a problem). Thanks!

youurayy commented Sep 4, 2012

Actually yeah - it's "Unspecified Error" in the stack trace. For some reason that's what I'm getting on my project when I test with Mocha.

Would you see it feasible to to change it so that it's the correct message also in the stack trace?

dscape added a commit that referenced this issue Sep 4, 2012

dscape added a commit that referenced this issue Sep 4, 2012

[dist] 3.3.1
* Better error description as per #93
Owner

dscape commented Sep 4, 2012

Use specify? :P

youurayy commented Sep 4, 2012

Thanks man.

I'll have a look at specify for my next project. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment