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

Generate failed for Xero Accounting OpenAPI spec #955

Open
dunxen opened this issue Oct 21, 2024 · 7 comments
Open

Generate failed for Xero Accounting OpenAPI spec #955

dunxen opened this issue Oct 21, 2024 · 7 comments

Comments

@dunxen
Copy link

dunxen commented Oct 21, 2024

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
Copy link
Author

dunxen commented Oct 22, 2024

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

@ahl
Copy link
Collaborator

ahl commented Oct 22, 2024

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
Copy link
Author

dunxen commented Oct 23, 2024

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
Copy link
Collaborator

ahl commented Nov 11, 2024

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
Copy link
Author

dunxen commented Nov 11, 2024

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
Copy link
Collaborator

ahl commented Nov 12, 2024

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

@dunxen
Copy link
Author

dunxen commented Nov 18, 2024

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.

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

No branches or pull requests

2 participants