Skip to content

Commit

Permalink
Refactor AR to use serialize and query methods
Browse files Browse the repository at this point in the history
  • Loading branch information
bkeepers committed Sep 1, 2021
1 parent c816331 commit ae07e50
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 28 deletions.
40 changes: 13 additions & 27 deletions lib/flipper/adapters/active_record.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ class Feature < ::ActiveRecord::Base
"flipper_features",
::ActiveRecord::Base.table_name_suffix,
].join

has_many :gates, foreign_key: "feature_key", primary_key: "key"
end

# Private: Do not use outside of this adapter.
Expand All @@ -23,6 +25,8 @@ class Gate < ::ActiveRecord::Base
"flipper_gates",
::ActiveRecord::Base.table_name_suffix,
].join

serialize :value, JSON
end

# Public: The name of the adapter.
Expand Down Expand Up @@ -101,19 +105,13 @@ def get_multi(features)
end

def get_all
features = ::Arel::Table.new(@feature_class.table_name.to_sym)
gates = ::Arel::Table.new(@gate_class.table_name.to_sym)
rows_query = features.join(gates, Arel::Nodes::OuterJoin)
.on(features[:key].eq(gates[:feature_key]))
.project(features[:key].as('feature_key'), gates[:key], gates[:value])
rows = ::ActiveRecord::Base.connection.select_all rows_query
db_gates = rows.map { |row| Gate.new(row) }
grouped_db_gates = db_gates.group_by(&:feature_key)
result = Hash.new { |hash, key| hash[key] = default_config }
features = grouped_db_gates.keys.map { |key| Flipper::Feature.new(key, self) }
features.each do |feature|
result[feature.key] = result_for_feature(feature, grouped_db_gates[feature.key])

@feature_class.includes(:gates).all.each do |f|

This comment has been minimized.

Copy link
@jnunemaker

jnunemaker Sep 2, 2021

Collaborator

Will this change 1 query to 2 queries?

feature = Flipper::Feature.new(f.key, self)
result[feature.key] = result_for_feature(feature, f.gates)
end

result
end

Expand All @@ -130,10 +128,8 @@ def enable(feature, gate, thing)
set(feature, gate, thing, clear: true)
when :integer
set(feature, gate, thing)
when :set
when :set, :json
enable_multi(feature, gate, thing)
when :json
set_json(feature, gate, thing)
else
unsupported_data_type gate.data_type
end
Expand All @@ -157,7 +153,7 @@ def disable(feature, gate, thing)
when :set
@gate_class.where(feature_key: feature.key, key: gate.key, value: thing.value).destroy_all
when :json
@gate_class.where(feature_key: feature.key, key: gate.key, value: JSON.dump(thing.value)).destroy_all
@gate_class.where(feature_key: feature.key, key: gate.key, value: thing.value).destroy_all
else
unsupported_data_type gate.data_type
end
Expand Down Expand Up @@ -196,22 +192,14 @@ def enable_multi(feature, gate, thing)
@gate_class.create! do |g|
g.feature_key = feature.key
g.key = gate.key
g.value = thing.value.to_s
g.value = thing.value
end

nil
rescue ::ActiveRecord::RecordNotUnique
# already added so no need move on with life
end

def set_json(feature, gate, thing)
@gate_class.create! do |g|
g.feature_key = feature.key
g.key = gate.key
g.value = JSON.dump(thing.value)
end
end

def result_for_feature(feature, db_gates)
db_gates ||= []
result = {}
Expand All @@ -226,10 +214,8 @@ def result_for_feature(feature, db_gates)
if detected_db_gate = db_gates.detect { |db_gate| db_gate.key == gate.key.to_s }
detected_db_gate.value
end
when :set
when :set, :json
db_gates.select { |db_gate| db_gate.key == gate.key.to_s }.map(&:value).to_set
when :json
db_gates.select { |db_gate| db_gate.key == gate.key.to_s }.map { |gate| JSON.load(gate.value) }.to_set
else
unsupported_data_type gate.data_type
end
Expand Down
2 changes: 1 addition & 1 deletion spec/flipper/adapters/active_record_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
CREATE TABLE flipper_gates (
id integer PRIMARY KEY,
feature_key text NOT NULL,
key text NOT NULL,
key string NOT NULL,
value text DEFAULT NULL,
created_at datetime NOT NULL,
updated_at datetime NOT NULL
Expand Down

1 comment on commit ae07e50

@jnunemaker
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the one question, I think this is nice. One minor thing: would it work on rails 4.2? I still know a few larger companies running on paid forks with security updates that are using 4.2. I relaxed the gem spec for YMMV but I don't currently have a CI matrix for 4.2. Probably should add that so we can see how hard/how much work it is to officially support it and if this serialize would cause any problems.

Please sign in to comment.