[API] Add Poem as an alternative backend #1906
Conversation
banool
changed the title
banool
force-pushed
the
banool/generate_openapi
branch
2 times, most recently
from
dab6d5b to
bc8db5c
Compare
|
For the sake of completeness I experimented with utoipa: #1916. |
banool
force-pushed
the
banool/generate_openapi
branch
from
bc8db5c to
8f35ff9
Compare
banool
requested review from
JoshLind,
davidiw and
gregnazario
and removed request for
JoshLind and
davidiw
|
If you end up choosing Poem, I can help you with any Poem-related issues you might have (I hope not) 😁 |
|
Awesome, glad to hear it! I'll continue to contribute back to Poem too. |
banool
force-pushed
the
banool/generate_openapi
branch
3 times, most recently
from
01780fc to
f1f6cd1
Compare
| #[derive(Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd, Serialize, Deserialize)] | ||
| pub struct IdentifierWrapper(pub Identifier); | ||
|
|
||
| impl FromStr for IdentifierWrapper { | ||
| type Err = anyhow::Error; | ||
|
|
||
| fn from_str(s: &str) -> anyhow::Result<Self, anyhow::Error> { | ||
| Ok(IdentifierWrapper(Identifier::from_str(s)?)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Okay so we need the wrappers for some derivable thing in poem? We'll likely need similar things anyways for AccountAddress as well.
There was a problem hiding this comment.
If we had access to Identifier we wouldn't, we could just derive the necessary type right there, but we don't, so currently we have to wrap it. This issue might provide a simpler solution though: poem-web/poem#323.
api/types/src/derive_helpers.rs
Outdated
| #[macro_export] | ||
| macro_rules! impl_poem_type { |
| pub fn get_latest_ledger_info_poem(&self) -> Result<LedgerInfo, AptosErrorResponse> { | ||
| self.get_latest_ledger_info().map_err(|e| { | ||
| AptosErrorResponse::InternalServerError(Json( | ||
| AptosError::new(format_err!("Failed to retrieve ledger info: {}", e).to_string()) |
There was a problem hiding this comment.
You can probably just make these format! if you're converting them to a string anyways.
I do like this adding error codes though super slick.
If you're adding Json( to everything, I'd just make it a function AptosErrorResponse::internal_server_error(err: AptosError), or AptosErrorResponse::internal_server_error(msg: String, error_code: AptosErrorCode)
There was a problem hiding this comment.
The response / error stuff definitely needs love, I'm just deferring it to later as less critical. Agreed it's too verbose right now.
| match mime.as_ref() { | ||
| "application/json" => return Ok(AcceptType::Json), | ||
| "application/x-bcs" => return Ok(AcceptType::Bcs), | ||
| "*/*" => {} |
There was a problem hiding this comment.
Hmm, does it default to this? or would "application/*" be valid? Basically, if I curl the endpoint, do I need to specify the header?
There was a problem hiding this comment.
You do not need to specify the header, it'll be json by default. You need to not error on */* since most clients add it by default.
| #[oai( | ||
| path = "/accounts/:address/modules", | ||
| method = "get", | ||
| operation_id = "get_account_modules", | ||
| tag = "ApiTags::General" | ||
| )] |
There was a problem hiding this comment.
I like this it's very clean
| // TODO: Figure out how to have certain fields be borrowed, like in the | ||
| // original implementation. |
There was a problem hiding this comment.
These should be able to all be borrowed if it's using the one from the Aptos logger. But it's not high priority.
| pub struct AptosError { | ||
| message: String, | ||
| error_code: Option<AptosErrorCode>, | ||
| aptos_ledger_version: Option<U64>, |
There was a problem hiding this comment.
I can't wait until we can get rid of the U64 :(
There was a problem hiding this comment.
Do you know what needs to be done to make this happen? I'd love to get rid of it too.
There was a problem hiding this comment.
We should just write something that converts types to a JSON type if it's u64, so when we deserialize it it comes back as a u64 instead of bothering with this middle type.
api/src/poem_backend/runtime.rs
Outdated
| let apis = ( | ||
| AccountsApi { | ||
| context: context.clone(), | ||
| }, | ||
| BasicApi { | ||
| context: context.clone(), | ||
| }, | ||
| IndexApi { context }, | ||
| ); |
api/src/poem_backend/runtime.rs
Outdated
| .nest("/", api_service) | ||
| // TODO: I prefer "spec" here but it's not backwards compatible. | ||
| // Consider doing it later if we cut over to this entirely. | ||
| // TODO: Consider making these part of the API itself. | ||
| .at("/openapi.json", spec_json) | ||
| .at("/openapi.yaml", spec_yaml) | ||
| .with(cors) | ||
| .around(middleware_log); |
There was a problem hiding this comment.
Let's change it to whatever we want it to be and not worry about the backwards compatibility. If there's a known way to commonly do it, we should use it.
banool
force-pushed
the
banool/generate_openapi
branch
5 times, most recently
from
6426760 to
e86b21c
Compare
banool
force-pushed
the
banool/generate_openapi
branch
from
e86b21c to
433d3c5
Compare
banool
force-pushed
the
banool/generate_openapi
branch
from
433d3c5 to
151bc2d
Compare
There was a problem hiding this comment.
This looks pretty straightforward, but let's do two things:
- Wait until after the branch cut
- Once we commit this, let's break it up into subtasks that you and I can split and work on together. There's a lot here and I know it's asking a bunch for one person to handle it alone.
| #[macro_export] | ||
| macro_rules! impl_poem_type { | ||
| ($($ty:ty),*) => { | ||
| $( | ||
| impl ::poem_openapi::types::Type for $ty { | ||
| const IS_REQUIRED: bool = true; |
There was a problem hiding this comment.
Could you put a comment describing what this does above it?
| pub enum WriteSetChange { | ||
| DeleteModule { | ||
| address: Address, | ||
| state_key_hash: String, | ||
| module: MoveModuleId, | ||
| }, | ||
| DeleteResource { | ||
| address: Address, | ||
| state_key_hash: String, | ||
| resource: MoveStructTag, | ||
| }, | ||
| DeleteTableItem { | ||
| state_key_hash: String, | ||
| handle: HexEncodedBytes, | ||
| key: HexEncodedBytes, | ||
| }, | ||
| WriteModule { | ||
| address: Address, | ||
| state_key_hash: String, | ||
| data: MoveModuleBytecode, | ||
| }, | ||
| WriteResource { | ||
| address: Address, | ||
| state_key_hash: String, | ||
| data: MoveResource, | ||
| }, | ||
| WriteTableItem { | ||
| state_key_hash: String, | ||
| handle: HexEncodedBytes, | ||
| key: HexEncodedBytes, | ||
| value: HexEncodedBytes, | ||
| }, | ||
| DeleteModule(DeleteModule), | ||
| DeleteResource(DeleteResource), | ||
| DeleteTableItem(DeleteTableItem), | ||
| WriteModule(WriteModule), | ||
| WriteResource(WriteResource), | ||
| WriteTableItem(WriteTableItem), | ||
| } |
There was a problem hiding this comment.
So all of these changes, change how they serialize right? So it's a breaking change to the REST API?
There was a problem hiding this comment.
It doesn't change the structure of the serialization, but there are other breaking changes, e.g. around data format. For example, the new API tries to format numbers as numbers, not strings. Also some structs are unpacked, e.g. this kind of thing:
curl http://localhost:8080/accounts/0x1/resources | jq .[0]
{
"type": "0x1::Coin::CoinInfo<0x1::TestCoin::TestCoin>",
"data": {
"decimals": "6",
"name": "Test Coin",
"supply": {
"vec": []
},
"symbol": "TC"
}
}
curl http://localhost:8080/v1/accounts/0x1/resources | jq .[0]
{
"type": {
"address": "0x1",
"module": "Coin",
"name": "CoinInfo",
"generic_type_params": [
"0x1::TestCoin::TestCoin"
]
},
"data": {
"decimals": "6",
"name": "Test Coin",
"supply": {
"vec": []
},
"symbol": "TC"
}
}
| impl_poem_type!(EventKey); | ||
| impl_poem_parameter!(EventKey); |
There was a problem hiding this comment.
Someday it'll be nice to have these as derive macros
| // Poem will run on a different port. | ||
| let poem_address = attach_poem_to_runtime(&runtime, context.clone(), config) | ||
| .context("Failed to attach poem to runtime")?; |
There was a problem hiding this comment.
We should make sure we open the port on 127.0.0.1 then
There was a problem hiding this comment.
Yeah I'll need to experiment with the various deployments to make sure this works. If we need to open a port, I'll have to make this a predefined port, not a randomly selected one.
| pub struct AptosError { | ||
| message: String, | ||
| error_code: Option<AptosErrorCode>, | ||
| aptos_ledger_version: Option<U64>, |
There was a problem hiding this comment.
We should just write something that converts types to a JSON type if it's u64, so when we deserialize it it comes back as a u64 instead of bothering with this middle type.
banool
force-pushed
the
banool/generate_openapi
branch
from
7fa4e92 to
ca5c73b
Compare
banool
added
CICD:build-images
✅ Forge test successForge is land-blocking |
* [API] Add crate to help types self-describe for the OpenAPI spec generator
* [API] Add boilerplate for using Poem as an alternative backend
* [API] Add support for index with Poem backend
* [API] Add TLS support to Poem backend
* [API] Add CORS for Poem API backend
* [API] Add support for logging middleware to the Poem backend
* [API] Add response status tracking to Poem backend
* [API] Break APIs up in Poem backend
* [API] Make a BCS payload type
* [API] Make all API types self-describe for generating an OpenAPI spec
* [API] Fix up Rosetta following adding IdentifierWrapper
* [API] Add request and response types for the JSON / BCS enum
* [API] Add headers to response in Poem backend
* [API] Add endpoint for get_account for Poem backend
* [API] Add /accounts/{address}/resources endpoint
* [API] Add /accounts/{address}/modules endpoint
* [API] Add /events/{event_key} endpoint
* [API] Add /accounts/{address}/events/{event_handle_struct}/{field_name} endpoint
* [API] Run both APIs alongside each other
* [API] Add GET /transactions
* fixup: Remove use_poem_backend_flag
* Add comments explaining what the aptos-openapi macros do
Description
Read me: The new API is not ready to be deployed to prod as default, but I'm sharing it out now to get thoughts on the approach so far. Focus on the high level stuff. Each commit is mostly a neat unit, if you'd like to look at it that way. At this stage I'd like to land the PR as is and continue work in another PR (because this is getting too big). It should be fine to do this since it doesn't mess with the original API in any way.
Today we write our OpenAPI spec by hand, which is a constant source of errors stemming from the spec not matching the reality of the API. These manifest in the clients, which are generated from the spec file, resulting in errors from the user perspective. To fix this, we should instead generate our spec directly from our API. That is what this PR aims to do.
In this PR, I add an alternative web framework to the API crate using Poem. I try to be as uninvasive to the existing API as possible in this PR, so we can choose to cut over to / experiment with the alternative backend at our leisure.
Why Poem? Essentially it's the only legitimate framework in the Rust ecosystem today that supports generating an OpenAPI spec from a Rust API, particularly one as complex as ours. I looked at alternatives and found them lacking:
Nonetheless, Poem is a bit of a gamble, mostly because it's quite new. I'm using it for NHC already, but the stakes are lower there, plus NHC is less complex. Saying that, it works great there and I like the overall layout and feature completeness of Poem.
In its current form, the PR only adds the basic framework / boilerplate and a couple of endpoints, just to demonstrate what this kind of change would look like. As far as I can tell, this would ultimately result in much less boilerplate (no need for both the
get_xandhandle_xfunctions, no need to have a filter for context, etc.) in addition to the stated goal of being able to generate an OpenAPI spec.Some other things missing from this are:
.with(metrics("get_x")): Poem has middleware that we could use for this. Poem also supports prometheus and opentelemetry directly.CORS: Poem has middleware for this.Logging: Middleware again.Proper response objects, particularly for errors: This is easy, I just haven't done it yet.failpoint: I just need to add this.TLS stuff: Just need to add it, Poem supports this.Test Plan
Run a node:
Hit it with queries. At this point, beyond the basics like
/and the spec endpoints, I have implemented just the accounts endpoints. Here are some A/B tests for those, where the first request hits the warp path and the second hits the Poem path (v1):Notes:
This change is