Add DELETE /api/v0/pipeline-nodes/:id with transactions, error handli…#21
Add DELETE /api/v0/pipeline-nodes/:id with transactions, error handli…#21OmarElhagagy wants to merge 3 commits intopatchwork-body:mainfrom
Conversation
|
Hi @OmarElhagagy, thank you for your PR, I will review your work right away, regarding Feel free to ask any questions, I know that README could be better :) |
There was a problem hiding this comment.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
api/src/routes/api/v0/pipeline_nodes.rs:55
- [nitpick] The struct name 'ApiError' is ambiguous. It should be renamed to 'PipelineNodeApiError' to be more descriptive.
struct ApiError {
api/src/routes/api/v0/pipeline_nodes.rs:89
- [nitpick] The error message 'Pipeline node not found' could be more descriptive by including the node ID.
error: "Pipeline node not found".to_string(),
patchwork-body
left a comment
patchwork-body
left a comment
There was a problem hiding this comment.
Good start, there's a few things that need closer look, I believe there be no problem for you to handle them, thanks in advance!
| DatabaseConnection(mut conn): DatabaseConnection, | ||
| Path(id): Path<Uuid>, | ||
| ) -> Result<StatusCode, (StatusCode, Json<ApiError>)> { | ||
| info!("Attempting to delete pipeline node: {}", id); |
There was a problem hiding this comment.
| info!("Attempting to delete pipeline node: {}", id); | |
| info!("Attempting to delete pipeline node: {id}"); |
| let mut tx = conn | ||
| .begin() | ||
| .await | ||
| .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, Json(ApiError::from(e))))?; |
There was a problem hiding this comment.
let's use internal_error helper form utils
you can modify this fn if you think we need a better approach but I would prefer to stick into common reusable solution across the codebase
| } | ||
|
|
||
| // Delete connections | ||
| sqlx::query!( |
There was a problem hiding this comment.
we don't actually need to manually delete pipeline connections, inputs and outputs bc PostgreSQL will make sure that gone once we delete related pipeline_node, thanks for the ON DELETE CASCADE rule
|
Hi @patchwork-body, thanks for the feedback! i'll work on them right away |
|
Hi @patchwork-body, Thanks for the great review and suggestions! I’ve updated the code based on your feedback:
I also ran |
patchwork-body
left a comment
patchwork-body
left a comment
There was a problem hiding this comment.
thank you Omar, sorry it took so long to provide a feedback, I've been busy this week, wish you all the best!
| use axum::{extract::Path, Json}; | ||
| use hyper::StatusCode; | ||
| use serde::{Deserialize, Serialize}; | ||
| use serde_json::json; |
| pub async fn delete( | ||
| DatabaseConnection(mut conn): DatabaseConnection, | ||
| Path(id): Path<Uuid>, | ||
| ) -> Result<StatusCode, (StatusCode, Json<ApiError>)> { |
There was a problem hiding this comment.
the return type will conflict with what internal_error will return
| })) | ||
| } | ||
|
|
||
| #[cfg(test)] |
There was a problem hiding this comment.
I would suggest to erase tests for now, we'll deal with that later on
btw, types seems to be broken, you you want to keep them, please double check that
Hi,
I've implemented the DELETE
/api/v0/pipeline-nodes/:idendpoint as requested in #20. Here’s what’s included:internal_errorapproach.tracing::infologs for start and success, aligning with the project’s observability setup.test_delete_pipeline_node_successandtest_delete_pipeline_node_not_found) to verify behavior.Note:
cargo sqlx prepareto update query cache. Someone with database access needs to run it.The code is tested locally with a mock database URL, so please adjust the test URL to match your setup. Let me know if there’s anything else to tweak or if I should add authorization checks (I skipped them since they weren’t specified in the issue).
Looking forward to feedback!