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

feat(admin): allow deleting package versions #1011

Merged
merged 5 commits into from
Apr 2, 2025
Merged

feat(admin): allow deleting package versions #1011

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2ce5080
feat(admin): allow deleting package versions
crowlKats Mar 10, 2025
c1b53f7
sqlx
crowlKats Mar 10, 2025
f3f2028
add frontend control
crowlKats Mar 27, 2025
f845115
fix
crowlKats Apr 2, 2025
90ea0d6
Merge branch 'main' into package-version-deletion
crowlKats Apr 2, 2025
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
16 changes: 16 additions & 0 deletions api/.sqlx/query-1d1876dcffdc0d3d1eead78a1514f3cac87830214e21abdeb5be583c1f6804b1.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

27 changes: 27 additions & 0 deletions api/migrations/20250310055436_package_version_drop.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
alter table package_version_dependencies
drop constraint package_version_dependencies_package_scope_package_name_fkey;

alter table package_version_dependencies
add foreign key (package_scope, package_name) references packages ON UPDATE CASCADE ON DELETE CASCADE;


alter table package_version_dependencies
drop constraint package_version_dependencies_package_scope_package_name_pa_fkey;

alter table package_version_dependencies
add constraint package_version_dependencies_package_scope_package_name_pa_fkey
foreign key (package_scope, package_name, package_version) references package_versions ON UPDATE CASCADE ON DELETE CASCADE;

alter table package_files
drop constraint package_files_scope_name_version_fkey;

alter table package_files
add foreign key (scope, name, version) references package_versions
ON UPDATE CASCADE ON DELETE CASCADE;

alter table npm_tarballs
drop constraint npm_tarballs_scope_name_version_fkey;

alter table npm_tarballs
add foreign key (scope, name, version) references package_versions
on UPDATE CASCADE ON DELETE CASCADE;
4 changes: 4 additions & 0 deletions api/src/api/errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,10 @@ errors!(
status: BAD_REQUEST,
"The requested package is archived. Unarchive it to modify settings or publish to it.",
},
DeleteVersionHasDependents {
status: BAD_REQUEST,
"The requested package version has dependents. Only a version without dependents can be deleted.",
},
);

pub fn map_unique_violation(err: sqlx::Error, new_err: ApiError) -> ApiError {
Expand Down
160 changes: 158 additions & 2 deletions api/src/api/package.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,10 @@ pub fn package_router() -> Router<Body, ApiError> {
"/:package/versions/:version",
util::auth(version_update_handler),
)
.delete(
"/:package/versions/:version",
util::auth(version_delete_handler),
)
.post(
"/:package/versions/:version/provenance",
util::auth(version_provenance_statements_handler),
Expand Down Expand Up @@ -942,8 +946,98 @@ pub async fn version_update_handler(
gzip_encoded: false,
},
)
.await
.unwrap();
.await?;

let npm_version_manifest_path =
crate::gcs_paths::npm_version_manifest_path(&scope, &package);
let npm_version_manifest =
generate_npm_version_manifest(db, npm_url, &scope, &package).await?;
let content = serde_json::to_vec_pretty(&npm_version_manifest)?;
buckets
.npm_bucket
.upload(
npm_version_manifest_path.into(),
UploadTaskBody::Bytes(content.into()),
GcsUploadOptions {
content_type: Some("application/json".into()),
cache_control: Some(CACHE_CONTROL_DO_NOT_CACHE.into()),
gzip_encoded: false,
},
)
.await?;

Ok(
Response::builder()
.status(StatusCode::NO_CONTENT)
.body(Body::empty())
.unwrap(),
)
}

#[instrument(
name = "DELETE /api/scopes/:scope/packages/:package/versions/:version",
skip(req),
err,
fields(scope, package, version)
)]
pub async fn version_delete_handler(
req: Request<Body>,
) -> ApiResult<Response<Body>> {
let scope = req.param_scope()?;
let package = req.param_package()?;
let version = req.param_version()?;
Span::current().record("scope", field::display(&scope));
Span::current().record("package", field::display(&package));
Span::current().record("version", field::display(&version));

let db = req.data::<Database>().unwrap();
let buckets = req.data::<Buckets>().unwrap().clone();
let npm_url = &req.data::<NpmUrl>().unwrap().0;

let iam = req.iam();
iam.check_admin_access()?;

let count = db
.count_package_dependents(
crate::db::DependencyKind::Jsr,
&format!("@{}/{}", scope, package),
)
.await?;

if count > 0 {
return Err(ApiError::DeleteVersionHasDependents);
}
Comment on lines +1000 to +1009
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this check is not correct, as it doesnt use the version as a requirement, and as such this check applies to all versions and is stricter than should be. This is good enough for now, however ideally this would build a graph and check if there are any other versions that fit the constraints used by dependents, and only if there is no such constraint would it error.


db.delete_package_version(&scope, &package, &version)
.await?;

let path = crate::gcs_paths::docs_v1_path(&scope, &package, &version);
buckets.docs_bucket.delete_file(path.into()).await?;

let path = crate::gcs_paths::version_metadata(&scope, &package, &version);
buckets.modules_bucket.delete_file(path.into()).await?;

let path =
crate::gcs_paths::file_path_root_directory(&scope, &package, &version);
buckets.modules_bucket.delete_directory(path.into()).await?;

let package_metadata_path =
crate::gcs_paths::package_metadata(&scope, &package);
let package_metadata = PackageMetadata::create(db, &scope, &package).await?;

let content = serde_json::to_vec_pretty(&package_metadata)?;
buckets
.modules_bucket
.upload(
package_metadata_path.into(),
UploadTaskBody::Bytes(content.into()),
GcsUploadOptions {
content_type: Some("application/json".into()),
cache_control: Some(CACHE_CONTROL_DO_NOT_CACHE.into()),
gzip_encoded: false,
},
)
.await?;

let npm_version_manifest_path =
crate::gcs_paths::npm_version_manifest_path(&scope, &package);
Expand Down Expand Up @@ -4093,4 +4187,66 @@ ggHohNAjhbzDaY2iBW/m3NC5dehGUP4T2GBo/cwGhg==
assert_eq!(tasks.len(), 1);
assert_eq!(tasks[0].id, task2.id);
}

#[tokio::test]
async fn delete_version() {
let mut t = TestSetup::new().await;
let staff_token = t.staff_user.token.clone();

// unpublished package
let mut resp = t
.http()
.get("/api/scopes/scope/packages/foo/versions/0.0.1/dependencies/graph")
.call()
.await
.unwrap();
resp
.expect_err_code(StatusCode::NOT_FOUND, "packageVersionNotFound")
.await;

let task = process_tarball_setup(&t, create_mock_tarball("ok")).await;
assert_eq!(task.status, PublishingTaskStatus::Success, "{:?}", task);

// Now publish a package that has a few deps
let package_name = PackageName::try_from("bar").unwrap();
let version = Version::try_from("1.2.3").unwrap();
let task = crate::publish::tests::process_tarball_setup2(
&t,
create_mock_tarball("depends_on_ok"),
&package_name,
&version,
false,
)
.await;
assert_eq!(task.status, PublishingTaskStatus::Success, "{:?}", task);

let mut resp = t
.http()
.delete("/api/scopes/scope/packages/foo/versions/0.0.1")
.token(Some(&staff_token))
.call()
.await
.unwrap();
resp
.expect_err_code(StatusCode::BAD_REQUEST, "deleteVersionHasDependents")
.await;

let mut resp = t
.http()
.delete("/api/scopes/scope/packages/bar/versions/1.2.3")
.token(Some(&staff_token))
.call()
.await
.unwrap();
resp.expect_ok_no_content().await;

let mut resp = t
.http()
.delete("/api/scopes/scope/packages/foo/versions/0.0.1")
.token(Some(&staff_token))
.call()
.await
.unwrap();
resp.expect_ok_no_content().await;
}
}
97 changes: 97 additions & 0 deletions api/src/buckets.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use futures::Future;
use futures::FutureExt;
use futures::Stream;
use futures::StreamExt;
use futures::TryStreamExt;
use tokio::sync::mpsc;
use tokio_stream::wrappers::UnboundedReceiverStream;
use tracing::instrument;
Expand All @@ -24,6 +25,8 @@ pub struct BucketWithQueue {
pub bucket: gcp::Bucket,
upload_queue: DynamicBackgroundTaskQueue<UploadTask>,
download_queue: DynamicBackgroundTaskQueue<DownloadTask>,
delete_queue: DynamicBackgroundTaskQueue<DeleteFileTask>,
list_queue: DynamicBackgroundTaskQueue<ListDirectoryTask>,
}

impl BucketWithQueue {
Expand All @@ -32,6 +35,8 @@ impl BucketWithQueue {
bucket,
upload_queue: DynamicBackgroundTaskQueue::default(),
download_queue: DynamicBackgroundTaskQueue::default(),
delete_queue: DynamicBackgroundTaskQueue::default(),
list_queue: DynamicBackgroundTaskQueue::default(),
}
}

Expand Down Expand Up @@ -72,6 +77,40 @@ impl BucketWithQueue {
.await
.unwrap()
}

#[instrument(name = "BucketWithQueue::delete_file", skip(self), err)]
pub async fn delete_file(&self, path: Arc<str>) -> Result<bool, GcsError> {
self
.delete_queue
.run(DeleteFileTask {
bucket: self.bucket.clone(),
path,
})
.await
.unwrap()
}

#[instrument(name = "BucketWithQueue::delete_directory", skip(self), err)]
pub async fn delete_directory(&self, path: Arc<str>) -> Result<(), GcsError> {
let list = self
.list_queue
.run(ListDirectoryTask {
bucket: self.bucket.clone(),
path,
})
.await
.unwrap()?;

if let Some(list) = list {
let stream = futures::stream::iter(list.items)
.map(|item| self.delete_file(item.name.into()))
.buffer_unordered(64);

let _ = stream.try_collect::<Vec<_>>().await?;
}

Ok(())
}
}

#[derive(Clone)]
Expand Down Expand Up @@ -191,3 +230,61 @@ impl RestartableTask for DownloadTask {
.boxed()
}
}

struct DeleteFileTask {
bucket: gcp::Bucket,
path: Arc<str>,
}

impl RestartableTask for DeleteFileTask {
type Ok = bool;
type Err = gcp::GcsError;
type Fut =
Pin<Box<dyn Future<Output = RestartableTaskResult<Self>> + Send + 'static>>;

fn run(self) -> Self::Fut {
async move {
let res = self.bucket.delete_file(&self.path).await;
match res {
Ok(data) => RestartableTaskResult::Ok(data),
Err(e) if e.is_retryable() => {
RestartableTaskResult::Backoff(DeleteFileTask {
bucket: self.bucket,
path: self.path,
})
}
Err(e) => RestartableTaskResult::Error(e),
}
}
.boxed()
}
}

struct ListDirectoryTask {
bucket: gcp::Bucket,
path: Arc<str>,
}

impl RestartableTask for ListDirectoryTask {
type Ok = Option<gcp::List>;
type Err = gcp::GcsError;
type Fut =
Pin<Box<dyn Future<Output = RestartableTaskResult<Self>> + Send + 'static>>;

fn run(self) -> Self::Fut {
async move {
let res = self.bucket.list(&self.path).await;
match res {
Ok(data) => RestartableTaskResult::Ok(data),
Err(e) if e.is_retryable() => {
RestartableTaskResult::Backoff(ListDirectoryTask {
bucket: self.bucket,
path: self.path,
})
}
Err(e) => RestartableTaskResult::Error(e),
}
}
.boxed()
}
}
20 changes: 20 additions & 0 deletions api/src/db/database.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1649,6 +1649,26 @@ impl Database {
.await
}

#[instrument(name = "Database::delete_package_version", skip(self), err)]
pub async fn delete_package_version(
&self,
scope: &ScopeName,
name: &PackageName,
version: &Version,
) -> Result<()> {
sqlx::query_as!(
PackageVersion,
r#"DELETE FROM package_versions WHERE scope = $1 AND name = $2 AND version = $3"#,
scope as _,
name as _,
version as _
)
.execute(&self.pool)
.await?;

Ok(())
}

#[instrument(name = "Database::get_package_file", skip(self), err)]
pub async fn get_package_file(
&self,
Expand Down
Loading