Don't throw exception if 'unsign' fails, just return data about buddy-sign HOT 5 CLOSED

There are few different approaches for report errors as return value instead exceptions; either/try monad, [result, error] vector, return exception as is if error is happens, are some examples of that.
Choise one will make the code opinionated on one concrete solution and I don't want to put opinions in that.

Create a simple wrapper on buddy-sign functions for the appropriate call signature (and return value) adapted to your application needs is very simple and is the recommended approach in my opinion instead of exposing additional and opinionated solution from the buddy-sign side.

If you think that I'm wrong, please feel free to expose more arguments about your proposal and I'm gladly try to reconsider may posture.

from buddy-sign.

Hi,

thank you for considering my feedback.

I am trying to argue that an unsuccessful unsign due to an invalid signature or expired token (in JWT) is just another result, there is nothing special about them. Why not return that result the same way you return the result of an unsign where the signature happened to be valid. These are all results that caller should expect and should be able to handle them.

You will have to choose a particular representation for these non-success results either way, why not just use the simpler way to communicate them to the caller as return values rather than through the more complicated and less efficient exception machinery.
Wrapping all other results other than the valid signature into an exception makes your code actually more opinionated, as you made two decisions instead of one.

To reiterate on my earlier point, exceptions IMHO should be reserved to signal unexpected/exceptional situations/errors where the caller may not be in the position to handle them and where exceptions are more convenient mechanism to propagate these errors higher up the caller chain where they can be appropriately handled without requiring the levels in between to know about every possible downstream exception.

With that said, I think buddy is a fantastic and very high quality addition to the Clojure open-source ecosystem.

from buddy-sign.

I mostly agree with your arguments. But I'm not clearly understand some parts.

Are you suggesting return the same result in successful and unsuccessful validation of the token?
In this case, this seems like a bad idea because it is completely counterintuitive and not evident.

Also, unfortunately to our opinion about exceptions, they seems like the most used way to handle this kind of stuff in the clojure ecosystem and this is the reason of using them as default approach.

In order to expose additional api that return errors as values, a proper way to signal the validation error should be choiced. Returning the same value for success and validation error is not an option. As I said previously is very confusing and not evident to the final user. Additionally, not all validation errors can return the data (on JWE as example valid tokens but encrypted with different key, will cause a validation error but the data can't be retrieved).

On the end, I don't know that abstraction is better for create the new functions that return validation errors as value. I think that vector of [result, error] is the proper approach for it, but I'm not pretty sure...

from buddy-sign.

Thanks,

that is good additional insight. Having a fresh look at the public API, it occurred to me that unsign actually does two things

1. check signature of unsigned data
2. remove signature and return the naked data

This is perfectly fine, as you may do it in this order and step 2. is likely to be conditioned on step 1. 99% of the time. I'd like to have those two operations a la carte as well as the convenience method of having them as one.

I think in this case using exceptions seems semantically correct. The way you use it it almost suggests that good data with invalid signature is invalid data, i.e an invalid argument to unsign, which is widely handled via throwing an exception (IllegalArgument or whatever)....

So if I had 1) and 2) as separate methods, I think my argument for returning all results as return values of the function would be stronger. There is still the efficiency argument though, I think, but I understand it is far less compelling.

Now as to how should you return successful results and errors, you are right, you need to wrap them into a container that indicates to the caller all useful outcomes of an unsign.

I don't necessarily agree that in the Clojure ecosystem it is uniformly accepted that one should use exceptions in this case. The Prismatic (now Plumatic) library uses exceptions in their validate function, whereas its semantic equivalent conform in the upcoming Clojure.Spec library returns an value indicating an error.

There are different approaches to this, as you noted, using [:ok {..naked data..}] vs. [:error {some data describing the error}] is perfectly idiomatic and play well with destructuring. This is also the preferred way in ML languages.

Finally, the matter of evidence. I think it depends on ones previous experiences and exposure to other libraries, common practices etc. Everyone will read the docstring at least once (hopefully). :)

from buddy-sign.

On the end, I think that define own functions that adapts the buddy-sign to the needs of the concrete application is very very easy. And it there are a lot of different ways to represent the error as value, I prefer leave it to the user.

This is a very quick example that the user can do in the application code to adapt buddy-sign function for return errors as value in form of a vector ([err result]):

(defn sign*
  [& args]
  (try
    [nil (apply sign args)]
    (catch clojure.lang.ExceptionInfo e
      (let [error (assoc (ex-data e) :message (.getMessage e))]
        [error nil]))))

(defn unsign*
  [& args]
  (try
    [nil (apply unsign args)]
    (catch clojure.lang.ExceptionInfo e
      (let [error (assoc (ex-data e) :message (.getMessage e))]
        [error nil]))))

Other people may want to use some kind of Either or Try monad (see more in funcool/cats library) and I can't offer all posible variants for that.

Thank you very much for your feedback and for this issue ;)

from buddy-sign.