kittycad / kittycad.rs Goto Github PK

openapitor's generated code includes documentation on struct fields. The docs are taken directly from the OpenAPI spec. For example, this fragment of Ramp's OpenAPI spec:

            "ApiCardUpdate": {
                "type": "object",
                "properties": {
                    "card_program_id": {
                        "type": "string",
                        "format": "uuid",
                        "nullable": true,
                        "description": "Specify a card program to link with.\n            This will override the card's spending restrictions with those of the card program.\n            Pass card_program_id = None to detach the card's current card program.\n\n            If the card_program_id field is specified, then the card program's changes will override any other changes.\n            For example, if both spending_restrictions and card_program_id are passed, then the new spending restrictions\n            will match those of the card program (not the passed spending restrictions).\n            "
                    },
}
}

As of openapitor commit a442e7d (main), generates this fragment of Rust:

pub struct ApiCardUpdate {
    #[doc = "Specify a card program to link with.\n            This will override the card's \
             spending restrictions with those of the card program.\n            Pass \
             card_program_id = None to detach the card's current card program.\n\n            If \
             the card_program_id field is specified, then the card program's changes will \
             override any other changes.\n            For example, if both spending_restrictions \
             and card_program_id are passed, then the new spending restrictions\n            will \
             match those of the card program (not the passed spending restrictions).\n            "]
    #[serde(default, skip_serializing_if = "Option::is_none")]
    pub card_program_id: Option<uuid::Uuid>,

However, something is wrong with the newlines here -- the doc on card_program_id makes cargo test fail. The failure is:

---- src/types.rs - types::ApiCardUpdate::card_program_id (line 1565) stdout ----
error: expected one of `!`, `.`, `::`, `;`, `?`, `{`, `}`, or an operator, found `the`
 --> src/types.rs:1566:4
  |
3 | If the card_program_id field is specified, then the card program's changes will override any other changes.
  |    ^^^ expected one of 8 possible tokens

I can reproduce this in Rust Playground. Basically it's outputting this which is invalid, but if I escape the newlines differently, or I remove the leading spaces, it works.

I think what's happening is that the leading spaces on the lines in the doc are being interpreted as a code block. See rustdoc reference.

so the user gets warned

It's not really IO-bound at all, it does one big block of code generation (CPU-bound) then one big IO
write to stdout. So no need for async. This isn't a high-priority issue, but it would speed up openapitor
builds.

Right now all the openapi generated types derive traits from the the clap crate. Not every user will want that feature, though, and clap is a relatively weighty dependency, so I think it should be controlled by a crate feature.

either make issues or fix

• make gusto test work
• fix docs for when there is oauth 2.0 token refresh
• pagination for different types (gusto)

github api spec uses this

Current KittyCAD API schema includes a type Point3d. For some reason, the code generation from openapitor is generating examples incorrectly. Note that in the example below, the Point3d type isn't being used like other types are.

use std::str::FromStr;
async fn example_modeling_cmd() -> anyhow::Result<()> {
    let client = kittycad::Client::new_from_env();
    let result: serde_json::Value = client
        .modeling()
        .cmd(&kittycad::types::ModelingCmdReq {
            cmd: kittycad::types::ModelingCmd::AddLine {
                from: Point3D {
                    x: 3.14 as f64,
                    y: 3.14 as f64,
                    z: 3.14 as f64,
                },
                to: Point3D {
                    x: 3.14 as f64,
                    y: 3.14 as f64,
                    z: 3.14 as f64,
                },
            },
            cmd_id: uuid::Uuid::from_str("d9797f8d-9ad6-4e08-90d7-2ec17e13471c")?,
            file_id: "some-string".to_string(),
        })
        .await?;
    println!("{:?}", result);
    Ok(())
}

This example won't compile, so cargo test fails. It suggests:

error[E0422]: cannot find struct, variant or union type `Point3D` in this scope
  --> src/modeling.rs:32:21
   |
16 |                 to: Point3D {
   |                     ^^^^^^^ not found in this scope
   |
help: consider importing this struct
   |
2  | use kittycad::types::Point3D;
   |

We should fix openapitor so that its generated examples properly qualify the Point3d name. @jessfraz says this is related to the in_crate: bool parameter.

same w kittycad generation even tho it is working, temporarily disabling for now

Right now, every time you run openapitor, it generates different random example strings (for example, usernames). This is fine, but it's annoying because the generated code isn't reproducible, and it clutters your git history.

This should be easy to fix, though. Openapitor should choose a specific random seed when generating example values. It doesn't need to be secure or even random because it's only being used to generate examples.

Tested on Rust 1.71, 1.72 and 1.74.0-nightly (d14c85f4e 2023-09-05).

Compiling this crate with default features takes over 10 minutes. This is unacceptable.

This didn't use to happen. An earlier kittycad.rs release (one of the commits under 0.2.2) takes only 43 seconds to compile in release mode.

Similarly, you can see that compile times for the KittyCAD cli app go from ~30 minutes to ~3 hours in this commit. KittyCAD/cli@9c77080#diff-13ee4b2252c9e516a0547f2891aa2105c3ca71c6d7a1e682c69be97998dfc87e.

Currently bisecting to figure this out.

Currently the kittycad.rs test test_stream is failing. Why? Because it's trying to deserialize ApiCallWithPrice, but it finds an empty string when it tries to deserialize an IP address.

Test failure

---- tests::test_stream stdout ----
thread 'tests::test_stream' panicked at 'Serde Error: 
 1 | ...T07:00:23.846Z","ip_address":"","status_code":500,"method":"G...
   |                                   ^ invalid IP address syntax at line 1 column 8482785
', kittycad/src/tests.rs:112:25
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

The relevant field is defined in kittycad.rs:

    #[doc = "The ip address of the origin."]
    #[serde(default, skip_serializing_if = "Option::is_none")]
    pub ip_address: Option<std::net::IpAddr>,

and comes from this definition in api-deux

    /// The ip address of the origin.
    #[serde(default)]
    pub ip_address: crate::db::ipaddr::IpAddr,

Note that crate::db::ipaddr::IpAddr is a newtype around Option<std::net::IpAddr>, so kittycad.rs should know it can always be None and should deserialize the empty string into None.

This will require tweaking the Rust that openapitor outputs for openAPI fields like this, so, let's examine the OpenAPI spec for that field. It is:

          "ip_address": {
            "default": "",
            "description": "The ip address of the origin.",
            "format": "ip",
            "title": "String",
            "type": "string"
          },

In #184 I got websocket support working, but I didn't impl examples for them.

When openapitor generates code, it includes documentation and examples. For the Common Room API, its example code for the update_user_account function doesn't actually compile.

---- src/scim.rs - scim::Scim::update_user_account (line 145) stdout ----
error[E0308]: mismatched types
  --> src/scim.rs:154:39
   |
12 |                   operations: Some(vec![commonroom_api::types::UpdateUserAccountRequestBodyOperations {
   |  _______________________________________^
13 | |                     op: Some("some-string".to_string()),
14 | |                     value: Some(commonroom_api::types::UpdateUserAccountRequestBodyOperationsValue {
15 | |                         active: Some(false),
16 | |                     }),
17 | |                 }]),
   | |_________________^ expected struct `Operations`, found struct `UpdateUserAccountRequestBodyOperations`

error: aborting due to previous error

The problem is that the generated code thinks the operations field of commonroom_api::types::UpdateUserAccountRequestBody is a Vec of commonroom_api::types::Operations. But the example thinks the field is a Vec of commonroom_api::types::UpdateUserAccountRequestBodyOperations. Both these types actually exist, so I'm not sure whether the generated code is incorrect or the example is incorrect.

For now let's assume that the generated code is correct and the example incorrect. The fn generate_example_json_from_schema generates the struct instantiation used in the example code, including the initialization of the operations field. So that's where the problem is.

This file:

https://github.com/KittyCAD/kittycad.rs/blob/main/openapitor/tests/library/kittycad.tests.rs

isn't necessary, as KittyCAD has its own test file: https://github.com/KittyCAD/kittycad.rs/blob/main/kittycad/src/tests.rs

It's being used by this OpenAPItor test:

• github
• oxide
• gusto
• mailchimp
• ramp
• docusign
• any other weird ones
• stripe
  #29

github api spec uses this

Looks like we're getting different results for the mass endpoint as of last week. Is the new result more accurate, or is there a bug here? @jessfraz @hanbollar

thread 'tests::test_create_file_volume' panicked at 'assertion failed: (left == right)
Diff < left / right > :
Some(
< 53.601147,

48.800293,
)

Currently openapitor generates a method for each API endpoint. This method creates a reqwest::Request with a body and path/query/fragment taken from the method's parameters, sends it, handles errors, then parses the response into some output type.

So, the autogenerated methods need to do a lot of things -- create requests, send them, handle errors, parse responses.

To be clear, the current approach works great. But I'd like to propose a general principle: if we can reduce the amount of autogenerated code without causing programmers to do any boilerplate work when specs change, we should.

I propose an alternative, inspired by cloudflare-rs. In that crate, endpoints are structs, not methods. When a team adds an endpoint, they just add a new struct to the codebase and implement certain traits on it. The core maintainers of cloudflare-rs wrote an API client struct with one "call_api" method which takes any endpoint struct as parameter. The only problem with this approach is that every team has to handwrite the endpoint structs and check they're correct with the API spec. If those endpoint structs were autogenerated instead, there'd be no problem.

So basically, I propose that we (KittyCAD) automate via codegen everything which comes from OpenAPI, and we handwrite the other parts (e.g. the core HTTP, networking, error handling).

• For every HTTP endpoint, openapitor generates a Request struct and Response struct (e.g. UnitConversionReq, UnitConversionResp). The request type has fields for the request body, query params, path params, etc, all taken from the API spec. The response type similarly has fields for the response schema.
  • Put another way, openapitor only generates Rust types for each request and response. It does not generate code to actually send to API endpoints.
  • This includes generating docs for each struct showing exactly how to instantiate it.
• The actual API client and its concerns (sending API requests, receiving API responses, handling errors) are handled by handwritten types.

This works because the per-endpoint work is the repetitive, always-growing part. Sending/receiving/error-handling the API networking doesn't change when we add new endpoints or change an OpenAPI spec. This reduces the amount of code being generated, which means faster compile times and easier maintenance for KittyCAD engineers, because we've automated the part which changes frequently and handwritten the part that always stays the same.

How would the code-generated Request/Response structs get used by the handwritten client code?

1. Define a trait Endpoint, which is implemented by all the autogenerated structs. Something like

trait ApiEndpoint<Response: Deserialize> {
    // These methods default to None to reduce the boilerplate for endpoints that don't have body/query params
    fn body(&self) -> Option<Bytes> { None }
    fn query(&self) -> Option<String> { None }
    fn headers(&self) -> Option<http::HeaderMap> { None }
    // These methods are always required, because every endpoint has a different path/method
    fn path(&self) -> String;
    fn method(&self) -> http::Method;
}

2. Define a Client struct which takes any Endpoint and can send it.

struct Client{
    client: reqwest::Client,
    base_url: String,
}

impl Client {
    async fn call<Api: ApiEndpoint>(api: Api) -> Result<Api::Response> {
        let req = reqwest::RequestBuilder::new()
            .body(api.body())
            .query(api.query())
            .path(api.path())
            .method(api.method())
            .headers(api.headers())
            .build()
        let resp = self.0.send(req).await?.bytes().await?;
        let resp: Api::Response = serde_json::from_vec(resp)?;
        Ok(resp)
    }
}

Because the code for sending/receiving/error-handling is defined once and is handwritten, the Rust compiler has less work to do per-endpoint, lowering code generation pressures that result in really slow compile times in crates like async-stripe which also rely on autogenerated methods.

Currently openapitor's websocket support isn't actually very useful. When Openapitor reads in the Websocket endpoint that Dropshot generates, openapitor outputs an API client endpoint that returns Result<()>.

Instead, openapitor should generate an API client endpoint which returns a Websocket client. Probably return either

• a tokio_tungstenite::WebsocketStream.
• a general-purpose Rust stream with a source and sink (abstracting away from the underlying websocket library)
  • Maybe enforce that the body messages must be JSON and must be ModelingCmd types

from convo: https://github.com/KittyCAD/website/issues/742#issuecomment-1229055472

Code generation for enums with fields, e.g. my ModelingCmd type is broken. It produces an enum like this:

pub enum ModelingCmd {
    ModelingCmdOneOf(ModelingCmdOneOf),
    ModelingCmdOneOfOneOf(ModelingCmdOneOfOneOf),
    ModelingCmdOneOfOneOfOneOf(ModelingCmdOneOfOneOfOneOf),
    ModelingCmdOneOfOneOfOneOfOneOf(ModelingCmdOneOfOneOfOneOfOneOf),
}

Instead it should produce something like

pub enum ModelingCmd {
    ModelingCmdAddLine(ModelingCmdAddLine),
    ModelingCmdMouseDragStart(ModelingCmdMouseDragStart),
    ModelingCmdMouseDragMove(ModelingCmdMouseDragMove),
    ModelingCmdMouseDragEnd(ModelingCmdMouseDragEnd),
}

duplicate of #184

Dependabot keeps bumping crates within the generated kittycad crate. It bumps crates within kittycad/Cargo.lock, but then the Cargo.toml file that's generated within openapitor/src/lib.rs still says to generate a kittycad crate with the old version.

So then the next time you run make kittycad it overwrites the updated dep from dependabot with the old dep from the generated Cargo.toml

IMO we should stop Dependabot updating the generated kittycad crate.

When the KittyCAD API returns a HTTP 4xx or 5xx error to the KittyCAD rust client, it triggers the Error::UnexpectedResponse variant.

Generally our unit tests show the debug output of errors if they occur.

Unfortunately neither the debug nor display implementations for UnexpectedResponse are very helpful, because it doesn't show users the response body. Why not? Well, the UnexpectedResponse variant has one field, reqwest::Response. But this variant doesn't actually show you the response body, because reading the response body consumes the response. This means if you wanted to put the response body in the Display impl, it would consume the error object, so you'd only be able to read the error once.

Solution: the UnexpectedResponse variant should not contain a reqwest::Response. Instead, when the variant is constructed, it should:

1. Store useful data like headers and HTTP status
2. read the response body as text
3. Store the headers and response body in a field of the enum

This way the errors will be helpful, contain the response body, and don't consume data when read.

Problem

The enum variants of enums like ModelingCmd don't have docs! See https://docs.rs/kittycad/latest/kittycad/types/enum.ModelingCmd.html

The docs are in the spec.json so this is an openapitor problem.