Cleanup duplicate pods in database

The unique index doesn't work when the port is `NULL`. So use `-1`
instead for when using the default ports (80/443), as if we would use
the real ports, we could still have both 80 and 443 in the database at
the same time.

6 files changed, 70 insertions, 8 deletions

diff --git a/app/assets/javascripts/app/views/pod_entry_view.js b/app/assets/javascripts/app/views/pod_entry_view.js
index 112b6708f..5e45267d1 100644
--- a/app/assets/javascripts/app/views/pod_entry_view.js
+++ b/app/assets/javascripts/app/views/pod_entry_view.js
@@ -32,7 +32,7 @@ app.views.PodEntry = app.views.Base.extend({
       has_errors: (this.model.get("status")!=="no_errors"),
       status_text: Diaspora.I18n.t("admin.pods.states."+this.model.get("status")),
       pod_url: (this.model.get("ssl") ? "https" : "http") + "://" + this.model.get("host") +
-                 (this.model.get("port") ? ":" + this.model.get("port") : ""),
+                 (this.model.get("port") >= 0 ? ":" + this.model.get("port") : ""),
       response_time_fmt: this._fmtResponseTime()
       /* jshint camelcase: true */
     });
diff --git a/app/models/pod.rb b/app/models/pod.rb
index 1edc4e899..ff61af5b3 100644
--- a/app/models/pod.rb
+++ b/app/models/pod.rb
@@ -22,7 +22,7 @@ class Pod < ApplicationRecord
     ConnectionTester::SSLFailure      => :ssl_failed,
     ConnectionTester::HTTPFailure     => :http_failed,
     ConnectionTester::NodeInfoFailure => :version_failed
-  }
+  }.freeze
 
   # this are only the most common errors, the rest will be +unknown_error+
   CURL_ERROR_MAP = {
@@ -34,7 +34,13 @@ class Pod < ApplicationRecord
     redirected_to_other_hostname: :http_failed
   }.freeze
 
-  DEFAULT_PORTS = [URI::HTTP::DEFAULT_PORT, URI::HTTPS::DEFAULT_PORT]
+  # use -1 as port for default ports
+  # we can't use the real default port (80/443) because we need to handle them
+  # like both are the same and not both can exist at the same time.
+  # we also can't use nil, because databases don't handle NULL in unique indexes
+  # (except postgres >= 15 with "NULLS NOT DISTINCT").
+  DEFAULT_PORT = -1
+  DEFAULT_PORTS = [URI::HTTP::DEFAULT_PORT, URI::HTTPS::DEFAULT_PORT].freeze
 
   has_many :people
 
@@ -51,7 +57,7 @@ class Pod < ApplicationRecord
   class << self
     def find_or_create_by(opts) # Rename this method to not override an AR method
       uri = URI.parse(opts.fetch(:url))
-      port = DEFAULT_PORTS.include?(uri.port) ? nil : uri.port
+      port = DEFAULT_PORTS.include?(uri.port) ? DEFAULT_PORT : uri.port
       find_or_initialize_by(host: uri.host, port: port).tap do |pod|
         pod.ssl ||= (uri.scheme == "https")
         pod.save
@@ -147,13 +153,21 @@ class Pod < ApplicationRecord
 
   # @return [URI]
   def uri
-    @uri ||= (ssl ? URI::HTTPS : URI::HTTP).build(host: host, port: port)
+    @uri ||= (ssl ? URI::HTTPS : URI::HTTP).build(host: host, port: real_port)
     @uri.dup
   end
 
+  def real_port
+    if port == DEFAULT_PORT
+      ssl ? URI::HTTPS::DEFAULT_PORT : URI::HTTP::DEFAULT_PORT
+    else
+      port
+    end
+  end
+
   def not_own_pod
     pod_uri = AppConfig.pod_uri
-    pod_port = DEFAULT_PORTS.include?(pod_uri.port) ? nil : pod_uri.port
+    pod_port = DEFAULT_PORTS.include?(pod_uri.port) ? DEFAULT_PORT : pod_uri.port
     errors.add(:base, "own pod not allowed") if pod_uri.host == host && pod_port == port
   end
 end
diff --git a/db/migrate/20221030193943_cleanup_duplicate_pods.rb b/db/migrate/20221030193943_cleanup_duplicate_pods.rb
new file mode 100644
index 000000000..fe1bc5fa2
--- /dev/null
+++ b/db/migrate/20221030193943_cleanup_duplicate_pods.rb
@@ -0,0 +1,36 @@
+# frozen_string_literal: true
+
+class CleanupDuplicatePods < ActiveRecord::Migration[6.1]
+  class Pod < ApplicationRecord
+  end
+
+  def change
+    reversible do |change|
+      change.up do
+        remove_duplicates
+
+        Pod.where(port: nil).update_all(port: -1) # rubocop:disable Rails/SkipsModelValidations
+      end
+
+      change.down do
+        Pod.where(port: -1).update_all(port: nil) # rubocop:disable Rails/SkipsModelValidations
+      end
+    end
+
+    change_column_null :pods, :port, false
+  end
+
+  private
+
+  def remove_duplicates
+    Pod.where(port: nil).group(:host).having("count(*) > 1").pluck(:host).each do |host|
+      duplicate_ids = Pod.where(host: host).order(:id).ids
+      target_pod_id = duplicate_ids.shift
+
+      duplicate_ids.each do |pod_id|
+        Person.where(pod_id: pod_id).update_all(pod_id: target_pod_id) # rubocop:disable Rails/SkipsModelValidations
+        Pod.delete(pod_id)
+      end
+    end
+  end
+end
diff --git a/spec/factories.rb b/spec/factories.rb
index 04da06dbc..6d0e8de47 100644
--- a/spec/factories.rb
+++ b/spec/factories.rb
@@ -249,6 +249,7 @@ FactoryBot.define do
 
   factory :pod do
     sequence(:host) {|n| "pod#{n}.example#{r_str}.com" }
+    port { -1 }
     ssl { true }
   end
 
diff --git a/spec/javascripts/app/views/pod_entry_view_spec.js b/spec/javascripts/app/views/pod_entry_view_spec.js
index 2c04effc8..c7b6f8968 100644
--- a/spec/javascripts/app/views/pod_entry_view_spec.js
+++ b/spec/javascripts/app/views/pod_entry_view_spec.js
@@ -30,7 +30,8 @@ describe("app.views.PodEntry", function() {
       this.pod.set({
         status: "no_errors",
         ssl: true,
-        host: "pod.example.com"
+        host: "pod.example.com",
+        port: -1
       });
       var actual = this.view.presenter();
       expect(actual).toEqual(jasmine.objectContaining({
diff --git a/spec/models/pod_spec.rb b/spec/models/pod_spec.rb
index ac5520c73..4c41f0173 100644
--- a/spec/models/pod_spec.rb
+++ b/spec/models/pod_spec.rb
@@ -16,26 +16,31 @@ describe Pod, type: :model do
     it "ignores default ports" do
       pod = Pod.find_or_create_by(url: "https://example.org:443/")
       expect(pod.host).to eq("example.org")
-      expect(pod.port).to be_nil
+      expect(pod.port).to eq(Pod::DEFAULT_PORT)
     end
 
     it "sets ssl boolean" do
       pod = Pod.find_or_create_by(url: "https://example.org/")
       expect(pod.ssl).to be true
+      expect(pod.port).to eq(Pod::DEFAULT_PORT)
     end
 
     it "updates ssl boolean if upgraded to https" do
       pod = Pod.find_or_create_by(url: "http://example.org/")
       expect(pod.ssl).to be false
+      expect(pod.port).to eq(Pod::DEFAULT_PORT)
       pod = Pod.find_or_create_by(url: "https://example.org/")
       expect(pod.ssl).to be true
+      expect(pod.port).to eq(Pod::DEFAULT_PORT)
     end
 
     it "does not update ssl boolean if downgraded to http" do
       pod = Pod.find_or_create_by(url: "https://example.org/")
       expect(pod.ssl).to be true
+      expect(pod.port).to eq(Pod::DEFAULT_PORT)
       pod = Pod.find_or_create_by(url: "http://example.org/")
       expect(pod.ssl).to be true
+      expect(pod.port).to eq(Pod::DEFAULT_PORT)
     end
 
     context "validation" do
@@ -205,6 +210,11 @@ describe Pod, type: :model do
       pod = FactoryBot.create(:pod)
       expect(pod.url_to("/receive/public")).to eq("https://#{pod.host}/receive/public")
     end
+
+    it "includes non-default port in pod url" do
+      pod = FactoryBot.create(:pod, port: 3000)
+      expect(pod.url_to("/receive/public")).to eq("https://#{pod.host}:#{pod.port}/receive/public")
+    end
   end
 
   describe "#update_offline_since" do