From cc390d0f8759e80e66ece409d5ffa63b473bf003 Mon Sep 17 00:00:00 2001
From: Al Davidson <apdavidson@gmail.com>
Date: Wed, 28 Jun 2023 14:11:18 +0100
Subject: [PATCH] reintroduce hash sorting for expanded content-item and links,
 but correctly this time enforce internal consistency of sym/string keys for
 ExpandedLinksPresenter and ContentItemPresenter

---
 Gemfile                                       |  1 +
 Gemfile.lock                                  |  2 +
 app/lib/hash_sorter.rb                        | 10 +++++
 app/presenters/content_item_presenter.rb      |  7 ++--
 app/presenters/expanded_links_presenter.rb    |  3 +-
 spec/lib/hash_sorter_spec.rb                  | 37 +++++++++++++++++++
 .../presenters/content_item_presenter_spec.rb |  4 +-
 .../expanded_links_presenter_spec.rb          | 26 +++++++++++++
 8 files changed, 84 insertions(+), 6 deletions(-)
 create mode 100644 app/lib/hash_sorter.rb
 create mode 100644 spec/lib/hash_sorter_spec.rb

diff --git a/Gemfile b/Gemfile
index 37b858545..106d0003f 100644
--- a/Gemfile
+++ b/Gemfile
@@ -3,6 +3,7 @@ source "https://rubygems.org"
 gem "rails", "7.0.5"
 
 gem "bootsnap", require: false
+gem "deepsort"
 gem "gds-api-adapters"
 gem "gds-sso"
 gem "govuk_app_config"
diff --git a/Gemfile.lock b/Gemfile.lock
index a68acda36..4fd7e652f 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -103,6 +103,7 @@ GEM
       database_cleaner-core (~> 2.0.0)
       mongoid
     date (3.3.3)
+    deepsort (0.4.5)
     diff-lcs (1.5.0)
     docile (1.4.0)
     domain_name (0.5.20190701)
@@ -438,6 +439,7 @@ DEPENDENCIES
   ci_reporter_rspec
   climate_control
   database_cleaner-mongoid
+  deepsort
   factory_bot
   gds-api-adapters
   gds-sso
diff --git a/app/lib/hash_sorter.rb b/app/lib/hash_sorter.rb
new file mode 100644
index 000000000..6c2f7e9f8
--- /dev/null
+++ b/app/lib/hash_sorter.rb
@@ -0,0 +1,10 @@
+require "deepsort"
+
+class HashSorter
+  def self.sort(hash)
+    # Deepsort by default will sort keys in an array too -
+    # we don't want that, as (for instance) order of links
+    # may be important
+    hash.deep_sort(array: false)
+  end
+end
diff --git a/app/presenters/content_item_presenter.rb b/app/presenters/content_item_presenter.rb
index ae4e0555b..b23b600c0 100644
--- a/app/presenters/content_item_presenter.rb
+++ b/app/presenters/content_item_presenter.rb
@@ -32,13 +32,14 @@ def initialize(item, api_url_method)
   end
 
   def as_json(options = nil)
-    item.as_json(options).slice(*PUBLIC_ATTRIBUTES).merge(
+    hash = item.as_json(options).slice(*PUBLIC_ATTRIBUTES).merge(
       "links" => RESOLVER.resolve(links),
       "description" => RESOLVER.resolve(item.description),
       "details" => RESOLVER.resolve(item.details),
-    ).tap do |i|
+    ).tap { |i|
       i["redirects"] = item["redirects"] if i["schema_name"] == "redirect"
-    end
+    }.deep_stringify_keys
+    HashSorter.sort(hash)
   end
 
 private
diff --git a/app/presenters/expanded_links_presenter.rb b/app/presenters/expanded_links_presenter.rb
index 2f45a8a8b..dc7a8a1fa 100644
--- a/app/presenters/expanded_links_presenter.rb
+++ b/app/presenters/expanded_links_presenter.rb
@@ -6,7 +6,7 @@ def initialize(expanded_links)
   end
 
   def present
-    expanded_links.deep_symbolize_keys.each_with_object({}) do |(type, links), memo|
+    result = expanded_links.deep_symbolize_keys.each_with_object({}) do |(type, links), memo|
       links = Array.wrap(links)
       memo[type] = links.map do |link|
         link.dup.except(secret_fields).merge(
@@ -17,6 +17,7 @@ def present
         ).compact
       end
     end
+    HashSorter.sort(result)
   end
 
 private
diff --git a/spec/lib/hash_sorter_spec.rb b/spec/lib/hash_sorter_spec.rb
new file mode 100644
index 000000000..679bfb157
--- /dev/null
+++ b/spec/lib/hash_sorter_spec.rb
@@ -0,0 +1,37 @@
+require "rails_helper"
+
+describe HashSorter do
+  describe ".sort" do
+    let(:links) do
+      {
+        group_2: [
+          { base_path: "/group-1/link-1", api_path: "/api/content/group-1/link-1" },
+          { base_path: "/group-1/link-2", api_path: "/api/content/group-1/link-2" },
+          { base_path: "/group-1/link-3", api_path: "/api/content/group-1/link-3" },
+        ],
+        group_1: [
+          { base_path: "/group-2/link-3", api_path: "/api/content/group-2/link-3" },
+          { base_path: "/group-2/link-2", api_path: "/api/content/group-2/link-2" },
+          { base_path: "/group-2/link-1", api_path: "/api/content/group-2/link-1" },
+        ],
+      }
+    end
+    let(:result) { described_class.sort(links) }
+
+    it "sorts the top-level keys" do
+      expect(result.keys).to eq(%i[group_1 group_2])
+    end
+
+    it "does not sort the arrays" do
+      expect(result[:group_1].map { |e| e[:base_path] }).to eq(links[:group_1].map { |e| e[:base_path] })
+    end
+
+    it "sorts the keys on Hashes within arrays" do
+      %i[group_1 group_2].each do |group|
+        result[group].each do |hash|
+          expect(hash.keys).to eq(hash.keys.sort)
+        end
+      end
+    end
+  end
+end
diff --git a/spec/presenters/content_item_presenter_spec.rb b/spec/presenters/content_item_presenter_spec.rb
index 20a2cf524..5e50a2388 100644
--- a/spec/presenters/content_item_presenter_spec.rb
+++ b/spec/presenters/content_item_presenter_spec.rb
@@ -66,11 +66,11 @@
     end
 
     it "inlines the 'text/html' content type in the details" do
-      expect(presenter.as_json["details"]).to eq(body: "<p>content</p>")
+      expect(presenter.as_json["details"]).to eq("body" => "<p>content</p>")
     end
 
     it "inlines the 'text/html' content type in the links" do
-      expect(presenter.as_json["links"][:person].first[:details]).to eq(body: "<p>content</p>")
+      expect(presenter.as_json["links"]["person"].first["details"]).to eq("body" => "<p>content</p>")
     end
   end
 
diff --git a/spec/presenters/expanded_links_presenter_spec.rb b/spec/presenters/expanded_links_presenter_spec.rb
index 65955f38f..3d4ce24f8 100644
--- a/spec/presenters/expanded_links_presenter_spec.rb
+++ b/spec/presenters/expanded_links_presenter_spec.rb
@@ -205,4 +205,30 @@
       end
     end
   end
+
+  describe "sorting the hash" do
+    let(:links) do
+      {
+        group_2: [
+          { base_path: "/group-1/link-1", api_path: "/api/content/group-1/link-1" },
+          { base_path: "/group-1/link-2", api_path: "/api/content/group-1/link-2" },
+          { base_path: "/group-1/link-3", api_path: "/api/content/group-1/link-3" },
+        ],
+        group_1: [
+          { base_path: "/group-2/link-3", api_path: "/api/content/group-2/link-3" },
+          { base_path: "/group-2/link-2", api_path: "/api/content/group-2/link-2" },
+          { base_path: "/group-2/link-1", api_path: "/api/content/group-2/link-1" },
+        ],
+      }
+    end
+    let(:result) { described_class.new(links).present }
+
+    it "sorts the results" do
+      expect(result.keys).to eq(result.keys.sort)
+
+      result.each_key do |group|
+        expect(result[group].all? { |link| link.keys == link.keys.sort }).to eq(true)
+      end
+    end
+  end
 end