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

[BUG] Nullable object reference produce invalid code in kotlin, php, java, typescript-fetch and probably more #10593

Open
etremblay opened this issue Oct 13, 2021 · 7 comments

Comments

@etremblay
Copy link
Contributor

etremblay commented Oct 13, 2021

Description

To represent nullable object property, openapi recommend using a oneOf constuct. In version 3.0 in can be done with the nullable property, and in 3.1, with the null type.

3.0

propertyName:
          nullable: true
          oneOf:
            - $ref: '#/components/schemas/PropertyType'

3.1

        otherPropertyName:
          oneOf:
            - 'null'
            - $ref: '#/components/schemas/PropertyType'

In kotlin, the generatated type name is incorrect : OneOfLessThanPropertyTypeGreaterThan

@Json(name = "propertyName")
val propertyName: OneOfLessThanPropertyTypeGreaterThan? = null,

In php, the code is ok but the annotations produce an invalid class name : OneOfPropertyType|null

/**
     * Sets property_name
     *
     * @param OneOfPropertyType|null $property_name property_name
     *
     * @return self
     */
    public function setPropertyName($property_name)
    {
        $this->container['property_name'] = $property_name;

        return $this;
    }

In java the type is also wrong

public OneOfPropertyType getPropertyName() {
  return propertyName;
}

public void setPropertyName(OneOfPropertyType propertyName) {
  this.propertyName = propertyName;
}

In typescipt-fetch it produces a different result with 3.0 or 3.1 syntax

// 3.0

    /**
     * 
     * @type {PropertyType}
     * @memberof ModelWithNullableObjectProperty
     */
    propertyName?: PropertyType | null;

// 3.1
    /**
     * 
     * @type {Null | PropertyType}
     * @memberof ModelWithNullableObjectProperty
     */
    otherPropertyName?: Null | PropertyType | null;
openapi-generator version

master - 5.2.1

OpenAPI declaration file content or url
openapi: 3.0.0
info:
  title: 'Title'
  version: latest
paths:
  '/':
    get:
      operationId: operation
      responses:
        '200':
          description: Success
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/ModelWithNullableObjectProperty'
components:
  schemas:
    ModelWithNullableObjectProperty:
      properties:
        propertyName:
          nullable: true
          oneOf:
            - $ref: '#/components/schemas/PropertyType'
        otherPropertyName:
          oneOf:
            - 'null'
            - $ref: '#/components/schemas/PropertyType'
    PropertyType:
      properties:
        foo:
          type: string
Generation Details

docker run --rm -v ${PWD}:/local --user "${USER_ID}:${GROUP_ID}" openapitools/openapi-generator-cli:latest generate -i /local/model.yaml -g kotlin -o /local/kotlin
docker run --rm -v ${PWD}:/local --user "${USER_ID}:${GROUP_ID}" openapitools/openapi-generator-cli:latest generate -i /local/model.yaml -g php -o /local/php
docker run --rm -v ${PWD}:/local --user "${USER_ID}:${GROUP_ID}" openapitools/openapi-generator-cli:latest generate -i /local/model.yaml -g java -o /local/java
docker run --rm -v ${PWD}:/local --user "${USER_ID}:${GROUP_ID}" openapitools/openapi-generator-cli:latest generate -i /local/model.yaml -g typescript-fetch -o /local/typescript

Related issues/PRs

#2090

Suggest a fix

I went deep into the source code to understand how nullable types are handled.

ModelUtils.isNullable seems to have some logic to handle this but I'm unable to make an unit test pass in this code path.

I'm willing to contribute but may need some guidance.

@etremblay etremblay changed the title [BUG] Nullable object referenced produce invalid code in kotlin, php, java and probably more [BUG] Nullable object reference produce invalid code in kotlin, php, java and probably more Oct 13, 2021
@etremblay
Copy link
Contributor Author

I think we can achieve something overriding

public String toOneOfName(List<String> names, ComposedSchema composedSchema) {
        Map<String, Object> exts = composedSchema.getExtensions();
        if (exts != null && exts.containsKey("x-one-of-name")) {
            return (String) exts.get("x-one-of-name");
        }
        return "oneOf<" + String.join(",", names) + ">";
    }

for some languages

@etremblay
Copy link
Contributor Author

Apprently typescript does not like the 3.1 syntax either

/**
     * 
     * @type {Null | PropertyType}
     * @memberof ModelWithNullableObjectProperty
     */
    otherPropertyName?: Null | PropertyType | null;

@etremblay etremblay changed the title [BUG] Nullable object reference produce invalid code in kotlin, php, java and probably more [BUG] Nullable object reference produce invalid code in kotlin, php, java, typescript-fetch and probably more Oct 14, 2021
etremblay pushed a commit to kronostechnologies/openapi-generator that referenced this issue Oct 14, 2021
@etremblay
Copy link
Contributor Author

etremblay commented Oct 14, 2021

It's still in draft and the unit tests aren't completed yet, but I hava a working solutions for this issue.

#10602

I will try to complete a proper PR tomorrow

etremblay pushed a commit to kronostechnologies/openapi-generator that referenced this issue Jun 15, 2022
@charlie-harvey
Copy link

charlie-harvey commented Aug 30, 2022

Just hit this issue. Is it still an issue? Did all the commits above get released?

Thanks.

@etremblay
Copy link
Contributor Author

Just hit this issue. Is it still an issue? Did all the commits above get released?

Thanks.

I try to keep the pull request up to date but noboty ever merge it even if it's approved. I see that it's now conflicted, I'll fix it soon.

@wing328
Copy link
Member

wing328 commented Aug 31, 2022

@etremblay our apologies. We're busy with too many PRs. I'll try to spend some time this week to next to go through it and let you know I've any feedback or question.

@etremblay
Copy link
Contributor Author

@etremblay our apologies. We're busy with too many PRs. I'll try to spend some time this week to next to go through it and let you know I've any feedback or question.

Don't worry, I know you have a lot on your plate.

I updated the pull request with master and re-generated the samplesé

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