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 collectionFormat not working despite existing support #139

Closed
agologan opened this issue Oct 19, 2020 · 1 comment
Closed

Query collectionFormat not working despite existing support #139

agologan opened this issue Oct 19, 2020 · 1 comment

Comments

@agologan
Copy link
Contributor

While there is support added via annotations and CollectionFormatConverterFactory.kt this does not work and will instead pass the parameters as multiple values.
E.g. snippet definition snippet:

      # expected: color=red,black,white
      # actual: color=red&color=black&color=white
      parameters:
        - in: query
          name: color
          type: array
          collectionFormat: csv
          items:
            type: string

This happens because Retrofit applies the converter on a per value basis so the type check in CollectionFormatConverter will always go through the string branch returning an unmodified value which than gets appended to the query as an extra parameter.

Looking at the retrofit code (RequestFactory.java#L442) if we get an Iterable, than each value in that list will get converted and added to the query. So while our CSV stringConverter is selected, it will never get applied as we want it to be.

While I have a few ideas on how to approach this, they all involve making ParameterHandler.java in Retrofit, aware of apply'ing the conversion as a single value if a converter exists while keeping the current multi-value behavior for the BuiltInConverter string converter.

This does imply a significant change in Retrofit but maybe I'm missing something and there's already a better way to do this.
Any ideas? @JakeWharton maybe you could share your perspective.

@cortinico
Copy link
Collaborator

This does imply a significant change in Retrofit but maybe I'm missing something and there's already a better way to do this.

You're right. That's a limitation of Retrofit at the moment.

I invite you to subscribe to this issue for further updates: square/retrofit#626 (comment)

Closing here as it's unrelated to this project.

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