PERF: Eager load Theme associations in Stylesheet Manager.

Before this change, calling `StyleSheet::Manager.stylesheet_details`
for the first time resulted in multiple queries to the database. This is
because the code was modelled in a way where each `Theme` was loaded
from the database one at a time.

This PR restructures the code such that it allows us to load all the
theme records in a single query. It also allows us to eager load the
required associations upfront. In order to achieve this, I removed the
support of loading multiple themes per request. It was initially added
to support user selectable theme components but the feature was never
completed and abandoned because it wasn't a feature that we thought was
worth building.

spec/models/color_scheme_spec.rb

@@ -20,13 +20,14 @@ describe ColorScheme do
     theme.set_field(name: :scss, target: :desktop, value: '.bob {color: $primary;}')
     theme.save!
 
-    href = Stylesheet::Manager.stylesheet_data(:desktop_theme, theme.id)[0][:new_href]
-    colors_href = Stylesheet::Manager.color_scheme_stylesheet_details(scheme.id, "all", nil)
+    manager = Stylesheet::Manager.new(theme_id: theme.id)
+    href = manager.stylesheet_data(:desktop_theme)[0][:new_href]
+    colors_href = manager.color_scheme_stylesheet_details(scheme.id, "all")
 
     ColorSchemeRevisor.revise(scheme, colors: [{ name: 'primary', hex: 'bbb' }])
 
-    href2 = Stylesheet::Manager.stylesheet_data(:desktop_theme, theme.id)[0][:new_href]
-    colors_href2 = Stylesheet::Manager.color_scheme_stylesheet_details(scheme.id, "all", nil)
+    href2 = manager.stylesheet_data(:desktop_theme)[0][:new_href]
+    colors_href2 = manager.color_scheme_stylesheet_details(scheme.id, "all")
 
     expect(href).not_to eq(href2)
     expect(colors_href).not_to eq(colors_href2)

spec/models/theme_spec.rb

@@ -66,24 +66,22 @@ describe Theme do
   end
 
   it "can automatically disable for mismatching version" do
-    expect(theme.supported?).to eq(true)
     theme.create_remote_theme!(remote_url: "", minimum_discourse_version: "99.99.99")
     theme.save!
-    expect(theme.supported?).to eq(false)
 
-    expect(Theme.transform_ids([theme.id])).to be_empty
+    expect(Theme.transform_ids(theme.id)).to eq([])
   end
 
-  xit "#transform_ids works with nil values" do
+  it "#transform_ids works with nil values" do
     # Used in safe mode
-    expect(Theme.transform_ids([nil])).to eq([nil])
+    expect(Theme.transform_ids(nil)).to eq([])
   end
 
   it '#transform_ids filters out disabled components' do
     theme.add_relative_theme!(:child, child)
-    expect(Theme.transform_ids([theme.id], extend: true)).to eq([theme.id, child.id])
+    expect(Theme.transform_ids(theme.id)).to eq([theme.id, child.id])
     child.update!(enabled: false)
-    expect(Theme.transform_ids([theme.id], extend: true)).to eq([theme.id])
+    expect(Theme.transform_ids(theme.id)).to eq([theme.id])
   end
 
   it "doesn't allow multi-level theme components" do
@@ -171,19 +169,6 @@ HTML
     expect(Theme.lookup_field(theme.id, :desktop, :body_tag)).to match(/<b>test<\/b>/)
   end
 
-  it 'can find fields for multiple themes' do
-    theme2 = Fabricate(:theme)
-
-    theme.set_field(target: :common, name: :body_tag, value: "<b>testtheme1</b>")
-    theme2.set_field(target: :common, name: :body_tag, value: "<b>theme2test</b>")
-    theme.save!
-    theme2.save!
-
-    field = Theme.lookup_field([theme.id, theme2.id], :desktop, :body_tag)
-    expect(field).to match(/<b>testtheme1<\/b>/)
-    expect(field).to match(/<b>theme2test<\/b>/)
-  end
-
   describe "#switch_to_component!" do
     it "correctly converts a theme to component" do
       theme.add_relative_theme!(:child, child)
@@ -229,25 +214,13 @@ HTML
     end
 
     it "returns an empty array if no ids are passed" do
-      expect(Theme.transform_ids([])).to eq([])
+      expect(Theme.transform_ids(nil)).to eq([])
     end
 
     it "adds the child themes of the parent" do
       sorted = [child.id, child2.id].sort
 
-      expect(Theme.transform_ids([theme.id])).to eq([theme.id, *sorted])
-
-      expect(Theme.transform_ids([theme.id, orphan1.id, orphan2.id])).to eq([theme.id, orphan1.id, *sorted, orphan2.id])
-    end
-
-    it "doesn't insert children when extend is false" do
-      fake_id = orphan2.id
-      fake_id2 = orphan3.id
-      fake_id3 = orphan4.id
-
-      expect(Theme.transform_ids([theme.id], extend: false)).to eq([theme.id])
-      expect(Theme.transform_ids([theme.id, fake_id3, fake_id, fake_id2, fake_id2], extend: false))
-        .to eq([theme.id, fake_id, fake_id2, fake_id3])
+      expect(Theme.transform_ids(theme.id)).to eq([theme.id, *sorted])
     end
   end
 
@@ -317,7 +290,12 @@ HTML
       theme.reload
       expect(theme.theme_fields.find_by(name: :scss).error).to eq(nil)
 
-      scss, _map = Stylesheet::Manager.new(:desktop_theme, theme.id).compile(force: true)
+      manager = Stylesheet::Manager.new(theme_id: theme.id)
+
+      scss, _map = Stylesheet::Manager::Builder.new(
+        target: :desktop_theme, theme: theme, manager: manager
+      ).compile(force: true)
+
       expect(scss).to include(upload.url)
     end
   end
@@ -328,7 +306,12 @@ HTML
       theme.set_field(target: :common, name: :scss, value: 'body {background-color: $background_color; font-size: $font-size}')
       theme.save!
 
-      scss, _map = Stylesheet::Manager.new(:desktop_theme, theme.id).compile(force: true)
+      manager = Stylesheet::Manager.new(theme_id: theme.id)
+
+      scss, _map = Stylesheet::Manager::Builder.new(
+        target: :desktop_theme, theme: theme, manager: manager
+      ).compile(force: true)
+
       expect(scss).to include("background-color:red")
       expect(scss).to include("font-size:25px")
 
@@ -336,7 +319,10 @@ HTML
       setting.value = '30px'
       theme.save!
 
-      scss, _map = Stylesheet::Manager.new(:desktop_theme, theme.id).compile(force: true)
+      scss, _map = Stylesheet::Manager::Builder.new(
+        target: :desktop_theme, theme: theme, manager: manager
+      ).compile(force: true)
+
       expect(scss).to include("font-size:30px")
 
       # Escapes correctly. If not, compiling this would throw an exception
@@ -348,7 +334,10 @@ HTML
       theme.set_field(target: :common, name: :scss, value: 'body {font-size: quote($font-size)}')
       theme.save!
 
-      scss, _map = Stylesheet::Manager.new(:desktop_theme, theme.id).compile(force: true)
+      scss, _map = Stylesheet::Manager::Builder.new(
+        target: :desktop_theme, theme: theme, manager: manager
+      ).compile(force: true)
+
       expect(scss).to include('font-size:"#{$fakeinterpolatedvariable}\a andanothervalue \'withquotes\'; margin: 0;\a"')
     end
 
@@ -804,8 +793,13 @@ HTML
     }}
 
     let(:compiler) {
-      manager = Stylesheet::Manager.new(:desktop_theme, theme.id)
-      manager.compile(force: true)
+      manager = Stylesheet::Manager.new(theme_id: theme.id)
+
+      builder = Stylesheet::Manager::Builder.new(
+        target: :desktop_theme, theme: theme, manager: manager
+      )
+
+      builder.compile(force: true)
     }
 
     it "works when importing file by path" do
@@ -829,8 +823,13 @@ HTML
       child_theme.set_field(target: :common, name: :scss, value: '@import "my_files/moremagic"')
       child_theme.save!
 
-      manager = Stylesheet::Manager.new(:desktop_theme, child_theme.id)
-      css, _map = manager.compile(force: true)
+      manager = Stylesheet::Manager.new(theme_id: child_theme.id)
+
+      builder = Stylesheet::Manager::Builder.new(
+        target: :desktop_theme, theme: child_theme, manager: manager
+      )
+
+      css, _map = builder.compile(force: true)
       expect(css).to include("body{background:green}")
     end
   end