Skip to content

Commit 50fe181

Browse files
Fix rollback procedure for GraphQL endpoint errors (#1478)
* Initial plan * Fix: GraphQL handler now returns error on response with errors to trigger proper transaction rollback Co-authored-by: zaychenko-sergei <8875909+zaychenko-sergei@users.noreply.github.com> * Refactor: Extract shared GqlResponseError and graphql_handler into a dedicated module Co-authored-by: zaychenko-sergei <8875909+zaychenko-sergei@users.noreply.github.com> * Fix formatting: consolidate axum imports on one line Co-authored-by: zaychenko-sergei <8875909+zaychenko-sergei@users.noreply.github.com> * Update copilot instructions and changelog - Add formatting step (cargo fmt) before Clippy in instructions - Add changelog section to instructions for full PRs - Add changelog entry for GraphQL rollback fix Co-authored-by: zaychenko-sergei <8875909+zaychenko-sergei@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: zaychenko-sergei <8875909+zaychenko-sergei@users.noreply.github.com>
1 parent 9fdd085 commit 50fe181

6 files changed

Lines changed: 99 additions & 51 deletions

File tree

.github/copilot-instructions.md

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,12 @@
44

55
This project uses Rust with Clippy for linting and code quality checks. When performing any code analysis, validation, or after making code changes, you should:
66

7-
1. **Always run Clippy** using `make clippy` to check for linting issues
8-
2. Run Clippy before and after making code changes to ensure no new warnings are introduced
9-
3. If Clippy reports warnings or errors, address them as part of the code changes
10-
4. Use `cargo check` for basic compilation checks when needed
11-
5. The project follows strict linting standards - treat Clippy warnings as errors that need to be fixed
7+
1. **Always run formatting first** using `cargo fmt` before running any linting checks
8+
2. **Always run Clippy** using `make clippy` to check for linting issues
9+
3. Run Clippy before and after making code changes to ensure no new warnings are introduced
10+
4. If Clippy reports warnings or errors, address them as part of the code changes
11+
5. Use `cargo check` for basic compilation checks when needed
12+
6. The project follows strict linting standards - treat Clippy warnings as errors that need to be fixed
1213

1314
## Code Style
1415
- Follow the project's existing code style and formatting
@@ -30,4 +31,12 @@ This project uses Rust with Clippy for linting and code quality checks. When per
3031
- Document each event type with comments explaining its usage and context.
3132
- Prefer using separate structs for event data rather than inline data in the enum to enhance clarity and maintainability.
3233

33-
Remember: **Always run `make clippy` as part of your validation process when working with this codebase.**
34+
## Changelog
35+
36+
When completing a full PR (not incremental local changes), update the `CHANGELOG.md` file:
37+
- Add entries to the `[Unreleased]` section at the top of the file
38+
- Use the appropriate subsection: `### Added` for new features, `### Changed` for modifications, `### Fixed` for bug fixes
39+
- Write a brief, high-level summary of the change from an end-user perspective
40+
- This only applies to full PRs, not to incremental work done via local IDE Copilot
41+
42+
Remember: **Always run `cargo fmt` followed by `make clippy` as part of your validation process when working with this codebase.**

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ Recommendation: for ease of reading, use the following order:
2222
clearly indicating the access check on the resolved dataset was already performed
2323
### Fixed
2424
- Flow events now processed transactionally
25+
- GraphQL endpoint errors now properly trigger transaction rollback
2526

2627
## [0.253.1] - 2025-11-24
2728
### Changed

src/app/cli/src/explore/api_server.rs

Lines changed: 1 addition & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,9 @@ use std::sync::Arc;
1616

1717
use async_utils::BackgroundAgent;
1818
use axum::{Extension, middleware};
19-
use database_common_macros::transactional_handler;
20-
use http_common::ApiError;
2119
use internal_error::*;
2220
use kamu::domain::{FileUploadLimitConfig, Protocols, ServerUrlConfig, TenancyConfig};
2321
use kamu_accounts_services::PasswordPolicyConfig;
24-
use kamu_adapter_graphql::data_loader::{account_entity_data_loader, dataset_handle_data_loader};
2522
use kamu_adapter_http::DatasetAuthorizationLayer;
2623
use kamu_adapter_http::e2e::e2e_router;
2724
use observability::axum::{panic_handler, unknown_fallback_handler};
@@ -30,7 +27,7 @@ use tower_http::catch_panic::CatchPanicLayer;
3027
use url::Url;
3128
use utoipa_axum::router::OpenApiRouter;
3229

33-
use super::{UIConfiguration, UIFeatureFlags};
30+
use super::{UIConfiguration, UIFeatureFlags, graphql_handler};
3431

3532
////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
3633

@@ -325,21 +322,3 @@ async fn ui_configuration_handler(
325322
}
326323

327324
////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
328-
329-
#[transactional_handler]
330-
async fn graphql_handler(
331-
Extension(schema): Extension<kamu_adapter_graphql::Schema>,
332-
Extension(catalog): Extension<dill::Catalog>,
333-
req: async_graphql_axum::GraphQLRequest,
334-
) -> Result<async_graphql_axum::GraphQLResponse, ApiError> {
335-
let graphql_request = req
336-
.into_inner()
337-
.data(account_entity_data_loader(&catalog))
338-
.data(dataset_handle_data_loader(&catalog))
339-
.data(catalog);
340-
let graphql_response = schema.execute(graphql_request).await.into();
341-
342-
Ok(graphql_response)
343-
}
344-
345-
////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
// Copyright Kamu Data, Inc. and contributors. All rights reserved.
2+
//
3+
// Use of this software is governed by the Business Source License
4+
// included in the LICENSE file.
5+
//
6+
// As of the Change Date specified in that file, in accordance with
7+
// the Business Source License, use of this software will be governed
8+
// by the Apache License, Version 2.0.
9+
10+
use axum::Extension;
11+
use axum::response::IntoResponse;
12+
use database_common_macros::transactional_handler;
13+
use dill::Catalog;
14+
use internal_error::InternalError;
15+
use kamu_adapter_graphql::data_loader::{account_entity_data_loader, dataset_handle_data_loader};
16+
17+
////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
18+
19+
/// An error type that wraps a GraphQL response containing errors.
20+
/// When returned as `Err`, it triggers transaction rollback while still
21+
/// returning the GraphQL response (with errors) to the client.
22+
pub struct GqlResponseError(async_graphql_axum::GraphQLResponse);
23+
24+
impl std::fmt::Display for GqlResponseError {
25+
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
26+
write!(f, "GraphQL response contains errors")
27+
}
28+
}
29+
30+
impl std::fmt::Debug for GqlResponseError {
31+
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
32+
write!(f, "GqlResponseError")
33+
}
34+
}
35+
36+
impl std::error::Error for GqlResponseError {}
37+
38+
impl From<InternalError> for GqlResponseError {
39+
fn from(e: InternalError) -> Self {
40+
let response = async_graphql::Response::from_errors(vec![async_graphql::ServerError::new(
41+
e.to_string(),
42+
None,
43+
)]);
44+
GqlResponseError(response.into())
45+
}
46+
}
47+
48+
impl IntoResponse for GqlResponseError {
49+
fn into_response(self) -> axum::response::Response {
50+
self.0.into_response()
51+
}
52+
}
53+
54+
////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
55+
56+
#[transactional_handler]
57+
pub async fn graphql_handler(
58+
Extension(schema): Extension<kamu_adapter_graphql::Schema>,
59+
Extension(catalog): Extension<Catalog>,
60+
req: async_graphql_axum::GraphQLRequest,
61+
) -> Result<async_graphql_axum::GraphQLResponse, GqlResponseError> {
62+
let graphql_request = req
63+
.into_inner()
64+
.data(account_entity_data_loader(&catalog))
65+
.data(dataset_handle_data_loader(&catalog))
66+
.data(catalog);
67+
let graphql_response: async_graphql_axum::GraphQLResponse =
68+
schema.execute(graphql_request).await.into();
69+
70+
// Check if the response contains errors - if so, return Err to trigger
71+
// transaction rollback while still returning the GraphQL response to the client
72+
if !graphql_response.0.is_ok() {
73+
Err(GqlResponseError(graphql_response))
74+
} else {
75+
Ok(graphql_response)
76+
}
77+
}
78+
79+
////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////

src/app/cli/src/explore/mod.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
mod api_server;
1111
mod dependencies;
1212
mod flight_sql_service_factory;
13+
mod graphql_handler;
1314
mod notebook_server_factory;
1415
mod spark_livy_server_factory;
1516
mod sql_shell_impl;
@@ -21,6 +22,7 @@ mod web_ui_server;
2122
pub use api_server::*;
2223
pub use dependencies::*;
2324
pub use flight_sql_service_factory::*;
25+
pub(crate) use graphql_handler::*;
2426
pub use notebook_server_factory::*;
2527
pub use spark_livy_server_factory::*;
2628
pub use sql_shell_impl::*;

src/app/cli/src/explore/web_ui_server.rs

Lines changed: 1 addition & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -12,13 +12,10 @@ use std::net::{IpAddr, Ipv4Addr, SocketAddr};
1212
use std::pin::Pin;
1313
use std::sync::Arc;
1414

15-
use axum::Extension;
1615
use axum::http::Uri;
1716
use axum::response::{IntoResponse, Response};
1817
use database_common::DatabaseTransactionRunner;
19-
use database_common_macros::transactional_handler;
2018
use dill::{Catalog, CatalogBuilder};
21-
use http_common::ApiError;
2219
use internal_error::*;
2320
use kamu::domain::{FileUploadLimitConfig, Protocols, ServerUrlConfig, TenancyConfig};
2421
use kamu_accounts::{
@@ -28,15 +25,14 @@ use kamu_accounts::{
2825
PredefinedAccountsConfig,
2926
};
3027
use kamu_accounts_services::{PasswordLoginCredentials, PasswordPolicyConfig};
31-
use kamu_adapter_graphql::data_loader::{account_entity_data_loader, dataset_handle_data_loader};
3228
use kamu_adapter_http::DatasetAuthorizationLayer;
3329
use observability::axum::unknown_fallback_handler;
3430
use rust_embed::RustEmbed;
3531
use serde::Serialize;
3632
use url::Url;
3733
use utoipa_axum::router::OpenApiRouter;
3834

39-
use super::{UIConfiguration, UIFeatureFlags};
35+
use super::{UIConfiguration, UIFeatureFlags, graphql_handler};
4036

4137
////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
4238

@@ -313,21 +309,3 @@ async fn ui_configuration_handler(
313309
}
314310

315311
////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
316-
317-
#[transactional_handler]
318-
async fn graphql_handler(
319-
Extension(schema): Extension<kamu_adapter_graphql::Schema>,
320-
Extension(catalog): Extension<Catalog>,
321-
req: async_graphql_axum::GraphQLRequest,
322-
) -> Result<async_graphql_axum::GraphQLResponse, ApiError> {
323-
let graphql_request = req
324-
.into_inner()
325-
.data(account_entity_data_loader(&catalog))
326-
.data(dataset_handle_data_loader(&catalog))
327-
.data(catalog);
328-
let graphql_response = schema.execute(graphql_request).await.into();
329-
330-
Ok(graphql_response)
331-
}
332-
333-
////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////

0 commit comments

Comments
 (0)