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

Query param of type: array with items.type: string leads to to_string() and unsatisfied trait on &&Vec<String> #692

Open
smndtrl opened this issue Jan 17, 2024 · 3 comments · May be fixed by #872

Comments

@smndtrl
Copy link

smndtrl commented Jan 17, 2024

While working with Scaleways API spec for Accounts, I encountered a array of strings in the URL. To me it is not clear how exactly it should work. The doc suggests it ends up as string in the URL &project_ids=[string], so I assume they want it joined with a , and not &project_ids=1&project_ids=2.

From Swaggers docs on OpenAPI serialization, the default style is style: form and explode: true aka &project_ids=1&project_ids=2. That would, I think, require a change here

OperationParameterKind::Query(required) => {
to account for Vec<_> and iterate + push for that case.

Any thoughts?

@MichielHegemans
Copy link

I ran into the same issue and I've been working on it a bit. The Typify https://github.com/oxidecomputer/typify?tab=readme-ov-file#arrays crate mentions that a type: array in the JSON schema can be parsed as 3 different types:

  • Vec
  • HashSet
  • (T, T, ...) (Depending on size)

Which are all doable, but need their own code paths.

Next issue is that it is determining the capacity of the Vec for the query to make based on the amount of params the function gets, however if we input an exploded version into the query vec this size will potentially be larger, or empty if the vec is empty. This is will not affect the functionality though, but it's a minor performance impact.

If we push a non-exploded version (1,2,3) the size is not an issue.

Progenitor also does not seem to grab the explode information from openapiv3::Parameter::Query, but it is available and can easily be added to the OperationParameter struct to be processed.

@antifuchs
Copy link

It looks to me like the implementation of the code fragment you linked is wrong for any kind of array type: Vec doesn't have a ToString implementation for any T.

But even there was a .to_string() impl for vectors, the way progenitor tries to do that (by pushing api_parameter_name=<value.to_string()> to the query string) makes it impossible to pass even the RFC6570/3.2.8 kind of api parameter in a vector (where it should encode api_parameter_name=foo&api_parameter_name=bar): You could try to make a newtype that has a ToString impl, which could create a query string fragment, interspersing the values with &api_parameter_name=... which then would get escaped by the reqwest query builder.

I think this might need a new trait for converting to/modifying a query string? Maybe something like this sketch (not tested, of course!):

trait AddToQuery {
  fn add_to_query(self, query: &mut Vec<(String, String)>, name: &str);
}

impl<T: ToString> AddToQuery for T {
  fn add_to_query(self, query: &mut Vec<(String, String)>, name: &str) {
    query.push(name, self.to_string());
  }
}

impl<T: ToString, I: IntoIterator<Item=T>> AddToQuery for T {
  fn add_to_query(self, query: &mut Vec<(String, String)>, name: &str) {
    for item in self.into_iter() {
      query.push(name, item.to_string());
    }
  }
}

...or something.

@iganev
Copy link

iganev commented May 2, 2024

Any fix or workaround for this yet?

pvandommelen added a commit to pvandommelen/progenitor that referenced this issue Jul 28, 2024
This assumes style=form and explode=true, which is the default.
Partially resolves oxidecomputer#692
@pvandommelen pvandommelen linked a pull request Jul 28, 2024 that will close this issue
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

Successfully merging a pull request may close this issue.

4 participants