oxidecomputer / progenitor

An OpenAPI client generator
535 stars 68 forks source link

Generate failed for Xero Accounting OpenAPI spec #955

Open dunxen opened 1 month ago

dunxen commented 1 month ago

Hey, thanks for the awesome tool! 💚

It seems that cargo-progenitor (as of 2f32cde, but probably all prior versions 🤷) fails to generate a crate for the Xero Accounting OpenAPI spec.

After downloading the above spec to reproduce, run:

RUST_BACKTRACE="full" cargo progenitor -i xero_accounting.yaml -o xero-client -n xero-client -v 6.3.0

The output is as follows:

gen fail: TypeError(InvalidValue)
Error: generation experienced errors

Stack backtrace:
   0: anyhow::error::<impl anyhow::Error>::msg
   1: cargo_progenitor::main
   2: std::sys::backtrace::__rust_begin_short_backtrace
   3: std::rt::lang_start::{{closure}}
   4: std::rt::lang_start_internal
   5: main
   6: __libc_start_call_main
   7: __libc_start_main_alias_2
   8: _start

There are no errors for the same spec in the Swagger editor.

dunxen commented 1 month ago

I've looked at this again with RUST_LOG=debug. I patched typify and don't think it's related to https://github.com/oxidecomputer/typify/issues/626. I'll keep digging somewhat with what's strange in the Xero spec file.

ahl commented 1 month ago

I'll need to look more closely, but I suspect it might be constructs such as this:

        HasAttachments:
          description: boolean to indicate if an account has an attachment (read only)
          readOnly: true
          type: boolean
          default: "false"
          example: "false"

In particular, the default value is a string "false" which may be failing the validation against the schema, i.e. a boolean. This is speculation and requires some validation.

dunxen commented 1 month ago

Thanks! Yeah, that was it. Now there also appears to be an invalid schema for application/octet-stream. I'll dig a little more:

gen fail: UnexpectedFormat("invalid schema for application/octet-stream: Item(Schema { schema_data: SchemaData { nullable: false, read_only: false, write_only: false, deprecated: false, external_docs: None, example: None, title: None, description: None, discriminator: None, default: None, extensions: {} }, schema_kind: Type(String(StringType { format: Item(Byte), pattern: None, enumeration: [], min_length: None, max_length: None })) })")
Error: generation experienced errors

I'll open an issue on Xero's side once I figure the final bits out if there's nothing else related to typify/progenitor being an issue.

EDIT: Seems unhappy with format: byte:

application/octet-stream:
  schema:
    type: string
    format: byte
ahl commented 1 week ago

This would be a good one to add. Per https://spec.openapis.org/registry/format/, My inclination is to model it as something like

struct ByteString(pub Vec<u8>);

Where the serde::Deserialize implementation would decode the base64 data. What do you think?

It does seem potentially odd to have application/octet-stream with { type: string, format: byte } -- my understanding is that { type: string, format: binary } would be more typical with an octet-stream.

I see this in the progenitor code:

            BodyContentType::OctetStream => {
                // For an octet stream, we expect a simple, specific schema:
                // "schema": {
                //     "type": "string",
                //     "format": "binary"
                // }

Are you currently seeing invalid schema for application/octet-stream?

dunxen commented 1 week ago

This would be a good one to add. Per https://spec.openapis.org/registry/format/, My inclination is to model it as something like

struct ByteString(pub Vec<u8>);

Where the serde::Deserialize implementation would decode the base64 data. What do you think?

It does seem potentially odd to have application/octet-stream with { type: string, format: byte } -- my understanding is that { type: string, format: binary } would be more typical with an octet-stream.

I see this in the progenitor code:

            BodyContentType::OctetStream => {
                // For an octet stream, we expect a simple, specific schema:
                // "schema": {
                //     "type": "string",
                //     "format": "binary"
                // }

Are you currently seeing invalid schema for application/octet-stream?

Yeah that sounds great!

I'd be happy to open a PR but I might only be able to get to it later this week.

Yip, I currently get invalid schema for application/octet-stream.

ahl commented 1 week ago

Can you confirm empirically that the API in question is indeed responding with an octet stream that contains base64 encoded data?

dunxen commented 4 days ago

Can you confirm empirically that the API in question is indeed responding with an octet stream that contains base64 encoded data?

Sorry for the late reply. Seems they went and changed all format: byte to format: binary. Also, they were only ever used in requests for posts and puts. All octet stream responses were always using format: binary.

So for this specific API it's not an issue anymore so it can be closed. This could change into a general tracking issue for format: byte, though.