oxidecomputer / progenitor

An OpenAPI client generator
531 stars 67 forks source link

unclear requirement on query types, compilation of compiled code errors out #409

Open drahnr opened 1 year ago

drahnr commented 1 year ago

I am trying to generate some German tax related foo and an API service, but both with the cli program and from the builder pattern I get the following (after replacing the chrono::* types with custom impls, since they complain about lack of to_string impls too):

error[E0599]: the method `to_string` exists for reference `&ExportInvoiceSevQuery`, but its trait bounds were not satisfied
     --> src/codegen.rs:16773:43
      |
1159  |     pub struct ExportInvoiceSevQuery {
      |     --------------------------------
      |     |
      |     doesn't satisfy `ExportInvoiceSevQuery: ToString`
      |     doesn't satisfy `ExportInvoiceSevQuery: std::fmt::Display`
...
16773 |         query.push(("sevQuery", sev_query.to_string()));
      |                                           ^^^^^^^^^ method cannot be called on `&ExportInvoiceSevQuery` due to unsatisfied trait bounds
      |
      = note: the following trait bounds were not satisfied:
              `ExportInvoiceSevQuery: std::fmt::Display`
              which is required by `ExportInvoiceSevQuery: ToString`
              `&ExportInvoiceSevQuery: std::fmt::Display`
              which is required by `&ExportInvoiceSevQuery: ToString`
note: the following trait must be implemented
     --> /home/bernhard/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/fmt/mod.rs:751:1
      |
751   | pub trait Display {
      | ^^^^^^^^^^^^^^^^^

I am not entirely sure what the expectation is, am I supposed to provide the to_string impl i.e. with a custom trait in local scope or is this a generation bug?

Any help would be appreciated.

ahl commented 1 year ago

Are you able to share the OpenAPI document?

drahnr commented 1 year ago

I just invited you to the repo in question, I don't want to publish anything taxation related (which it is). All the hacks needed to make it work are included.

ahl commented 1 year ago

Progenitor doesn't properly handle query type parameters that have a non-scalar schema. After reading the spec some more, I'm still not quite clear on how a nested object should appear in the query parameters.

https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.0.3.md#parameter-object

I see the default for query parameters is style=form and explode=true. Does that mean that we'd expect to see ...?objectName=foo&filter.invoiceType=MA&filter.invoiceType=ER? It's very unclear to me how an array of objects could be represented.

Can you let me know how the server expects to see this schema represented in the query string?

drahnr commented 1 year ago

I frankly do not know, and I just raised a support request. The example shown with a object as query parameter, don't have meaningful examples https://api.sevdesk.de/#tag/Export/operation/exportInvoice (note the openapi.yaml included in the shared project is fixed, since it had numerous inaccuracies/flaws/bugs)

DerZade commented 2 weeks ago

I just ran into the same issue with the Typesense HTTP API. Their OpenAPI spec also includes a query parameter that is of type object:

openapi: 3.0.3
# [...]
paths:
  # [...]
  /collections/{collectionName}/documents:
    # [...]
    delete:
      # [...]
      parameters:
        # [...]
        - name: deleteDocumentsParameters
          in: query
          schema:
            type: object
            required:
              - filter_by
            properties:
              filter_by:
                type: string
                example: "num_employees:>100 && country: [USA, UK]"
              batch_size:
                type: integer
              ignore_not_found:
                type: boolean

see here for full API spec


After reading the spec some more, I'm still not quite clear on how a nested object should appear in the query parameters.

I can only talk from my experience with the Typesense API. There all props from the object are attached individually as separate query params. So for example:

{
  "filter_by": "num_employees:>100",
  "batch_size": 0,
  "ignore_not_found": true
}

would result in a URL query:

?filter_by=num_employees%3A%3E100&batch_size=0&ignore_not_found=true

This is how all SDKs of Typesense handle it. The Swagger Editor does the exact same thing and I would say that their implementation can be trusted fairly well.

DerZade commented 2 weeks ago

@ahl I am happy to attempt a PR that implements this behavior. Provided this is sufficient proof for you that what I have described is the right way to build the query parameters.