MrSphay/Modrinth-plus
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Batch of tech review backend fixes (#5398)

Browse Source 
* Don't enter project into tech review if no new traces

* Send tech review exited message if files are deleted

* change PATCH /issue-detail/{id} to batch update details

* Fix sorting

* store delphi jar in backend

* show jar in tech review card

* improve jar display in frontend

* Fix live/in review label for tech review cards

* sqlx prepare

* polish: decode segments + code qual fix

* fix: skip first seg

* fix: only slice if needed

* Fix tech rev card styling

---------

Co-authored-by: Calum H. (IMB11) <contact@cal.engineer>
This commit is contained in:
aecsocket
2026-02-20 12:54:36 +00:00
committed by GitHub
parent 33ff2a0759
commit f052ecd702
18 changed files with 693 additions and 311 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
apps/labrinth/src/database/models/delphi_report_item.rs
View File

@@ -221,6 +221,9 @@ pub struct ReportIssueDetail {
/// This acts as a stable identifier for an issue detail, even across
/// different versions of the same file.
pub key: String,
/// If this detail was found inside a JAR embedded inside the scanned JAR,
/// this will point to the path of that JAR inside the outer JAR.
pub jar: Option<String>,
/// Name of the Java class path in which this issue was found.
pub file_path: String,
/// Decompiled, pretty-printed source of the Java class.
@@ -241,12 +244,13 @@ impl ReportIssueDetail {
) -> Result<DelphiReportIssueDetailsId, DatabaseError> {
Ok(DelphiReportIssueDetailsId(sqlx::query_scalar!(
"
INSERT INTO delphi_report_issue_details (issue_id, key, file_path, decompiled_source, data, severity)
VALUES ($1, $2, $3, $4, $5, $6)
INSERT INTO delphi_report_issue_details (issue_id, key, jar, file_path, decompiled_source, data, severity)
VALUES ($1, $2, $3, $4, $5, $6, $7)
RETURNING id
",
self.issue_id as DelphiReportIssueId,
self.key,
self.jar,
self.file_path,
self.decompiled_source,
sqlx::types::Json(&self.data) as Json<&HashMap<String, serde_json::Value>>,

153
apps/labrinth/src/routes/internal/delphi.rs
View File

@@ -59,15 +59,42 @@ static DELPHI_CLIENT: LazyLock<reqwest::Client> = LazyLock::new(|| {
.unwrap()
});
#[derive(Deserialize)]
/// Type of [`DelphiReportIssueDetails::key`].
///
/// Delphi may provide `null` for the key, but we require a key for storing
/// issue details in the database, since detail verdicts are keyed by
/// (project id, issue detail key). Keys are opaque strings generated by Delphi
/// which refer to some "unique location" in a JAR file, such that subsequent
/// Delphi scans of different JARs with the same issue detail will result in
/// having the same key.
///
/// If Delphi doesn't provide us with a key, we generate a random one.
#[derive(Debug, Clone)]
pub struct IssueDetailKey(pub String);
impl<'de> Deserialize<'de> for IssueDetailKey {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: serde::Deserializer<'de>,
{
let value = Option::<String>::deserialize(deserializer)?;
let value = value.unwrap_or_else(|| {
format!("<no-key-{:016x}>", rand::random::<u64>())
});
Ok(Self(value))
}
}
#[derive(Debug, Deserialize)]
struct DelphiReportIssueDetails {
pub file: String,
pub key: String,
pub key: IssueDetailKey,
pub jar: Option<String>,
pub data: HashMap<String, serde_json::Value>,
pub severity: DelphiSeverity,
}
#[derive(Deserialize)]
#[derive(Debug, Deserialize)]
struct DelphiReport {
pub url: String,
pub project_id: crate::models::ids::ProjectId,
@@ -205,11 +232,34 @@ async fn ingest_report_deserialized(
.await
.wrap_internal_err("failed to check if pending issue details exist")?;
if record.pending_issue_details_exist {
info!(
"File's project already has pending issue details, is not entering tech review queue"
);
} else {
let issue_detail_keys = report
.issues
.values()
.flatten()
.map(|issue_detail| issue_detail.key.0.clone())
.collect::<Vec<_>>();
let has_unflagged_issue_details = sqlx::query!(
r#"
SELECT EXISTS(
SELECT 1
FROM unnest($2::text[]) AS incoming(detail_key)
LEFT JOIN delphi_issue_detail_verdicts didv
ON didv.project_id = $1 AND didv.detail_key = incoming.detail_key
WHERE didv.project_id IS NULL
) AS "has_unflagged_issue_details!"
"#,
DBProjectId::from(report.project_id) as _,
&issue_detail_keys
)
.fetch_one(&mut transaction)
.await
.wrap_internal_err("failed to check if report has unflagged issue details")?;
let should_enter_tech_review = !record.pending_issue_details_exist
&& has_unflagged_issue_details.has_unflagged_issue_details;
if should_enter_tech_review {
info!("File's project is entering tech review queue");
ThreadMessageBuilder {
@@ -221,6 +271,10 @@ async fn ingest_report_deserialized(
.insert(&mut transaction)
.await
.wrap_internal_err("failed to add entering tech review message")?;
} else {
info!(
"File's project is not entering tech review queue (already pending or no new unflagged issue details)"
);
}
// TODO: Currently, the way we determine if an issue is in tech review or not
@@ -232,7 +286,7 @@ async fn ingest_report_deserialized(
// This is undesirable, but we can't rework the database schema to fix it
// right now. As a hack, we add a dummy report issue which blocks the
// project from exiting the tech review queue.
{
if should_enter_tech_review {
let dummy_issue_id = DBDelphiReportIssue {
id: DelphiReportIssueId(0), // This will be set by the database
report_id,
@@ -245,6 +299,7 @@ async fn ingest_report_deserialized(
id: DelphiReportIssueDetailsId(0), // This will be set by the database
issue_id: dummy_issue_id,
key: "".into(),
jar: None,
file_path: "".into(),
decompiled_source: None,
data: HashMap::new(),
@@ -275,7 +330,8 @@ async fn ingest_report_deserialized(
ReportIssueDetail {
id: DelphiReportIssueDetailsId(0), // This will be set by the database
issue_id,
key: issue_detail.key,
key: issue_detail.key.0,
jar: issue_detail.jar,
file_path: issue_detail.file,
decompiled_source: decompiled_source.cloned().flatten(),
data: issue_detail.data,
@@ -333,6 +389,83 @@ pub async fn run(
Ok(HttpResponse::NoContent().finish())
}
pub async fn is_project_in_tech_review(
project_id: DBProjectId,
exec: impl crate::database::Executor<'_, Database = sqlx::Postgres>,
) -> Result<bool, ApiError> {
let row = sqlx::query!(
r#"
SELECT EXISTS(
SELECT 1
FROM delphi_issue_details_with_statuses didws
INNER JOIN delphi_report_issues dri ON dri.id = didws.issue_id
WHERE
didws.project_id = $1
AND didws.status = 'pending'
-- see delphi.rs todo comment
AND dri.issue_type != '__dummy'
) AS "is_in_tech_review!"
"#,
project_id as _,
)
.fetch_one(exec)
.await
.wrap_internal_err("failed to fetch project tech review state")?;
Ok(row.is_in_tech_review)
}
pub async fn send_tech_review_exit_file_deleted_message(
project_id: DBProjectId,
txn: &mut crate::database::PgTransaction<'_>,
) -> Result<(), ApiError> {
let thread = sqlx::query!(
r#"
SELECT id AS "thread_id: DBThreadId"
FROM threads
WHERE mod_id = $1
LIMIT 1
"#,
project_id as _,
)
.fetch_optional(&mut *txn)
.await
.wrap_internal_err("failed to fetch thread for tech review exit message")?;
if let Some(thread) = thread {
ThreadMessageBuilder {
author_id: None,
body: MessageBody::TechReviewExitFileDeleted,
thread_id: thread.thread_id,
hide_identity: false,
}
.insert(txn)
.await
.wrap_internal_err("failed to add tech review exit message")?;
}
Ok(())
}
pub async fn send_tech_review_exit_file_deleted_message_if_exited(
project_id: DBProjectId,
was_in_tech_review: bool,
txn: &mut crate::database::PgTransaction<'_>,
) -> Result<(), ApiError> {
if !was_in_tech_review {
return Ok(());
}
let is_still_in_tech_review =
is_project_in_tech_review(project_id, &mut *txn).await?;
if !is_still_in_tech_review {
send_tech_review_exit_file_deleted_message(project_id, txn).await?;
}
Ok(())
}
#[post("run")]
async fn _run(
req: HttpRequest,

118
apps/labrinth/src/routes/internal/moderation/tech_review.rs
View File

@@ -42,7 +42,7 @@ pub fn config(cfg: &mut utoipa_actix_web::service_config::ServiceConfig) {
.service(get_report)
.service(get_issue)
.service(submit_report)
.service(update_issue_detail)
.service(update_issue_details)
.service(add_report);
}
@@ -388,6 +388,8 @@ pub struct ProjectModerationInfo {
pub thread_id: ThreadId,
/// Project name.
pub name: String,
/// Current project status.
pub status: ProjectStatus,
/// The aggregated project typos of the versions of this project
#[serde(default)]
pub project_types: Vec<String>,
@@ -498,6 +500,7 @@ async fn fetch_project_reports(
drid.id AS "id!: DelphiReportIssueDetailsId",
drid.issue_id AS "issue_id!: DelphiReportIssueId",
drid.key AS "key!: String",
drid.jar AS "jar?: String",
drid.file_path AS "file_path!: String",
drid.data AS "data!: sqlx::types::Json<HashMap<String, serde_json::Value>>",
drid.severity AS "severity!: DelphiSeverity",
@@ -561,6 +564,7 @@ async fn fetch_project_reports(
id: d.id,
issue_id: d.issue_id,
key: d.key,
jar: d.jar,
file_path: d.file_path,
decompiled_source: None,
data: d.data.0,
@@ -698,9 +702,9 @@ async fn search_projects(
let rows = sqlx::query!(
r#"
SELECT DISTINCT ON (m.id)
SELECT
m.id AS "project_id: DBProjectId",
t.id AS "thread_id: DBThreadId"
MIN(t.id) AS "thread_id!: DBThreadId"
FROM mods m
INNER JOIN threads t ON t.mod_id = m.id
INNER JOIN versions v ON v.mod_id = m.id
@@ -734,12 +738,14 @@ async fn search_projects(
OR ($5::text = 'unreplied' AND (tm_last.id IS NULL OR u_last.role IS NULL OR u_last.role NOT IN ('moderator', 'admin')))
OR ($5::text = 'replied' AND tm_last.id IS NOT NULL AND u_last.role IS NOT NULL AND u_last.role IN ('moderator', 'admin'))
)
GROUP BY m.id, t.id
ORDER BY m.id,
CASE WHEN $3 = 'created_asc' THEN MIN(dr.created) ELSE TO_TIMESTAMP(0) END ASC,
CASE WHEN $3 = 'created_desc' THEN MAX(dr.created) ELSE TO_TIMESTAMP(0) END DESC,
CASE WHEN $3 = 'severity_asc' THEN MAX(dr.severity) ELSE 'low'::delphi_severity END ASC,
CASE WHEN $3 = 'severity_desc' THEN MAX(dr.severity) ELSE 'low'::delphi_severity END DESC
GROUP BY m.id
ORDER BY
CASE WHEN $3 = 'created_asc' THEN MIN(dr.created) ELSE TO_TIMESTAMP(0) END ASC,
CASE WHEN $3 = 'created_desc' THEN MIN(dr.created) ELSE TO_TIMESTAMP(0) END DESC,
CASE WHEN $3 = 'severity_asc' THEN MAX(dr.severity) ELSE 'low'::delphi_severity END ASC,
CASE WHEN $3 = 'severity_desc' THEN MAX(dr.severity) ELSE 'low'::delphi_severity END DESC,
-- tie-breaker: oldest reports
MIN(dr.created) ASC
LIMIT $1 OFFSET $2
"#,
limit,
@@ -831,6 +837,7 @@ async fn search_projects(
id,
thread_id: project.thread_id,
name: project.name,
status: project.status,
project_types: project.project_types,
icon_url: project.icon_url,
},
@@ -1119,6 +1126,8 @@ async fn submit_report(
/// See [`update_issue`].
#[derive(Debug, Clone, Serialize, Deserialize, utoipa::ToSchema)]
pub struct UpdateIssue {
/// ID of the issue detail to update.
pub detail_id: DelphiReportIssueDetailsId,
/// What the moderator has decided the outcome of this issue is.
pub verdict: DelphiVerdict,
}
@@ -1131,14 +1140,13 @@ pub struct UpdateIssue {
security(("bearer_auth" = [])),
responses((status = NO_CONTENT))
)]
#[patch("/issue-detail/{id}")]
async fn update_issue_detail(
#[patch("/issue-detail")]
async fn update_issue_details(
req: HttpRequest,
pool: web::Data<PgPool>,
redis: web::Data<RedisPool>,
session_queue: web::Data<AuthQueue>,
update_req: web::Json<UpdateIssue>,
path: web::Path<(DelphiReportIssueDetailsId,)>,
update_reqs: web::Json<Vec<UpdateIssue>>,
) -> Result<(), ApiError> {
check_is_moderator_from_headers(
&req,
@@ -1148,44 +1156,76 @@ async fn update_issue_detail(
Scopes::PROJECT_WRITE,
)
.await?;
let (issue_detail_id,) = path.into_inner();
let mut txn = pool
.begin()
.await
.wrap_internal_err("failed to start transaction")?;
let status = match update_req.verdict {
DelphiVerdict::Safe => DelphiStatus::Safe,
DelphiVerdict::Unsafe => DelphiStatus::Unsafe,
};
let results = sqlx::query!(
let updates = update_reqs.into_inner();
let detail_ids = updates.iter().map(|u| u.detail_id.0).collect::<Vec<_>>();
let verdicts = updates
.iter()
.map(|u| match u.verdict {
DelphiVerdict::Safe => "safe".to_string(),
DelphiVerdict::Unsafe => "unsafe".to_string(),
})
.collect::<Vec<_>>();
let record = sqlx::query!(
r#"
INSERT INTO delphi_issue_detail_verdicts (
project_id,
detail_key,
verdict
WITH incoming AS (
SELECT *
FROM unnest($1::bigint[], $2::text[]) WITH ORDINALITY
AS u(detail_id, verdict, ord)
),
resolved AS (
SELECT
i.ord,
didws.project_id,
didws.key AS detail_key,
i.verdict::delphi_report_issue_status AS verdict
FROM incoming i
INNER JOIN delphi_issue_details_with_statuses didws ON didws.id = i.detail_id
INNER JOIN delphi_report_issues dri ON dri.id = didws.issue_id
WHERE
-- see delphi.rs todo comment
dri.issue_type != '__dummy'
),
validated AS (
SELECT
(SELECT COUNT(*) FROM incoming) AS incoming_count,
(SELECT COUNT(*) FROM resolved) AS resolved_count
),
upserted AS (
INSERT INTO delphi_issue_detail_verdicts (
project_id,
detail_key,
verdict
)
SELECT DISTINCT ON (project_id, detail_key)
project_id,
detail_key,
verdict
FROM resolved
ORDER BY project_id, detail_key, ord DESC
ON CONFLICT (project_id, detail_key)
DO UPDATE SET verdict = EXCLUDED.verdict
RETURNING 1
)
SELECT
didws.project_id,
didws.key,
$1
FROM delphi_issue_details_with_statuses didws
INNER JOIN delphi_report_issues dri ON dri.id = didws.issue_id
WHERE
didws.id = $2
-- see delphi.rs todo comment
AND dri.issue_type != '__dummy'
ON CONFLICT (project_id, detail_key)
DO UPDATE SET verdict = EXCLUDED.verdict
(v.incoming_count = v.resolved_count) AS "all_found!",
(SELECT COUNT(*) FROM upserted) AS "upserted_count!"
FROM validated v
"#,
status as _,
issue_detail_id as _,
&detail_ids,
&verdicts,
)
.execute(&mut txn)
.fetch_one(&mut txn)
.await
.wrap_internal_err("failed to update issue detail")?;
if results.rows_affected() == 0 {
.wrap_internal_err("failed to update issue details")?;
if !record.all_found {
return Err(ApiError::Request(eyre!("issue detail does not exist")));
}

13
apps/labrinth/src/routes/v3/projects.rs
View File

@@ -28,6 +28,7 @@ use crate::models::threads::MessageBody;
use crate::queue::moderation::AutomatedModerationQueue;
use crate::queue::session::AuthQueue;
use crate::routes::ApiError;
use crate::routes::internal::delphi;
use crate::search::indexing::remove_documents;
use crate::search::{SearchConfig, SearchError, search_for_project};
use crate::util::error::Context;
@@ -2218,6 +2219,18 @@ pub async fn project_delete(
.begin()
.await
.wrap_internal_err("failed to start transaction")?;
let was_in_tech_review =
delphi::is_project_in_tech_review(project.inner.id, &mut transaction)
.await?;
if was_in_tech_review {
delphi::send_tech_review_exit_file_deleted_message(
project.inner.id,
&mut transaction,
)
.await?;
}
let context = ImageContext::Project {
project_id: Some(project.inner.id.into()),
};

11
apps/labrinth/src/routes/v3/version_file.rs
View File

@@ -9,6 +9,7 @@ use crate::models::pats::Scopes;
use crate::models::projects::VersionType;
use crate::models::teams::ProjectPermissions;
use crate::queue::session::AuthQueue;
use crate::routes::internal::delphi;
use crate::{database, models};
use actix_web::{HttpRequest, HttpResponse, web};
use dashmap::DashMap;
@@ -688,6 +689,9 @@ pub async fn delete_file(
}
let mut transaction = pool.begin().await?;
let was_in_tech_review =
delphi::is_project_in_tech_review(row.project_id, &mut transaction)
.await?;
sqlx::query!(
"
@@ -709,6 +713,13 @@ pub async fn delete_file(
.execute(&mut transaction)
.await?;
delphi::send_tech_review_exit_file_deleted_message_if_exited(
row.project_id,
was_in_tech_review,
&mut transaction,
)
.await?;
transaction.commit().await?;
Ok(HttpResponse::NoContent().body(""))

15
apps/labrinth/src/routes/v3/versions.rs
View File

@@ -25,6 +25,7 @@ use crate::models::projects::{
use crate::models::projects::{Loader, skip_nulls};
use crate::models::teams::ProjectPermissions;
use crate::queue::session::AuthQueue;
use crate::routes::internal::delphi;
use crate::search::SearchConfig;
use crate::search::indexing::remove_documents;
use crate::util::error::Context;
@@ -959,6 +960,12 @@ pub async fn version_delete(
}
let mut transaction = pool.begin().await?;
let was_in_tech_review = delphi::is_project_in_tech_review(
version.inner.project_id,
&mut transaction,
)
.await?;
let context = ImageContext::Version {
version_id: Some(version.inner.id.into()),
};
@@ -977,6 +984,14 @@ pub async fn version_delete(
&mut transaction,
)
.await?;
delphi::send_tech_review_exit_file_deleted_message_if_exited(
version.inner.project_id,
was_in_tech_review,
&mut transaction,
)
.await?;
transaction.commit().await?;
database::models::DBProject::clear_cache(