utoipa proof of concept on orderbook endpoint #2338

Flugplatz · 2024-01-29T18:03:57Z

Description

Proof of concept usage of utoipa to automatically generate openapi yaml files for 1 orderbook api endpoint for the issue here: #2203.

Looking for feedback from core dev team before progression.

Changes

Include utoipa crate.
Add annotations on create_order endpoint and dependent structs.
Create an ApiDoc struct inside api.rs to reference / describe the endpoint(s) via the annotations.
Add an openapi binary which references api::ApiDoc and prints the openapi schema to stdout (to be included in release automation later).

How to test

cargo r --bin openapi > openapi.yml #(example attached)
view swagger output via https://editor.swagger.io/

Notes

#[ToSchema] annotation does not work on enums
Some of the more complicated types, or ones which we don't own have been casted to simpler types in the openapi #[schema(value_type = XXX)] annotation.

github-actions · 2024-01-29T18:04:11Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

Flugplatz · 2024-01-29T18:06:22Z

I have read the CLA Document and I hereby sign the CLA

Flugplatz · 2024-01-29T18:08:51Z

recheck

MartinquaXD

Thanks for taking a crack at this! This will probably make our lives a whole lot easier long term.
Good thing you started with this endpoint. It's probably the most complex and shows most of the challenges that will have to be tackled along the way.
I think we might have to introduce a new translation layer that turns a struct that is purely there for defining the API into a struct that is used throughout the code base. For the least amount of churn the used struct should probably stay the current order struct. Then the new type can be more geared towards optimal support for utoipa.

As a follow up idea it would be cool if we could have a CI job that checks whether some PR changed a public facing API so we don't have changes by accident.

MartinquaXD · 2024-01-30T07:19:42Z

crates/model/src/order.rs

 #[serde(rename_all = "camelCase")]
+#[schema(example = json!({


Does it get checked that this type actually satisfies the interface during compilation?

I think it would be better to specify example value closer to the field or even type. Currently it would be a bit difficult to maintain any changes.

Is this even necessary, given that you specify example values in each field's definition?

This json structure was not type checked. I agree with the group that it's better to add example values on each fields definition.

I couldn't get example types on each field definition to work previously but now I've determined its cause is the serde flattened types. Will investigate further.

MartinquaXD · 2024-01-30T07:25:11Z

crates/model/src/order.rs

    #[serde_as(as = "HexOrDecimalU256")]
+    #[schema(value_type = String)]
    pub sell_amount: U256,


For things like U256 and H160 I think it makes sense to add a new type that implements ToSchema and the sometimes custom deserialization logic. This will be annoying for the types in the orderbook because we used the API types in the business logic but eventually we should have a separation of concerns and the introduction of utoipa seems like a good option.

crates/model/src/order.rs

MartinquaXD · 2024-01-30T07:33:46Z

crates/orderbook/src/api/post_order.rs

+    request_body = OrderCreation,
+    responses(
+        (status = 200, description = "Order has been accepted.", body = OrderUid),
+        (status = 400, description = "Error during order validation."),


Is it possible to define this on an error struct returned by this endpoint instead?
That way it could probably be enforced that every error must have a description.

Maybe we can implement https://docs.rs/utoipa/latest/utoipa/trait.IntoResponses.html on AddOrderError

squadgazzz · 2024-01-30T18:17:00Z

crates/orderbook/src/openapi.rs

+    let builder: OpenApiBuilder = orderbook::api::ApiDoc::openapi().into();
+    let doc = builder.servers(Some(servers)).info(info).build();
+
+    println!("{}", doc.to_pretty_json().unwrap());


Currently, we need this file for debugging only?

Should be to_yaml

@squadgazzz I assume there is an offline process for hosting the openapi yaml file on the website, this binary is used to produce that file. This could be automated somehow at build time in build.rs or with CI jobs.

In the future there is warp / swagger integration which could be explored to actually host swagger from the rest service but I've left this as follow up work.

fleupold

This is a really nice contribution already!

There are some small inconsistencies (maybe you can submit the generated yml file in the PR for ease of review). It would be great if we can move the response code definitions closer to where they actually get created.

Already for the struct definitions (which change more frequently) this is a great improvement.

crates/orderbook/openapi.yml

crates/orderbook/src/api/post_order.rs

fleupold · 2024-01-31T08:42:55Z

crates/model/src/order.rs

+    /// If set, the backend enforces that this address matches what is decoded
+    /// as the *signer* of the signature. This helps catch errors with
+    /// invalid signature encodings as the backend might otherwise silently
+    /// work with an unexpected address that for example does not have
+    /// any balance.


If you look at the generated yaml, for some reason the flattened types don't show up as named arguments on the top of OrderCreation but instead unnamed in the allOf definition. Any idea why that is?

This seems to cause a bigger issue, the field attributes (ie. "example") on the flattened type seem to overwrite the parent types definition, investigating further.

fleupold · 2024-01-31T08:44:00Z

crates/model/src/order.rs

 #[serde(rename_all = "camelCase")]
+#[schema(example = json!({


Is this even necessary, given that you specify example values in each field's definition?

fleupold · 2024-01-31T08:48:56Z

crates/orderbook/src/openapi.rs

+    let builder: OpenApiBuilder = orderbook::api::ApiDoc::openapi().into();
+    let doc = builder.servers(Some(servers)).info(info).build();
+
+    println!("{}", doc.to_pretty_json().unwrap());


Should be to_yaml

fleupold · 2024-01-31T08:52:03Z

crates/orderbook/src/api/post_order.rs

+    request_body = OrderCreation,
+    responses(
+        (status = 200, description = "Order has been accepted.", body = OrderUid),
+        (status = 400, description = "Error during order validation."),


Maybe we can implement https://docs.rs/utoipa/latest/utoipa/trait.IntoResponses.html on AddOrderError

crates/orderbook/src/openapi.rs

Flugplatz · 2024-01-31T13:04:59Z

Added some small fixes, continuing with the following bigger items in priority order:

Serde flattened types / Utoipa integration issues.
Replace "example" json blob at struct level with per-field versions.
Annotate AddOrderError with openyaml error information.
Wrapping of complex types like H160 for "business logic" model.

github-actions · 2024-02-08T00:06:01Z

This pull request has been marked as stale because it has been inactive a while. Please update this pull request or it will be automatically closed.

Flugplatz · 2024-02-08T17:18:37Z

Changes:

Implemented IntoResponses on AddOrderError.
Managed to get serde flattened fields to work but the representation was not similar to the existing setup, see utoipa issue here: Serde flattened enum as top level fields (not oneOf) juhaku/utoipa#859
Because of difficulties with flatten this I decided to move forward with a orderbook::api::model representation of OrderCreation and appropriate conversions before / after calls to internal business code. This has resulted in duplicated code which I'm not proud of.
- I'm not sure it's good that both the internal struct and the API struct have the same name. Renaming the API struct will impact the swagger output.
- I tried wrapping external types in the new type pattern (to avoid duplicating them in api::model) but utoipa did not like this.
- I tried extracting out common types like TokenAddress but utoipa would not allow me to do this and still allow separate descriptions for two instances ie. "Token to buy." and "Token to sell.".
- This is still pretty rough, throwing it up early for feedback since it involves a lot of high-level design / architecture decisions.
Open API schema produced attached: openapi.yml.txt, can be viewed here: https://editor.swagger.io/

Known issues:

Swagger UI is representing the example hex strings for addresses etc as decimal, not sure why yet
Need to find a way to set the response body on AddOrderError for validation error which provides error details json.

fleupold · 2024-02-09T14:29:58Z

I decided to move forward with a orderbook::api::model representation of OrderCreation and appropriate conversions before / after calls to internal business code. This has resulted in duplicated code which I'm not proud of.

I think this is in line with our goal of phasing out the use of the shared model's crate type. Creating a stable API representation and letting the business logic use their own domain types going forward (eventually removing the models crate altogether) is something we've talked about internally.

Flugplatz · 2024-02-12T11:25:31Z

lint failure looks to be due to Cargo.lock change - I don't think this is something I can resolve?

local-node errors look genuine, as related to the changed endpoint - I'll reproduce this locally today.

forked-node - not sure about these looks like connectivity issues.

MartinquaXD · 2024-02-12T11:30:39Z

forked-node - not sure about these looks like connectivity issues.

These errors are due to the fact that this PR originated from a forked repo and github doesn't allow the use of secrets in workflows for those. Additionally these tests are known to not be 100% stable. 😅

MartinquaXD · 2024-02-12T11:34:46Z

lint failure looks to be due to Cargo.lock change - I don't think this is something I can resolve?

Try checking in your Cargo.lock file changes. Since you added a new dependency this needs to be added to the lock file and the workflow is not allowed to do it on its own.

Flugplatz · 2024-02-12T21:36:00Z

Committed Cargo.lock.

After reviewing the e2e (tests) crate, it looks like the OrderCreation model object is very useful and widely used there there for creating strongly typed requests and signing them. I think I will need to replace this use with a OrderCreationBuilder type struct which will accept strong types and sign the request to produce a api::model::OrderCreation (flat utoipa friendly struct) and then the server-side will receive this and transform it into a crate::model::OrderCreation with the more complex types.

I don't like using the same name for the two objects (utoipa friendly & complex internal) but If we're going to change one its going to need to be the internal type (otherwise it'll be a breaking change, forcing changes on API callers).

github-actions · 2024-02-20T00:06:17Z

This pull request has been marked as stale because it has been inactive a while. Please update this pull request or it will be automatically closed.

Flugplatz · 2024-02-26T17:31:47Z

I've opened an issue with utoipa to try and figure out how to quote H160 address strings so they are not interpreted as decimal numbers by swagger: juhaku/utoipa#875

…etail

…/cowprotocol-services into utoipa-auto-create-openapi-yml

github-actions · 2024-03-06T00:06:33Z

This pull request has been marked as stale because it has been inactive a while. Please update this pull request or it will be automatically closed.

utoipa proof of concept on orderbook endpoint

fe80a74

Flugplatz requested a review from a team as a code owner January 29, 2024 18:03

github-actions bot added a commit that referenced this pull request Jan 29, 2024

@Flugplatz has signed the CLA from Pull Request #2338

f2d98be

clippy fixes

8a07970

Flugplatz mentioned this pull request Jan 29, 2024

chore: Use utoipa to generate open-api schemas from the code #2203

Open

1 task

MartinquaXD reviewed Jan 30, 2024

View reviewed changes

squadgazzz reviewed Jan 30, 2024

View reviewed changes

fleupold reviewed Jan 31, 2024

View reviewed changes

minor fixes

202e452

github-actions bot added the stale label Feb 8, 2024

design changes

cde4b32

github-actions bot removed the stale label Feb 9, 2024

commit cargo lock

c26ccbd

Merge branch 'main' into utoipa-auto-create-openapi-yml

71813d9

github-actions bot added the stale label Feb 20, 2024

Flugplatz added 2 commits February 26, 2024 18:07

add Error ToSchema type for http error repsonses which return extra d…

fff4be8

…etail

Merge branch 'utoipa-auto-create-openapi-yml' of github.com:Flugplatz…

d06a306

…/cowprotocol-services into utoipa-auto-create-openapi-yml

github-actions bot removed the stale label Feb 27, 2024

github-actions bot added the stale label Mar 6, 2024

github-actions bot closed this Mar 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

utoipa proof of concept on orderbook endpoint #2338

utoipa proof of concept on orderbook endpoint #2338

Flugplatz commented Jan 29, 2024 •

edited

Loading

github-actions bot commented Jan 29, 2024 •

edited

Loading

Flugplatz commented Jan 29, 2024 •

edited

Loading

Flugplatz commented Jan 29, 2024

MartinquaXD left a comment

MartinquaXD Jan 30, 2024

squadgazzz Jan 30, 2024

fleupold Jan 31, 2024

Flugplatz Jan 31, 2024 •

edited

Loading

MartinquaXD Jan 30, 2024

MartinquaXD Jan 30, 2024

fleupold Jan 31, 2024

squadgazzz Jan 30, 2024

fleupold Jan 31, 2024

Flugplatz Jan 31, 2024

fleupold left a comment

fleupold Jan 31, 2024

Flugplatz Jan 31, 2024

fleupold Jan 31, 2024

fleupold Jan 31, 2024

fleupold Jan 31, 2024

Flugplatz commented Jan 31, 2024 •

edited

Loading

github-actions bot commented Feb 8, 2024

Flugplatz commented Feb 8, 2024 •

edited

Loading

fleupold commented Feb 9, 2024

Flugplatz commented Feb 12, 2024

MartinquaXD commented Feb 12, 2024 •

edited

Loading

MartinquaXD commented Feb 12, 2024

Flugplatz commented Feb 12, 2024 •

edited

Loading

github-actions bot commented Feb 20, 2024

Flugplatz commented Feb 26, 2024

github-actions bot commented Mar 6, 2024

		#[serde(rename_all = "camelCase")]
		#[schema(example = json!({

utoipa proof of concept on orderbook endpoint #2338

utoipa proof of concept on orderbook endpoint #2338

Conversation

Flugplatz commented Jan 29, 2024 • edited Loading

Description

Changes

How to test

Notes

github-actions bot commented Jan 29, 2024 • edited Loading

Flugplatz commented Jan 29, 2024 • edited Loading

Flugplatz commented Jan 29, 2024

MartinquaXD left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Flugplatz Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fleupold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Flugplatz commented Jan 31, 2024 • edited Loading

github-actions bot commented Feb 8, 2024

Flugplatz commented Feb 8, 2024 • edited Loading

Changes:

Known issues:

fleupold commented Feb 9, 2024

Flugplatz commented Feb 12, 2024

MartinquaXD commented Feb 12, 2024 • edited Loading

MartinquaXD commented Feb 12, 2024

Flugplatz commented Feb 12, 2024 • edited Loading

github-actions bot commented Feb 20, 2024

Flugplatz commented Feb 26, 2024

github-actions bot commented Mar 6, 2024

Flugplatz commented Jan 29, 2024 •

edited

Loading

github-actions bot commented Jan 29, 2024 •

edited

Loading

Flugplatz commented Jan 29, 2024 •

edited

Loading

Flugplatz Jan 31, 2024 •

edited

Loading

Flugplatz commented Jan 31, 2024 •

edited

Loading

Flugplatz commented Feb 8, 2024 •

edited

Loading

MartinquaXD commented Feb 12, 2024 •

edited

Loading

Flugplatz commented Feb 12, 2024 •

edited

Loading