NJsonSchema icon indicating copy to clipboard operation
NJsonSchema copied to clipboard

Optional string property with date format in nullable context generates not-nullable string property

Open aw129 opened this issue 4 years ago • 3 comments

Given a JSON schema definition like:

    myObject:
      type: object
      properties:
        dateFrom:
          type: string
          format: date
          description: Valid from
          example: "2004-09-16"

When a C# class is generated with settings including:

                    CSharpGeneratorSettings =
                    {
                        DateType = "string",
                        DateTimeType = "string",
                        GenerateNullableReferenceTypes = true,
                        GenerateOptionalPropertiesAsNullable = true
                    }

The generated property definition is:

        public string DateFrom { get; set; }= default!;

Whereas it should be:

        public string? DateFrom { get; set; }= default!;

I believe this is due to the following code in the ResolveString method in CSharpTypeResolver:

            if (schema.Format == JsonFormatStrings.Date)
            {
                return isNullable && Settings.DateType?.ToLowerInvariant() != "string" ? Settings.DateType + "?" : Settings.DateType;
            }

If the format is date and the date type configured to be string this will always return string without the nullable annotation. The same issue appears to exist for other date and time formats handled in this method.

I believe this could be fixed by moving the following to the top of the method:

            var nullableReferenceType = Settings.GenerateNullableReferenceTypes && isNullable ? "?" : string.Empty;

And changing the logic in question to:

            if (schema.Format == JsonFormatStrings.Date)
            {
                return isNullable && Settings.DateType?.ToLowerInvariant() != "string" ? Settings.DateType + "?" : Settings.DateType + nullableReferenceType;
            }

aw129 avatar Aug 21 '21 07:08 aw129

Is there anything blocking this from being merged? Are there any workarounds to fix the behaviour for strings we can employ with the current release?

Trolldemorted avatar Sep 24 '21 13:09 Trolldemorted

We use the following class as a workaround - note this is hard-coded to our specific use case based on our conventions, it's not a generic workaround.

    internal sealed class DateFixTypeResolver : CSharpTypeResolver
    {
        internal DateFixTypeResolver(CSharpGeneratorSettings settings)
            : base(settings)
        {
        }

        public override string Resolve(JsonSchema schema, bool isNullable, string typeNameHint)
        {
            // There is a bug in NJsonSchema CSharpTypeResolver when an optional string has a format of date or datetime
            // in a nullable context.
            // https://github.com/RicoSuter/NJsonSchema/issues/1405
            return schema switch
            {
                JsonSchemaProperty { Format: "date", IsRequired: false } => "string?",
                JsonSchemaProperty { Format: "datetime", IsRequired: false } => "string?",
                _ => base.Resolve(schema, isNullable, typeNameHint)
            };
        }
    }

We then hook this into NSwag like this:

                var settings = new CSharpClientGeneratorSettings
                {
                    ...
                };

                var resolver = new DateFixTypeResolver(settings.CSharpGeneratorSettings);
                resolver.RegisterSchemaDefinitions(document.Definitions);
                var generator = new CSharpClientGenerator(document, settings, resolver);

                var generatedCode = generator.GenerateFile();

If you're using NJsonSchema directly you should be able to pass the custom resolver to this CSharpGenerator constructor.

aw129 avatar Oct 02 '21 07:10 aw129

@RicoSuter is there something I need to do to make this fit to merge?

aw129 avatar Oct 07 '22 14:10 aw129