Keep nullability when inlining ref to primitive

[ben-schreiber]

Fixes #21612

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 || exit
  ./bin/generate-samples.sh ./bin/configs/*.yaml || exit
  ./bin/utils/export_docs_generators.sh || exit
  

  (For Windows users, please run the script in WSL)
  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*.
  IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
• File the PR against the correct branch: master (upcoming 7.x.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
• If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

[Mattias-Sehlstedt]

Hi,

Could you add one of the terms mentioned in this documentation before your issue reference so that github can better visualize that a PR exists for your issue? E.g., writing fixes #21612.

It will add the icon highlighed in red in the image below.

I would suggested that we add a test case to ModelUtilsTest to show exactly what this change implies. So for example implementing the code shown in your issue as a test case that shows that the new behavior indeed changes so that the unaliased schema will now retain the nullable annotation.

Note: I am in no way affiliated with the openapi-generator project, so these point are not part of a review. I am merely providing my own personal suggestions as another contributor to the project.

[ben-schreiber]

@Mattias-Sehlstedt Good ideas - done.

[Mattias-Sehlstedt]

Hello again, I was a bit curious as to whether this was the cause of the error, since I had assumed that the normalizer would adjust this properly so that the parser could interpret a more "standard structure specification". I thus created a branch to test it, and to me it seems like this will not fix the missing nullability?

I have taken your unaliasing change and I have then extended the test specification null-types-simple.yaml with an extra property that mimics yours (a oneOf with a reference to an alias for a primitive, and then the type null as well). But when I generate the samples for this, then the property is not marked as nullable.

The normalizer changes the property to

oneOfPrimitiveOrNull:
  $ref: #/components/schemas/PrimitiveAlias,
  nullable: true,
  oneOf: null,
  ...

[ben-schreiber]

I also expected that, however, after some extensive 'print-debugging' , it seems that the un-aliasing occurs after normalization.
In any event, I updated the PR to fix the issue. As of now, I hardcoded the cloneSchema call to not use 3.1 since I didn't see an elegant way to dynamically calculate it. I'll mark this PR as draft until it's improved

[Mattias-Sehlstedt]

I found that this debugging worked well with investigating this scenario. I came to the same conclusion as you that the unaliasing for parameters causes the issue, since the current implementation expects that the nullability is derived from the "actual schema". But for this case that won't work due to the "actual schema" being an alias itself.

TL;DR warning, see Jump to here if TL;DR for skipping.

So after normalization the model will have two schema properties:

Schema oneOfOrNull:
  $ref: #/components/schemas/SomeObject,
  nullable: true,
  oneOf: null,
  ...
Schema oneOfPrimitiveAliasOrNull:
  $ref: #/components/schemas/PrimitiveAlias,
  nullable: true,
  oneOf: null,
  ...

The first oneOfOrNull will retain its nullable since it enters else if (isObjectSchema(ref)) in the unaliasing and the current oneOfOrNull schema is considered proper/unaliased (oneOfOrNull terminated with returning itself).

The latter oneOfPrimitiveAliasOrNull does not fall into any of the else-if cases and rather ends up in the else (which is also what you have modified). The else case means it is unaliased once again with the ref (being PrimitiveAlias), and for this run we instantly terminate with StringUtils.isNotEmpty(schema.get$ref()), since PrimitiveAlias doesn't have a ref, but is rather an alias for an integer. Thus the non-nullable PrimitiveAlias is returned as the replacement for oneOfPrimitiveAliasOrNull, since PrimitiveAlias is the schema that was produced in the terminal state of the unaliasing call (i.e., oneOfPrimitiveAliasOrNull got replaced with a schema that is now not nullable).

Jump to here if TL;DR

With the above in mind, do one want:

...
} else {
  Schema unaliased = unaliasSchema(openAPI, allSchemas.get(ModelUtils.getSimpleRef(schema.get$ref())), schemaMappings);
   // Preserve nullable property from the original schema reference
  Schema newSchema = cloneSchema(unaliased, false);
  newSchema.setNullable(schema.getNullable());
  return newSchema;
}

or

...
} else if (isPrimitiveSchema(ref)) {
   // Preserve nullable property from the original schema reference
  return cloneSchema(ref, ref.getSpecVersion().equals(SpecVersion.V31)).setNullable(schema.getNullable());
} else {
  return unaliasSchema(openAPI, allSchemas.get(ModelUtils.getSimpleRef(schema.get$ref())), schemaMappings);
}

?

I have tried the latter approach since I find that it better describes that the case "an alias to primitive schema" is a "terminal alias state" (I read it as all if-cases being different terminal states, and that the else represents "we need to go one step deeper"). I tried it out in a branch here, but I made it in an overloaded function for typescript rather than the lower level DefaultCodegen just when testing so I did not affect anything else.

I hardcoded the cloneSchema call to not use 3.1 since I didn't see an elegant way to dynamically calculate it. I'll mark this PR as draft until it's improved

I found that maybe the above solution with ref.getSpecVersion().equals(SpecVersion.V31) might work for that? I have also seen a different solution that looks at the openAPI's version and try to deduce "later than V31" to try to better guarantee operability with any later OAS version.