From 73e12dc0defbb7fd39c0da70ae3344380c03c81b Mon Sep 17 00:00:00 2001 From: JonJagger Date: Thu, 18 Jun 2026 16:34:25 +0100 Subject: [PATCH] Return 400, not 500, for well-formed but non-existent ids API endpoints that resolve an id raised a generic RuntimeError from the manifest read when the id was well-formed but referenced nothing on disk (eg kata_events for a non-existent kata-id). The global error handler maps that to HTTP 500, telling the client the server broke when in fact the request named something that does not exist. Wrap each resolver (kata_version, group, cluster_manifest) so a missing entity surfaces as a RequestError (HTTP 400), while genuine failures on an entity that does exist are re-raised unchanged rather than masked as "does not exist". cluster_hierarchy likewise now raises for an id matching no kata, group or cluster instead of returning an empty hierarchy. Co-Authored-By: Claude Opus 4.8 (1M context) --- source/server/model.rb | 27 ++++++++++++++++ test/server/cluster_hierarchy.rb | 10 ++++++ test/server/cluster_manifest.rb | 32 +++++++++++++++++++ test/server/config/coverage_metrics_limits.rb | 6 ++-- test/server/config/test_metrics_limits.rb | 2 +- test/server/group_manifest.rb | 26 +++++++++++++++ test/server/kata_events.rb | 11 +++++++ 7 files changed, 110 insertions(+), 4 deletions(-) create mode 100644 test/server/cluster_manifest.rb diff --git a/source/server/model.rb b/source/server/model.rb index aa72bbdb..357a540b 100644 --- a/source/server/model.rb +++ b/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) diff --git a/test/server/cluster_hierarchy.rb b/test/server/cluster_hierarchy.rb index 2af734c1..9d4dfca4 100644 --- a/test/server/cluster_hierarchy.rb +++ b/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 diff --git a/test/server/cluster_manifest.rb b/test/server/cluster_manifest.rb new file mode 100644 index 00000000..7c0ad60d --- /dev/null +++ b/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 diff --git a/test/server/config/coverage_metrics_limits.rb b/test/server/config/coverage_metrics_limits.rb index b2406f04..08be054f 100644 --- a/test/server/config/coverage_metrics_limits.rb +++ b/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 diff --git a/test/server/config/test_metrics_limits.rb b/test/server/config/test_metrics_limits.rb index c362ff49..b122414c 100644 --- a/test/server/config/test_metrics_limits.rb +++ b/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 ], diff --git a/test/server/group_manifest.rb b/test/server/group_manifest.rb index 8d2ffc8d..ed69e7da 100644 --- a/test/server/group_manifest.rb +++ b/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 diff --git a/test/server/kata_events.rb b/test/server/kata_events.rb index 5a1e4c56..ed0df60e 100644 --- a/test/server/kata_events.rb +++ b/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)