From 8b83eca0c1f95c7ec1d6127e5a460210d906b75f Mon Sep 17 00:00:00 2001 From: damocles Date: Tue, 18 Aug 2026 15:24:49 +0200 Subject: [PATCH] fix(#3435): hive-forge issue/pr show fail on forgejo's null-for-empty reactions Forgejo returns a bare `null` body (not `[]`) for a reactions list when nothing has reacted yet. issue_reactions/comment_reactions deserialized straight into forgejo-api's typed Vec, which has no null tolerance, so issue show / pr show / view failed on every item with zero reactions - i.e. nearly everything. Generalize pr_status's existing null_as_empty (same quirk, hit earlier on combined-status statuses) into a shared helper in verbs/mod.rs, and fetch reactions via the raw JSON path (Client::get_api_json) with a NullableVec wrapper instead of the typed client's bare Vec. Also names the failing request in errors going forward, since get_api_json's error context includes the URL - closes the gap the issue itself flagged (the old error said what didn't parse but not what was fetched). --- hive-forge/src/verbs/mod.rs | 75 ++++++++++++++++++++++++++----- hive-forge/src/verbs/pr_status.rs | 14 ++---- 2 files changed, 67 insertions(+), 22 deletions(-) diff --git a/hive-forge/src/verbs/mod.rs b/hive-forge/src/verbs/mod.rs index df0f3f6f..b48e9965 100644 --- a/hive-forge/src/verbs/mod.rs +++ b/hive-forge/src/verbs/mod.rs @@ -49,6 +49,7 @@ use std::fmt::Write as _; use anyhow::Result; use forgejo_api::structs::{Attachment, Reaction}; +use serde::Deserialize; use serde_json::{Value, json}; use time::OffsetDateTime; use time::format_description::well_known::Rfc3339; @@ -381,6 +382,42 @@ pub(crate) fn dependency_summaries( .collect()) } +/// Deserialize a possibly-null JSON array as an empty `Vec`. +/// +/// Several Forgejo list endpoints return an explicit `null` body instead of +/// `[]` when the collection is empty — a commit's combined-status +/// `statuses` field when no CI is configured (`pr_status`'s own use of +/// this), and issue/comment reactions when nothing has reacted yet (a real +/// incident: `issue show`/`pr show` failing on *every* item because every +/// item's reaction fetch hit this). `#[serde(default)]` alone only +/// covers a *missing* key — a *present* `null` still fails to deserialize +/// into a bare `Vec`, which is why this explicit `deserialize_with` is +/// needed rather than the derive default. +pub(crate) fn null_as_empty<'de, D, T>(de: D) -> Result, D::Error> +where + D: serde::Deserializer<'de>, + T: Deserialize<'de>, +{ + Ok(Option::>::deserialize(de)?.unwrap_or_default()) +} + +/// A JSON array that tolerates Forgejo's null-for-empty quirk (see +/// [`null_as_empty`]). Used to fetch reactions directly via +/// [`Client::get_api_json`] rather than through forgejo-api's generated +/// client, which deserializes straight into a bare `Vec` with no +/// null tolerance and fails on exactly the common case of "nothing has +/// reacted to this yet". +struct NullableVec(Vec); + +impl<'de, T: Deserialize<'de>> Deserialize<'de> for NullableVec { + fn deserialize(de: D) -> Result + where + D: serde::Deserializer<'de>, + { + Ok(Self(null_as_empty(de)?)) + } +} + /// The current reactions on an issue or PR itself (not a comment) — each /// entry's `content` is Forgejo's shortcode (`"+1"`, `"heart"`, …, the /// same vocabulary GitHub uses), `user` its login. Same shared-index @@ -397,11 +434,11 @@ pub(crate) fn issue_reactions( name: &str, number: u64, ) -> Result> { - let (_, reactions) = client - .api() - .issue_get_issue_reactions(owner, name, index(number)?) - .send()?; - Ok(reaction_values(reactions)) + let reactions: NullableVec = client.get_api_json( + &format!("/repos/{owner}/{name}/issues/{number}/reactions"), + &[], + )?; + Ok(reaction_values(reactions.0)) } /// The current reactions on a single comment, by comment id (not the @@ -417,11 +454,11 @@ pub(crate) fn comment_reactions( name: &str, comment_id: u64, ) -> Result> { - let reactions = client - .api() - .issue_get_comment_reactions(owner, name, index(comment_id)?) - .send()?; - Ok(reaction_values(reactions)) + let reactions: NullableVec = client.get_api_json( + &format!("/repos/{owner}/{name}/issues/comments/{comment_id}/reactions"), + &[], + )?; + Ok(reaction_values(reactions.0)) } fn reaction_values(reactions: Vec) -> Vec { @@ -488,7 +525,23 @@ pub(crate) fn attachment_json(assets: Option<&[Attachment]>) -> Vec { #[cfg(test)] mod tests { - use super::{pct_encode, reviewed_older_head}; + use super::{NullableVec, pct_encode, reviewed_older_head}; + + #[test] + fn nullable_vec_treats_a_json_null_as_empty() { + // The exact shape of the real incident this guards: Forgejo answers + // a reactions (or combined-status) list with a bare `null` body + // when the collection is empty, not `[]` — deserializing straight + // into `Vec` fails on it, `NullableVec` must not. + let v: NullableVec = serde_json::from_str("null").unwrap(); + assert_eq!(v.0, Vec::::new()); + } + + #[test] + fn nullable_vec_passes_a_real_array_through() { + let v: NullableVec = serde_json::from_str("[1,2,3]").unwrap(); + assert_eq!(v.0, vec![1, 2, 3]); + } #[test] fn review_on_an_older_commit_is_stale() { diff --git a/hive-forge/src/verbs/pr_status.rs b/hive-forge/src/verbs/pr_status.rs index 3008e608..2d44014a 100644 --- a/hive-forge/src/verbs/pr_status.rs +++ b/hive-forge/src/verbs/pr_status.rs @@ -12,7 +12,6 @@ use anyhow::{Context, Result, bail}; use clap::Args as ClapArgs; use forgejo_api::structs::IssueGetCommentsQuery; -use serde::Deserialize; use serde_json::Value; use crate::client::{Client, index}; @@ -60,23 +59,16 @@ pub fn run(client: &Client, args: Args) -> Result<()> { /// than `[]`, and `#[serde(default)]` only covers a *missing* key — a /// present null still fails to deserialize. So a doc-only repo makes the /// whole verb error out on exactly the PRs where "no CI here" is the -/// answer worth printing. +/// answer worth printing. See [`super::null_as_empty`] (shared with the +/// reaction-listing helpers, which hit the same quirk). #[derive(serde::Deserialize, Default)] pub(crate) struct CombinedStatus { #[serde(default)] pub state: String, - #[serde(default, deserialize_with = "null_as_empty")] + #[serde(default, deserialize_with = "super::null_as_empty")] pub statuses: Vec, } -/// Deserialize a possibly-null JSON array as an empty `Vec`. -fn null_as_empty<'de, D>(de: D) -> Result, D::Error> -where - D: serde::Deserializer<'de>, -{ - Ok(Option::>::deserialize(de)?.unwrap_or_default()) -} - /// CI-only path for an explicit commit. Exit code mirrors the CI verdict. fn sha_status(client: &Client, sha: &str) -> Result<()> { let (state, statuses) = fetch_combined(client, sha)?;