Evaluate rule checks lazily

[adriansalamon]

When having longer authorization rules that perform more complex logic, we don't have to evaluate all of the rule checks if we find one that is matching. In this example, if each allow rule performs a database lookup, we previously needed to perform 3 database lookups even if the first allow role: admin rule matched. This can obviously be optimized.

object :articles do
    action :edit do
      allow role: :admin
      allow :own_resource
      allow :group_resource
   end
end

This PR makes makes rule evaluation lazy, ie. it will try each rule in sequence, and if one matches, it will not try to match with any more.

Checklist

• I added tests that cover my proposed changes.
• I updated the documentation related to my changes (if applicable).

[woylie]

Hey @adriansalamon, thank you! Sorry for taking a bit to comment. The formatter is complain, could you run mix format and commit the changes?

[woylie]

Could you add back the two lines you removed here? I think we may need them. There's a comment in line 127 - I don't see that warning when running Dialyzer in the library repo, but I've seen it under certain conditions when used in applications.

[bernardd]

Hi - I'd love to resurrect this and get it merged - it's solved a couple of performance problems we were having (that are not the fault of let_me per se, but more the specific way I'm using it - long story and not super relevant right now).

In terms of the two lines that were removed in builder.ex, I don't think adding them back in is the right solution - it throws some fairly verbose compiler warnings if you do. I think, instead, that altering the macro below it like this:

    quote do
      @dialyzer [{:nowarn_function, [authorize?: 4]}, :no_match]
      def authorize?(unquote(rule_name), subject, object, opts) do
        unquote(pre_hook_calls)
        unquote(combined_condition)
      end
    end

is the right solution (which is what I've done on my branch). Because it's a macro, sometimes it will generate redundant/pointless code that trips dialyzer, so it seems legitimate to supress that particular warning in this case.

Help yourself to my main branch at https://github.com/circles-learning-labs/let_me which has this change, if you like.

[bernardd]

An update to this: I discovered that some cases were still not being fully short-circuited due to the way compound conditions were being evaluated. I've fixed it and added a test case on my branch linked above. Let me know if you'd just like me to open a separate PR with my work.