Handle nullable types in OpenAPI schemas and add corresponding unit t…#7170
Handle nullable types in OpenAPI schemas and add corresponding unit t…#7170X39 wants to merge 5 commits intomicrosoft:mainfrom
Conversation
|
@microsoft-github-policy-service agree |
…nit tests. This commit provides a basic fix for microsoft#6776, unblocking nullable reference types for ASP.Net Core.
|
@gavinbarron Could you have a look at this? |
|
Please fix this issue |
|
Just to note for those in dire need of a fix: building kiota on your own is trivial, after that, just refer to the built version, rather than the installed one by path (aka the .../kiota/bin/release/kiota.exe one) That is what I am doing right now |
|
@baywet What do we need to get this merged? |
|
I have transitioned to a new team back in August 2025 and won't be replying to this issue/pull request moving forward. @gavinbarron and @adrian05-ms to take over |
gavinbarron
left a comment
There was a problem hiding this comment.
Thank you so much for this contribution.
There are a few areas that are untested and need coverage so that we can get this shippable.
| var serializationMethodName = GetSerializationMethodName(otherProp.Type, method); | ||
| writer.WriteLine($"{(includeElse ? "else " : string.Empty)}if({otherProp.Name.ToFirstCharacterUpperCase()} != null)"); | ||
| writer.WriteBlock(lines: $"writer.{GetSerializationMethodName(otherProp.Type, method)}(null, {otherProp.Name.ToFirstCharacterUpperCase()});"); | ||
| if (otherProp.Type is CodeType{TypeDefinition: CodeEnum {BackingType: JsonSchemaType.Integer}}) |
There was a problem hiding this comment.
I'm not seeing any tests for serialization of union models that include Integer Enums, let's make sure test cases for this are added.
It feels a bit weird that the integer case is passing otherProp.WireName which is inconsistent with how the existing case works.
There was a problem hiding this comment.
I am very aware of that, wanted to avoid cross-repository changes tho and have this easily integrateable on my end.
Could you hint me at where those tests are currently placed and how they look? Thanks in advance.
There was a problem hiding this comment.
Here's where we have an existing test for the union model serialization
It focuses on the code that is generated, rather than the behavior of the serialization library that we have as a dependency.
| var serializationMethodName = GetSerializationMethodName(otherProp.Type, method); | ||
| writer.WriteLine($"{(includeElse ? "else " : string.Empty)}if({otherProp.Name.ToFirstCharacterUpperCase()} != null)"); | ||
| writer.WriteBlock(lines: $"writer.{GetSerializationMethodName(otherProp.Type, method)}(null, {otherProp.Name.ToFirstCharacterUpperCase()});"); | ||
| if (otherProp.Type is CodeType{TypeDefinition: CodeEnum {BackingType: JsonSchemaType.Integer}}) |
There was a problem hiding this comment.
I'm not seeing any tests for serialization of intersection models that include Integer Enums, let's make sure test cases for this are added.
There was a problem hiding this comment.
I am not entirely sure what you mean by that, could you provide a sample json?
Also, where are those tests currently located at?
Thanks in advance
There was a problem hiding this comment.
Here's the existing test for generating the serialization method
I have supplied sample open api for these types of schema on the factory method/deserialization comments
| var valueVarName = $"{property.Name.ToFirstCharacterLowerCase()}Value"; | ||
| writer.WriteLine($"{(includeElse ? "else " : string.Empty)}if({parseNodeParameter.Name.ToFirstCharacterLowerCase()}.{GetDeserializationMethodName(propertyType, codeElement)} is {typeName} {valueVarName})"); | ||
| writer.WriteBlock(lines: $"{ResultVarName}.{property.Name.ToFirstCharacterUpperCase()} = {valueVarName};"); | ||
| if (propertyType.TypeDefinition is CodeType { TypeDefinition: CodeEnum {BackingType: JsonSchemaType.Integer}}) |
There was a problem hiding this comment.
This need test coverage to ensure the deserialization code is generated correctly for the case of a union model with an integer enum.
There was a problem hiding this comment.
I am not entirely sure what you mean by that, could you provide a sample json?
Also, where are those tests currently located at?
Thanks in advance
There was a problem hiding this comment.
Here's where the tests for this method start:
A union model with and integer enum might looks something like:
openapi: 3.0.3
info:
title: Union with Integer Enum API
version: 1.0.0
servers:
- url: https://api.example.com/v1
paths:
/status:
get:
summary: Get status that can be multiple types
responses:
'200':
description: A union type containing an integer enum
content:
application/json:
schema:
$ref: '#/components/schemas/StatusUnion'
components:
schemas:
# Integer-backed enum
StatusEnum:
type: integer
enum: [0, 1, 2]
x-ms-enum:
name: StatusEnum
modelAsString: false
values:
- name: Active
value: 0
description: "The resource is active"
- name: Inactive
value: 1
description: "The resource is inactive"
- name: Pending
value: 2
description: "The resource is pending"
# A complex object type
DetailedStatus:
type: object
required:
- message
properties:
message:
type: string
code:
type: integer
# Union model combining integer enum and object
StatusUnion:
oneOf:
- $ref: '#/components/schemas/StatusEnum'
- $ref: '#/components/schemas/DetailedStatus'
discriminator:
propertyName: '@odata.type'
mapping:
'#kiota.statusEnum': '#/components/schemas/StatusEnum'
'#kiota.detailedStatus': '#/components/schemas/DetailedStatus'
| var valueVarName = $"{property.Name.ToFirstCharacterLowerCase()}Value"; | ||
| writer.WriteLine($"{(includeElse ? "else " : string.Empty)}if({parseNodeParameter.Name.ToFirstCharacterLowerCase()}.{GetDeserializationMethodName(propertyType, codeElement)} is {typeName} {valueVarName})"); | ||
| writer.WriteBlock(lines: $"{ResultVarName}.{property.Name.ToFirstCharacterUpperCase()} = {valueVarName};"); | ||
| if (propertyType.TypeDefinition is CodeType { TypeDefinition: CodeEnum {BackingType: JsonSchemaType.Integer}}) |
There was a problem hiding this comment.
This need test coverage to ensure the deserialization code is generated correctly for the case of a intersection model with an integer enum.
There was a problem hiding this comment.
I am not entirely sure what you mean by that, could you provide a sample json?
Also, where are those tests currently located at?
Thanks in advance
There was a problem hiding this comment.
Intersection model tests start here:
Example of an intersection model based on the previous union model example
StatusIntersection:
allOf:
- $ref: '#/components/schemas/StatusEnum'
- $ref: '#/components/schemas/DetailedStatus'Theese cover what is discussed in microsoft#7170 (comment)
This test covers the requested changes, requested in https://github.com/microsoft/kiota/pull/7170/changes/BASE..592710aa6905adea20c73192cf204a3b6f83f2fa#r2722212419
This commit provides a basic fix for #6776, unblocking nullable reference types for ASP.Net Core.
It also, magically (aka: as a side effect), makes enums work