Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Read build-status and rustc-version directly from builds instead of release #2267

Closed
wants to merge 2 commits into from
Closed

Read build-status and rustc-version directly from builds instead of release #2267

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
58a74d4
Read build-status directly from builds instead of release
Nemo157 Jun 2, 2023
bfaaf07
Read rustc_version directly from builds instead of release
Nemo157 Jun 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 25 additions & 10 deletions src/web/crate_details.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ impl CrateDetails {
releases.readme,
releases.description_long,
releases.release_time,
releases.build_status,
COALESCE(builds.build_status, false) as build_status,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should already either remove or just rename the now unused columns.

This would also surface places which we might have forgotten.

releases.rustdoc_status,
releases.archive_storage,
releases.repository_url,
Expand All @@ -136,7 +136,7 @@ impl CrateDetails {
releases.license,
releases.documentation_url,
releases.default_target,
releases.doc_rustc_version,
builds.rustc_version,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should already either remove or just rename the now unused columns.

This would also surface places which we might have forgotten.

doc_coverage.total_items,
doc_coverage.documented_items,
doc_coverage.total_items_needing_examples,
Expand All @@ -145,6 +145,12 @@ impl CrateDetails {
INNER JOIN crates ON releases.crate_id = crates.id
LEFT JOIN doc_coverage ON doc_coverage.release_id = releases.id
LEFT JOIN repositories ON releases.repository_id = repositories.id
LEFT JOIN LATERAL (
SELECT * FROM builds
WHERE builds.rid = releases.id
ORDER BY builds.build_time
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the DESC should be on the same line as the ORDER BY for readablity

DESC LIMIT 1
) AS builds ON true
WHERE crates.name = $1 AND releases.version = $2;";

let rows = conn.query(query, &[&name, &version])?;
Expand Down Expand Up @@ -182,7 +188,10 @@ impl CrateDetails {
default_target: krate.get("default_target"),
doc_targets: MetaData::parse_doc_targets(krate.get("doc_targets")),
yanked: krate.get("yanked"),
rustdoc_css_file: get_correct_docsrs_style_file(krate.get("doc_rustc_version"))?,
rustdoc_css_file: krate
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

related question:
was there a time when we only stored records in the releases table? and nothing in builds?

Wouldn't this change not break old docs?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If so that was before my time, would have to check the prod database dump

.get::<_, Option<&str>>("rustc_version")
.map(get_correct_docsrs_style_file)
.transpose()?,
};

let mut crate_details = CrateDetails {
Expand Down Expand Up @@ -305,14 +314,20 @@ pub(crate) fn releases_for_crate(
let mut releases: Vec<Release> = conn
.query(
"SELECT
id,
version,
build_status,
yanked,
is_library,
rustdoc_status,
target_name
releases.id,
releases.version,
COALESCE(builds.build_status, false) as build_status,
releases.yanked,
releases.is_library,
releases.rustdoc_status,
releases.target_name
FROM releases
LEFT JOIN LATERAL (
SELECT build_status FROM builds
WHERE builds.rid = releases.id
ORDER BY builds.build_time
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the DESC should be on the same line as the ORDER BY for readablity

DESC LIMIT 1
) AS builds ON true
WHERE
releases.crate_id = $1",
&[&crate_id],
Expand Down
66 changes: 38 additions & 28 deletions src/web/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,44 +509,54 @@ pub(crate) struct MetaData {
pub(crate) yanked: bool,
/// CSS file to use depending on the rustdoc version used to generate this version of this
/// crate.
pub(crate) rustdoc_css_file: String,
pub(crate) rustdoc_css_file: Option<String>,
}

impl MetaData {
#[fn_error_context::context("getting metadata for {name} {version}")]
fn from_crate(
conn: &mut Client,
name: &str,
version: &str,
version_or_latest: &str,
) -> Result<MetaData> {
conn.query_opt(
"SELECT crates.name,
releases.version,
releases.description,
releases.target_name,
releases.rustdoc_status,
releases.default_target,
releases.doc_targets,
releases.yanked,
releases.doc_rustc_version
FROM releases
INNER JOIN crates ON crates.id = releases.crate_id
WHERE crates.name = $1 AND releases.version = $2",
let row = conn.query_one(
"SELECT
crates.name,
releases.version,
releases.description,
releases.target_name,
releases.rustdoc_status,
releases.default_target,
releases.doc_targets,
releases.yanked,
builds.rustc_version
FROM releases
INNER JOIN crates ON crates.id = releases.crate_id
LEFT JOIN LATERAL (
SELECT * FROM builds
WHERE builds.rid = releases.id
ORDER BY builds.build_time
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the DESC should be on the same line as the ORDER BY for readablity

DESC LIMIT 1
) AS builds ON true
WHERE crates.name = $1 AND releases.version = $2",
&[&name, &version],
)?
.map(|row| MetaData {
name: row.get(0),
version: row.get(1),
)?;
Ok(MetaData {
name: row.get("name"),
version: row.get("version"),
version_or_latest: version_or_latest.to_string(),
description: row.get(2),
target_name: row.get(3),
rustdoc_status: row.get(4),
default_target: row.get(5),
doc_targets: MetaData::parse_doc_targets(row.get(6)),
yanked: row.get(7),
rustdoc_css_file: get_correct_docsrs_style_file(row.get(8)).unwrap(),
description: row.get("description"),
target_name: row.get("target_name"),
rustdoc_status: row.get("rustdoc_status"),
default_target: row.get("default_target"),
doc_targets: MetaData::parse_doc_targets(row.get("doc_targets")),
yanked: row.get("yanked"),
rustdoc_css_file: row
.get::<_, Option<&str>>("rustc_version")
.map(get_correct_docsrs_style_file)
.transpose()?,
})
.ok_or_else(|| anyhow!("missing metadata for {} {}", name, version))
}

fn parse_doc_targets(targets: Value) -> Vec<String> {
Expand Down Expand Up @@ -938,7 +948,7 @@ mod test {
"arm64-unknown-linux-gnu".to_string(),
],
yanked: false,
rustdoc_css_file: "rustdoc.css".to_string(),
rustdoc_css_file: Some("rustdoc.css".to_string()),
};

let correct_json = json!({
Expand Down Expand Up @@ -1016,7 +1026,7 @@ mod test {
default_target: "x86_64-unknown-linux-gnu".to_string(),
doc_targets: vec![],
yanked: false,
rustdoc_css_file: "rustdoc.css".to_string(),
rustdoc_css_file: Some("rustdoc.css".to_string()),
},
);
Ok(())
Expand Down
4 changes: 2 additions & 2 deletions src/web/releases.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ pub(crate) fn get_releases(
INNER JOIN builds ON releases.id = builds.rid
LEFT JOIN repositories ON releases.repository_id = repositories.id
WHERE
((NOT $3) OR (releases.build_status = FALSE AND releases.is_library = TRUE))
((NOT $3) OR (builds.build_status = FALSE AND releases.is_library = TRUE))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps I'm missing something, but this would change behaviour of the query, for the case when we have multiple builds for a release.

releases.build_status will contain the last build result.

So assuming that the last build was successful, and the first one broke, in the old query we wouldn't see that release in the result, because the last build succeeded. In the new query we would see the release in the result because an older build failed, while the most recent one passed.

AND {0} IS NOT NULL

ORDER BY {0} DESC
Expand Down Expand Up @@ -658,7 +658,7 @@ pub(crate) async fn activity_handler(
SELECT
release_time::date AS date_,
COUNT(*) AS counts,
SUM(CAST((is_library = TRUE AND build_status = FALSE) AS INT)) AS failures
SUM(CAST((is_library = TRUE AND (SELECT builds.build_status FROM builds WHERE builds.rid = releases.id ORDER BY builds.build_time DESC LIMIT 1) = FALSE) AS INT)) AS failures
FROM
releases
WHERE
Expand Down
44 changes: 11 additions & 33 deletions src/web/source.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use super::{error::AxumResult, match_version_axum};
use crate::{
db::Pool,
impl_axum_webpage,
utils::{get_correct_docsrs_style_file, spawn_blocking},
utils::spawn_blocking,
web::{
cache::CachePolicy, error::AxumNope, file::File as DbFile, headers::CanonicalUrl,
MatchSemver, MetaData,
Expand Down Expand Up @@ -75,29 +75,18 @@ impl FileList {
version_or_latest: &str,
folder: &str,
) -> Result<Option<FileList>> {
let row = match conn.query_opt(
"SELECT crates.name,
releases.version,
releases.description,
releases.target_name,
releases.rustdoc_status,
releases.files,
releases.default_target,
releases.doc_targets,
releases.yanked,
releases.doc_rustc_version
FROM releases
LEFT OUTER JOIN crates ON crates.id = releases.crate_id
WHERE crates.name = $1 AND releases.version = $2",
let Some(row) = conn.query_opt(
"SELECT releases.files
FROM releases
LEFT OUTER JOIN crates ON crates.id = releases.crate_id
WHERE crates.name = $1 AND releases.version = $2",
&[&name, &version],
)? {
Some(row) => row,
None => return Ok(None),
)?
else {
return Ok(None);
};

let files = if let Some(files) = row.try_get::<_, Option<Value>>(5)? {
files
} else {
let Some(files) = row.try_get::<_, Option<Value>>("files")? else {
return Ok(None);
};

Expand Down Expand Up @@ -143,18 +132,7 @@ impl FileList {
});

Ok(Some(FileList {
metadata: MetaData {
name: row.get(0),
version: row.get(1),
version_or_latest: version_or_latest.to_string(),
description: row.get(2),
target_name: row.get(3),
rustdoc_status: row.get(4),
default_target: row.get(6),
doc_targets: MetaData::parse_doc_targets(row.get(7)),
yanked: row.get(8),
rustdoc_css_file: get_correct_docsrs_style_file(row.get(9))?,
},
metadata: MetaData::from_crate(conn, name, version, version_or_latest)?,
files: file_list,
}))
} else {
Expand Down
2 changes: 2 additions & 0 deletions templates/rustdoc/head.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
{%- import "macros.html" as macros -%}
{% if metadata.rustdoc_css_file %}
<link rel="stylesheet" href="/-/static/{{metadata.rustdoc_css_file}}?{{ docsrs_version() | slugify }}" media="all" />
{% endif %}

<link rel="search" href="/-/static/opensearch.xml" type="application/opensearchdescription+xml" title="Docs.rs" />

Expand Down