Issue #10593 Fix nullable property implemented with oneOf construct #10602

etremblay · 2021-10-14T12:20:56Z

See issue #10593

PR checklist

Read the contribution guidelines.
Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
Run the following to build the project and update samples:
```
./mvnw clean package 
./bin/generate-samples.sh
./bin/utils/export_docs_generators.sh
```
Commit all changed files.
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
For Windows users, please run the script in Git BASH.
File the PR against the correct branch: master (5.3.0), 6.0.x
If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

Typescript
@TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01) @topce (2018/10) @akehir (2019/07) @petejohansonxo (2019/11) @amakhrov (2020/02)

Kotlin
@jimschubert (2017/09) heart, @dr4ke616 (2018/08) @karismann (2019/03) @Zomzog (2019/04) @andrewemery (2019/10) @4brunu (2019/11) @yutaka0m (2020/03)

PHP
@jebentier (2017/07), @dkarlovi (2017/07), @mandrean (2017/08), @jfastnacht (2017/09), @ackintosh (2017/09) heart, @ybelenko (2018/07), @renepardon (2018/12)

Java
@bbdouglas (2017/07) @sreeshas (2017/08) @jfiala (2017/08) @lukoyanov (2017/09) @cbornet (2017/09) @jeff9finger (2018/01) @karismann (2019/03) @Zomzog (2019/04) @lwlee2608 (2019/10) @nmuesch (2021/01)

etremblay · 2021-10-14T13:32:01Z

samples/client/petstore/typescript-axios/builds/test-petstore/api.ts

@@ -846,7 +846,7 @@ export type Fruit = Apple | Banana;
 * @type FruitReq
 * @export
 */
-export type FruitReq = AppleReq | BananaReq | Null;
+export type FruitReq = AppleReq | BananaReq | null;


Look like we fixed some bug here too

etremblay · 2021-10-14T13:35:19Z

...ct-propery/src/main/kotlin/org/openapitools/client/models/ModelWithNullableObjectProperty.kt

+ * @param nonNullableProperty 
+ */
+
+data class ModelWithNullableObjectProperty (


This is what we tried to achieve : val propertyName31: PropertyType? = null,

etremblay · 2021-10-14T13:36:04Z

...e/typescript-fetch/builds/nullable-object-property/models/ModelWithNullableObjectProperty.ts

+ * @export
+ * @interface ModelWithNullableObjectProperty
+ */
+export interface ModelWithNullableObjectProperty {


here the fix for typescript

etremblay · 2021-10-14T13:48:52Z

...ct-property/src/main/java/org/openapitools/client/model/ModelWithNullableObjectProperty.java

+})
+@javax.annotation.processing.Generated(value = "org.openapitools.codegen.languages.JavaClientCodegen")
+public class ModelWithNullableObjectProperty {
+  public static final String JSON_PROPERTY_PROPERTY_NAME30 = "propertyName30";


Here is the result for java

amakhrov

Thanks for your contribution!
I haven't even realized nullable was so broken :)

amakhrov · 2021-10-14T17:58:10Z

modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java

@@ -2092,6 +2092,10 @@ public String toAnyOfName(List<String> names, ComposedSchema composedSchema) {
     * Otherwise, a name is constructed by creating a comma-separated list of all the names
     * of the oneOf schemas.
     *
+     * If the oneOf schema contains only one element and have the openapi 3.0 nullable flag,
+     * or one element and the openapi 3.1 'null' type, only this element is returned.


how about the case with more than one type in oneOf?

oneOf: - 'null' - ref: ModelA - ref: ModelB

How do we handle this?

I didn't change that one, it will behave like before.

Since many language plugin does not seen to really support oneOf, it will still be broken.

I think typescript will be ok and produce ModelA | Model B | null

Do you think it's worth adding to the test spec and see what the generated typescript code looks like?
The reason I'm specifically curious about it is that you seem to filter out the "null" part of the union. And I'm not sure whether it still preserves in some flags and resurfaces later

I added a case in the yaml. We can see the result in this commit :

9b9f58659cefc284e9a4c0ce3c361c8c404d2292

amakhrov · 2021-10-14T18:04:10Z

...e/typescript-fetch/builds/nullable-object-property/models/ModelWithNullableObjectProperty.ts

+export interface ModelWithNullableObjectProperty {
+    /**
+     * 
+     * @type {PropertyType}


@type annotation is no longer correct (should be PropertyType | null).

honestly, I don't know why it's needed anyway, in the presence of the real type definitions.
but if it's there and not in sync with the actual type, I'm concerned it might confuse some tools/IDEs.
Any thoughts?

Humm, I did not change the templates, but if you want my personal advice, I would remove the @type. Not in this pr of course.

amakhrov · 2021-10-14T18:10:25Z

...e/typescript-fetch/builds/nullable-object-property/models/ModelWithNullableObjectProperty.ts

+
+        'propertyName30': PropertyTypeToJSON(value.propertyName30),
+        'propertyName31': PropertyTypeToJSON(value.propertyName31),
+        'nonNullableProperty': string | numberToJSON(value.nonNullableProperty),


broken typescript code

it doesn't mean it's a regression - maybe we never had a similar case in the samples, and it's been always effectively broken. but need to verify

Indeed, this is weird. I added this property int the last minute to check non-regression on my unit tests and did not check the generated code.

The template takes the dataType as is whish is string | number

I think is is indeed not a regression.

'{{baseName}}': {{datatype}}ToJSON(value.{{name}}),

yep, you're right! I just recalled it getting surfaced in another PR earlier: #10017 (comment)

This won't be trivial to fix. I think it should be documented in another issue.

Absolutely. I just wanted to double check it's not something we're breaking in this PR - that's it :)

amakhrov · 2021-10-14T18:12:21Z

...r/src/test/java/org/openapitools/codegen/typescript/AbstractTypeScriptClientCodegenTest.java

+        // Non regression on regular oneOf construct
+        propertyName = "nonNullableProperty";
+        property = codegen.fromProperty(propertyName, (Schema) schema.getProperties().get(propertyName));
+        assertEquals(property.openApiType, "string | number");


so for regular non-nullable oneOf we produce e.g string | number.
why do we need different handling for null? I feel if it could be string | null, we would have more consistent templates / generated code. for typescript, at least

| null is somehow added later in the process.

It's not very easy to navigate this code, not all language plugin are made in a consistant way and some things that could have been handled in the DefaultCodegen looks hacked in some languages.

The post-precessing process is not all clear to me. Also it's not clear how to apply it in unit tests.

amakhrov

Looks good from my standpoint (typescript).

I didn't look closely into java/kotlin generators output though.

TiFu

Agree with @amakhrov - TypeScript looks good from my perspective as well.

etremblay · 2021-11-10T22:16:37Z

...ient/petstore/php/nullable-object-property-php/lib/Model/ModelWithNullableObjectProperty.php

+      * @var string[]
+      */
+    protected static $openAPITypes = [
+        'property_name30' => 'PropertyType',


We just noticed that we still have a problem for PHP.

This name must be typed with namespaces for ObjectSerializer to be able to handle it

etremblay · 2021-11-10T22:20:06Z

Just updated this branch with master. I will put it on hold until I fix PHP serialization

etremblay · 2021-11-11T00:56:15Z

PHP namespaces are now fixed. This pr is ready for re-review.

etremblay

forgot to submit these comments days ago

etremblay · 2021-11-10T22:18:40Z

...ient/petstore/php/nullable-object-property-php/lib/Model/ModelWithNullableObjectProperty.php

+        return $this;
+    }
+
+    /**


Regular non-nullable oneOf properties are not handled by this PR. It's normal to still get strange types for the two following properties

etremblay · 2021-11-10T22:19:03Z

...ient/petstore/php/nullable-object-property-php/lib/Model/ModelWithNullableObjectProperty.php

+ * @template TKey int|null
+ * @template TValue mixed|null
+ */
+class ModelWithNullableObjectProperty implements ModelInterface, ArrayAccess, \JsonSerializable


Begin of the interesting part for PHP

etremblay · 2022-06-15T13:53:22Z

@wing328

I just updated this pr on master (6.0.0) and some of my fixes does not work anymore. I could trace it to a big refacor on inline schema resolver done in #12104

I also noticed some weird stuff with 6.0.0 on some of our generated sdks.

This is not relate to this pr, but you can notice the diff on ma last commit f4043c9

there is a new model generated from one of the inline field : ModelWithNullableObjectPropertyNonNullableProperty

components:
  schemas:
    ModelWithNullableObjectProperty:
      properties:
        propertyName:
          $ref: '#/components/schemas/PropertyType'
    PropertyType:
      properties:
        foo:
          type: string

I can file a new issue for that, but for this pr I would like your advice on the best way I could handle nullable oneOf with your new resolver.

Regards,

etremblay · 2022-06-27T20:04:35Z

@wing328 I managed to make my nullable fixes work again 7dee4c1

There are still some generated types from oneOf or anyOf but I think it was intended.

For exemple a oneOf composed of only basic types would resolve in typescript to a simple union type but now a type is created.

export type ModelWithNullableObjectPropertyNonNullableProperty = number | string;

etremblay · 2023-12-12T16:03:18Z

Most of the features introduced by this pull request are now available in the 7.1 branch.

The only case that did not work for us was the

    Schema:
      required:
        - property
      properties:
        propety:
          oneOf:
            -
              $ref: '#/components/schemas/NullableProperty'
          nullable: true

I was able to fix it in the OpenAPI Normalizer

kronostechnologies@7ca8611

etremblay mentioned this pull request Oct 14, 2021

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

Open

etremblay changed the title ~~Issue #10593 Fix nullable type handling~~ Issue #10593 Fix nullable implemented with oneOf type handling Oct 14, 2021

etremblay force-pushed the fix-nullable-field branch from dccc2c1 to cb848b3 Compare October 14, 2021 13:21

etremblay marked this pull request as ready for review October 14, 2021 13:24

etremblay force-pushed the fix-nullable-field branch from cb848b3 to fd37884 Compare October 14, 2021 13:30

etremblay commented Oct 14, 2021

View reviewed changes

etremblay changed the title ~~Issue #10593 Fix nullable implemented with oneOf type handling~~ Issue #10593 Fix nullable property implemented with oneOf construct Oct 14, 2021

etremblay commented Oct 14, 2021

View reviewed changes

amakhrov reviewed Oct 14, 2021

View reviewed changes

amakhrov approved these changes Oct 14, 2021

View reviewed changes

TiFu reviewed Oct 22, 2021

View reviewed changes

etremblay force-pushed the fix-nullable-field branch from 9b9f586 to db24b68 Compare November 10, 2021 21:56

etremblay commented Nov 10, 2021

View reviewed changes

etremblay marked this pull request as draft November 10, 2021 22:20

etremblay marked this pull request as ready for review November 11, 2021 00:55

etremblay mentioned this pull request Nov 11, 2021

Fix php generator nullable types kronostechnologies/standards#85

Merged

etremblay force-pushed the fix-nullable-field branch from a90fa57 to b795909 Compare November 30, 2021 14:29

etremblay commented Nov 30, 2021

View reviewed changes

etremblay mentioned this pull request Nov 30, 2021

fix php codegen kronostechnologies/standards#93

Merged

Eric Durand-Tremblay added 7 commits June 14, 2022 14:11

Issue OpenAPITools#10593 Fix nullable type handling

e668aea

Add java sample

0a43e25

Add a oneOf test case with null and two other types

9d9c2ea

Regenerate samples after rebase on master

3978ee0

PHP sample

0825ed6

Fixed php namespaces in nullable properties

d7c4fdc

fix nullable without oneOff null check

c3f4006

regenerate

687dbf9

etremblay force-pushed the fix-nullable-field branch from 1ba7bc9 to 81ff959 Compare June 15, 2022 13:39

regenerate on top of master 6.0.0

f4043c9

etremblay force-pushed the fix-nullable-field branch from 81ff959 to f4043c9 Compare June 15, 2022 13:59

Merge branch 'master' into fix-nullable-field

b535f32

etremblay force-pushed the fix-nullable-field branch from 6f22b61 to 1e54f0f Compare June 27, 2022 19:44

fixed nullable schemas after 6.x

7dee4c1

etremblay force-pushed the fix-nullable-field branch from 1e54f0f to 7dee4c1 Compare June 27, 2022 19:50

etremblay added 4 commits June 27, 2022 16:21

regenerate documentation

242c846

bring back deleted sample files from master

b2b434c

Merge branch 'master' into fix-nullable-field

0a2512e

Update samples and docs

3c7586a

etremblay closed this Dec 12, 2023

etremblay deleted the fix-nullable-field branch December 12, 2023 16:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #10593 Fix nullable property implemented with oneOf construct #10602

Issue #10593 Fix nullable property implemented with oneOf construct #10602

etremblay commented Oct 14, 2021 •

edited

Loading

etremblay Oct 14, 2021

etremblay Oct 14, 2021

etremblay Oct 14, 2021

etremblay Oct 14, 2021

amakhrov left a comment

amakhrov Oct 14, 2021

etremblay Oct 14, 2021

amakhrov Oct 14, 2021 •

edited

Loading

etremblay Oct 14, 2021

amakhrov Oct 14, 2021

etremblay Oct 14, 2021

amakhrov Oct 14, 2021

etremblay Oct 14, 2021

etremblay Oct 14, 2021

amakhrov Oct 14, 2021

etremblay Oct 14, 2021

amakhrov Oct 14, 2021

amakhrov Oct 14, 2021

etremblay Oct 14, 2021 •

edited

Loading

amakhrov left a comment

TiFu left a comment

etremblay Nov 10, 2021

etremblay commented Nov 10, 2021

etremblay commented Nov 11, 2021

etremblay left a comment

etremblay Nov 10, 2021

etremblay Nov 10, 2021

etremblay commented Jun 15, 2022 •

edited

Loading

etremblay commented Jun 27, 2022

etremblay commented Dec 12, 2023

Issue #10593 Fix nullable property implemented with oneOf construct #10602

Issue #10593 Fix nullable property implemented with oneOf construct #10602

Conversation

etremblay commented Oct 14, 2021 • edited Loading

PR checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amakhrov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amakhrov Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etremblay Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

amakhrov left a comment

Choose a reason for hiding this comment

TiFu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etremblay commented Nov 10, 2021

etremblay commented Nov 11, 2021

etremblay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etremblay commented Jun 15, 2022 • edited Loading

etremblay commented Jun 27, 2022

etremblay commented Dec 12, 2023

etremblay commented Oct 14, 2021 •

edited

Loading

amakhrov Oct 14, 2021 •

edited

Loading

etremblay Oct 14, 2021 •

edited

Loading

etremblay commented Jun 15, 2022 •

edited

Loading