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

Enums in query and MULTI #75

Open
carlosefonseca opened this issue Dec 13, 2019 · 3 comments
Open

Enums in query and MULTI #75

carlosefonseca opened this issue Dec 13, 2019 · 3 comments
Assignees

Comments

@carlosefonseca
Copy link

Hi,

The code generated ended up with an import import …yelpcodegen.tools.MULTI that does not exist.

I checked the swagger and the only places I found "multi" was on a query parameter that is an enum and contains "collectionFormat": "multi", (I didn't check what this meant). Looking at the generated code, the request contains a List in the place of the enum.

So, two questions. Is the import a bug? And could the request be generated with the enum class instead of a List?

Here's the generated code vs the swagger.

Screen Shot 2019-12-13 at 20 27 53

@cortinico
Copy link
Collaborator

Hi @carlosefonseca, thanks for reporting this issue.

So, two questions. Is the import a bug?

To answer your first question: yes, it's a bug.
On the other hand, this comes as a side effect that we don't support collectionFormat: multi at the moment (you can find here some documentation on this property).

To support this we would need to extend the CollectionFormatConverterFactory to support encoding multiple parameters (like foo=value&foo=another_value). This can be a bit tricky given that we don't have access to the parameter name (foo) but only to the List<String> of the value you're passing (value, another_value). This makes encoding a string with multi more complicated. I don't think it's currently worth to spend effort implementing this at the moment.

And could the request be generated with the enum class instead of a List?

An enum class: no, as this won't comply with the specs.
We could extend the generator to generate a parameter that accepts a list of enum and that looks like this:

fun requestGet(
    @retrofit2.http.Query("State") @MULTI state: List<StateEnum>?
): Single<List<RequestDTO>>

This unfortunately comes with the complexity of having to define an inline anonymous enum (StateEnum). The generator currently doesn't support inline enums inside the Retrofit interface.

So the workaround is to probably deal with List<String> and accept to lose the type safety of having an enum, at least for now.

I'd love to keep this issue opened in case someone wants to pick it up.

@macisamuele macisamuele self-assigned this Feb 6, 2020
@tajchert
Copy link

If you have this issue I would recommend for now as a workaround use explicit type of parameters separation instead of multi ex. csv,ssv, tsv, pipes (https://swagger.io/docs/specification/2-0/describing-parameters/) in your swagger definition. This will get rid of MULTI issue in generated class.

@cortinico
Copy link
Collaborator

If you have this issue I would recommend for now as a workaround use explicit type of parameters separation

Thanks for the input @tajchert
Sadly the collectionFormat to specify csv, ssv, tsv, pipes as you mentioned is affected by this other bug related to Retrofit #139

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

4 participants