fix(#2565): lenient notifications deserialize in hive-forge read-guard
This commit is contained in:
parent
742ed51d57
commit
b670291347
2 changed files with 92 additions and 14 deletions
|
|
@ -195,6 +195,33 @@ impl Client {
|
||||||
.map(|b| b.to_vec())
|
.map(|b| b.to_vec())
|
||||||
.with_context(|| format!("read bytes for GET {url}"))
|
.with_context(|| format!("read bytes for GET {url}"))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// GET a JSON `/api/v1` route and deserialize into `T`, bypassing the
|
||||||
|
/// typed `forgejo-api` client. Use this where the typed client's
|
||||||
|
/// structs are too strict against the running Forgejo version: the
|
||||||
|
/// crate pins one schema, but the server tracks the latest release
|
||||||
|
/// line, so a drifted field breaks deserialization for the whole
|
||||||
|
/// call. Callers pass a *lenient* local struct (only the fields they
|
||||||
|
/// use, all `#[serde(default)]`) so a schema change can't wedge the
|
||||||
|
/// call. `path` starts with `/` and is relative to `/api/v1`; `query`
|
||||||
|
/// is appended as URL query parameters.
|
||||||
|
///
|
||||||
|
/// # Errors
|
||||||
|
/// Returns an error on transport failure, a non-2xx response (body
|
||||||
|
/// included), or if the response body doesn't deserialize into `T`.
|
||||||
|
pub fn get_api_json<T: serde::de::DeserializeOwned>(
|
||||||
|
&self,
|
||||||
|
path: &str,
|
||||||
|
query: &[(&str, &str)],
|
||||||
|
) -> Result<T> {
|
||||||
|
let base = format!("{}/api/v1{path}", self.base);
|
||||||
|
let url = url::Url::parse_with_params(&base, query.iter().copied())
|
||||||
|
.with_context(|| format!("build url {base}"))?;
|
||||||
|
let resp = self.web.get(url.clone()).send().context("GET")?;
|
||||||
|
let resp = check_status(resp, &format!("GET {url}"))?;
|
||||||
|
resp.json::<T>()
|
||||||
|
.with_context(|| format!("decode JSON for GET {url}"))
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Split an `owner/name` repo string into its two path segments for
|
/// Split an `owner/name` repo string into its two path segments for
|
||||||
|
|
|
||||||
|
|
@ -13,7 +13,7 @@
|
||||||
//! `forge cli: agents keep commenting without reading prev comments`.)
|
//! `forge cli: agents keep commenting without reading prev comments`.)
|
||||||
|
|
||||||
use anyhow::Result;
|
use anyhow::Result;
|
||||||
use forgejo_api::structs::{NotifyGetRepoListQuery, NotifyReadThreadQuery};
|
use forgejo_api::structs::NotifyReadThreadQuery;
|
||||||
|
|
||||||
use crate::client::{Client, index, split_repo};
|
use crate::client::{Client, index, split_repo};
|
||||||
|
|
||||||
|
|
@ -33,25 +33,47 @@ const MAX_PAGES: u32 = 5;
|
||||||
/// notification (nothing to read / already read). Repo-scoped so a bare
|
/// notification (nothing to read / already read). Repo-scoped so a bare
|
||||||
/// number can't false-match another repo. Errors only on transport /
|
/// number can't false-match another repo. Errors only on transport /
|
||||||
/// non-2xx — callers degrade open on `Err`.
|
/// non-2xx — callers degrade open on `Err`.
|
||||||
|
/// Minimal, drift-tolerant view of a Forgejo notification thread — only
|
||||||
|
/// the two fields the read-before-comment guard needs: `id` (to mark the
|
||||||
|
/// thread read) and `subject.url` (to match the thread to an issue/PR
|
||||||
|
/// number). Both `#[serde(default)]` so a schema change anywhere else in
|
||||||
|
/// the notification object can't break this deserialize. This is the
|
||||||
|
/// deliberate replacement for `forgejo-api`'s strict `NotificationThread`,
|
||||||
|
/// whose schema drift vs the running (latest-line) Forgejo was breaking
|
||||||
|
/// the whole notifications call — silently disabling the guard and
|
||||||
|
/// wedging notification read-state (so wakes kept redelivering).
|
||||||
|
#[derive(serde::Deserialize)]
|
||||||
|
struct Notification {
|
||||||
|
#[serde(default)]
|
||||||
|
id: Option<i64>,
|
||||||
|
#[serde(default)]
|
||||||
|
subject: Option<Subject>,
|
||||||
|
}
|
||||||
|
|
||||||
|
/// The one notification-subject field the guard reads: the API `url`,
|
||||||
|
/// which ends in the issue/PR number (see [`subject_matches`]).
|
||||||
|
#[derive(serde::Deserialize)]
|
||||||
|
struct Subject {
|
||||||
|
#[serde(default)]
|
||||||
|
url: Option<String>,
|
||||||
|
}
|
||||||
|
|
||||||
pub fn unread_thread_id(client: &Client, repo: &str, number: u64) -> Result<Option<u64>> {
|
pub fn unread_thread_id(client: &Client, repo: &str, number: u64) -> Result<Option<u64>> {
|
||||||
let (owner, name) = split_repo(repo)?;
|
let (owner, name) = split_repo(repo)?;
|
||||||
|
let limit = PAGE_SIZE.to_string();
|
||||||
|
let path = format!("/repos/{owner}/{name}/notifications");
|
||||||
for page in 1..=MAX_PAGES {
|
for page in 1..=MAX_PAGES {
|
||||||
let query = NotifyGetRepoListQuery {
|
let page_str = page.to_string();
|
||||||
all: Some(false),
|
let threads: Vec<Notification> = client.get_api_json(
|
||||||
..Default::default()
|
&path,
|
||||||
};
|
&[("all", "false"), ("page", &page_str), ("limit", &limit)],
|
||||||
let (_, threads) = client
|
)?;
|
||||||
.api()
|
|
||||||
.notify_get_repo_list(owner, name, query)
|
|
||||||
.page(page)
|
|
||||||
.page_size(PAGE_SIZE)
|
|
||||||
.send()?;
|
|
||||||
for n in &threads {
|
for n in &threads {
|
||||||
let subject_url = n
|
let subject_url = n
|
||||||
.subject
|
.subject
|
||||||
.as_ref()
|
.as_ref()
|
||||||
.and_then(|s| s.url.as_ref())
|
.and_then(|s| s.url.as_deref())
|
||||||
.map_or("", url::Url::as_str);
|
.unwrap_or("");
|
||||||
if subject_matches(subject_url, number) {
|
if subject_matches(subject_url, number) {
|
||||||
return Ok(n.id.and_then(|id| u64::try_from(id).ok()));
|
return Ok(n.id.and_then(|id| u64::try_from(id).ok()));
|
||||||
}
|
}
|
||||||
|
|
@ -101,7 +123,36 @@ pub fn subject_matches(subject_url: &str, number: u64) -> bool {
|
||||||
|
|
||||||
#[cfg(test)]
|
#[cfg(test)]
|
||||||
mod tests {
|
mod tests {
|
||||||
use super::subject_matches;
|
use super::{Notification, subject_matches};
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn notification_deserializes_leniently_from_drifted_json() {
|
||||||
|
// A payload carrying fields the guard doesn't model (which would
|
||||||
|
// break `forgejo-api`'s strict struct) plus a missing subject —
|
||||||
|
// must still yield `id` + `subject.url` and never fail to parse.
|
||||||
|
let json = serde_json::json!([
|
||||||
|
{
|
||||||
|
"id": 42,
|
||||||
|
"unread": true,
|
||||||
|
"pinned": false,
|
||||||
|
"subject": {
|
||||||
|
"title": "whatever",
|
||||||
|
"url": "https://forge/api/v1/repos/o/r/issues/7",
|
||||||
|
"type": "SomeFutureSubjectTypeWeDoNotKnow"
|
||||||
|
},
|
||||||
|
"some_new_field": { "nested": 1 }
|
||||||
|
},
|
||||||
|
{ "id": 43 }
|
||||||
|
]);
|
||||||
|
let ns: Vec<Notification> = serde_json::from_value(json).expect("lenient deser");
|
||||||
|
assert_eq!(ns[0].id, Some(42));
|
||||||
|
assert_eq!(
|
||||||
|
ns[0].subject.as_ref().and_then(|s| s.url.as_deref()),
|
||||||
|
Some("https://forge/api/v1/repos/o/r/issues/7")
|
||||||
|
);
|
||||||
|
assert_eq!(ns[1].id, Some(43));
|
||||||
|
assert!(ns[1].subject.is_none());
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn matches_issue_subject_url() {
|
fn matches_issue_subject_url() {
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue