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.

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

[API] Rework response and error handling #2139

Merged
merged 1 commit into from
Jul 26, 2022
Merged

Conversation

banool
Copy link
Contributor

@banool banool commented Jul 21, 2022

Description

This PR reworks the response and error handling.

The good:

  • Previously, every endpoint returned the same response struct. This meant the spec would say every endpoint could return (for example), 200, 202 on the success path and 400, 404, 413, 500 on the failure path. This is misleading because not all endpoints return the same set of status codes (as evidenced by the existing spec). With this change, we have the ability to generate types at a more granular level for each endpoint.
  • Returning errors from endpoint handler functions is much less verbose now.
  • "Internal functions", as in those below the endpoint handlers (but still in the API code) can now return errors specific to the endpoints, with the status code data baked in. This is all guaranteed at compile time, we don't do any runtime checks to the effect of "can this endpoint handler handle the status code returned by this function).
  • It removes a great deal of repetition that was already getting out of hand in our response types.

The bad:

  • In some cases, we cannot use existing traits like TryFrom because it doesn't work with the pattern we're using for this internal function error returning model. See this SO question. This just means we have to write our own bare functions, not a big deal.

The ugly:

  • To make this all happen, I wrote a few macros. These macros are responsible for creating the error type traits that we use for all this status code based type checking, error response types that leverage them, and success response types. To mitigate the fact that macros are often hard to understand, I've added a gratuitous amount of comments explaining in what is happening.
  • Many endpoint handlers cannot imply the error type used in their results, meaning there are a lot of turbofish (::<>) uses. Down the line we can investigate how to hint to rustc what is happening, since I feel it should be possible for it to figure out what's going on. Figured it out, much nicer.

The macros are pretty much a way to concentrate complexity that was previously all over the API codebase into just one place. At any rate I don't think there are many other good ways to make such strong checks happen at compile-time.

Test Plan

Same as for #1906. The main test is to run the server see what spec it generates:

curl localhost:8080/v1/spec.yaml | gist -f spec.yaml 

Output: https://gist.github.com/banool/c9bc0659d4b155d7dd9584480378b8fc.


This change is Reviewable

@banool banool changed the base branch from main to banool/generate_openapi July 21, 2022 22:38
@banool banool force-pushed the banool/complete_poem_api branch from 209eea4 to 09b2729 Compare July 22, 2022 00:13
@banool banool changed the title [API] todo [API] Rework response and error handling Jul 22, 2022
@banool banool requested review from gregnazario and jjleng July 22, 2022 00:14
@banool banool force-pushed the banool/complete_poem_api branch from 09b2729 to d9a170e Compare July 22, 2022 00:37
@banool banool force-pushed the banool/generate_openapi branch 2 times, most recently from 7fa4e92 to ca5c73b Compare July 22, 2022 00:47
Base automatically changed from banool/generate_openapi to main July 22, 2022 01:52
@banool banool force-pushed the banool/complete_poem_api branch 2 times, most recently from 36599e3 to 64c8c08 Compare July 22, 2022 04:18
@banool banool marked this pull request as ready for review July 22, 2022 04:18
@banool banool force-pushed the banool/complete_poem_api branch from 64c8c08 to ef0a578 Compare July 22, 2022 14:40
Comment on lines 116 to 118
// TODO: Add error codes to these errors.
pub fn get_latest_ledger_info_poem<E: InternalError>(&self) -> Result<LedgerInfo, E> {
if let Some(oldest_version) = self.db.get_first_txn_version().map_err(E::internal)? {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did the error codes go away?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oop they don't need to, I can add them back.

anyhow::format_err!("Given limit value ({}) must not be zero", limit,).to_string(),
))));
return Err(E::bad_request_str(&format!(
"Given limit value ({}) must not be zero",
Copy link
Contributor

Choose a reason for hiding this comment

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

We should make sure to come back later and ensure these error messages are consistent

Comment on lines 349 to 394
generate_success_response!(BasicResponse, (200, Ok));

// Generate traits defining a "from" function for each of these status types.
// The error response then impls these traits for each status type they mention.
generate_error_traits!(Internal, BadRequest, NotFound);

// Generate an error response that only has options for 400 and 500.
generate_error_response!(BasicError, (400, BadRequest), (500, Internal));

// This type just simplifies using BasicResponse and BasicError together.
pub type BasicResult<T> = poem::Result<BasicResponse<T>, BasicError>;

// As above but with 404.
Copy link
Contributor

Choose a reason for hiding this comment

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

We've got a ton of macros, this isn't the easiest to read, but let's not index too much on these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed I'd like to come back to this, I'd like to find a non-macro approach here. Once you look past the macro complexity though the rest of the code becomes very nice and typed.

@banool banool force-pushed the banool/complete_poem_api branch from ef0a578 to f3b8f20 Compare July 26, 2022 00:29
@banool banool force-pushed the banool/complete_poem_api branch from f3b8f20 to b6576de Compare July 26, 2022 00:35
@banool banool enabled auto-merge (squash) July 26, 2022 00:38
@github-actions
Copy link
Contributor

✅ Forge test success on b6576ded87ac844f06afc9979f4d7b0ad6e23b50

all up : 5755 TPS, 2880 ms latency, 4600 ms p99 latency,no expired txns

@banool banool merged commit 32c5e4d into main Jul 26, 2022
@banool banool deleted the banool/complete_poem_api branch July 26, 2022 01:12
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.

None yet

2 participants