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<Reaction>, 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<T> wrapper instead of the typed client's bare Vec<Reaction>. 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).
This commit is contained in:
parent
fb9cc5635a
commit
8b83eca0c1
2 changed files with 67 additions and 22 deletions
|
|
@ -49,6 +49,7 @@ use std::fmt::Write as _;
|
||||||
|
|
||||||
use anyhow::Result;
|
use anyhow::Result;
|
||||||
use forgejo_api::structs::{Attachment, Reaction};
|
use forgejo_api::structs::{Attachment, Reaction};
|
||||||
|
use serde::Deserialize;
|
||||||
use serde_json::{Value, json};
|
use serde_json::{Value, json};
|
||||||
use time::OffsetDateTime;
|
use time::OffsetDateTime;
|
||||||
use time::format_description::well_known::Rfc3339;
|
use time::format_description::well_known::Rfc3339;
|
||||||
|
|
@ -381,6 +382,42 @@ pub(crate) fn dependency_summaries(
|
||||||
.collect())
|
.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<T>`, 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<Vec<T>, D::Error>
|
||||||
|
where
|
||||||
|
D: serde::Deserializer<'de>,
|
||||||
|
T: Deserialize<'de>,
|
||||||
|
{
|
||||||
|
Ok(Option::<Vec<T>>::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<Reaction>` with no
|
||||||
|
/// null tolerance and fails on exactly the common case of "nothing has
|
||||||
|
/// reacted to this yet".
|
||||||
|
struct NullableVec<T>(Vec<T>);
|
||||||
|
|
||||||
|
impl<'de, T: Deserialize<'de>> Deserialize<'de> for NullableVec<T> {
|
||||||
|
fn deserialize<D>(de: D) -> Result<Self, D::Error>
|
||||||
|
where
|
||||||
|
D: serde::Deserializer<'de>,
|
||||||
|
{
|
||||||
|
Ok(Self(null_as_empty(de)?))
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
/// The current reactions on an issue or PR itself (not a comment) — each
|
/// The current reactions on an issue or PR itself (not a comment) — each
|
||||||
/// entry's `content` is Forgejo's shortcode (`"+1"`, `"heart"`, …, the
|
/// entry's `content` is Forgejo's shortcode (`"+1"`, `"heart"`, …, the
|
||||||
/// same vocabulary GitHub uses), `user` its login. Same shared-index
|
/// same vocabulary GitHub uses), `user` its login. Same shared-index
|
||||||
|
|
@ -397,11 +434,11 @@ pub(crate) fn issue_reactions(
|
||||||
name: &str,
|
name: &str,
|
||||||
number: u64,
|
number: u64,
|
||||||
) -> Result<Vec<Value>> {
|
) -> Result<Vec<Value>> {
|
||||||
let (_, reactions) = client
|
let reactions: NullableVec<Reaction> = client.get_api_json(
|
||||||
.api()
|
&format!("/repos/{owner}/{name}/issues/{number}/reactions"),
|
||||||
.issue_get_issue_reactions(owner, name, index(number)?)
|
&[],
|
||||||
.send()?;
|
)?;
|
||||||
Ok(reaction_values(reactions))
|
Ok(reaction_values(reactions.0))
|
||||||
}
|
}
|
||||||
|
|
||||||
/// The current reactions on a single comment, by comment id (not the
|
/// The current reactions on a single comment, by comment id (not the
|
||||||
|
|
@ -417,11 +454,11 @@ pub(crate) fn comment_reactions(
|
||||||
name: &str,
|
name: &str,
|
||||||
comment_id: u64,
|
comment_id: u64,
|
||||||
) -> Result<Vec<Value>> {
|
) -> Result<Vec<Value>> {
|
||||||
let reactions = client
|
let reactions: NullableVec<Reaction> = client.get_api_json(
|
||||||
.api()
|
&format!("/repos/{owner}/{name}/issues/comments/{comment_id}/reactions"),
|
||||||
.issue_get_comment_reactions(owner, name, index(comment_id)?)
|
&[],
|
||||||
.send()?;
|
)?;
|
||||||
Ok(reaction_values(reactions))
|
Ok(reaction_values(reactions.0))
|
||||||
}
|
}
|
||||||
|
|
||||||
fn reaction_values(reactions: Vec<Reaction>) -> Vec<Value> {
|
fn reaction_values(reactions: Vec<Reaction>) -> Vec<Value> {
|
||||||
|
|
@ -488,7 +525,23 @@ pub(crate) fn attachment_json(assets: Option<&[Attachment]>) -> Vec<Value> {
|
||||||
|
|
||||||
#[cfg(test)]
|
#[cfg(test)]
|
||||||
mod tests {
|
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<T>` fails on it, `NullableVec<T>` must not.
|
||||||
|
let v: NullableVec<i64> = serde_json::from_str("null").unwrap();
|
||||||
|
assert_eq!(v.0, Vec::<i64>::new());
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn nullable_vec_passes_a_real_array_through() {
|
||||||
|
let v: NullableVec<i64> = serde_json::from_str("[1,2,3]").unwrap();
|
||||||
|
assert_eq!(v.0, vec![1, 2, 3]);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn review_on_an_older_commit_is_stale() {
|
fn review_on_an_older_commit_is_stale() {
|
||||||
|
|
|
||||||
|
|
@ -12,7 +12,6 @@
|
||||||
use anyhow::{Context, Result, bail};
|
use anyhow::{Context, Result, bail};
|
||||||
use clap::Args as ClapArgs;
|
use clap::Args as ClapArgs;
|
||||||
use forgejo_api::structs::IssueGetCommentsQuery;
|
use forgejo_api::structs::IssueGetCommentsQuery;
|
||||||
use serde::Deserialize;
|
|
||||||
use serde_json::Value;
|
use serde_json::Value;
|
||||||
|
|
||||||
use crate::client::{Client, index};
|
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
|
/// than `[]`, and `#[serde(default)]` only covers a *missing* key — a
|
||||||
/// present null still fails to deserialize. So a doc-only repo makes the
|
/// 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
|
/// 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)]
|
#[derive(serde::Deserialize, Default)]
|
||||||
pub(crate) struct CombinedStatus {
|
pub(crate) struct CombinedStatus {
|
||||||
#[serde(default)]
|
#[serde(default)]
|
||||||
pub state: String,
|
pub state: String,
|
||||||
#[serde(default, deserialize_with = "null_as_empty")]
|
#[serde(default, deserialize_with = "super::null_as_empty")]
|
||||||
pub statuses: Vec<Value>,
|
pub statuses: Vec<Value>,
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Deserialize a possibly-null JSON array as an empty `Vec`.
|
|
||||||
fn null_as_empty<'de, D>(de: D) -> Result<Vec<Value>, D::Error>
|
|
||||||
where
|
|
||||||
D: serde::Deserializer<'de>,
|
|
||||||
{
|
|
||||||
Ok(Option::<Vec<Value>>::deserialize(de)?.unwrap_or_default())
|
|
||||||
}
|
|
||||||
|
|
||||||
/// CI-only path for an explicit commit. Exit code mirrors the CI verdict.
|
/// CI-only path for an explicit commit. Exit code mirrors the CI verdict.
|
||||||
fn sha_status(client: &Client, sha: &str) -> Result<()> {
|
fn sha_status(client: &Client, sha: &str) -> Result<()> {
|
||||||
let (state, statuses) = fetch_combined(client, sha)?;
|
let (state, statuses) = fetch_combined(client, sha)?;
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue