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

feat: added additionalData field to Rule class #752

Merged
merged 3 commits into from
Apr 12, 2023

Conversation

drudrum
Copy link
Contributor

@drudrum drudrum commented Mar 29, 2023

This PR will add prop 'origin' to RuleClass. This prop will point to original RawRule.
It will give great abilities for debug and other.

Conversation saved at this PR. Description was changed.

@codecov-commenter
Copy link

Codecov Report

Merging #752 (1168f55) into master (c795e8b) will increase coverage by 0.34%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #752      +/-   ##
==========================================
+ Coverage   94.81%   95.16%   +0.34%     
==========================================
  Files          34       34              
  Lines         733      724       -9     
  Branches      174      170       -4     
==========================================
- Hits          695      689       -6     
  Misses         18       18              
+ Partials       20       17       -3     
Impacted Files Coverage Δ
packages/casl-ability/src/Rule.ts 89.18% <100.00%> (+0.30%) ⬆️

... and 8 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@stalniy
Copy link
Owner

stalniy commented Apr 3, 2023

sorry but I do not understand the usage, also you can store additionalData outside of rule using WeakMap or Map

@drudrum
Copy link
Contributor Author

drudrum commented Apr 10, 2023

Separate object with extra info is not comfortable. (And i can't understand how we can match rule with source rule without any id)
We want store little bit more debug info at each rule(we have a lot of rules) to identify rule source.
Also we need store some info to init new resource. Logically will be more correct to store this info at rule that allow this action.

For example if we will add this free prop we can create rule like this:

{
  subject: 'Article',
  action: 'create',
  additionalData: {
    ruleId: 'id1',
    initialProps: { badgeColor: 'red' },
    description: 'Articles with red badge',
    ruleSource: 'constant rules'
  }
}

this is optional prop, it don't break existing behavior. It just give freedom and ability to investigate issues.

We can't use WeakMap because RuleClass creating inside PureAbility. We give RawRules to constructor, and get ability with rules(RuleClass). It's different instances.

@stalniy
Copy link
Owner

stalniy commented Apr 12, 2023

I see. Thanks for the explanation, I plan some day to add ability to trace decision. Something like this

const tracer = new DecisionTracer()

const ability = new Ability([], { tracer })

const isAllowed = ability.can(....);
console.log(tracer.explainLatestCall())
/*
  [
     {
        testedRule: rule,
        conditionsMatched: true | false,
        conditions: [
             {
                 conditionOperator: 'and',
                 matched: true | false,
                 condition: /* raw condition */
                 children: [
                    { 
                         conditionOperator: 'eq',
                         matched: true | false,
                         condition: /* raw condition */
                    }
                 ]
             }
        ],
        matchedField: null | string
     },
     //...
  ]
*/

@stalniy
Copy link
Owner

stalniy commented Apr 12, 2023

I don't like the idea of storing unstructured data inside Rule, later anybody will want to put there anything and I will need to support many other cases later or keep that unstructured data forever :)

what I can do is to add public reference to origin rawRule (e.g., rule.origin). So, then you can use Map/WeakMap and store anything you want outside of casl

@drudrum
Copy link
Contributor Author

drudrum commented Apr 12, 2023

It's acceptable solution. I will change this PR.

@drudrum
Copy link
Contributor Author

drudrum commented Apr 12, 2023

Tracer will be useful for me. If you create it i will use it also.

@stalniy stalniy merged commit 073d355 into stalniy:master Apr 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants