Skip to content

Commit 4ba0f36

Browse files
Lee Richmondrichmolj
authored andcommitted
Fix schema diffing
* When the prior schema didn't have "stats", we would blow up * Because the prior schema is from a JSON file and the new one is generated programmatically, we could enter scenarios incorrectly comparing string values to symbol values. Always parse as JSON to avoid this.
1 parent 45a1517 commit 4ba0f36

File tree

3 files changed

+26
-12
lines changed

3 files changed

+26
-12
lines changed

lib/graphiti/schema_diff.rb

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
module Graphiti
22
class SchemaDiff
33
def initialize(old, new)
4-
@old = old.deep_symbolize_keys
5-
@new = new.deep_symbolize_keys
4+
@old = JSON.parse(old.to_json).deep_symbolize_keys
5+
@new = JSON.parse(new.to_json).deep_symbolize_keys
66
@errors = []
77
end
88

@@ -135,12 +135,12 @@ def compare_sorts(old_resource, new_resource)
135135
end
136136

137137
if new_sort[:only] && !old_sort[:only]
138-
@errors << "#{old_resource[:name]}: sort #{name.inspect} now limited to only #{new_sort[:only].inspect}."
138+
@errors << "#{old_resource[:name]}: sort #{name.inspect} now limited to only #{new_sort[:only].to_sym.inspect}."
139139
end
140140

141141
if new_sort[:only] && old_sort[:only]
142142
if new_sort[:only] != old_sort[:only]
143-
@errors << "#{old_resource[:name]}: sort #{name.inspect} was limited to only #{old_sort[:only].inspect}, now limited to only #{new_sort[:only].inspect}."
143+
@errors << "#{old_resource[:name]}: sort #{name.inspect} was limited to only #{old_sort[:only].to_sym.inspect}, now limited to only #{new_sort[:only].to_sym.inspect}."
144144
end
145145
end
146146
end
@@ -212,28 +212,30 @@ def compare_filter_group(old_resource, new_resource)
212212
new_names = new_resource[:filter_group][:names]
213213
old_names = old_resource[:filter_group][:names]
214214
diff = new_names - old_names
215-
if !diff.empty? && new_resource[:filter_group][:required] == :all
216-
@errors << "#{old_resource[:name]}: all required filter group #{old_names.inspect} added #{"member".pluralize(diff.length)} #{diff.inspect}."
215+
if !diff.empty? && new_resource[:filter_group][:required] == "all"
216+
@errors << "#{old_resource[:name]}: all required filter group #{old_names.map(&:to_sym).inspect} added #{"member".pluralize(diff.length)} #{diff.map(&:to_sym).inspect}."
217217
end
218218

219219
old_required = old_resource[:filter_group][:required]
220220
new_required = new_resource[:filter_group][:required]
221-
if old_required == :any && new_required == :all
222-
@errors << "#{old_resource[:name]}: filter group #{old_names.inspect} moved from required: :any to required: :all"
221+
if old_required == "any" && new_required == "all"
222+
@errors << "#{old_resource[:name]}: filter group #{old_names.map(&:to_sym).inspect} moved from required: :any to required: :all"
223223
end
224224
else
225-
@errors << "#{old_resource[:name]}: filter group #{new_resource[:filter_group][:names].inspect} was added."
225+
@errors << "#{old_resource[:name]}: filter group #{new_resource[:filter_group][:names].map(&:to_sym).inspect} was added."
226226
end
227227
end
228228
end
229229

230230
def compare_stats(old_resource, new_resource)
231+
return unless old_resource.key?(:stats)
232+
231233
old_resource[:stats].each_pair do |name, old_calculations|
232234
new_calculations = new_resource[:stats][name]
233235
if new_calculations
234236
old_calculations.each do |calc|
235237
unless new_calculations.include?(calc)
236-
@errors << "#{old_resource[:name]}: calculation #{calc.inspect} was removed from stat #{name.inspect}."
238+
@errors << "#{old_resource[:name]}: calculation #{calc.to_sym.inspect} was removed from stat #{name.inspect}."
237239
end
238240
end
239241
else

lib/graphiti/version.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,3 @@
11
module Graphiti
2-
VERSION = "1.2.36"
2+
VERSION = "1.2.37"
33
end

spec/schema_diff_spec.rb

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -891,6 +891,18 @@ def self.name
891891
end
892892
end
893893

894+
context "when the original schema does not have stats" do
895+
before do
896+
a[:resources].each { |r| r.delete(:stats) }
897+
end
898+
899+
it "does not blow up" do
900+
expect {
901+
expect(diff).to eq([])
902+
}.to_not raise_error
903+
end
904+
end
905+
894906
context "when a stat is added" do
895907
before do
896908
resource_a.stat age: [:sum]
@@ -1193,7 +1205,7 @@ def self.name
11931205

11941206
it "returns error" do
11951207
expect(diff).to eq([
1196-
'Endpoint "/schema_diff/employees" had incompatible sideload allowlist. Was [{:positions=>:department}, :same], now [:positions, :same].'
1208+
'Endpoint "/schema_diff/employees" had incompatible sideload allowlist. Was [{:positions=>"department"}, "same"], now ["positions", "same"].'
11971209
])
11981210
end
11991211
end

0 commit comments

Comments
 (0)