refactor(airflow): give the library a canonical error type - #694
Open
jvanbuel wants to merge 2 commits into
Open
refactor(airflow): give the library a canonical error type#694jvanbuel wants to merge 2 commits into
jvanbuel wants to merge 2 commits into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe Airflow crate adds typed errors and shared request handling, migrates V1/V2 clients to centralized execution and JSON decoding, standardizes authentication and discovery failures, and propagates errors through higher-level operations. ChangesAirflow error and shared transport
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Operation
participant V1V2Client
participant BaseClient
participant AirflowAPI
Operation->>V1V2Client: invoke Airflow operation
V1V2Client->>BaseClient: build and execute request
BaseClient->>AirflowAPI: send HTTP request
AirflowAPI-->>BaseClient: response status and body
BaseClient-->>V1V2Client: Response or AirflowError
V1V2Client-->>Operation: decoded value or propagated error
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/flowrs-airflow/src/error.rs (1)
84-106: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStatus error drops query string, losing useful diagnostic context.
AirflowError::statusstores onlyurl.path(), discarding the query string. Several client calls attach pagination (offset/limit) or filter params via.query(...), so a failing request's error message won't show which page/params triggered the failure — undermining the stated goal of making failures diagnosable.♻️ Proposed fix to include the query string
pub(crate) fn status( method: &reqwest::Method, url: &url::Url, status: u16, body: &str, ) -> Self { Self::Status { method: method.to_string(), - path: url.path().to_string(), + path: match url.query() { + Some(q) => format!("{}?{q}", url.path()), + None => url.path().to_string(), + }, status,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/flowrs-airflow/src/error.rs` around lines 84 - 106, Update AirflowError::status to preserve the URL query string when populating the stored path, using the URL’s path-and-query representation so pagination and filter parameters remain visible in status errors while retaining the existing body-detail behavior.crates/flowrs-airflow/src/client/base.rs (1)
68-84: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winVerify endpoint normalization:
Url::joinsilently drops a base path segment without a trailing slash.
self.endpoint.join(&path)resolvespathrelative toself.endpoint. Per theurlcrate docs, a trailing slash is significant: without it, the last path component is treated as a "file" name and replaced rather than kept. So if a deployment'sconfig.endpointis something likehttps://host/airflow(no trailing slash), joiningapi/v1/dagsyieldshttps://host/api/v1/dags, silently dropping the/airflowprefix — a real risk for reverse-proxied deployments with a non-root path.Since the endpoint is now parsed exactly once in
new(), that's the natural place to guarantee a trailing slash so every subsequentjoinis safe.♻️ Proposed normalization at construction
- let endpoint = Url::parse(&config.endpoint) + let mut endpoint = Url::parse(&config.endpoint) .map_err(|e| AirflowError::invalid_url(config.endpoint.clone(), e))?; + if !endpoint.path().ends_with('/') { + endpoint.set_path(&format!("{}/", endpoint.path())); + }Since my knowledge of this crate's exact intended endpoint-format contract is limited, please confirm whether configured endpoints are expected/documented to always include a trailing slash (or are always root-level) before applying this fix.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/flowrs-airflow/src/client/base.rs` around lines 68 - 84, Normalize the parsed endpoint in the client constructor new() by ensuring its path ends with a trailing slash before storing it, so base_api’s self.endpoint.join(&path) preserves reverse-proxy prefixes such as /airflow. Confirm and follow the existing configured-endpoint contract, and keep base_api unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/flowrs-airflow/src/client/base.rs`:
- Around line 68-84: Normalize the parsed endpoint in the client constructor
new() by ensuring its path ends with a trailing slash before storing it, so
base_api’s self.endpoint.join(&path) preserves reverse-proxy prefixes such as
/airflow. Confirm and follow the existing configured-endpoint contract, and keep
base_api unchanged.
In `@crates/flowrs-airflow/src/error.rs`:
- Around line 84-106: Update AirflowError::status to preserve the URL query
string when populating the stored path, using the URL’s path-and-query
representation so pagination and filter parameters remain visible in status
errors while retaining the existing body-detail behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 35d1c8ee-bcec-49f2-adbb-15d3d6c8e0c6
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lock,!**/*.lockCargo.tomlis excluded by!Cargo.toml,!**/Cargo.tomlcrates/flowrs-airflow/Cargo.tomlis excluded by!**/Cargo.toml
📒 Files selected for processing (31)
crates/flowrs-airflow/src/client.rscrates/flowrs-airflow/src/client/auth/basic.rscrates/flowrs-airflow/src/client/auth/command.rscrates/flowrs-airflow/src/client/auth/mod.rscrates/flowrs-airflow/src/client/auth/static_token.rscrates/flowrs-airflow/src/client/base.rscrates/flowrs-airflow/src/client/v1/dag.rscrates/flowrs-airflow/src/client/v1/dagrun.rscrates/flowrs-airflow/src/client/v1/dagstats.rscrates/flowrs-airflow/src/client/v1/log.rscrates/flowrs-airflow/src/client/v1/mod.rscrates/flowrs-airflow/src/client/v1/task.rscrates/flowrs-airflow/src/client/v1/taskinstance.rscrates/flowrs-airflow/src/client/v2/dag.rscrates/flowrs-airflow/src/client/v2/dagrun.rscrates/flowrs-airflow/src/client/v2/dagstats.rscrates/flowrs-airflow/src/client/v2/log.rscrates/flowrs-airflow/src/client/v2/mod.rscrates/flowrs-airflow/src/client/v2/task.rscrates/flowrs-airflow/src/client/v2/taskinstance.rscrates/flowrs-airflow/src/error.rscrates/flowrs-airflow/src/lib.rscrates/flowrs-airflow/src/managed_services/astronomer.rscrates/flowrs-airflow/src/managed_services/composer/provider.rscrates/flowrs-airflow/src/managed_services/conveyor.rscrates/flowrs-airflow/src/managed_services/expand.rscrates/flowrs-airflow/src/managed_services/mwaa.rssrc/airflow/client/impls/dag_ops.rssrc/airflow/client/impls/dagrun_ops.rssrc/airflow/client/impls/taskinstance_ops.rssrc/airflow/client/open_url.rs
jvanbuel
force-pushed
the
refactor/airflow-error-type
branch
from
July 22, 2026 17:28
e5af34f to
f5974e0
Compare
The library returned anyhow::Result from every public method, so callers could not tell a connection failure from a 404 from a schema mismatch, and any consumer was forced onto anyhow. Introduce AirflowError (thiserror) with the categories a caller can act on, and a crate-level Result alias. Three user-visible improvements fall out of this: - Non-success responses now carry the response body. error_for_status() throws it away, which is exactly where Airflow puts its 'detail' explaining why a trigger/clear/mark was rejected; the TUI popup rendered only the status line. - Every endpoint now reports parse failures with a body snippet. That handling existed in v1 only (parse_json_response); response.json() elsewhere gave a bare serde message with no context. - The endpoint URL is parsed once when the client is built rather than on every request, so a bad endpoint is reported at connect time. BaseClient::client/config and V1Client/V2Client::base become private, and endpoint() returns the parsed &Url, which also removes the re-parse in the TUI's open-url builders. The managed-service integrations keep anyhow internally for their layered .context() messages; the chain is flattened into the error message at the public boundary so nothing is lost in the Display the TUI renders.
Owner
Author
|
Also removed |
jvanbuel
force-pushed
the
refactor/airflow-error-type
branch
from
July 22, 2026 17:32
f5974e0 to
f4728a6
Compare
Normalize the parsed endpoint in BaseClient::new to end with a trailing slash so base_api's relative join extends the endpoint instead of replacing its final segment, keeping reverse-proxy prefixes like /airflow. Also keep the URL query string when building Status errors so pagination and filter parameters stay visible. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rows 20–26 of the review: the
flowrs-airflowlibrary API pass.The problem
Every public method returned
anyhow::Result. A caller could not tell a connection failure from a 404 from a schema mismatch, and any consumer of the crate was forced ontoanyhow(M-ERRORS-CANONICAL-STRUCTS, M-DONT-LEAK-TYPES).What changed
A new
AirflowError(thiserror) with the categories a caller can actually act on —InvalidUrl,Http,Status,Decode,Auth,Discovery,FeatureNotEnabled— plus a crate-levelResultalias.Three user-visible improvements fall out of it:
error_for_status()throws the body away, which is exactly where Airflow puts thedetailexplaining why a trigger/clear/mark was rejected. The error popup showedHTTP status client error (422 Unprocessable Entity) for url ...and nothing about why; it now shows Airflow's own message.parse_json_response); theresponse.json()calls everywhere else produced a bare serde message with no context. Both are now the sharedread_json.Encapsulation
BaseClient::client/::configandV1Client::base/V2Client::baseare now private (they werepuband leakedreqwest::Client; nothing outside the crate used them).endpoint()returns the parsed&Url, which also lets the TUI's open-url builders drop their re-parse and its error path.base_apiispub(crate)— it hands out areqwest::RequestBuilderand was never an intended extension point.Scope
The managed-service integrations keep
anyhowinternally for their layered.context()messages. The chain is flattened into the message at the public boundary (AirflowError::auth/::discovery) so nothing is lost in theDisplaythe TUI renders — the app only ever printse.to_string(), and a#[source]chain would have been silently dropped there.Two things deliberately still expose
reqwest, and I think correctly:AuthProvidertakes and returns aRequestBuilder(it is an HTTP-auth extension point; wrapping it would buy nothing), andAirflowError::Httpholds areqwest::Erroras its source (preserving the real cause is the point).The TUI's own trait layer keeps
anyhow— that is the application choosing its error type, which is the whole point of the library no longer forcing one.Tests
Four new tests, each covering a behaviour that actually changed and would fail against a plausible wrong implementation:
Statuserror surfaces the response bodyBaseClient::newrejects an unusable endpoint at constructionValidation
cargo clippy --workspace --all-targets --all-features -- -D warningscleancargo hack --feature-powerset -p flowrs-airflow clippy --all-targets -- -D warningsclean across all 16 feature combinations (this touched a lot ofcfg-gated code)cargo test --workspace --lib --bins— 227 passcargo fmt --all --checkcleanFollow-ups not in this PR
The individual managed-service helpers (
get_conveyor_environment_servers,MwaaClient::*, …) are stillpuband still returnanyhow::Result. The app only usesexpand_managed_services(now clean) and the Composer region helpers, so most of them could simply becomepub(crate)— a smaller, separate change than converting them.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes