Skip to content

Conversation

@alukach
Copy link
Contributor

@alukach alukach commented May 12, 2025

What I'm changing

This PR attempts to wrangle the errors within the handlers to support better logging.

Currently, we are exploring mysterious 404 responses when the get_object endpoint is being overwhelmed. While handling this endpoint, errors such as Err(()) are thrown, smothering any information about the underlying cause of failure.

How I did it

Utilize the thiserror crate to create an enum of possible failures.

Tip

This is the PR is the beginning of transitioning the entirety of the codebase to make use of our thiserror enum. Eventually, this enum will replace the ApiError trait that we are currently using:

pub trait APIError: std::error::Error + Send + Sync {
fn to_response(&self) -> HttpResponse;
}

In an attempt to reduce boilerplate code, I created a helper process_json_response function to handle routine errors from the Source API (not the proxy, but the API managed in https://github.com/source-cooperative/source.coop/). I feel like there may be a better way to do this.

pub async fn process_json_response<T: DeserializeOwned>(
response: Response,
not_found_error: BackendError,
) -> Result<T, BackendError> {
let status = response.status();
if status.is_success() {
response
.json::<T>()
.await
.map_err(|err| BackendError::JsonParseError {
url: err
.url()
.map(|u| u.to_string())
.unwrap_or("unknown".to_string()),
})
} else if status == StatusCode::NOT_FOUND {
Err(not_found_error)
} else {
let url = response.url().to_string();
let status = response.status().as_u16();
let is_server_error = response.status().is_server_error();
let message = response.text().await.unwrap_or("unknown".to_string());
if is_server_error {
Err(BackendError::ApiServerError {
url,
status,
message,
})
} else {
Err(BackendError::ApiClientError {
url,
status,
message,
})
}
}
}

Errors are raised and handled within the API controller, where they are logged. Being that thiserror implements the Display trait, I believe that the logs should render the error messages as described in the enum:

let client = match api_client
.get_backend_client(&account_id, &repository_id)
.await
{
Ok(client) => client,
Err(err) => {
error!("Error getting backend client: {}", err);
return HttpResponse::from(err);
}
};

pub enum BackendError {
#[error("repository not found")]
RepositoryNotFound,
#[error("failed to fetch repository permissions")]
RepositoryPermissionsNotFound,
#[error("source repository missing primary mirror")]
SourceRepositoryMissingPrimaryMirror,
#[error("data connection not found")]
DataConnectionNotFound,

The From trait allows these errors to be translated into appropriate HTTP Responses:

impl From<BackendError> for HttpResponse {
fn from(error: BackendError) -> HttpResponse {
match error {
BackendError::RepositoryNotFound => HttpResponse::NotFound().finish(),
BackendError::SourceRepositoryMissingPrimaryMirror => HttpResponse::NotFound().finish(),
BackendError::DataConnectionNotFound => HttpResponse::NotFound().finish(),
BackendError::ReqwestError(_e) => HttpResponse::BadGateway().finish(),
BackendError::ApiServerError {
url: _url,
status: _status,
message: _message,
} => HttpResponse::BadGateway().finish(),
BackendError::ApiClientError {
url: _url,
status: _status,
message,
} => HttpResponse::BadGateway().body(format!("{}", message)),
BackendError::JsonParseError { url: _url } => {
HttpResponse::InternalServerError().finish()
}
BackendError::UnexpectedDataConnectionProvider {
provider: _provider,
} => HttpResponse::InternalServerError().finish(),
BackendError::RepositoryPermissionsNotFound => HttpResponse::BadGateway().finish(), // _ => HttpResponse::InternalServerError().finish(),
}
}
}

Along the way, I attempted to flatten the deeply nested match statements found within the code.

PR Checklist:

  • This PR is made against the dev branch (all proposed changes except releases should be against dev, not main).
  • This PR has no breaking changes.
  • This PR contains only one commit.
  • I have updated or added new tests to cover the changes in this PR.
  • All tests are passing.
  • This PR affects the Source Cooperative Frontend & API,
    and I have opened issue/PR #XXX to track the change.

@alukach alukach requested a review from jedsundwall May 12, 2025 20:35
Copy link

@jedsundwall jedsundwall left a comment

Choose a reason for hiding this comment

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

Thank you for introducing me to 'thiserror'. This seems like obviously important and useful work. Thanks for getting it started. Fewer mysteries!

@jedsundwall jedsundwall marked this pull request as draft May 12, 2025 21:34
jedsundwall
jedsundwall previously approved these changes May 12, 2025
Copy link

@jedsundwall jedsundwall left a comment

Choose a reason for hiding this comment

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

Approved.

@jedsundwall jedsundwall marked this pull request as ready for review May 12, 2025 21:35
@alukach alukach merged commit 90e3475 into dev May 12, 2025
@alukach alukach deleted the feature/detailed-error-handling branch May 12, 2025 22:00
@alukach alukach mentioned this pull request May 13, 2025
6 tasks
venkat-ecov-dev added a commit that referenced this pull request May 15, 2025
*feat: Begin error handling rework (#58)

* fix: rectify crate version (#61)

* refactor: simplify repository fetching logic in SourceAPI (#62)

* Simplified error handling for missing primary mirror.
* Replaced fetch_repository method with direct API call in is_authorized.
* Improved clarity in API key processing and response handling.

---------

Co-authored-by: venkat-ecov-dev <ecovdev@ecovoice.ca>
Co-authored-by: source-coop-release[bot] <187876225+source-coop-release[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants