Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conditional rules & forbidden reasons #45

Closed
9 tasks done
stalniy opened this issue Mar 21, 2018 · 4 comments
Closed
9 tasks done

Conditional rules & forbidden reasons #45

stalniy opened this issue Mar 21, 2018 · 4 comments

Comments

@stalniy
Copy link
Owner

stalniy commented Mar 21, 2018

  • implement relevantRuleFor(action, subject, field)
  • add reason field to rules
  • add reason to pack/unpackRules functions
  • add forbidden action and subject to thrown error
  • throwUnlessCan should now throw exception with message of failed rule and use default in other cases
  • add because to RuleBuilder
  • add tests
  • add typings
  • add documentation

Sometimes you may want to define rules with possibility to specify when they should be applied and return more clear error message when ability.can fails. To do that we need to introduce a concept of conditional rule (rule with dynamic inverted property).

Lets consider example:

const ability = new Ability([])

ability.can('read', 'Post') // false

This example returns false but Ability doesn't know why (it just doesn't have defined rules) because it relates to business logic. But for the end user we would like to return something like You don't have a valid subscription or You exceeded your limits. To do so, ability explicitly should know why and what is going on.

To help ability to understand we could extend AbilityBuilder interface in 2 ways:

  • with help of cannot rules
const ability = AbilityBuilder.define((can, cannot) => {
  if (!user.haveActiveSubscription()) {
    cannot('post', 'Comment').because('has no active subscription')
  } else if (!loggedInUser.uploadsToday()) {
    cannot('post', 'Comment').because('exceeded amount of requests')
  } else {
    can('post', 'Comment', { user_id: user.id })
  }
})
  • with help of can rules:
AbilityBuilder.define((can, cannot) => {
  can('post', 'Comment')
    .when(() => loggedInUser.hasActiveSubscription(), { message: 'has no active subscription' })
    .when(() => loggedInUser.uploadsToday(), { message: 'exceeded amount of X comments per day' })
})

So, need to decided on the best approach which can be used here, receive feedback and implement this functionality

@stalniy
Copy link
Owner Author

stalniy commented Apr 10, 2018

It’s much easier to implement cannot.because than can.when.

The pros of first one:

  • easy, need to add reason property to inverted rules and we are done
  • can use language constructions

Cons:

  • user needs to call required functions in DSL in order to build ability even if it wasn’t used (solution: caching might help)
  • it may be hard to combine can & cannot rules, especially with big amount of ifs and checks

The pros of 2nd:

  • easier to combine direct rules, as checks are part of DSL
  • can evaluate when conditions lazily only in case when rule I used

Cons:

  • adds complexity into can & cannot checks. Sometimes it may require to return Promise from that methods (basically rule becomes inverted or direct based on evaluated conditions)
  • requires to add the same reason property to inverted rules
  • requires to add another async function to get the current state of rules
  • can checks now depend not only on the list of specified rules but also on evaluated conditions.

@stalniy
Copy link
Owner Author

stalniy commented Apr 10, 2018

After thinking a bit it becomes clear that combination of rules using can.when even harder than with cannot.because because these types of rules will be either inverted or direct.

So, the only benefit is that conditions will be evaluated lazily. This is better for performance but introduces few challenges: async can & cannot, another way to get current state of abilities.

@stalniy
Copy link
Owner Author

stalniy commented Apr 10, 2018

From one point of view, lazy evaluation looks cool but that also means, each time you call can these conditions will be evaluated what may be not what you eventually want. So, you will want to add caching mechanisms.

In that case I don’t see the reason why should we complicate things. The cannot.because already will work as caching mechanism for rules and if you want to cache your function invocation, then you can do this whatever way is better for you.

That means cannot.because wins and will be implemented in the next releases.

@stalniy stalniy closed this as completed Apr 10, 2018
@stalniy stalniy reopened this Apr 10, 2018
@stalniy
Copy link
Owner Author

stalniy commented Apr 10, 2018

Requirements:

  • add because to RuleBuilder
  • add reason property to RawRule & Rule
  • add possibility to find (or store last) failed rule (relevantRuleFor(action, subject, field))
  • throwUnlessCan should now throw exception with message of failed rule and use default in other cases
  • add forbidden action and subject to thrown error

@stalniy stalniy added this to the 2.1 milestone Apr 16, 2018
stalniy added a commit that referenced this issue May 4, 2018
Now, it's possible to specify reason of why rule was added

Relates to #45
@stalniy stalniy closed this as completed in edf5755 May 4, 2018
stalniy added a commit that referenced this issue May 4, 2018
stalniy added a commit that referenced this issue May 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants