From cc7091e265a1d774153895835a549113c86127dc Mon Sep 17 00:00:00 2001 From: Sergii Date: Thu, 16 Jul 2020 12:29:24 +0300 Subject: [PATCH] refactor(ability): removes deprecated types and fields Removes `RuleAnalyzer` and moves its logic under `Rule` and `RuleIndex` Relates to #355 BREAKING CHANGE: removes deprecated options and types: * `AbilityOptions['subjectName']` has been removed, use `detectSubjectType` instead * `LegacyClaimRawRule` and `LegacySubjectRawRule` are both removed, so you are no longer allowed to use `actions` in rule definition, use `action` property instead * `Ability` throws an error if you specify a rule with property `field` to be an empty array * `Ability` no longer warns about using only inverted rules. This may be done by intention, so now it's left up to developer to decide whether it's fine or not --- packages/casl-ability/spec/ability.spec.js | 20 +--------- packages/casl-ability/spec/builder.spec.js | 4 +- packages/casl-ability/src/AbilityBuilder.ts | 8 ++-- packages/casl-ability/src/RawRule.ts | 22 +---------- packages/casl-ability/src/Rule.ts | 37 ++++++++++++------- packages/casl-ability/src/RuleIndex.ts | 26 +++++-------- packages/casl-ability/src/RulesAnalyzer.ts | 41 --------------------- 7 files changed, 43 insertions(+), 115 deletions(-) delete mode 100644 packages/casl-ability/src/RulesAnalyzer.ts diff --git a/packages/casl-ability/spec/ability.spec.js b/packages/casl-ability/spec/ability.spec.js index eae595b48..adf398183 100755 --- a/packages/casl-ability/spec/ability.spec.js +++ b/packages/casl-ability/spec/ability.spec.js @@ -188,16 +188,6 @@ describe('Ability', () => { expect(secondSubscription).to.have.been.called() }) - it('warns if ability contains only inverted rules', () => { - spy.on(console, 'warn', () => {}) - ability.update([{ inverted: true, action: 'read', subject: 'Post' }]) - - // eslint-disable-next-line - expect(console.warn).to.have.been.called() - - spy.restore(console, 'warn') - }) - function setupListenerChangesInListener() { const unsubscribe = spy(ability.on('update', function listen() { unsubscribe() @@ -442,14 +432,8 @@ describe('Ability', () => { expect(() => new PureAbility(rules)).to.throw(/"fieldMatcher" option/) }) - it('warns if there is a rule with "fields" property to be an empty array', () => { - spy.on(console, 'warn', () => {}) - ability = defineAbility(can => can('read', 'Post', [])) - - // eslint-disable-next-line - expect(console.warn).to.have.been.called() - - spy.restore(console, 'warn') + it('throws if there is a rule with "fields" property to be an empty array', () => { + expect(() => defineAbility(can => can('read', 'Post', []))).to.throw(/`rawRule.fields` cannot be an empty array/) }) describe('when field patterns', () => { diff --git a/packages/casl-ability/spec/builder.spec.js b/packages/casl-ability/spec/builder.spec.js index 1d369c40e..3b307aa37 100644 --- a/packages/casl-ability/spec/builder.spec.js +++ b/packages/casl-ability/spec/builder.spec.js @@ -137,8 +137,8 @@ describe('AbilityBuilder', () => { }) it('accepts options for `Ability` instance as the 1st parameter', () => { - const subjectName = subject => typeof subject === 'string' ? subject : subject.ModelName - const ability = defineAbility({ subjectName }, (can) => { + const detectSubjectType = subject => typeof subject === 'string' ? subject : subject.ModelName + const ability = defineAbility({ detectSubjectType }, (can) => { can('read', 'Book') }) diff --git a/packages/casl-ability/src/AbilityBuilder.ts b/packages/casl-ability/src/AbilityBuilder.ts index 0d3ad5592..0954369cb 100755 --- a/packages/casl-ability/src/AbilityBuilder.ts +++ b/packages/casl-ability/src/AbilityBuilder.ts @@ -12,14 +12,14 @@ import { import { RawRule } from './RawRule'; class RuleBuilder> { - public rule!: T; + public _rule!: T; constructor(rule: T) { - this.rule = rule; + this._rule = rule; } because(reason: string): this { - this.rule.reason = reason; + this._rule.reason = reason; return this; } } @@ -88,7 +88,7 @@ export class AbilityBuilder { conditions?: Generics['conditions'], ): RuleBuilder> { const builder = (this as any).can(action, subject, conditionsOrFields, conditions); - builder.rule.inverted = true; + builder._rule.inverted = true; return builder; } diff --git a/packages/casl-ability/src/RawRule.ts b/packages/casl-ability/src/RawRule.ts index c910f76a6..0dfd7c276 100644 --- a/packages/casl-ability/src/RawRule.ts +++ b/packages/casl-ability/src/RawRule.ts @@ -19,26 +19,8 @@ export interface SubjectRawRule exte subject: S | S[] } -export interface LegacyClaimRawRule extends BaseRawRule { - /** @deprecated use "action" field instead */ - actions: A | A[] - subject?: undefined -} - -export interface LegacySubjectRawRule - extends BaseRawRule { - /** @deprecated use "action" field instead */ - actions: A | A[] - subject: S | S[] -} - -type DefineRule< - T extends AbilityTypes = AbilityTupleType, - C = unknown, - Else = ClaimRawRule> | LegacyClaimRawRule> -> = T extends AbilityTupleType - ? SubjectRawRule | LegacySubjectRawRule - : Else; +type DefineRule>> = + T extends AbilityTupleType ? SubjectRawRule : Else; export type RawRule = DefineRule; export type RawRuleFrom = RawRule, C>; diff --git a/packages/casl-ability/src/Rule.ts b/packages/casl-ability/src/Rule.ts index 2103297d4..be63d2af8 100644 --- a/packages/casl-ability/src/Rule.ts +++ b/packages/casl-ability/src/Rule.ts @@ -7,10 +7,24 @@ import { ToAbilityTypes, Normalize } from './types'; -import { RawRule } from './RawRule'; +import { RawRule, RawRuleFrom } from './RawRule'; type Tuple = Normalize>; +function validate(rule: RawRuleFrom, options: RuleOptions) { + if (Array.isArray(rule.fields) && !rule.fields.length) { + throw new Error('`rawRule.fields` cannot be an empty array. https://bit.ly/390miLa'); + } + + if (rule.fields && !options.fieldMatcher) { + throw new Error('You need to pass "fieldMatcher" option in order to restrict access by fields'); + } + + if (rule.conditions && !options.conditionsMatcher) { + throw new Error('You need to pass "conditionsMatcher" option in order to restrict access by conditions'); + } +} + export class Rule { private readonly _matchConditions: MatchConditions | undefined; private readonly _matchField: MatchField | undefined; @@ -22,26 +36,21 @@ export class Rule { public readonly reason!: string | undefined; constructor(rule: RawRule, C>, options: RuleOptions) { - this.action = options.resolveAction((rule as any).actions || (rule as any).action); + validate(rule, options); + + this.action = options.resolveAction(rule.action); this.subject = rule.subject; this.inverted = !!rule.inverted; this.conditions = rule.conditions; this.reason = rule.reason; - this.fields = !rule.fields || rule.fields.length === 0 - ? undefined - : wrapArray(rule.fields); - - if ('actions' in rule) { - // eslint-disable-next-line - console.warn('Rule `actions` field is deprecated. Use `action` field instead'); - } + this.fields = rule.fields ? wrapArray(rule.fields) : undefined; - if (this.conditions && options.conditionsMatcher) { - this._matchConditions = options.conditionsMatcher(this.conditions); + if (this.conditions) { + this._matchConditions = options.conditionsMatcher!(this.conditions); } - if (this.fields && options.fieldMatcher) { - this._matchField = options.fieldMatcher(this.fields); + if (this.fields) { + this._matchField = options.fieldMatcher!(this.fields); } } diff --git a/packages/casl-ability/src/RuleIndex.ts b/packages/casl-ability/src/RuleIndex.ts index 2e6a6a6e5..05515d663 100644 --- a/packages/casl-ability/src/RuleIndex.ts +++ b/packages/casl-ability/src/RuleIndex.ts @@ -11,13 +11,10 @@ import { RuleOptions, } from './types'; import { wrapArray, detectSubjectType, identity } from './utils'; -import RulesAnalyzer from './RulesAnalyzer'; type AnyRuleIndex = RuleIndex; export interface RuleIndexOptions { - /** @deprecated use "detectSubjectType" option instead */ - subjectName?: this['detectSubjectType'] detectSubjectType?: DetectSubjectType[1]> conditionsMatcher?: ConditionsMatcher fieldMatcher?: FieldMatcher @@ -86,7 +83,7 @@ export class RuleIndex this._rules }); this.update(rules); @@ -101,31 +98,22 @@ export class RuleIndex(rules.length > 0); - const indexedRules = this._buildIndexFor(rules, analyser._analyze); - - analyser._validate(this._ruleOptions); - this._indexedRules = indexedRules; + this._indexedRules = this._buildIndexFor(rules); this._rules = rules; - this._hasPerFieldRules = analyser._hasPerFieldRules; this._emit('updated', event); return this; } - private _buildIndexFor( - rawRules: RawRuleFrom[], - iterator: RuleIterator = identity - ) { + private _buildIndexFor(rawRules: RawRuleFrom[]) { const indexedRules: IndexTree = Object.create(null); for (let i = 0; i < rawRules.length; i++) { - iterator(rawRules[i], i); const rule = new Rule(rawRules[i], this._ruleOptions); const priority = rawRules.length - i - 1; const actions = wrapArray(rule.action); const subjects = wrapArray(rule.subject); + this._analyze(rule); for (let k = 0; k < subjects.length; k++) { const subject = this.detectSubjectType(subjects[k]); @@ -142,6 +130,12 @@ export class RuleIndex) { + if (!this._hasPerFieldRules && rule.fields) { + this._hasPerFieldRules = true; + } + } + possibleRulesFor(...args: CanParameters) { const [action, subject] = args; const subjectName = this.detectSubjectType(subject); diff --git a/packages/casl-ability/src/RulesAnalyzer.ts b/packages/casl-ability/src/RulesAnalyzer.ts deleted file mode 100644 index 87768520f..000000000 --- a/packages/casl-ability/src/RulesAnalyzer.ts +++ /dev/null @@ -1,41 +0,0 @@ -import { RawRuleFrom } from './RawRule'; -import { Abilities, RuleOptions } from './types'; - -export default class RulesAnalyzer { - public _isAllInverted: boolean; - public _hasPerFieldRules: boolean = false; - public _hasRuleWithEmptyFields: boolean = false; - - constructor(hasRules: boolean) { - this._isAllInverted = hasRules; - this._analyze = this._analyze.bind(this); - } - - _analyze({ fields, inverted }: RawRuleFrom) { - this._isAllInverted = this._isAllInverted && !!inverted; - - if (!this._hasRuleWithEmptyFields && Array.isArray(fields) && !fields.length) { - this._hasRuleWithEmptyFields = true; - } - - if (!this._hasPerFieldRules && fields && fields.length) { - this._hasPerFieldRules = true; - } - } - - _validate(ruleOptions: RuleOptions) { - if (this._isAllInverted) { - // eslint-disable-next-line - console.warn('Make sure your ability has direct rules, not only inverted ones. Otherwise `ability.can` will always return `false`.'); - } - - if (this._hasRuleWithEmptyFields) { - // eslint-disable-next-line - console.warn('[error in next major version]: There are rules with `fields` property being an empty array. This results in the same as not passing fields at all. Make sure to remove empty array or pass fields.'); - } - - if (this._hasPerFieldRules && !ruleOptions.fieldMatcher) { - throw new Error('Field level restrictions are ignored because "fieldMatcher" option is not specified. Did you unintentionally used PureAbility instead of Ability?'); - } - } -}