Merge pull request #1357 from alphagov/remove-router-api

Remove route registration to Router API

app/controllers/content_items_controller.rb

@@ -52,7 +52,6 @@ def update
   def destroy
     content_item = ContentItem.find_by!(base_path: encoded_base_path)
 
-    content_item.delete_routes
     content_item.destroy!
 
     render status: :ok

app/models/content_item.rb

@@ -12,7 +12,6 @@ def self.revert(previous_item:, item:)
 
   def self.create_or_replace(base_path, attributes, log_entry)
     previous_item = ContentItem.where(base_path:).first
-    item_state_before_change = previous_item.dup
 
     lock = UpdateLock.new(previous_item)
 
@@ -46,7 +45,6 @@ def self.create_or_replace(base_path, attributes, log_entry)
     begin
       transaction do
         item.save!
-        item.register_routes(previous_item: item_state_before_change)
       end
     rescue StandardError
       result = false
@@ -105,10 +103,6 @@ def description
     end
   end
 
-  def redirect?
-    schema_name == "redirect"
-  end
-
   def gone?
     # we've overloaded gone a bit by adding explanation and alternative
     # url to the schema to support Whitehall unpublishing. We need to consider
@@ -119,21 +113,6 @@ def gone?
     schema_name == "gone" && details_is_empty?
   end
 
-  def router_rendering_app
-    # This is an extension of the hack in `gone?` method.
-    #
-    # For items that are registered in the content store which are not redirects
-    # or gones we need to have a rendering_app. This is fine for all but the
-    # "gone but not gone" exception defined in `gone?` where rendering_app is
-    # not part of the schema for a gone but is required to register the route.
-    #
-    # This rather nastily fallsback to government frontend for gones that are
-    # not gone and lack a rendering_app
-    return rendering_app if schema_name != "gone" || gone?
-
-    rendering_app || "government-frontend"
-  end
-
   def valid_auth_bypass_id?(auth_bypass_id)
     return false unless auth_bypass_id
     return true if auth_bypass_ids&.include?(auth_bypass_id)
@@ -156,40 +135,12 @@ def access_limited?
     access_limited_user_ids.any? || access_limited_organisation_ids.any?
   end
 
-  def register_routes(previous_item: nil)
-    return unless should_register_routes?(previous_item:)
-
-    tries = Rails.application.config.register_router_retries
-    begin
-      route_set.register!
-    rescue GdsApi::BaseError
-      tries -= 1
-      tries.positive? ? retry : raise
-    end
-  end
-
-  def delete_routes
-    return unless should_register_routes?
-
-    route_set.delete!
-  end
-
   def base_path_without_root
     base_path&.sub(%r{^/}, "")
   end
 
-  def route_set
-    @route_set ||= RouteSet.from_content_item(self)
-  end
-
 private
 
-  def should_register_routes?(previous_item: nil)
-    return previous_item.route_set != route_set if previous_item
-
-    true
-  end
-
   def access_limited_user_ids
     access_limited.fetch("users", [])
   end

app/models/publish_intent.rb

@@ -31,8 +31,6 @@ def self.find_by_path(path)
   validates :publish_time, presence: true
   validates :rendering_app, presence: true, format: /\A[a-z0-9-]*\z/
 
-  after_save :register_routes
-
   def as_json(options = nil)
     super(options).tap do |hash|
       hash["errors"] = errors.as_json.stringify_keys if errors.any?
@@ -55,14 +53,4 @@ def self.cleanup_expired
   def base_path_without_root
     base_path&.sub(%r{^/}, "")
   end
-
-private
-
-  def route_set
-    @route_set ||= RouteSet.from_publish_intent(self)
-  end
-
-  def register_routes
-    route_set.register!
-  end
 end

config/application.rb

@@ -121,7 +121,5 @@ class Application < Rails::Application
     config.minimum_ttl = [config.default_ttl, 5.seconds].min
 
     config.paths["log"] = ENV["LOG_PATH"] if ENV["LOG_PATH"]
-
-    config.register_router_retries = 3
   end
 end

docs/route_registration.md

@@ -1,9 +1,8 @@
-## Registering routes with the router.
+## How router loads routes
 
-After saving an item, the content store will register routes with the router.
-All items listed in the routes array will be created as routes pointing at the
-`rendering_app`. The routes for a content item must contain a route for the
-`base_path`.
+Router will query the content store database to load routes. All items listed
+in the routes array will be created as routes pointing at the `rendering_app`.
+The routes for a content item must contain a route for the `base_path`.
 
 All entries in the routes array must be under the `base_path` (ie either a
 subpath of the `base_path`, or the `base_path` with an extension)

spec/integration/deleting_a_content_item_spec.rb

@@ -18,12 +18,6 @@
   context "when the content item exists" do
     before do
       FactoryBot.create(:content_item, base_path:)
-
-      @delete_stubs = ContentItem.find_by(
-        base_path:,
-      ).routes.map do |route|
-        stub_route_deleted(route["path"], hard_delete: true)
-      end
     end
 
     it "deletes the content item" do
@@ -32,12 +26,6 @@
       expect(ContentItem.where(base_path:).count).to eq(0)
     end
 
-    it "deletes the routes" do
-      delete "/content/vat-rates"
-
-      @delete_stubs.each { |stub| assert_requested(stub, times: 1) }
-    end
-
     it "returns a 200" do
       delete "/content/vat-rates"