FirebaseExtended / firebase-queue

MIT License
787 stars 108 forks source link

add error_stack on reject also reject error not string #21

Closed dreadjr closed 9 years ago

dreadjr commented 9 years ago

Wanted to start a discussion about 2 things,

  1. adding error_stack
  2. reject new Error() vs string
googlebot commented 9 years ago

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


dreadjr commented 9 years ago

I signed it!

googlebot commented 9 years ago

CLAs look good, thanks!

cbraynor commented 9 years ago

I'll take a look at this today - initially I used strings because they are easiest to represent in Firebase, and perhaps we don't want to coerce every rejection with a string into an Error object, but a stack trace would be a useful addition

dreadjr commented 9 years ago

Ok cool, yeah i wasn't sure about the error stack as far as privacy/security is concerned either. It might need to go in an "errors" bucket and just reference it. I was thinking could just do a JSON.stringify as well.

cbraynor commented 9 years ago

In using the queue, I've tended to lock down the /tasks location to write only (and create only) for clients that push items onto the queue, and only allowed the trusted server processes read access. This means that for me, adding the stack trace to the task is a non-issue.

I'm curious whether that's a pattern other people are using, or whether read access is indeed required in some use-cases? If read access is required, then you're right that adding stack traces could be a security concern

dreadjr commented 9 years ago

Yeah true. I was thinking too of converting stack trace into Jain so it could be saved to firebase more easily.

cbraynor commented 9 years ago

I included these changes in #24 with a few minor adjustments. The major changes were

  1. We don't specifically coerce all errors to Error objects - if they're strings then they behave the same as they do now (without a stack trace)
  2. There's now a suppressStack option for the queue that prevents stack traces from ending up in the Firebase

Take a look and let me know if there's something I missed. Let's move the conversation there