Skip to content

codegen, oas-3-compat: Treatment of parameter style "deepObject" is shady #469

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

Open
lorenzleutgeb opened this issue Jul 5, 2018 · 5 comments

Comments

@lorenzleutgeb
Copy link

lorenzleutgeb commented Jul 5, 2018

Description

It seems to me that your codegen layer does not treat the deepObject style for parameters. For example it is missing in the method DefaultCodegen#getCollectionFormat where it should probably be treated in one way or another.

The result for the YAML below is that I get a client implementation that does not seem to handle the deepObject case in ApiClient:

    public List<Pair> parameterToPair(String name, Object value) {
        List<Pair> params = new ArrayList<Pair>();

        // preconditions
        if (name == null || name.isEmpty() || value == null || value instanceof Collection) return params;

        params.add(new Pair(name, parameterToString(value)));
        return params;
    }

    public String parameterToString(Object param) {
        if (param == null) {
            return "";
        } else if (param instanceof Date || param instanceof OffsetDateTime || param instanceof LocalDate) {
            //Serialize to json string and remove the " enclosing characters
            String jsonStr = json.serialize(param);
            return jsonStr.substring(1, jsonStr.length() - 1);
        } else if (param instanceof Collection) {
            StringBuilder b = new StringBuilder();
            for (Object o : (Collection)param) {
                if (b.length() > 0) {
                    b.append(",");
                }
                b.append(String.valueOf(o));
            }
            return b.toString();
        } else {
            return String.valueOf(param);
        }
    }

The resulting query parameter looks like:

filter=class%20Filter%20%7B%0A%20%20%20%20id%3A%20null%0A%20%20%20%20updatedAtGt%3A%20null%0A%7D

where I would expect no parameters to be present (all values inside the filter object are null). Generally I expect it to look like:

filter[id]=x&filter[updated_at|gt]=y
openapi-generator version

020883f (master from 2018-07-04)

OpenAPI declaration file content or url
paths:
  '/foo':
    get:
      parameters:
        - name: filter
          in: query
          required: false
          style: deepObject
          schema:
            properties:
              "updated_at|gt":
                type: string
                format: date-time
Command line used for generation

java -jar openapi-generator-cli.jar generate -l java -i api.yml -o jclient

Steps to reproduce

Generate client.

Suggest a fix/enhancement

You should treat deepObject style parameters as specified in OAS 3.0.1, there even is an example: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.1.md#style-examples

@jmini
Copy link
Member

jmini commented Jul 5, 2018

Right now the Codegen classes and templates are driven a lot by the OAS2 concepts (in, collectionFormat)

In my opinion:

  • OAS3 concepts (style, explode) should be exposed to Codegen classes (in parallel to OAS2 concepts for backward compatibility)
  • Then we can try to work on support for stuff like style: deepObject in the templates (in java client templates for your use case in particular).

@lorenzleutgeb
Copy link
Author

Yeah, that makes sense.

By the way, is there any issue that aggregates incompatibilities with OAS-3? If not I would argue that this is really needed. In one of the first sentences of README.md I read "OpenAPI Spec (both 2.0 and 3.0 are supported)" which sounds like OAS is supported, e.g. anything that is mention in the spec will be reflected in the generated clients. But issues like this contradict that fact directly. So in my opinion some more transparency would be good. A meta-issue that aggregates all smaller incompatibilities would also help to have an overview of what needs to be fixed in a single place. The "Feature: OAS 3.0 spec support" is not used as often as I would expect.

@jmini
Copy link
Member

jmini commented Jul 5, 2018

Feel free to open an aggregation issue for all "Feature: OAS 3.0 spec support" issues. I will try to tag all what you find and I welcome any effort that helps with issue-triage.

I think that we never tried to hide that some corner cases of OpenAPI v2 or v3 are not supported. For v3, it might be more than for v2 because the support of v3 is new. We have even one line that mention it in the roadmap.

This project is issue-driven and I appreciate all the issues that you are filling. It helps to make the project better. The next step is to propose pull-requests to fix those.

@lorenzleutgeb
Copy link
Author

Actually if the label is well-maintained, that should be fine. I just did not believe that actually these issues are all known counterexamples to OAS 3 conformance. Just in the last two days I found a number of bugs. But that's not to talk badly, just to help point the finger at where more work would make sense.

@Reinhard-PTV
Copy link
Contributor

We made an implementation to support deepObject for .NET Core and Java native client in Reinhard-PTV@4fbd210 .

Can we submit these changes as a pull request?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants