un needed try/catch blocks

[Calvin Metcalf]

the try catch blocks in security here https://bazaar.launchpad.net/~marten-de-vries/python-pouchdb/0.x/view/head:/js/pouchdb-security/index.js#L31 and here https://bazaar.launchpad.net/~marten-de-vries/python-pouchdb/0.x/view/head:/js/pouchdb-security/index.js#L40 simply listen for errors that they then rethrow.

[Marten de Vries (marten-de-vries)]

The error messages are different from the ones provided by the pouchdb-wrappers module. They're a bit more specific (security already installed vs. random method xyz is already installed on obj qrs, or something similar).

[Calvin Metcalf (calvin-metcalf)]

yes but it doesn't actually check if that is the reason it is getting the error, for all it knows a different plugin is causing that error

[Marten de Vries (marten-de-vries)]

The amount of errors wrappers.installWrapperMethods can throw (same for the static variant) is quite limited. It's a function that doesn't call anything other than a few helper functions inside pouchdb-wrappers itself, and all the code it consists of is 100% covered by the tests. I'm pretty confident that when it throws, that's the one. But, for future debugability, I'll add a check for err.name === "Error" (or something more specific). That should be enough to stop any other error the function can possibly produce from behing caught.

[Marten de Vries (marten-de-vries)]

And if I'm doing that, can as well use this bug to make sure I make the same changes in pouchdb-validation and probably also pouchdb-auth and a few others.

[Calvin Metcalf (calvin-metcalf)]

I guess the main thing was if it's obvious it's always this error, why
isn't the original one more specific? (also fyi try/catches aren't free in
chrome and node.js else otherwise I wouldn't care)

On Tue, Sep 16, 2014 at 1:21 PM, Marten de Vries <email address hidden> wrote:

> And if I'm doing that, can as well use this bug to make sure I make the
> same changes in pouchdb-validation and probably also pouchdb-auth and a
> few others.
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1370115
>
> Title:
> un needed try/catch blocks
>
> Status in Python-PouchDB:
> Triaged
>
> Bug description:
> the try catch blocks in security here https://bazaar.launchpad.net
> /~marten-de-vries/python-pouchdb/0.x/view/head:/js/pouchdb-
> security/index.js#L31 and here https://bazaar.launchpad.net/~marten-
> de-vries/python-pouchdb/0.x/view/head:/js/pouchdb-
> security/index.js#L40 simply listen for errors that they then rethrow.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/python-pouchdb/+bug/1370115/+subscriptions
>

--
-Calvin W. Metcalf

[Marten de Vries (marten-de-vries)]

Well, if you're the pouchdb-security user, which message do you think more clearly describes what's going on when you're calling installSecurityMethods() twice?

- Security methods already installed
- Wrapper method for .get() already installed: function () {/* body of handler - i.e. some security logic here */}

The second requires inside knowledge of pouchdb-wrappers/pouchdb-security, which is why I change the error message. The second one is great for debugging pouchdb-wrappers, but not so much as a high-level error when you don't actually need to know pouchdb-wrappers is used internally.

As for speed: You'd need a situation where you need to wrap dbs a lot of times. I think you'd quickly start caching db objects - like express-pouchdb does. Once wrapped, it's not necessary to call it again. Since I don't see this end up in a hot code path anytime soon, I prefer the clearer error message.

[Calvin Metcalf (calvin-metcalf)]

the other thing to keep in mind is the call stack of the error, when you re
throw it you loose it but if you didn't then the stack trace would zero in
to where it was, maybe just change the message of the original error

On Wed, Sep 17, 2014 at 5:11 AM, Marten de Vries <email address hidden> wrote:

> Well, if you're the pouchdb-security user, which message do you think
> more clearly describes what's going on when you're calling
> installSecurityMethods() twice?
>
> - Security methods already installed
> - Wrapper method for .get() already installed: function () {/* body of
> handler - i.e. some security logic here */}
>
> The second requires inside knowledge of pouchdb-wrappers/pouchdb-
> security, which is why I change the error message. The second one is
> great for debugging pouchdb-wrappers, but not so much as a high-level
> error when you don't actually need to know pouchdb-wrappers is used
> internally.
>
> As for speed: You'd need a situation where you need to wrap dbs a lot of
> times. I think you'd quickly start caching db objects - like express-
> pouchdb does. Once wrapped, it's not necessary to call it again. Since I
> don't see this end up in a hot code path anytime soon, I prefer the
> clearer error message.
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1370115
>
> Title:
> un needed try/catch blocks
>
> Status in Python-PouchDB:
> Triaged
>
> Bug description:
> the try catch blocks in security here https://bazaar.launchpad.net
> /~marten-de-vries/python-pouchdb/0.x/view/head:/js/pouchdb-
> security/index.js#L31 and here https://bazaar.launchpad.net/~marten-
> de-vries/python-pouchdb/0.x/view/head:/js/pouchdb-
> security/index.js#L40 simply listen for errors that they then rethrow.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/python-pouchdb/+bug/1370115/+subscriptions
>

--
-Calvin W. Metcalf

[Marten de Vries (marten-de-vries)]

Good point. Setting err.message and then throw(err);-ing again instead of creating a new Error object should be no problem. Result would be something like:

try {
  //method call
} catch (err) {
  if (err.name === "Error" /* && maybe another check, need to check the source for that */) {
    err.message = "Security methods already installed"; //or something similar
  }
  throw(err);
}

Is that ok with you considering the above?

[Calvin Metcalf (calvin-metcalf)]

Your probably going to want to do a regex on the message or you could
consider it a range error (it's out side the range of times it can be
called)
On Sep 17, 2014 7:31 AM, "Marten de Vries" <email address hidden> wrote:

> Good point. Setting err.message and then throw(err);-ing again instead
> of creating a new Error object should be no problem. Result would be
> something like:
>
> try {
> //method call
> } catch (err) {
> if (err.name === "Error" /* && maybe another check, need to check the
> source for that */) {
> err.message = "Security methods already installed"; //or something
> similar
> }
> throw(err);
> }
>
> Is that ok with you considering the above?
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1370115
>
> Title:
> un needed try/catch blocks
>
> Status in Python-PouchDB:
> Triaged
>
> Bug description:
> the try catch blocks in security here https://bazaar.launchpad.net
> /~marten-de-vries/python-pouchdb/0.x/view/head:/js/pouchdb-
> security/index.js#L31 and here https://bazaar.launchpad.net/~marten-
> de-vries/python-pouchdb/0.x/view/head:/js/pouchdb-
> security/index.js#L40 simply listen for errors that they then rethrow.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/python-pouchdb/+bug/1370115/+subscriptions
>

[Marten de Vries (marten-de-vries)]

A regex is probably the easiest way of doing the '&& maybe another check' part. A RangeError would've been better to use in hindsight, but not sure it's worth the api change it is now. I'll decide on that when doing the final fix.

[Marten de Vries (marten-de-vries)]

Moved to: https://github.com/pouchdb/pouchdb-security/issues/1