Skip to content

Commit

Permalink
feat: allow partial subjects
Browse files Browse the repository at this point in the history
- Throw `Feature::Subject::NotApplicable` when an invalid subject is
  specified.  This allows for better differentiation of error cases.

Given a feature specified as:

```yaml
user:
  id: 1
```

and a feature check like `Feature.enabled?(:check, user_id: 1)` then
when the feature was modified to include *additional* constraints like

```yaml
user:
  id: 1
age:
  gt: 3
```

every caller would immediately fail with `Feature::Subject::Invalid`.
In production this creates an intensely tight coupling between the
caller and feature data source that is effectively impossible to safely
modify.

By allowing *partial subject checks* the coupling is removed, allowing
for data source additions without active caller errors.
  • Loading branch information
lanej committed Apr 13, 2024
1 parent b5668b0 commit e09cc6f
Show file tree
Hide file tree
Showing 6 changed files with 54 additions and 33 deletions.
2 changes: 1 addition & 1 deletion Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,5 @@ source 'https://rubygems.org'
gemspec

group :test do
gem "pry"
gem "pry-byebug"
end
31 changes: 18 additions & 13 deletions lib/toggles/feature/permissions.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,25 +18,30 @@ def subjects
end

def valid_for?(entities)
subject_difference = Feature::Subject.difference(subjects, entities.keys)
if subject_difference.any?
raise Feature::Subject::Invalid, subject_difference
invalid_subjects = entities.keys - subjects
if invalid_subjects.any?
raise Feature::Subject::NotApplicable, invalid_subjects
end

rules.all? do |name, rule|
entity = entities[name.to_sym]
subject_rules = rules.select { |name, _| entities.key?(name.to_sym) }

return false if entity.nil?
raise Feature::Subject::Empty if subject_rules.empty?

if entity.class.ancestors.find { |ancestor| ancestor == Comparable }
entity = OpenStruct.new(name => entity)
rule = { name => rule }
end
subject_rules
.all? do |name, rule|
entity = entities[name.to_sym]

return false if entity.nil?

rule.all? do |key, value|
Feature.operations.fetch(key.to_sym, Feature::Attribute).call(entity, key, value)
if entity.class.ancestors.find { |ancestor| ancestor == Comparable }
entity = OpenStruct.new(name => entity)
rule = { name => rule }
end

rule.all? do |key, value|
Feature.operations.fetch(key.to_sym, Feature::Attribute).call(entity, key, value)
end
end
end
end

def enabled_for?(subjects = {})
Expand Down
9 changes: 7 additions & 2 deletions lib/toggles/feature/subject.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,13 @@ def initialize(args)
end
end

def self.difference(subjects, others)
Set.new((subjects - others) + (others - subjects)).to_a
class NotApplicable < StandardError
def initialize(args)
super("Subjects not applicable for permissions: #{args}")
end
end

class Empty < StandardError
end
end
end
10 changes: 5 additions & 5 deletions spec/toggles/feature/acceptance/multiple_subjects_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@
user: double(id: 1, logged_in?: false), widget: double(id: 2))).to eq false
expect(Feature::MultipleSubjects.enabled_for?(
user: double(id: 1, logged_in?: true), widget: double(id: 3))).to eq false
end

specify "invalid permissions" do
expect { Feature::MultipleSubjects.enabled_for?(widget: double) }.
to raise_error Feature::Subject::Invalid,
"Invalid or missing subjects for permissions: [:user]"
expect(Feature::MultipleSubjects.enabled_for?(
user: double(id: 1, logged_in?: true))).to eq true
expect(Feature::MultipleSubjects.enabled_for?(
widget: double(id: 3)
)).to eq false
end
end
27 changes: 23 additions & 4 deletions spec/toggles/feature/permissions_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,29 @@
user: double(id: 2, logged_in?: true))).to eq false
end

specify "invalid subjects" do
expect { subject.valid_for?(user: double) }.
to raise_error Feature::Subject::Invalid,
"Invalid or missing subjects for permissions: [:widget]"
specify 'partial subjects are acceptable', :aggregate_failures do
# partial subjects that match
expect(subject.valid_for?(user: double(id: 1, logged_in?: true))).to eq true
expect(subject.valid_for?(widget: double(id: 2))).to eq true

# partial subjects that do not match
expect(subject.valid_for?(widget: double(id: 3))).to eq false
expect(subject.valid_for?(user: double(id: 2, logged_in?: true))).to eq false
end

specify "unspecified subjects are not acceptable" do
expect { subject.valid_for?(foo: double) }.
to raise_error Feature::Subject::NotApplicable,
"Subjects not applicable for permissions: [:foo]"

# partial specification with an invalid subject is still an error
expect { subject.valid_for?(user: double(id: 1), foo: double) }.
to raise_error Feature::Subject::NotApplicable,
"Subjects not applicable for permissions: [:foo]"
end

specify "empty subjects are invalid" do
expect { subject.valid_for?({}) }.to raise_error Feature::Subject::Empty
end
end
end
8 changes: 0 additions & 8 deletions spec/toggles/feature/subject_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,4 @@

its(:message) { is_expected.to eq "Invalid or missing subjects for permissions: [:user]" }
end

describe "#difference" do
specify do
expect(Feature::Subject.difference([:foo, :bar], [:bar])).to eq [:foo]
expect(Feature::Subject.difference([:bar], [:foo, :bar])).to eq [:foo]
expect(Feature::Subject.difference([:bar], [:foo])).to eq [:bar, :foo]
end
end
end

0 comments on commit e09cc6f

Please sign in to comment.