-
Notifications
You must be signed in to change notification settings - Fork 311
Enable nullable reference types for generated models #1709
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,7 +23,7 @@ | |
| scriptObject.Import(nameof(IfType), new Func<JsonSchemaProperty, string, bool>(IfType)); | ||
| } | ||
|
|
||
| private string GetDotNetType(JsonObjectType jsonType, string name, bool required, string format) | ||
| private string GetDotNetType(JsonObjectType jsonType, string name, bool required, string? format) | ||
| { | ||
| if (name == "pretty" && !required) | ||
| { | ||
|
|
@@ -81,7 +81,7 @@ | |
| switch (format) | ||
| { | ||
| case "byte": | ||
| return "byte[]"; | ||
| return required ? "byte[]" : "byte[]?"; | ||
| case "date-time": | ||
|
|
||
| // eventTime is required but should be optional, see https://github.com/kubernetes-client/csharp/issues/1197 | ||
|
|
@@ -100,58 +100,58 @@ | |
| } | ||
| } | ||
|
|
||
| return "string"; | ||
| return required ? "string" : "string?"; | ||
| case JsonObjectType.Object: | ||
| return "object"; | ||
| return required ? "object" : "object?"; | ||
| default: | ||
| throw new NotSupportedException(); | ||
| } | ||
| } | ||
|
|
||
| private string GetDotNetType(JsonSchema schema, JsonSchemaProperty parent) | ||
| private string GetDotNetType(JsonSchema? schema, JsonSchemaProperty parent, bool isCollectionItem = false) | ||
| { | ||
| if (schema != null) | ||
| { | ||
| if (schema.IsArray) | ||
| if (schema.IsArray && schema.Item != null) | ||
| { | ||
| return $"IList<{GetDotNetType(schema.Item, parent)}>"; | ||
| return $"IList<{GetDotNetType(schema.Item, parent, isCollectionItem: true)}>?"; | ||
| } | ||
|
|
||
| if (schema.IsDictionary && schema.AdditionalPropertiesSchema != null) | ||
| { | ||
| return $"IDictionary<string, {GetDotNetType(schema.AdditionalPropertiesSchema, parent)}>"; | ||
| return $"IDictionary<string, {GetDotNetType(schema.AdditionalPropertiesSchema, parent, isCollectionItem: true)}>?"; | ||
| } | ||
|
|
||
| if (schema?.Reference != null) | ||
| if (schema.Reference != null) | ||
| { | ||
| return classNameHelper.GetClassNameForSchemaDefinition(schema.Reference); | ||
| var typeName = classNameHelper.GetClassNameForSchemaDefinition(schema.Reference); | ||
| // Collection items are always non-nullable, unless we're at the root level | ||
| return (isCollectionItem || parent.IsRequired) ? typeName : typeName + "?"; | ||
| } | ||
|
|
||
| if (schema != null) | ||
| { | ||
| return GetDotNetType(schema.Type, parent.Name, parent.IsRequired, schema.Format); | ||
| } | ||
| return GetDotNetType(schema.Type, parent.Name, isCollectionItem || parent.IsRequired, schema.Format); | ||
| } | ||
|
|
||
| return GetDotNetType(parent.Type, parent.Name, parent.IsRequired, parent.Format); | ||
| return GetDotNetType(parent.Type, parent.Name, isCollectionItem || parent.IsRequired, parent.Format); | ||
| } | ||
|
|
||
| public string GetDotNetType(JsonSchemaProperty p) | ||
| { | ||
| if (p.Reference != null) | ||
| { | ||
| return classNameHelper.GetClassNameForSchemaDefinition(p.Reference); | ||
| var typeName = classNameHelper.GetClassNameForSchemaDefinition(p.Reference); | ||
| return p.IsRequired ? typeName : typeName + "?"; | ||
| } | ||
|
|
||
| if (p.IsArray) | ||
| if (p.IsArray && p.Item != null) | ||
| { | ||
| // getType | ||
| return $"IList<{GetDotNetType(p.Item, p)}>"; | ||
| // getType - items in arrays are non-nullable | ||
| return $"IList<{GetDotNetType(p.Item, p, isCollectionItem: true)}>?"; | ||
| } | ||
|
|
||
| if (p.IsDictionary && p.AdditionalPropertiesSchema != null) | ||
| { | ||
| return $"IDictionary<string, {GetDotNetType(p.AdditionalPropertiesSchema, p)}>"; | ||
| return $"IDictionary<string, {GetDotNetType(p.AdditionalPropertiesSchema, p, isCollectionItem: true)}>?"; | ||
|
Comment on lines
+117
to
+154
|
||
| } | ||
|
|
||
| return GetDotNetType(p.Type, p.Name, p.IsRequired, p.Format); | ||
|
|
@@ -161,7 +161,8 @@ | |
| { | ||
| if (parameter.Schema?.Reference != null) | ||
| { | ||
| return classNameHelper.GetClassNameForSchemaDefinition(parameter.Schema.Reference); | ||
| var typeName = classNameHelper.GetClassNameForSchemaDefinition(parameter.Schema.Reference); | ||
| return parameter.IsRequired ? typeName : typeName + "?"; | ||
| } | ||
| else if (parameter.Schema != null) | ||
| { | ||
|
|
@@ -298,7 +299,7 @@ | |
| var listSchema = response?.Schema?.Reference; | ||
| if (listSchema?.Properties?.TryGetValue("items", out var itemsProperty) != true) | ||
| { | ||
| return null; | ||
|
Check warning on line 302 in src/LibKubernetesGenerator/TypeHelper.cs
|
||
| } | ||
|
|
||
| if (itemsProperty.Reference != null) | ||
|
|
@@ -311,6 +312,6 @@ | |
| return classNameHelper.GetClassNameForSchemaDefinition(itemsProperty.Item.Reference); | ||
| } | ||
|
|
||
| return null; | ||
|
Check warning on line 315 in src/LibKubernetesGenerator/TypeHelper.cs
|
||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| using System.Collections.Generic; | ||
| using k8s.Models; | ||
| using Xunit; | ||
|
|
||
| namespace k8s.Tests | ||
| { | ||
| public class NullableReferenceTypesTests | ||
| { | ||
| [Fact] | ||
| public void ContainerVolumeMountsIsNullableProperty() | ||
| { | ||
| // Arrange & Act | ||
| var container = new V1Container(); | ||
|
|
||
| // Assert | ||
| // VolumeMounts should be null by default (nullable property) | ||
| Assert.Null(container.VolumeMounts); | ||
|
|
||
| // This should not throw NullReferenceException anymore - users should check for null | ||
| // container.VolumeMounts.Add(new V1VolumeMount()); // This would throw | ||
|
|
||
| // Proper usage: Initialize the list first | ||
| container.VolumeMounts = new List<V1VolumeMount> | ||
| { | ||
| new V1VolumeMount(), | ||
| }; | ||
|
|
||
| Assert.NotNull(container.VolumeMounts); | ||
| Assert.Single(container.VolumeMounts); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void ContainerNameIsRequiredProperty() | ||
| { | ||
| // Arrange & Act | ||
| var container = new V1Container | ||
| { | ||
| Name = "test-container", | ||
| }; | ||
|
|
||
| // Assert | ||
| // Name is a required property (non-nullable string) | ||
| Assert.NotNull(container.Name); | ||
| Assert.Equal("test-container", container.Name); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void ContainerImageIsOptionalProperty() | ||
| { | ||
| // Arrange & Act | ||
| var container = new V1Container(); | ||
|
||
|
|
||
| // Assert | ||
| // Image is optional (nullable string) | ||
| Assert.Null(container.Image); | ||
|
|
||
| container.Image = "nginx:latest"; | ||
| Assert.Equal("nginx:latest", container.Image); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void ContainerLifecycleIsOptionalComplexProperty() | ||
| { | ||
| // Arrange & Act | ||
| var container = new V1Container(); | ||
|
|
||
| // Assert | ||
| // Lifecycle is optional (nullable reference type) | ||
| Assert.Null(container.Lifecycle); | ||
|
|
||
| container.Lifecycle = new V1Lifecycle(); | ||
| Assert.NotNull(container.Lifecycle); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void ContainerCollectionItemsAreNonNullable() | ||
| { | ||
| // Arrange | ||
| var container = new V1Container | ||
| { | ||
| // Initialize the list - the list itself can be null, but items cannot be null | ||
| VolumeMounts = new List<V1VolumeMount> | ||
| { | ||
| new V1VolumeMount { Name = "vol1", MountPath = "/data", }, | ||
| new V1VolumeMount { Name = "vol2", MountPath = "/config", }, | ||
| }, | ||
| }; | ||
|
|
||
| // Act & Assert | ||
| Assert.NotNull(container.VolumeMounts); | ||
| Assert.Equal(2, container.VolumeMounts.Count); | ||
| Assert.All(container.VolumeMounts, vm => Assert.NotNull(vm)); | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enabling nullable reference types globally in Directory.Build.props will affect all projects in the solution, including existing handwritten code that may not be nullable-aware. This is a significant breaking change that could introduce numerous compiler warnings throughout the codebase. Consider:
This is a major architectural decision that could impact maintainability and should be carefully validated before merging.