From f5768fee9d0977a42f344cf0cfdae74ca80a23b9 Mon Sep 17 00:00:00 2001 From: Francois Kritzinger Date: Fri, 29 Nov 2024 08:34:09 +0200 Subject: Review exceptions thrown by github-ci API functions --- mod/mod-ci-github-gh.cxx | 64 +++++++++++++++++++++++++++++++------- mod/mod-ci-github-gh.hxx | 9 +++++- mod/mod-ci-github-gq.cxx | 46 ++++++++++++++++++++------- mod/mod-ci-github-gq.hxx | 8 +++++ mod/mod-ci-github-service-data.cxx | 60 ++++++++++++++++++++++++++++------- mod/mod-ci-github-service-data.hxx | 8 +++++ mod/mod-ci-github.cxx | 6 ++-- mod/mod-ci-github.hxx | 4 ++- 8 files changed, 168 insertions(+), 37 deletions(-) diff --git a/mod/mod-ci-github-gh.cxx b/mod/mod-ci-github-gh.cxx index 6ab93d7..a1e4d53 100644 --- a/mod/mod-ci-github-gh.cxx +++ b/mod/mod-ci-github-gh.cxx @@ -7,10 +7,19 @@ namespace brep { + [[noreturn]] static void + throw_json (const json::parser& p, const string& m) + { + throw json::invalid_json_input ( + p.input_name, + p.line (), p.column (), p.position (), + m); + } + // Return the GitHub check run status corresponding to a build_state. // string - gh_to_status (build_state st) + gh_to_status (build_state st) noexcept { // Just return by value (small string optimization). // @@ -102,10 +111,7 @@ namespace brep [[noreturn]] static void missing_member (const json::parser& p, const char* o, const char* m) { - throw json::invalid_json_input ( - p.input_name, - p.line (), p.column (), p.position (), - o + string (" object is missing member '") + m + '\''); + throw_json (p, o + string (" object is missing member '") + m + '\''); } using event = json::event; @@ -577,7 +583,21 @@ namespace brep }; if (c (tk, "token")) token = p.next_expect_string (); - else if (c (ea, "expires_at")) expires_at = gh_from_iso8601 (p.next_expect_string ()); + else if (c (ea, "expires_at")) + { + string v (p.next_expect_string ()); + + try + { + expires_at = gh_from_iso8601 (v); + } + catch (const invalid_argument& e) + { + throw_json (p, + "invalid IAT expires_at value '" + v + + "': " + e.what ()); + } + } else p.next_expect_value_skip (); } @@ -603,15 +623,37 @@ namespace brep string gh_to_iso8601 (timestamp t) { - return butl::to_string (t, - "%Y-%m-%dT%TZ", - false /* special */, - false /* local */); + try + { + return butl::to_string (t, + "%Y-%m-%dT%TZ", + false /* special */, + false /* local */); + } + catch (const system_error& e) + { + throw runtime_error ( + string ("failed to convert timestamp to ISO 8601 string: ") + + e.what ()); + } } timestamp gh_from_iso8601 (const string& s) { - return butl::from_string (s.c_str (), "%Y-%m-%dT%TZ", false /* local */); + try + { + // @@ TMP butl::from_string()'s comment says it also throws + // invalid_argument but that seems to be false. + // + return butl::from_string (s.c_str (), + "%Y-%m-%dT%TZ", + false /* local */); + } + catch (const system_error& e) + { + throw invalid_argument ("invalid ISO 8601 timestamp value '" + s + + "': " + e.what ()); + } } } diff --git a/mod/mod-ci-github-gh.hxx b/mod/mod-ci-github-gh.hxx index 6ede697..5fff2bc 100644 --- a/mod/mod-ci-github-gh.hxx +++ b/mod/mod-ci-github-gh.hxx @@ -107,7 +107,7 @@ namespace brep // Return the GitHub check run status corresponding to a build_state. // string - gh_to_status (build_state st); + gh_to_status (build_state st) noexcept; // Return the build_state corresponding to a GitHub check run status // string. Throw invalid_argument if the passed status was invalid. @@ -118,6 +118,9 @@ namespace brep // If warning_success is true, then map result_status::warning to SUCCESS // and to FAILURE otherwise. // + // Throw invalid_argument in case of unsupported result_status value + // (currently skip, interrupt). + // string gh_to_conclusion (result_status, bool warning_success); @@ -211,9 +214,13 @@ namespace brep gh_installation_access_token () = default; }; + // Throw runtime_error if the conversion fails. + // string gh_to_iso8601 (timestamp); + // Throw invalid_argument if the conversion fails. + // timestamp gh_from_iso8601 (const string&); diff --git a/mod/mod-ci-github-gq.cxx b/mod/mod-ci-github-gq.cxx index fa701a8..a0a0d6b 100644 --- a/mod/mod-ci-github-gq.cxx +++ b/mod/mod-ci-github-gq.cxx @@ -163,6 +163,8 @@ namespace brep // Parse a response to a check_run GraphQL mutation such as `createCheckRun` // or `updateCheckRun`. // + // Throw invalid_json_input. + // // Example response (only the part we need to parse here): // // { @@ -229,7 +231,7 @@ namespace brep gq_mutate_check_runs (const basic_mark& error, vector& crs, const string& iat, - string rq) noexcept + string rq) { vector rcrs; @@ -302,7 +304,7 @@ namespace brep error << "failed to mutate check runs: error HTTP response status " << sc; } - catch (const json::invalid_json_input& e) + catch (const json::invalid_json_input& e) // struct resp (via github_post()) { // Note: e.name is the GitHub API endpoint. // @@ -310,16 +312,16 @@ namespace brep << e.line << ", column: " << e.column << ", byte offset: " << e.position << ", error: " << e; } - catch (const invalid_argument& e) + catch (const invalid_argument& e) // github_post() { error << "malformed header(s) in response: " << e; } - catch (const system_error& e) + catch (const system_error& e) // github_post() { error << "unable to mutate check runs (errno=" << e.code () << "): " << e.what (); } - catch (const runtime_error& e) // From gq_parse_response_check_runs(). + catch (const runtime_error& e) // gq_parse_response_check_runs() { // GitHub response contained error(s) (could be ours or theirs at this // point). @@ -361,6 +363,9 @@ namespace brep // The details URL argument (`du`) can be empty for queued but not for the // other states. // + // Throw invalid_argument if any of the observed check run members are not + // valid GraphQL values (string, enum, etc). + // static string gq_mutation_create_check_runs (const string& ri, // Repository ID const string& hs, // Head SHA @@ -422,6 +427,9 @@ namespace brep // The details URL argument (`du`) can be empty for queued but not for the // other states. // + // Throw invalid_argument if any of the arguments or observed check run + // members are not valid GraphQL values (string, enum, etc). + // static string gq_mutation_create_check_run (const string& ri, // Repository ID const string& hs, // Head SHA @@ -484,6 +492,9 @@ namespace brep // because GitHub does not allow updating a check run to completed without a // conclusion. // + // Throw invalid_argument if any of the arguments are invalid values (of + // GraphQL types or otherwise). + // static string gq_mutation_update_check_run (const string& ri, // Repository ID. const string& ni, // Node ID. @@ -505,8 +516,17 @@ namespace brep << " status: " << gq_enum (st); if (sa) { - os << '\n'; - os << " startedAt: " << gq_str (gh_to_iso8601 (*sa)); + try + { + os << '\n'; + os << " startedAt: " << gq_str (gh_to_iso8601 (*sa)); + } + catch (const runtime_error& e) + { + throw invalid_argument ("invalid started_at value " + + to_string (system_clock::to_time_t (*sa)) + + ": " + e.what ()); + } } if (du) { @@ -634,6 +654,8 @@ namespace brep // Serialize a GraphQL query that fetches a pull request from GitHub. // + // Throw invalid_argument if the node id is not a valid GraphQL string. + // static string gq_query_pr_mergeability (const string& nid) { @@ -658,6 +680,8 @@ namespace brep const string& iat, const string& nid) { + // Let invalid_argument from gq_query_pr_mergeability() propagate. + // string rq (gq_serialize_request (gq_query_pr_mergeability (nid))); try @@ -760,7 +784,7 @@ namespace brep error << "failed to fetch pull request: error HTTP response status " << sc; } - catch (const json::invalid_json_input& e) + catch (const json::invalid_json_input& e) // struct resp (via github_post()) { // Note: e.name is the GitHub API endpoint. // @@ -768,16 +792,16 @@ namespace brep << e.line << ", column: " << e.column << ", byte offset: " << e.position << ", error: " << e; } - catch (const invalid_argument& e) + catch (const invalid_argument& e) // github_post() { error << "malformed header(s) in response: " << e; } - catch (const system_error& e) + catch (const system_error& e) // github_post() { error << "unable to fetch pull request (errno=" << e.code () << "): " << e.what (); } - catch (const runtime_error& e) // From response type's parsing constructor. + catch (const runtime_error& e) // struct resp { // GitHub response contained error(s) (could be ours or theirs at this // point). diff --git a/mod/mod-ci-github-gq.hxx b/mod/mod-ci-github-gq.hxx index 7c564d7..86ab859 100644 --- a/mod/mod-ci-github-gq.hxx +++ b/mod/mod-ci-github-gq.hxx @@ -24,6 +24,8 @@ namespace brep // `check_runs` with the new data (node id and state_synced). Return false // and issue diagnostics if the request failed. // + // Throw invalid_argument. + // // Note that creating a check_run named `foo` will effectively replace any // existing check_runs with that name. They will still exist on the GitHub // servers but GitHub will only consider the latest one (for display in the @@ -40,6 +42,8 @@ namespace brep // data (node id, state, and state_synced). Return false and issue // diagnostics if the request failed. // + // Throw invalid_argument. + // // If the details_url is absent GitHub will use the app's homepage. // // The gq_built_result is required if the build_state is built because @@ -66,6 +70,8 @@ namespace brep // Update a check run on GitHub. Update `cr` with the new data (state and // state_synced). Return false and issue diagnostics if the request failed. // + // Throw invalid_argument. + // // Note that GitHub allows any state transitions except from built (but // built to built is allowed). The latter case is signalled by setting the // check_run state_synced member to false and the state member to built. @@ -99,6 +105,8 @@ namespace brep // Issue diagnostics and return absent if the request failed (which means it // will be treated by the caller as still being generated). // + // Throw invalid_argument if the node id is invalid. + // // Note that the first request causes GitHub to start preparing the test // merge commit. // diff --git a/mod/mod-ci-github-service-data.cxx b/mod/mod-ci-github-service-data.cxx index 68a1426..d3071b2 100644 --- a/mod/mod-ci-github-service-data.cxx +++ b/mod/mod-ci-github-service-data.cxx @@ -10,6 +10,15 @@ namespace brep { using event = json::event; + [[noreturn]] static void + throw_json (json::parser& p, const string& m) + { + throw json::invalid_json_input ( + p.input_name, + p.line (), p.column (), p.position (), + m); + } + service_data:: service_data (const string& json) { @@ -32,11 +41,7 @@ namespace brep if (v == "local") kind = local; else if (v == "remote") kind = remote; else - { - throw json::invalid_json_input ( - p.input_name, p.line (), p.column (), p.position (), - "invalid service data kind: '" + v + '\''); - } + throw_json (p, "invalid service data kind: '" + v + '\''); } pre_check = p.next_expect_member_boolean ("pre_check"); @@ -44,7 +49,7 @@ namespace brep warning_success = p.next_expect_member_boolean ("warning_success"); - // Installation access token. + // Installation access token (IAT). // p.next_expect_name ("installation_access"); installation_access = gh_installation_access_token (p); @@ -79,7 +84,16 @@ namespace brep nid = *v; } - build_state s (to_build_state (p.next_expect_member_string ("state"))); + build_state s; + try + { + s = to_build_state (p.next_expect_member_string ("state")); + } + catch (const invalid_argument& e) + { + throw_json (p, e.what ()); + } + bool ss (p.next_expect_member_boolean ("state_synced")); optional rs; @@ -87,7 +101,14 @@ namespace brep string* v (p.next_expect_member_string_null ("status")); if (v != nullptr) { - rs = bbot::to_result_status (*v); + try + { + rs = bbot::to_result_status (*v); + } + catch (const invalid_argument& e) + { + throw_json (p, e.what ()); + } assert (s == build_state::built); } } @@ -186,11 +207,28 @@ namespace brep s.member ("warning_success", warning_success); - // Installation access token. + // Installation access token (IAT). // s.member_begin_object ("installation_access"); s.member ("token", installation_access.token); - s.member ("expires_at", gh_to_iso8601 (installation_access.expires_at)); + + // IAT expires_at timestamp. + // + { + string v; + try + { + v = gh_to_iso8601 (installation_access.expires_at); + } + catch (const runtime_error& e) + { + throw invalid_argument ("invalid IAT expires_at value " + + to_string (system_clock::to_time_t ( + installation_access.expires_at))); + } + s.member ("expires_at", move (v)); + } + s.end_object (); s.member ("installation_id", installation_id); @@ -232,7 +270,7 @@ namespace brep if (cr.status) { assert (cr.state == build_state::built); - s.value (to_string (*cr.status)); + s.value (to_string (*cr.status)); // Doesn't throw. } else s.value (nullptr); diff --git a/mod/mod-ci-github-service-data.hxx b/mod/mod-ci-github-service-data.hxx index cabd19a..776ec8d 100644 --- a/mod/mod-ci-github-service-data.hxx +++ b/mod/mod-ci-github-service-data.hxx @@ -139,6 +139,9 @@ namespace brep // // Throw invalid_argument if the schema version is not supported. // + // Throw invalid_argument (invalid_json_input) in case of malformed JSON + // or any invalid values. + // explicit service_data (const string& json); @@ -179,6 +182,11 @@ namespace brep // Serialize to JSON. // + // Throw invalid_argument if any values are invalid. + // + // May also throw invalid_json_output but that would be a programming + // error. + // string json () const; }; diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx index 46cd0cc..c703f32 100644 --- a/mod/mod-ci-github.cxx +++ b/mod/mod-ci-github.cxx @@ -2996,6 +2996,8 @@ namespace brep // iat.expires_at -= chrono::minutes (5); } + // gh_installation_access_token (via github_post()) + // catch (const json::invalid_json_input& e) { // Note: e.name is the GitHub API endpoint. @@ -3005,12 +3007,12 @@ namespace brep << e.position << ", error: " << e; return nullopt; } - catch (const invalid_argument& e) + catch (const invalid_argument& e) // github_post() { error << "malformed header(s) in response: " << e; return nullopt; } - catch (const system_error& e) + catch (const system_error& e) // github_post() { error << "unable to get installation access token (errno=" << e.code () << "): " << e.what (); diff --git a/mod/mod-ci-github.hxx b/mod/mod-ci-github.hxx index b88d5e4..c38462a 100644 --- a/mod/mod-ci-github.hxx +++ b/mod/mod-ci-github.hxx @@ -107,7 +107,9 @@ namespace brep optional generate_jwt (const basic_mark& trace, const basic_mark& error) const; - // Authenticate to GitHub as an app installation. + // Authenticate to GitHub as an app installation. Return the installation + // access token (IAT). Issue diagnostics and return nullopt if something + // goes wrong. // optional obtain_installation_access_token (uint64_t install_id, -- cgit v1.1