Return 400, not 500, for well-formed but non-existent ids

source/server/model.rb

@@ -20,6 +20,13 @@ def cluster_create(manifest:)
 
   def cluster_manifest(id:)
     Cluster.new(@externals).manifest(id)
+  rescue StandardError => error
+    # Classify a failed cluster resolution: a well-formed but non-existent
+    # cluster-id is a client error (400), not the generic server error (500)
+    # raised by Cluster#manifest's disk.assert read. Real failures on a cluster
+    # that does exist are re-raised unchanged.
+    raise error if cluster_exists?(id:id)
+    fail RequestError, "cluster #{id} does not exist"
   end
 
   # True if a cluster with this id exists.
@@ -47,6 +54,12 @@ def cluster_hierarchy(id:)
     if cluster_exists?(id:id)
       result << { 'type' => 'cluster', 'id' => id }
     end
+    # An empty result means the original id matched no kata, group or cluster
+    # (id is only reassigned inside a block that also appends an entry). That is
+    # a client error (400), not an empty hierarchy.
+    if result.empty?
+      fail RequestError, "id #{id} does not exist"
+    end
     result
   end
 
@@ -203,6 +216,13 @@ def diff_summary(id:, was_index:, now_index:)
 
   def group(id)
     GROUPS[from_path(group_id_path(id, 'manifest.json'))].new(@externals)
+  rescue StandardError => error
+    # Classify a failed group resolution: a well-formed but non-existent
+    # gid is a client error (400), not the generic server error (500) raised
+    # by from_path's manifest read. Real failures on a group that does exist
+    # are re-raised unchanged.
+    raise error if group_exists?(id:id)
+    fail RequestError, "group #{id} does not exist"
   end
 
   def kata(id)
@@ -229,6 +249,13 @@ def kata_version(id)
       end
       version
     end
+  rescue StandardError => error
+    # Classify a failed version-detection: a well-formed but non-existent
+    # kata-id is a client error (400), not the generic server error (500)
+    # raised by from_path's manifest read. Real failures on a kata that does
+    # exist are re-raised unchanged.
+    raise error if kata_exists?(id:id)
+    fail RequestError, "kata #{id} does not exist"
   end
 
   def from_manifest(manifest)

test/server/cluster_hierarchy.rb

@@ -88,4 +88,14 @@ def two_ltf_cluster
     end
   end
 
+  test 'Hy1a11', %w(
+  | cluster_hierarchy for a well-formed id that does not exist
+  | raises a RequestError (HTTP 400 client error)
+  | rather than returning [] (an id matching no kata, group or cluster
+  | is a client error, not an empty hierarchy)
+  ) do
+    error = assert_raises(RequestError) { cluster_hierarchy('123AbZ') }
+    assert_equal 'id 123AbZ does not exist', error.message
+  end
+
 end

test/server/cluster_manifest.rb

@@ -0,0 +1,32 @@
+require_relative 'test_base'
+
+class ClusterManifestTest < TestBase
+
+  def two_ltf_cluster
+    cluster_create('exercise' => 'Tennis', 'ltfs' => [
+      manifest_Tennis_refactoring_Python_unitttest,
+      manifest_Tennis_refactoring_Ruby_minitest ])
+  end
+
+  test 'Cm5d10', %w(
+  | cluster_manifest for a well-formed id that does not exist
+  | raises a RequestError (HTTP 400 client error)
+  | rather than a generic error (HTTP 500 server error)
+  ) do
+    error = assert_raises(RequestError) { cluster_manifest('123AbZ') }
+    assert_equal 'cluster 123AbZ does not exist', error.message
+  end
+
+  test 'Cm5d11', %w(
+  | cluster_manifest re-raises the original error, rather than masking it as
+  | "does not exist", when the cluster exists but its manifest is unreadable.
+  | Covers the cluster_exists?==true branch of the rescue.
+  ) do
+    id = two_ltf_cluster
+    path = "/clusters/#{id[0..1]}/#{id[2..3]}/#{id[4..5]}/manifest.json"
+    disk.run(disk.file_write_command(path, '{ this is not valid json'))
+    assert cluster_exists?(id), :cluster_dir_still_exists
+    assert_raises(JSON::ParserError) { cluster_manifest(id) }
+  end
+
+end

test/server/config/coverage_metrics_limits.rb

@@ -2,14 +2,14 @@
 def metrics
   [
     [ nil ],
-    [ 'test.lines.total'    , '<=', 2987 ],
+    [ 'test.lines.total'    , '<=', 3015 ],
     [ 'test.lines.missed'   , '<=', 0    ],
     [ 'test.branches.total' , '<=', 4    ],
     [ 'test.branches.missed', '<=', 0    ],
     [ nil ],
-    [ 'code.lines.total'    , '<=', 1623 ],
+    [ 'code.lines.total'    , '<=', 1631 ],
     [ 'code.lines.missed'   , '<=', 0    ],
-    [ 'code.branches.total' , '<=', 197  ],
+    [ 'code.branches.total' , '<=', 205  ],
     [ 'code.branches.missed', '<=', 0    ],
   ]
 end

test/server/config/test_metrics_limits.rb

@@ -2,7 +2,7 @@
 def metrics
   [
     [ nil ],
-    [ 'test_count',    '>=', 363 ],
+    [ 'test_count',    '>=', 373 ],
     [ 'total_time',    '<=',  50 ],
     [ nil ],
     [ 'failure_count', '<=', 0   ],

test/server/group_manifest.rb

@@ -49,4 +49,30 @@ class GroupManifestTest < TestBase
     assert_equal version, manifest['version'], :version
   end
 
+  #- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+
+  versions_test '5ZtN0X', %w(
+  | group_manifest for a well-formed id that does not exist
+  | raises a RequestError (HTTP 400 client error)
+  | rather than a generic error (HTTP 500 server error)
+  ) do
+    error = assert_raises(RequestError) { group_manifest('123AbZ') }
+    assert_equal 'group 123AbZ does not exist', error.message
+  end
+
+  #- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+
+  version_test 2, '5ZtN11', %w(
+  | group_manifest re-raises the original error, rather than masking it as
+  | "does not exist", when the group exists but its manifest is unreadable.
+  | Covers the group_exists?==true branch of the rescue.
+  ) do
+    in_group do |id|
+      path = "/groups/#{id[0..1]}/#{id[2..3]}/#{id[4..5]}/manifest.json"
+      disk.run(disk.file_write_command(path, '{ this is not valid json'))
+      assert group_exists?(id), :group_dir_still_exists
+      assert_raises(JSON::ParserError) { group_manifest(id) }
+    end
+  end
+
 end

test/server/kata_events.rb

@@ -117,6 +117,17 @@ class KataEventsTest < TestBase
     end
   end
 
+  # - - - - - - - - - - - - - - - - - - - - -
+
+  versions_test 'D9wn0X', %w(
+  | kata_events for a well-formed id that does not exist
+  | raises a RequestError (HTTP 400 client error)
+  | rather than a generic error (HTTP 500 server error)
+  ) do
+    error = assert_raises(RequestError) { kata_events('123AbZ') }
+    assert_equal 'kata 123AbZ does not exist', error.message
+  end
+
   private
 
   def kata_create_event(index, time)